OpenCV - это библиотека алгоритмов компьютерного зрения, обработки изображений и численных алгоритмов общего назначения. Реализована на Си/Си++. Может свободно использоваться в академических и коммерческих целях, так как распространяется в условиях лицензии BSD. Пришло время проверить эту библиотеку с помощью анализатора кода PVS-Studio.
OpenCV это большая библиотека. Она содержит в себе более 2500 оптимизированных алгоритмов. Число строк кода более 1 миллиона. Цикломатическая сложность самой сложной функции cv::cvtColor() составляет 415. Так что нет ничего удивительно, что нам удалось обнаружить достаточно много ошибок и крайне подозрительных мест. Однако если учесть объем исходного кода, можно говорить о высоком качестве этой библиотеки.
Но вначале небольшое лирическое отступление. Рассматривая примеры ошибок, обнаруженные PVS-Studio, программисты не хотят верить, что эти ошибки настоящие. Наверное, им неприятно осознавать шаткость своих и чужих программ. Поэтому они говорят: "Ok. Действительно найдены какие-то ошибки. Но на самом деле они не оказывают влияние на поведение программы. Этот код, скорее всего не используется. Проблемы нет".
К сожалению, это конечно не так. Сейчас у меня есть подходящий момент, чтобы продемонстрировать это. Исследуя один из проектов, мы проверили входящую в его состав библиотеку OpenCV. В проект была включена старая версия библиотеки. Поэтому мы познакомились с найденными в ней ошибками, но не стали их описывать. Разумно было проверить и написать о проверке свежей версии библиотеки OpenCV. И сейчас мы это сделали.
Результат ожидаем для нас. Многие ошибки, присутствовавшие в старой версии библиотеки, были исправлены в новой версии. Приведу парочку примеров таких ошибок.
Первая исправленная ошибка:
CV_IMPL CvGLCM* cvCreateGLCM(....)
{
CvGLCM* newGLCM = 0;
....
memset( newGLCM, 0, sizeof(newGLCM) );
....
}V512 A call of the 'memset' function will lead to underflow of the buffer 'newGLCM'. cvtexture.cpp 138
Вторая исправленная ошибка:
CvDTreeSplit* CvDTree::find_split_cat_reg(....)
{
....
double** sum_ptr = 0;
.... // sum_ptr not in use
for( i = 0; i < mi; i++ )
{
R += counts[i];
rsum += sum[i];
sum[i] /= MAX(counts[i],1);
sum_ptr[i] = sum + i;
}
....
}V522 Dereferencing of the null pointer 'sum_ptr' might take place. mltree.cpp 2001
Есть и другие примеры. Но описывать ошибки, которые уже исправлены, не интересно. Главное, что напрашиваются неумолимые выводы:
1. Обнаруживаемые анализатором PVS-Studio ошибки действительно самые настоящие ошибки. Они мучают и пьют кровь пользователей библиотеки и её разработчиков. Их надо искать и исправлять. Это делается медленно и печально после того, как они будут обнаружены пользователями.
2. Эти и многие другие ошибки можно обнаружить с помощью анализатора PVS-Studio ещё на этапе написания кода. Это существенно сокращает стоимость разработки. Особенно может быть полезен режим инкрементального анализа.
Примечание. Проверяя проект, мы не делаем различия, относится ошибка именно к самому проекту, или к одной из используемых сторонних библиотек. Это неинтересно описывать каждую маленькую библиотеку по отдельности.
Ещё надо отметить, что не следует рассматривать статью, как перечень всех ошибок, которых смог найти PVS-Studio в проекте OpenCV. В статье приведены только те фрагменты кода, которые показались наиболее подозрительными при беглом просмотре сообщений, выданных анализатором. Если вы участвуете в разработке проекта OpenCV, то мы рекомендуем воспользоваться демонстрационной версией инструмента, чтобы изучить список выдаваемых анализатором предупреждений более подробно.
Анализатор PVS-Studio хорошо выявляет ошибки связанные с опечатками и copy-paste. Рассмотрим классический пример копирования кода. Есть набор функций, таких как augAssignAnd, augAssignOr, augAssignXor, augAssignDivide и так далее. Эти функции различаются только одним оператором. Естественно велик соблазн размножить тело функции, а затем исправить оператор, отвечающий за выполняемое функцией действие. Беда в том, что и велик шанс ошибиться.
void MatOp::augAssignAnd(const MatExpr& expr, Mat& m) const
{
Mat temp;
expr.op->assign(expr, temp);
m &= temp;
}
void MatOp::augAssignOr(const MatExpr& expr, Mat& m) const
{
Mat temp;
expr.op->assign(expr, temp);
m |= temp;
}
void MatOp::augAssignDivide(const MatExpr& expr, Mat& m) const
{
Mat temp;
expr.op->assign(expr, temp);
m /= temp;
}
void MatOp::augAssignXor(const MatExpr& expr, Mat& m) const
{
Mat temp;
expr.op->assign(expr, temp);
m /= temp;
}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'. В результате оформление программы не совпадает с логикой её работы.
Следующая ошибка, скорее всего, связана с опечаткой, а не с копированием строчек кода. Возможно, подвело автоматическое дополнение при написании имени переменной.
static jpc_enc_cp_t *cp_create(....)
{
....
ccp->sampgrdsubstepx = 0;
ccp->sampgrdsubstepx = 0;
....
}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;.
typedef struct CvStereoCamera
{
....
float fundMatr[9]; /* fundamental matrix */
....
};
CvStereoCamera stereo;
void CvCalibFilter::Stop( bool calibrate )
{
....
for( i = 0; i < 9; i++ )
{
stereo.fundMatr[i] = stereo.fundMatr[i];
}
....
}V570 The 'stereo.fundMatr[i]' 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
По поводу других аналогичных ошибок, ограничимся диагностическими сообщениями:
Очень распространена ошибка, когда вначале указатель используется, а только потом проверяется на равенство нулю. Библиотека 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 и проанализировать все эти места.
Примечание. Диагностика V595 не всегда означает, что фрагмент кода точно ошибочен. Иногда указатель даже теоретически не может быть равен нулю. В этом случае можно удалить проверку, чтобы она не вносила путаницу при чтении кода. А ещё лучше передавать объект по ссылке, а не по указателю.
Весьма распространены ошибки, из-за которых обрабатывается не весь буфер, а только первые его байты. Чаще всего это связано с путаницей, когда вычисляется размер указателя, а не массива на который он указывает (примеры). Здесь по все видимости аналогичная ситуация.
CAPDRIVERCAPS caps;
bool CvCaptureCAM_VFW::open( int wIndex )
{
....
memset( &caps, 0, sizeof(caps));
capDriverGetCaps( hWndC, &caps, sizeof(&caps));
....
}V568 It's odd that the argument of sizeof() operator is the '& caps' expression. cap_vfw.cpp 409
В функцию capDriverGetCaps() передается не размер структуры CAPDRIVERCAPS, а размер указателя.
А вот другой фрагмент кода. Скорее всего, причиной ошибки является опечатка. Нулями заполняется массив 'latestCounts', а размер вычисляется для массива 'latestPoints'.
class CV_EXPORTS CvCalibFilter
{
....
enum { MAX_CAMERAS = 3 };
int latestCounts[MAX_CAMERAS];
CvPoint2D32f* latestPoints[MAX_CAMERAS];
....
};
void CvCalibFilter::SetCameraCount( int count )
{
....
memset( latestCounts, 0, sizeof(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.
void CV_Resize_Test::resize_1d(....)
{
....
for (int r = 0; r < cn; ++r)
{
xyD[r] = 0;
for (int k = 0; k < ksize; ++k)
xyD[r] += w[k] * xyS[k * cn + r];
xyD[r] = xyD[r];
}
....
}V570 The 'xyD[r]' variable is assigned to itself. test_imgwarp_strict.cpp 560
Выражение "xyD[r] = xyD[r];" выглядит очень подозрительно. Возможно, этот тест проверяет не совсем то, что следует.
А вот другая странная строчка: "cls_map[r];". Что-бы это могло значить?
void ann_get_new_responses(....)
{
....
for( int si = 0; si < train_sidx->cols; si++ )
{
int sidx = train_sidx_ptr[si];
int r = cvRound(responses_ptr[sidx*r_step]);
CV_DbgAssert(fabs(responses_ptr[sidx*r_step]-r) < FLT_EPSILON);
int cls_map_size = (int)cls_map.size();
cls_map[r];
if ( (int)cls_map.size() > cls_map_size )
cls_map[r] = cls_count++;
}
....
}V607 Ownerless expression 'cls_map[r]'. test_mltests2.cpp 342
Есть и другие подозрительные места, например такое:
void Core_DetTest::get_test_array_types_and_sizes(....)
{
....
sizes[INPUT][0].width =
sizes[INPUT][0].height = sizes[INPUT][0].height;
....
}V570 The 'sizes[INPUT][0].height' variable is assigned to itself. test_math.cpp 1356
Приведенный ниже код может работать в вашей программе именно так как вы этого хотите. Но знайте, что это до поры до времени. Речь идёт о сдвиге отрицательного числа. Подробнее про такие сдвиги можно почитать в статье "Не зная брода, не лезь в воду. Часть третья".
CvSeq * cvFindNextContour( CvContourScanner scanner )
{
....
new_mask = INT_MIN >> 1;
....
}V610 Unspecified behavior. Check the shift operator '>>. The left operand '(- 2147483647 - 1)' is negative. contours.cpp 1012
void CvFuzzyMeanShiftTracker::SearchWindow::initDepthValues(....)
{
unsigned int d=0, mind = 0xFFFF, maxd = 0,
m0 = 0, m1 = 0, mc, dd;
....
for (int j = 0; j < height; j++)
{
....
if (d > maxd)
maxd = d;
....
}
}V547 Expression 'd > maxd' is always false. Unsigned type value is never < 0. fuzzymeanshifttracker.cpp 386
В цикле переменная 'd' не изменяется. Это значит, что условие 'd > maxd' никогда не выполняется.
void jpc_init_t2state(jpc_enc_t *enc, int raflag)
{
....
for (pass = cblk->passes; pass != endpasses; ++pass) {
pass->lyrno = -1;
pass->lyrno = 0;
}
....
}V519 The 'pass->lyrno' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 539, 540. jpc_t2enc.c 540
void KeyPointsFilter::retainBest(vector<KeyPoint>& keypoints, int
n_points)
{
....
if( n_points > 0 && keypoints.size() > (size_t)n_points )
{
if (n_points==0)
{
keypoints.clear();
return;
}
....
}V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 195, 197. keypoint.cpp 195
void HOGDescriptor::detectMultiScaleROI(....) const
{
....
double *linearwt = new double[totwords+1];
....
delete linearwt;
....
}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. В противном случае, цена нахождения и исправления этих ошибок возрастает на порядок.