Computer vision for code: What PVS-Studio saw in OpenCV

• Important info
• Errors!
  • N1. Do you like playing with undefined behavior?
  • N2. Farewell, annoying temporary object!
  • N3. How much memory do you have?
  • N4. Is it UB? Yep, it's UB.
  • N5. Hello, compiler optimizations!
  • N6. Don't worry —an exception will take care of it
  • N7. Caution: Null pointer dereference in progress
  • N8. To infinity and beyond!
  • N9. More errors is less errors!
  • N10. There's only one correct answer
  • N11. I don't need your answer, I've already made up my mind
  • N12. Unlucky numbers
  • N13. Things took a 360 turn!
  • N14. The illusion of free choice
• Conclusion

What do computer vision and static analysis have in common? Both seek meaning in data. OpenCV finds images among millions of pixels, while PVS-Studio finds errors among thousands of code lines. This article delves into the source code of the largest computer vision library.

Important info

OpenCV is the world's largest open-source computer vision library, supported by the non-profit organization, Open Source Computer Vision Foundation. It offers a wide range of algorithms that cover a variety of tasks, from basic image processing to advanced object recognition and motion analysis.

PVS-Studio is a static code analyzer for detecting errors and vulnerabilities, which has been actively developed since 2008. Over the past 12 years (which is how long it has been since the last OpenCV check), we have significantly enhanced our analysis by adding more than 300 diagnostic rules and improving existing ones. Thanks to our work on fixing false positives reported by users and found when checking open-source projects, the analyzer efficiency is constantly growing.

The OpenCV project keeps evolving at a steady pace—you can see that in its 27,000 commits and roughly 120 tags added over the past 12 years. The library powers everything from object recognition and industrial automation to medical diagnostics, where it helps accurately detect anomalies in images. Because OpenCV is used in so many different areas, rigorous code review is critical to ensuring its quality and reliability.

In this article, I'd like to show how static analysis helps keep bugs out of releases and makes developers' lives easier. I won't list every warning the analyzer has issued; I'll only cover the ones I find most interesting.

Errors!

N1. Do you like playing with undefined behavior?

PVS-Studio warning: 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

Extending the std namespace violates the C++ standard and usually results in undefined behavior. The standard explicitly prohibits adding custom definitions to std, except for template specializations for user-defined types.

In the code, the regular PrintTo function is added to the std namespace, which is prohibited. The developer may have intended the function to be visible through ADL (Argument-Dependent Lookup), so that it would be automatically picked up when working with certain types. However, this approach is dangerous and needs fixing.

The code snippet is pretty tangled, and I can't think of a solution on the spot, so I'll leave it to the developers. If you're interested in learning more about how ADL works, feel free to ask in the comments section.

N2. Farewell, annoying temporary object!

PVS-Studio warning: 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];
  }
  // ....
}

Using smart pointers doesn't resolve the issues of dangling references and memory access here. Let's dig into this. The code works as follows:

• The create function creates and returns a smart pointer to the FlannNeighborhoodGraphImpl type, and its object reference count is one;
• The graph reference is created for the value of this smart pointer while the object reference count remains unchanged.
• Since the pointer is a temporary object, the reference counter will reach zero after initialization is done, releasing the managed object. Now, the reference points to a destroyed object.
• The for loop references an invalid reference.

As a result, the code that seemed correct leads to undefined behavior. Moreover, PVS-Studio isn't the only tool that detects this issue; the sanitizer does too. Here's the proof.

To fix this, we need to save the smart pointer so that the FlannNeighborhoodGraph object stays until the end of the block. For example, we can do this:

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];
  }
}

We also limited the graph visibility area so that the resource would be freed after the loops were executed.

N3. How much memory do you have?

PVS-Studio warning: 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));
  }
}

This code reveals a dangerous pattern: inserting a raw pointer to an allocated resource into a smart pointer vector using the emplace_back member function. It's a template for a function that perfectly passes its arguments. In our case, the emplace_back argument type is QueueSourceBase*.

In an exceptional situation, an object allocated on a heap may be lost. Here's how it works:

• Developers try to add a new element to the vector.
• The following precondition applies to the vector: capacity() == size(). So, a new, larger buffer is needed to add a new item.
• An error occurs when attempting to allocate memory for a new buffer, and the std::bad_alloc exception is thrown.
• The exception goes up the stack, but delete was not called for the allocated memory.

To avoid the error, the devs should've wrap the raw pointer in a smart pointer and replace emplace_back with push_back:

m_sources.push_back(std::make_unique<cv::gapi::wip::QueueSourceBase>(m));

N4. Is it UB? Yep, it's UB.

PVS-Studio warning: 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;
  // ....

}

The analyzer has detected a dangerous left-shift operation by a negative value. According to the C++ standard, shifting by a negative number leads to undefined behavior.

The variable shifts to the aperture_size - 1 expression. When initializing the variable, we see a check whether the _aperture_size value is negative, but it doesn't consider the case of a zero value. So, if the _aperture_size value is zero, then the aperture_size variable will also be zero, and the aperture_size - 1 expression will be -1.

To fix the code, we need to change the inequality sign:

int aperture_size = _aperture_size <= 0 ? 3 : _aperture_size;

To learn more about the features of shift operators, I invite you to read the article "Wade not in unknown waters. Part three".

N5. Hello, compiler optimizations!

