Мы используем куки, чтобы пользоваться сайтом
было удобно.
Что общего у компьютерного зрения и статического анализа? Оба ищут смысл в данных. OpenCV находит образы среди миллионов пикселей, а PVS-Studio — ошибки среди тысяч строк кода. В статье изучим исходники крупнейшей библиотеки компьютерного зрения.
OpenCV — крупнейшая в мире библиотека компьютерного зрения с открытым исходным кодом, поддерживаемая некоммерческой организацией Open Source Computer Vision Foundation. Она предлагает обширный набор алгоритмов, охватывающих огромный список задач: от базовых функций обработки изображений до продвинутых методов распознавания объектов и анализа движения.
PVS-Studio — это статический анализатор кода для поиска ошибок и уязвимостей, который активно развивается с 2008 года. За прошедшие 12 лет (именно столько прошло с последней проверки OpenCV) мы значительно усилили анализ, добавив более 300 диагностических правил и улучшив уже существующие. Эффективность анализатора постоянно повышается также благодаря работе над устранением ложных срабатываний, о которых сообщают пользователи и которые мы находим сами при проверке открытых проектов.
Проект OpenCV также активно развивается, о чём свидетельствуют 27 тысяч коммитов и около 120 тегов, появившиеся за последние 12 лет. Библиотека нашла применение в самых разных сферах: от распознавания объектов и промышленной автоматизации до медицинской диагностики, позволяя выявлять аномалии на снимках с высокой точностью. И именно из-за такого широкого использования для OpenCV крайне важна тщательная проверка кода.
В этой статье предлагаю посмотреть, как статический анализ помогает избежать попадания багов в релиз и облегчить жизнь разработчикам. Стоит отметить, что я рассматриваю не все предупреждения, выданные анализатором, а только те, которые кажутся мне наиболее интересными.
Анализ проекта выполнен для коммита 12b64b0 с применением PVS-Studio 7.39.
Проект был собран на Windows с помощью CMake под Visual Studio.
Предупреждение PVS-Studio: V1061 Extending the 'std' namespace may result in undefined behavior. test_descriptors_invariance.impl.hpp 195
typedef std::function<cv::Ptr<cv::FeatureDetector>()> DetectorFactory;
typedef tuple<std::string, DetectorFactory, float, float>
String_FeatureDetector_Float_Float_t;
// ....
namespace std {
using namespace opencv_test;
static inline void PrintTo(
const String_FeatureDetector_DescriptorExtractor_Float_t& v,
std::ostream* os)
{
*os << "(\"" << get<0>(v)
<< "\", " << get<3>(v)
<< ")";
}
} // namespaceРасширение пространства имён std является нарушением стандарта C++ и в большинстве случаев ведёт к неопределённому поведению. Стандарт явно запрещает добавление собственных определений в std, за исключением специализаций шаблонов для пользовательских типов.
The behavior of a C++ program is undefined if it adds declarations or definitions to namespace std or to a namespace within namespace std unless otherwise specified. A program may add a template specialization for any standard library template to namespace std only if the declaration depends on a user-defined type and the specialization meets the standard library requirements for the original template and is not explicitly prohibited.
The behavior of a C++ program is undefined if it declares
— an explicit specialization of any member function of a standard library class template, or
— an explicit specialization of any member function template of a standard library class or class template, or
— an explicit or partial specialization of any member class template of a standard library class or class template, or
— a deduction guide for any standard library class template.
A program may explicitly instantiate a template defined in the standard library only if the declaration depends on the name of a user-defined type and the instantiation meets the standard library requirements for the original template.
A translation unit shall not declare namespace std to be an inline namespace (10.3.1).
В коде в пространство std добавляется обычная функция PrintTo, что недопустимо. Разработчик, скорее всего, хотел обеспечить видимость этой функции через механизм ADL (Argument-Dependent Lookup), чтобы она автоматически находилась при работе с соответствующими типами. Однако такой код опасен и его следует исправить.
Этот фрагмент кода довольно запутанный, и я с ходу не могу предложить для него решения, поэтому оставляю задачу разработчикам. Но если вам хочется подробнее узнать о механизме ADL, пишите в комментарии.
V1061. Extending the 'std' namespace may result in undefined behavior. test_descriptors_invariance.impl.hpp 195
V1061. Extending the 'std' namespace may result in undefined behavior. gapi_fluid_parallel_rois_test.cpp 292
Предупреждение PVS-Studio: V758 The 'graph' reference becomes invalid when smart pointer returned by a function is destroyed. utils.cpp 391
template<typename T>
struct Ptr : public std::shared_ptr<T>;
// ....
Ptr<FlannNeighborhoodGraph> FlannNeighborhoodGraph::create(
const Mat &points, int points_size,
int k_nearest_neighbors_, bool get_distances,
int flann_search_params_, int num_kd_trees)
{
return makePtr<FlannNeighborhoodGraphImpl>(points, points_size,
k_nearest_neighbors_, get_distances,
flann_search_params_, num_kd_trees);
}
void Utils::densitySort (const Mat &points, int knn,
Mat &sorted_points, std::vector<int> &sorted_mask)
{
// ....
// get neighbors
FlannNeighborhoodGraph &graph = // <=
*FlannNeighborhoodGraph::create(points, points_size, knn,
true /*get distances */, 6, 1);
std::vector<double> sum_knn_distances (points_size, 0);
for (int p = 0; p < points_size; p++) {
const std::vector<double> &dists = graph.getNeighborsDistances(p);
for (int k = 0; k < knn; k++)
sum_knn_distances[p] += dists[k];
}
// ....
}template<typename T>
struct Ptr : public std::shared_ptr<T>
{
inline Ptr(const std::shared_ptr<T>& o)
CV_NOEXCEPT : std::shared_ptr<T>(o) {}
inline Ptr(std::shared_ptr<T>&& o)
CV_NOEXCEPT : std::shared_ptr<T>(std::move(o)) {}
typename std::add_lvalue_reference<T>::type operator*() const
CV_NOEXCEPT { return *std::shared_ptr<T>::get(); }
// ....
}
template<typename _Tp, typename ... A1> static inline
Ptr<_Tp> makePtr(const A1&... a1)
{
static_assert( !has_custom_delete<_Tp>::value,
"Can't use this makePtr with custom DefaultDeleter");
return (Ptr<_Tp>)std::make_shared<_Tp>(a1...);
}Если вы думаете, что использование умных указателей раз и навсегда решает проблему "висячих" ссылок и доступов к памяти, то здесь всё пошло не так. Давайте разбираться. Сейчас код работает следующим образом:
create создаёт и возвращает умный указатель на тип FlannNeighborhoodGraphImpl, и его счётчик ссылок на объект равен единице;graph на значение этого умного указателя, при этом счётчик ссылок на объект не изменяется;for происходит обращение к невалидной ссылке.В итоге код, который казался правильным, приводит к неопределённому поведению. Кроме того, эту проблему находит не только PVS-Studio, но и санитайзер. Пруф.
Чтобы исправить проблему, необходимо сохранить умный указатель, тогда объект типа FlannNeighborhoodGraph будет жить до конца блока. Например, можно сделать так:
std::vector<double> sum_knn_distances (points_size, 0);
{
// get neighbors
auto graph = FlannNeighborhoodGraph::create(points, points_size, knn,
true /*get distances */, 6, 1);
for (int p = 0; p < points_size; p++) {
const std::vector<double> &dists = graph->getNeighborsDistances(p);
for (int k = 0; k < knn; k++)
sum_knn_distances[p] += dists[k];
}
}Мы дополнительно ограничили область видимости graph, чтобы ресурс освободился после выполнения циклов.
Больше других подводных камней, связанных с временными переменными, можно найти в третьей части книги "Путеводитель C++ программиста по неопределённому поведению".
Предупреждение PVS-Studio: V1023 A pointer without owner is added to the 'm_sources' container by the 'emplace_back' method. A memory leak will occur in case of an exception. queue_source.cpp 70
class GAPI_EXPORTS QueueInput {
std::vector<std::shared_ptr<QueueSourceBase> > m_sources;
// ....
};
QueueInput::QueueInput(const cv::GMetaArgs &args) {
for (auto &&m : args) {
m_sources.emplace_back(new cv::gapi::wip::QueueSourceBase(m));
}
}В этом коде можно увидеть опасный паттерн: вставка сырого указателя на аллоцированный ресурс в вектор умных указателей с использованием функции-члена emplace_back. Она представляет собой шаблон функции, которая осуществляет идеальную передачу своих аргументов. Получается, что тип аргумента emplace_back в нашем случае будет QueueSourceBase *.
В исключительной ситуации аллоцированный на куче объект может быть утерян. Вот как это происходит:
capacity() == size(). Это означает, что для вставки нового элемента потребуется новый буфер большего размера.std::bad_alloc.delete.Чтобы избежать ошибки, нужно обернуть сырой указатель в умный, ну и заодно можно поменять emplace_back на push_back:
m_sources.push_back(std::make_unique<cv::gapi::wip::QueueSourceBase>(m));Предупреждение PVS-Studio: V610 Undefined behavior. Check the shift operator '<<'. The right operand is negative ('(aperture_size - 1)' = [-1..2147483646]). test_filter.cpp 1397
static void
test_cornerEigenValsVecs(....)
{
int i, j;
Scalar borderValue = _borderValue;
int aperture_size = _aperture_size < 0 ? 3 : _aperture_size;
// ....
double denom = (1 << (aperture_size-1))*block_size;
// ....}
Анализатор обнаружил опасную операцию сдвига влево на отрицательное число. Согласно стандарту C++, поведение при сдвиге на отрицательное число не определено.
The shift operators << and >> group left-to-right.
shift-expression << additive-expression
shift-expression >> additive-expression
The operands shall be of integral or unscoped enumeration type and integral promotions are performed.
1. The type of the result is that of the promoted left operand. The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand.
2. The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If E1 has an unsigned type, the value of the result is E1 * 2^E2, reduced modulo one more than the maximum value representable in the result type. Otherwise, if E1 has a signed type and non-negative value, and E1*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.
3. The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.
В коде выполняется сдвиг на выражение aperture_size - 1. При инициализации переменной мы видим проверку на отрицательное значение _aperture_size, однако в ней не учитывается случай нулевого значения. А значит, если _aperture_size равна нулю, то и переменная aperture_size также будет равна нулю, а выражение aperture_size - 1 примет значение -1.
Чтобы исправить код, надо заменить знак неравенства:
int aperture_size = _aperture_size <= 0 ? 3 : _aperture_size;А чтобы узнать больше об особенностях операторов сдвига, предлагаю ознакомиться со статьёй "Не зная брода, не лезь в воду. Часть третья".
Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'heap' object. The RtlSecureZeroMemory() function should be used to erase the private data. zmaxheap.cpp 87
void zmaxheap_destroy(zmaxheap_t *heap)
{
free(heap->values);
free(heap->data);
memset(heap, 0, sizeof(zmaxheap_t));
free(heap);
}Мы видим метод, который очищает и зануляет память после использования указателя heap. Если при работе с конфиденциальными данными этого не сделать, то они останутся в памяти, что может привести к неприятным последствиям.
К сожалению, такой код может оставить буфер неочищенным. Компилятор вправе считать вызов memset избыточным, поскольку сразу после зануления объект освобождается.
Вряд ли в этой части OpenCV торчат какие-либо конфиденциальные данные. Скорее разработчик просто хотел дополнительно занулить память перед возвратом системе.
А вот если вы работаете с приватными данными, то нужно найти способ сделать это правильно. Например, в С23 была добавлена функция memset_explicit, которая позволяет указать, что область памяти должна быть очищена. Есть все шансы, что она также попадёт и в C++26 (P3348). А до этих стандартов о гарантированных способах очистки памяти можно почитать здесь.
Предупреждение PVS-Studio: V668 There is no sense in testing the 'm_file' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. grfmt_exr.cpp 153
bool ExrDecoder::readHeader()
{
bool result = false;
m_file = new InputFile( m_filename.c_str() );
if( !m_file ) // probably paranoid
return false;
// ....
}Разработчик попытался перестраховаться от ошибки выделения памяти, добавив проверку указателя m_file после вызова new. Однако используемая перегрузка оператора new никогда не вернёт nullptr, а сгенерирует исключение: или std::bad_alloc, или брошенное пользователем в конструкторе. Таким образом, проверка if (!m_file) никогда не сработает: либо память будет выделена успешно, либо выполнение прервётся исключением. Действительно, проверка вышла параноидальной :)
Для корректной обработки этой ситуации необходимо обернуть выделение памяти в блок try:
try
{
m_file = new InputFile( m_filename.c_str() );
}
catch (...)
{
return false;
}Предупреждения PVS-Studio:
V522 There might be dereferencing of a potential null pointer 'uf'. Check lines: 38, 37. unionfind.hpp 38
V522 There might be dereferencing of a potential null pointer 'uf->data'. Check lines: 41, 39. unionfind.hpp 41
static inline unionfind_t *unionfind_create(uint32_t maxid){
unionfind_t *uf = (unionfind_t*) calloc(1, sizeof(unionfind_t));
uf->maxid = maxid;
uf->data = (struct ufrec*) malloc((maxid+1) * sizeof(struct ufrec));
for (unsigned int i = 0; i <= maxid; i++) {
uf->data[i].size = 1;
uf->data[i].parent = i;
}
return uf;
}Всего в пяти строках кода скрываются сразу две возможности для неопределённого поведения. Проблема возникает из-за того, что функции calloc и malloc могут вернуть нулевой указатель при невозможности выделения памяти, а здесь отсутствует обработка на случай этой ситуации.
Чтобы избежать ошибок, необходимо добавить проверку возвращаемых значений и ранний выход из функции в случае неудачи, например так:
unionfind_t *uf = (unionfind_t*) calloc(1, sizeof(unionfind_t));
if (!uf) return NULL;
uf->data = (struct ufrec*) malloc((maxid+1) * sizeof(struct ufrec));
if (!uf->data)
{
free(uf);
return NULL;
}Предупреждение PVS-Studio: V654 The condition '_row' of loop is always true. chessboard.cpp 2172
cv::Point2f &Chessboard::Board::getCorner(int _row, int _col)
{
int _rows = int(rowCount());
int _cols = int(colCount());
if(_row >= _rows || _col >= _cols)
CV_Error(Error::StsBadArg,"out of bound");
if(_row == 0)
{
// ....
}
else
{
Cell *row_start = top_left;
int count = 1;
do
{
if(count == _row)
{
PointIter iter(row_start,BOTTOM_LEFT);
int count2 = 0;
do
{
if(count2 == _col)
return *(*iter);
++count2;
}while(iter.right());
}
++count;
row_start = row_start->bottom;
}while(_row); // <=
}
CV_Error(Error::StsInternal,"cannot find corner");
// return *top_left->top_left; // never reached
}Анализатор обнаружил цикл с всегда истинным условием while (_row). Цикл находится в else-ветке, и в этом контексте _row != 0. При этом в теле цикла переменная не изменяется, что делает условие выхода недостижимым. Если что-то пойдёт не так, то сообщение об ошибке не будет выдано, ведь цикл окажется бесконечным. Предложить решение для этой ошибки я не могу, поэтому оставляю это разработчикам.
Предупреждение PVS-Studio: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 20, 187. any.hpp 187
#if defined(_MSC_VER)
// disable MSVC warning on "multiple copy constructors specified"
# pragma warning(disable: 4521)
#endif
// ....
#if defined(_MSC_VER)
// Enable "multiple copy constructors specified" back
# pragma warning(default: 4521)
#endifПрограммисты зачастую думают, что после директивы #pragma warning(default: X) опять начнут действовать предупреждения, отключённые ранее с помощью pragma warning(disable: X), но это не так. Эта директива устанавливает состояние по умолчанию, а оно могло отличаться от того, которое было до отключения.
Чтобы вернуть прежнее состояние, следует воспользоваться директивами #pragma warning(push) и #pragma warning(pop). Исправленный код будет выглядеть следующим образом:
#if defined(_MSC_VER)
// disable MSVC warning on "multiple copy constructors specified"
# pragma warning(push)
# pragma warning(disable: 4521)
#endif
// ....
#if defined(_MSC_VER)
// Enable "multiple copy constructors specified" back
# pragma warning(pop)
#endifПредупреждение PVS-Studio: V785 Constant expression in switch statement. approx.cpp 782
CV_IMPL CvSeq*
cvApproxPoly( const void* array, int header_size,
CvMemStorage* storage, int method,
double parameter, int parameter2 )
{
// ....
if( method != CV_POLY_APPROX_DP )
CV_Error( cv::Error::StsOutOfRange, "Unknown approximation method" );
while( src_seq != 0 )
{
CvSeq *contour = 0;
switch (method)
{
case CV_POLY_APPROX_DP:
// ....
default:
CV_Error( cv::Error::StsBadArg, "Invalid approximation method" );
}
// ....
}
return dst_seq;
}В этом фрагменте можно увидеть избыточный код: переменная method сначала проверяется в условии if, где допустимым является только значение CV_POLY_APPROX_DP (в ином случае вызывается noreturn-функция cv::error внутри макроса CV_Error). Затем она попадает в конструкцию switch, создающую видимость множественного выбора.
Фактически свитч теперь бессмысленен: ветка CV_POLY_APPROX_DP будет выполняться всегда, а код внутри default становится недостижимым, потому что проверка ранее уже отсекла все другие значения.
Возможно, разработчик планировал добавить другие методы, но не реализовал их. Или код остался от рефакторинга, когда убрали другие варианты.
Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 1e-2. test_fundam.cpp 626
double CV_RodriguesTest::get_success_error_level( int /*test_case_idx*/,
int /*i*/, int j )
{
return j == 4 ? 1e-2 : 1e-2;
}Следующие три фрагмента — классический пример ошибки из-за copy-paste. Здесь тернарный оператор проверяет условие j == 4, но всегда возвращает одинаковое значение 1e-2. Скорее всего, забыли изменить код после копирования.
Мы уже очень много раз находили разные варианты опечаток и описывали их в разных статьях. Предлагаю посмотреть одну из них: "Распространённые паттерны опечаток при программировании".
Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 629, 643. nary_eltwise_layers.cpp 629
void ternary_forward_impl(....)
{
// ....
if (nplanes == 1) { // parallelize within the plane
const T *ptr1 = (const T*)data1;
const T *ptr2 = (const T*)data2;
const T *ptr3 = (const T*)data3;
T* ptr = (T*)data;
auto worker = [&](const Range &r) {
if (dp1 == 1 && dp2 == 1 && dp3 == 1 && dp == 1) { // <=
for (int i = r.start; i < r.end; i++) {
ptr[i] = op(ptr1[i], ptr2[i], ptr3[i]);
}
} else if (dp1 == 0 && dp2 == 1 && dp3 == 1 && dp == 1){
T x1 = *ptr1;
for (int i = r.start; i < r.end; i++) {
ptr[i] = op(x1, ptr2[i], ptr3[i]);
}
} else if (dp1 == 1 && dp2 == 0 && dp3 == 1 && dp == 1){
T x2 = *ptr2;
for (int i = r.start; i < r.end; i++) {
ptr[i] = op(ptr1[i], x2, ptr3[i]);
}
} else if (dp1 == 1 && dp2 == 1 && dp3 == 1 && dp == 1) { // <=
T x3 = *ptr3;
for (int i = r.start; i < r.end; i++) {
ptr[i] = op(ptr1[i], ptr2[i], x3);
}
} else {
for(int i = r.start; i < r.end;
i++, ptr1 += dp1, ptr2 += dp2, ptr3 += dp3, ptr += dp) {
*ptr = op(*ptr1, *ptr2, *ptr3);
}
}
};
// ....
}Огромный фрагмент кода, который не захочет ревьюить ни один программист, но анализатору несложно просмотреть его полностью и найти опечатку.
Здесь PVS-Studio нашёл два одинаковых условия. Вероятно, разработчику нужно было перебрать все варианты, но в последнем он забыл изменить одну проверку. Корректное условие, скорее всего, должно быть таким:
} else if (dp1 == 1 && dp2 == 1 && dp3 == 0 && dp == 1) {А ещё больше ошибок с несчастливыми числами, которые часто допускают разработчики, можно найти в статье "Ноль, один, два, Фредди заберёт тебя".
Предупреждение PVS-Studio: V678 An object is used as an argument to its own method. Consider checking the first actual argument of the 'copyTo' function. ocl_test.cpp 107
double TestUtils::checkRectSimilarity(const Size & sz,
std::vector<Rect>& ob1, std::vector<Rect>& ob2)
{
double final_test_result = 0.0;
size_t sz1 = ob1.size();
size_t sz2 = ob2.size();
if (sz1 != sz2)
return sz1 > sz2 ? (double)(sz1 - sz2) : (double)(sz2 - sz1);
else
{
if (sz1 == 0 && sz2 == 0)
return 0;
cv::Mat cpu_result(sz, CV_8UC1);
cpu_result.setTo(0);
for (vector<Rect>::const_iterator r = ob1.begin(); r != ob1.end(); ++r)
{
cv::Mat cpu_result_roi(cpu_result, *r);
cpu_result_roi.setTo(1);
cpu_result.copyTo(cpu_result); // <=
}
// ....
}
}В этом фрагменте можно увидеть попытку копирования матрицы в себя же. Хотя метод copyTo содержит ранний выход при совпадении матриц, перед этой проверкой выполняется значительный объем кода: создание матрицы, проверки типов и размеров, инициализация итераторов.
Эта операция создаёт избыточную нагрузку на систему и, возможно, тоже является опечаткой. Может быть, разработчик хотел применить копирование матрицы cpu_result_roi.
Предупреждение PVS-Studio: V549 The first argument of 'min' function is equal to the second argument. test_goodfeaturetotrack.cpp 512
class CV_GoodFeatureToTTest : public cvtest::ArrayTest
{
protected:
std::vector<float> cornersQuality;
std::vector<float> RefcornersQuality;
// ....
};
int CV_GoodFeatureToTTest::validate_test_results( int test_case_idx )
{
// ....
if (e > eps)
{
EXPECT_LE(e, eps); // never true
ts->set_failed_test_info(cvtest::TS::FAIL_BAD_ACCURACY);
for (int i = 0;
i < (int)std::min((unsigned int)(cornersQuality.size()),
(unsigned int)(cornersQuality.size()));
i++)
{
if ( std::abs(cornersQuality[i] - RefcornersQuality[i])
> eps * std::max(cornersQuality[i], RefcornersQuality[i]))
printf("i = %i Quality %2.6f Quality ref %2.6f\n",
i, cornersQuality[i], RefcornersQuality[i]);
}
}
return BaseTest::validate_test_results(test_case_idx);
}Здесь функция min сравнивает один и тот же аргумент (unsigned int)(cornersQuality.size()). Это не только бессмысленно, но и опасно. Может произойти выход за границу массива RefcornersQuality, если его размер меньше массива cornersQuality.
Возможно, опечатка произошла из-за громоздкого преобразования, которое тяжело воспринимать. Однако есть простое решение: использовать size_t и убрать лишние преобразования, оставив только сравнение размеров массивов.
for (size_t i = 0; i < std::min(cornersQuality.size(),
RefcornersQuality.size()); i++)Этот фрагмент кода мы нашли в тестах. Да, тесты не застрахованы от багов и их тоже необходимо проверять. Кстати, этой теме мы даже посвятили отдельную статью: "Поиск ошибок в юнит-тестах".
Статический анализ выявляет скрытые дефекты даже в больших работающих проектах. А регулярное использование позволяет находить ошибки на ранних этапах, повышая надёжность кода и снижая затраты на отладку. Проверьте свой проект с помощью PVS-Studio и избавьте его от проблем уже сейчас.
0
0
0
77
0
0