OpenCV  - это библиотека алгоритмов компьютерного зрения, обработки изображений и численных алгоритмов общего назначения. Реализована на Си/Си++. Может свободно использоваться в академических и коммерческих целях, так как распространяется в условиях лицензии BSD. Пришло время проверить эту библиотеку с помощью анализатора кода PVS-Studio.

OpenCV это большая библиотека. Она содержит в себе более 2500 оптимизированных алгоритмов. Число строк кода более 1 миллиона. Цикломатическая сложность самой сложной функции cv::cvtColor() составляет 415. Так что нет ничего удивительно, что нам удалось обнаружить достаточно много ошибок и крайне подозрительных мест. Однако если учесть объем исходного кода, можно говорить о высоком качестве этой библиотеки.

Старые ошибки

Но вначале небольшое лирическое отступление. Рассматривая примеры ошибок, обнаруженные PVS-Studio, программисты не хотят верить, что эти ошибки настоящие. Наверное, им неприятно осознавать шаткость своих и чужих программ. Поэтому они говорят: "Ok. Действительно найдены какие-то ошибки. Но на самом деле они не оказывают влияние на поведение программы. Этот код, скорее всего не используется. Проблемы нет".

К сожалению, это конечно не так. Сейчас у меня есть подходящий момент, чтобы продемонстрировать это. Исследуя один из проектов, мы проверили входящую в его состав библиотеку OpenCV. В проект была включена старая версия библиотеки. Поэтому мы познакомились с найденными в ней ошибками, но не стали их описывать. Разумно было проверить и написать о проверке свежей версии библиотеки OpenCV. И сейчас мы это сделали.

Результат ожидаем для нас. Многие ошибки, присутствовавшие в старой версии библиотеки, были исправлены в новой версии. Приведу парочку примеров таких ошибок.

Первая исправленная ошибка:



V512 A call of the 'memset' function will lead to underflow of the buffer 'newGLCM'. cvtexture.cpp 138



Вторая исправленная ошибка:



V522 Dereferencing of the null pointer 'sum_ptr' might take place. mltree.cpp 2001

Есть и другие примеры. Но описывать ошибки, которые уже исправлены, не интересно. Главное, что напрашиваются неумолимые выводы:

1. Обнаруживаемые анализатором PVS-Studio ошибки действительно самые настоящие ошибки. Они мучают и пьют кровь пользователей библиотеки и её разработчиков. Их надо искать и исправлять. Это делается медленно и печально после того, как они будут обнаружены пользователями.

2. Эти и многие другие ошибки можно обнаружить с помощью анализатора PVS-Studio ещё на этапе написания кода. Это существенно сокращает стоимость разработки. Особенно может быть полезен режим инкрементального анализа.

Новые ошибки

Примечание. Проверяя проект, мы не делаем различия, относится ошибка именно к самому проекту, или к одной из используемых сторонних библиотек. Это неинтересно описывать каждую маленькую библиотеку по отдельности.

Ещё надо отметить, что не следует рассматривать статью, как перечень всех ошибок, которых смог найти PVS-Studio в проекте OpenCV. В статье приведены только те фрагменты кода, которые показались наиболее подозрительными при беглом просмотре сообщений, выданных анализатором. Если вы участвуете в разработке проекта OpenCV, то мы рекомендуем воспользоваться демонстрационной версией инструмента, чтобы изучить список выдаваемых анализатором предупреждений более подробно.

Ошибка Copy-Paste

Анализатор PVS-Studio хорошо выявляет ошибки связанные с опечатками и copy-paste. Рассмотрим классический пример копирования кода. Есть набор функций, таких как augAssignAnd, augAssignOr, augAssignXor, augAssignDivide и так далее. Эти функции различаются только одним оператором. Естественно велик соблазн размножить тело функции, а затем исправить оператор, отвечающий за выполняемое функцией действие. Беда в том, что и велик шанс ошибиться.



V524 It is odd that the body of 'augAssignXor' function is fully equivalent to the body of 'augAssignDivide' function (matop.cpp, line 294). matop.cpp 318

Обратите внимание, что функция augAssignXor() делает тоже самое, что и функция 'augAssignDivide(). Это естественно неправильно. В функции augAssignXor() должно быть написано: "m ^= temp;".

Логика кода, которая не соответствует форматированию кода

А вот ещё одна ошибка, связанная с copy-paste. Рассматриваемые строки программы слишком длинные. Если их отформатировать для текста статьи, то будет непонятно, в чем собственно заключается ошибка. Придется продемонстрировать ошибку с помощью рисунка.

        

            Рисунок 1. Логика программы не совпадает с её оформлением. Нажмите на рисунок для его увеличения.

V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. test_stereomatching.cpp 464

Заметно, что длинная строка кода была скопирована и помещена после оператора 'if'. В результате оформление программы не совпадает с логикой её работы.