PVS-Studio warning: 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);
}

Here's a method that clears and zeroes memory after using the heap pointer. Skipping this step when working with sensitive data means the information will stay in memory, which could cause trouble later.

Unfortunately, such code may not clear the buffer. The compiler may treat the call to memset as redundant, since the object gets freed immediately after being zeroed.

Nothing here suggests OpenCV handles confidential data in this particular part of the code. More likely, the developers simply wanted to zero memory as an extra precaution before returning it to the system.

However, when dealing with private data, it's important to do things the right way. For example, C23 introduced the memset_explicit function, which marks a memory region as one that must be cleared. There's a good chance C++26 (P3348) will adopt it as well. Until then, you can look here for methods that ensure proper memory erasure.

N6. Don't worry —an exception will take care of it

PVS-Studio warning: 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;

  // ....
}

The developers attempted to prevent a memory allocation failure by checking m_file after calling new. However, this operator new overload never returns nullptr; instead, it throws either std::bad_alloc, or an exception thrown by the user in the constructor. So, the if (!m_file) check will never trigger: either the memory allocation will succeed, or execution will stop with an exception. Indeed, the check ends up looking rather paranoid :)

To correctly handle this situation, let's wrap the memory allocation in the try block:

try
{ 
  m_file = new InputFile( m_filename.c_str() ); 
}
catch (...)
{
  return false;
}

N7. Caution: Null pointer dereference in progress

PVS-Studio warnings:

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;
}

Just five lines of code hide two possibilities for undefined behavior. Both calloc and malloc may return a null pointer when they fail to allocate memory, and nothing here handles this.

To prevent errors, we'll add a check for the return values and an early exit if allocation fails. For example:

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;
}

N8. To infinity and beyond!

PVS-Studio warning: 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
}

The analyzer has detected a loop where the while (_row) condition is always true. The loop is in the else branch, and in this context, _row != 0. However, the variable doesn't change in the loop body, which makes the exit condition unreachable. If something goes wrong, an error message will not appear because the loop will become infinite. I can't think of a fix for this bug, so I'll leave it to the developers.

N9. More errors is less errors!

PVS-Studio warning: 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

Developers often assume that #pragma warning(default: X) re-enables warnings previously disabled using #pragma warning(disable: X), but it doesn't work that way. This directive restores the default state, which may differ from the state before the warning was disabled.

To restore the warnings to their original state, use #pragma warning(push) and #pragma warning(pop). The fixed code looks like this:

#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

N10. There's only one correct answer

PVS-Studio warning: 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;
}

You can spot redundant logic in this snippet: First, the method variable is checked, where only CV_POLY_APPROX_DP is considered valid. Otherwise, the noreturn function of cv::error is called in the CV_Error macro. Then, the same variable enters switch that makes it look as if multiple options exist.

In reality, the switch no longer serves any purpose. The CV_POLY_APPROX_DP branch always runs, and the code withindefault can't be reached because the earlier check already filtered out all other values.

Maybe the developers planned to support additional methods later but never implemented them; or maybe the switch have survived the refactoring process after other branches were removed.

N11. I don't need your answer, I've already made up my mind

PVS-Studio warning: 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;
}

The following three code snippets are classic examples of copy-paste errors. Here, the ternary operator checks the j == 4 condition but always returns the same 1e-2 value. Most likely, the developers forgot to change the code after copying it.

We've encountered all kinds of typos many times before and covered them in several articles. One example worth checking out is "Common patterns of typos in programming."

N12. Unlucky numbers

PVS-Studio warning: 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);
        }
      }
    };
  // ....
}

No programmer would want to review such a huge piece of code, but an analyzer can easily check it thoroughly and find a typo.

PVS-Studio detected two identical conditions here. The developers probably needed to go through all the options but forgot to change one check in the last one. The correct condition is most likely as follows:

} else if (dp1 == 1 && dp2 == 1 && dp3 == 0 && dp == 1) {

To learn about more mistakes involving unlucky numbers that developers often make, read the article "Zero, one, two, Freddy's coming for you."

N13. Things took a 360 turn!

PVS-Studio warning: 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);          // <=
    }
    // ....
  }
}

In this fragment, we can see an attempt to copy the matrix into itself. Even though the copyTo member function has an early exit when matrices match, a lot of code runs before this check: creating the matrix, checking types and sizes, and initializing iterators.

This operation overloads the system and may also be a typo. The developers may have wanted to copy the cpu_result_roi matrix.

N14. The illusion of free choice

PVS-Studio warning: 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);
}

Here, the min function compares the same (unsigned int)(cornersQuality.size()) argument. It serves no purpose and can cause issues. An access out of bounds of the RefcornersQuality array may occur if its size is smaller than the cornersQuality array.

The typo may have occurred due to a cumbersome and difficult-to-understand conversion. However, there's a simple solution: Use size_t and remove the unnecessary conversions, leaving only the comparison of array sizes.

for (size_t i = 0; i < std::min(cornersQuality.size(), 
                                RefcornersQuality.size()); i++)

We found this code fragment in tests. Yes, tests aren't immune to bugs and need to be checked, too. By the way, we have an article on this topic, "Finding errors in unit tests."

Conclusion

Static analysis detects hidden defects even in large and actively developed projects. You can catch bugs early on by using it regularly. This makes your code more reliable and reduces debugging costs. Check your project using PVS-Studio and fix issues today.