Опечатка

Следующая ошибка, скорее всего, связана с опечаткой, а не с копированием строчек кода. Возможно, подвело автоматическое дополнение при написании имени переменной.



V519 The 'ccp->sampgrdsubstepx' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 414, 415. jpc_enc.c 415

По всей видимости, вторая строка кода обязана быть такой: ccp->sampgrdsubstepy = 0;.

Бессмысленные циклы



V570 The 'stereo.fundMatr' variable is assigned to itself. calibfilter.cpp 339

В таком виде цикл не имеет смысла. Скорее всего, с элементами массива должно происходить что-то ещё.



А вот цикл, тело которого выполняется только один раз:

Код

virtual CvBlob* Process(....) {   ....   while(!m_Collision && m_FGWeight>0)   {     ....     break;   }   .... }

V612 An unconditional 'break' within a loop. blobtrackingmsfg.cpp 600

Тело цикла не содержит операторов 'continue'. В конце тела цикла находится оператор 'break'. Это очень странно и скорее всего функция работает некорректно.

Путаница между нулевым символом и нулевым указателем

Код

int jpc_atoaf(char *s, int *numvalues, double **values) {   char *cp;   ....   while ((cp = strtok(0, delim))) {     if (cp != '\0') {       ++n;     }   }   .... }

V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *cp != '\0'. jpc_util.c 105

Аналогичная ошибка находится также здесь: jpc_util.c 123.

Проверка if(cp != '\0') не имеет смысла. Если функция strtok() вернёт нулевой указатель, то цикл остановится. По всей видимости, здесь хотели проверить что найден конец строки. В этом случае, проверка должна выглядеть так: if(*cp != '\0').

Опечатки в условиях

Есть целый ряд ошибок, когда из-за опечаток проверяются значения не всех переменных.

Не проверена переменная dr3dr2:

Код

CV_IMPL void cvComposeRT(   const CvMat* _rvec1, const CvMat* _tvec1,   const CvMat* _rvec2, const CvMat* _tvec2,   CvMat* _rvec3, CvMat* _tvec3,   CvMat* dr3dr1, CvMat* dr3dt1,   CvMat* dr3dr2, CvMat* dr3dt2,   CvMat* dt3dr1, CvMat* dt3dt1,   CvMat* dt3dr2, CvMat* dt3dt2) {   ....   if( _rvec3 || dr3dr1 || dr3dr1 )   .... }

V501 There are identical sub-expressions to the left and to the right of the '||' operator: _rvec3 || dr3dr1 || dr3dr1 calibration.cpp 415



Не проверен элемент массива cmptlut[2]:

Код

bool Jpeg2KDecoder::readHeader() {   ....   cmptlut[0] = ....   cmptlut[1] = ....   cmptlut[2] = ....   if( cmptlut[0] < 0 || cmptlut[1] < 0 || cmptlut[0] < 0 )     result = false;   .... }

V501 There are identical sub-expressions 'cmptlut[0] < 0' to the left and to the right of the '||' operator. grfmt_jpeg2000.cpp 215



Переменная dst_size.height сравнивается сама с собой:

Код

CV_IMPL IplImage* icvCreateIsometricImage(....) {   ....   if( !dst || dst->depth != desired_depth ||       dst->nChannels != desired_num_channels ||       dst_size.width != src_size.width ||       dst_size.height != dst_size.height )   .... }

V501 There are identical sub-expressions to the left and to the right of the '!=' operator: dst_size.height != dst_size.height epilines.cpp 2118



Вообще бессмысленное условие:

Код

void CvDTreeTrainData::read_params(....) {   ....   if( cat_var_count != cat_var_count ||       ord_var_count != ord_var_count )     CV_ERROR(CV_StsParseError,     "var_type is inconsistent with cat_var_count and ord_var_count");   .... }

V501 There are identical sub-expressions to the left and to the right of the '!=' operator: cat_var_count != cat_var_count tree.cpp 1415

V501 There are identical sub-expressions to the left and to the right of the '!=' operator: ord_var_count != ord_var_count tree.cpp 1415



По поводу других аналогичных ошибок, ограничимся диагностическими сообщениями:

• V501 There are identical sub-expressions to the left and to the right of the '==' operator: M.size() == M.size() imgwarp.cpp 3672
• V501 There are identical sub-expressions to the left and to the right of the '&&' operator: data && dims >= 1 && data mat.hpp 434
• V501 There are identical sub-expressions to the left and to the right of the '&&' operator: 0 <= d && _sizes && d <= 32 && _sizes matrix.cpp 186
• V501 There are identical sub-expressions to the left and to the right of the '==' operator: M.size() == M.size() imgwarp.cpp 3685

Указатель используется до проверки

Очень распространена ошибка, когда вначале указатель используется, а только потом проверяется на равенство нулю. Библиотека OpenCV не исключение. Эти ошибки выглядят так:

Код

CV_IMPL CvStringHashNode*  cvGetHashedKey( CvFileStorage* fs, .... ) {   ....   CvStringHash* map = fs->str_hash;   if( !fs )     return 0;   .... }

V595 The 'fs' pointer was utilized before it was verified against nullptr. Check lines: 617, 619. persistence.cpp 617

Код

void CvBlobTrackerAuto1::Process(IplImage* pImg, IplImage* pMask) {   ....   CvBlob* pBN = NewBlobList.GetBlob(i);   pBN->ID = m_NextBlobID;   if(pBN &&      pBN->w >= CV_BLOB_MINW &&      pBN->h >= CV_BLOB_MINH)   .... }

V595 The 'pBN' pointer was utilized before it was verified against nullptr. Check lines: 432, 434. blobtrackingauto.cpp 432

Пожалуй, нет смысла далее приводить фрагменты кода, где выдается диагностическое сообщение V595. Их много и все они однотипны. Проще запустить анализ PVS-Studio и проанализировать все эти места.

[i]Примечание. Диагностика V595 не всегда означает, что фрагмент кода точно ошибочен. Иногда указатель даже теоретически не может быть равен нулю. В этом случае можно удалить проверку, чтобы она не вносила путаницу при чтении кода. А ещё лучше передавать объект по ссылке, а не по указателю.

Путаница с размерами

Весьма распространены ошибки, из-за которых обрабатывается не весь буфер, а только первые его байты. Чаще всего это связано с путаницей, когда вычисляется размер указателя, а не массива на который он указывает (примеры). Здесь по все видимости аналогичная ситуация.



V568 It's odd that the argument of sizeof() operator is the '& caps' expression. cap_vfw.cpp 409

В функцию capDriverGetCaps() передается не размер структуры CAPDRIVERCAPS, а размер указателя.



А вот другой фрагмент кода. Скорее всего, причиной ошибки является опечатка. Нулями заполняется массив 'latestCounts', а размер вычисляется для массива 'latestPoints'.



V512 A call of the 'memset' function will lead to overflow of the buffer 'latestCounts'. calibfilter.cpp 238

Приведенный фрагмент программы содержит 64-битную ошибку. Этот код будет отлично работать в 32-битной программе. Дело в том, что 32-битный приложениях размер указателя совпадает с размером типа 'int'. А вот при компиляции 64-битного кода, произойдет переполнение буфера. 

Как ни странно, такие ошибки могут подолгу оставаться незамеченными. Во-первых, 32-битная программа всегда работает корректно. Но даже если программа 64-битная, обнуление памяти за массивом может не причинить вреда. Как правило, такие ошибки проявляют себя при смене компилятора или рефакторинге соседних участков кода.

Неудачные внутренние тесты

Не так давно я писал заметку о том, что одним из уязвимых мест методологии TDD являются ошибки в тестах. Нередко тесты только создают видимость надежности программы. Очень хорошим дополнением к методологии TDD является статический анализ кода. Он не только находит в коде программы, но и помогает устранить многие ошибки в тестах.

Вполне естественно, что ошибки могут встретиться в тестах библиотеки OpenCV.



V570 The 'xyD[r]' variable is assigned to itself. test_imgwarp_strict.cpp 560

Выражение "xyD[r] = xyD[r];" выглядит очень подозрительно. Возможно, этот тест проверяет не совсем то, что следует.

А вот другая странная строчка: "cls_map[r];". Что-бы это могло значить?



V607 Ownerless expression 'cls_map[r]'. test_mltests2.cpp 342



Есть и другие подозрительные места, например такое:



V570 The 'sizes[INPUT][0].height' variable is assigned to itself. test_math.cpp 1356

Unspecified behavior

Приведенный ниже код может работать в вашей программе именно так как вы этого хотите. Но знайте, что это до поры до времени. Речь идёт о сдвиге отрицательного числа. Подробнее про такие сдвиги можно  почитать в статье "Не зная брода, не лезь в воду. Часть третья".



V610 Unspecified behavior. Check the shift operator '>>. The left operand '(- 2147483647 - 1)' is negative. contours.cpp 1012



Разное



V547 Expression 'd > maxd' is always false. Unsigned type value is never < 0. fuzzymeanshifttracker.cpp 386

В цикле переменная 'd' не изменяется. Это значит, что условие 'd > maxd' никогда не выполняется.





V519 The 'pass->lyrno' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 539, 540. jpc_t2enc.c 540





V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 195, 197. keypoint.cpp 195





V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] linearwt;'. hog.cpp 2630

Заключение

Даже высококвалифицированные программисты допускают ошибки. Многие из этих ошибок можно устранить ещё на этапе написания кода, используя инструмент PVS-Studio. В противном случае, цена нахождения и исправления этих ошибок возрастает на порядок.