# Fixing bugs in your AI: let's analyze bugs in OpenVINO

Fellow developers, we invite you to continue our exciting journey through the depths of Intel OpenVINO code\! Equipped with a static analyzer, just like detectives, we'll uncover the most insidious and interesting bugs, and reveal the secrets of hidden errors and tricky typos left outside the first part of the article\.

![1145_OpenVINO2/image1.png](https://import.viva64.com/docx/blog/1145_OpenVINO2/image1.png)

## Introduction

So, in the [first part](https://pvs-studio.com/en/blog/posts/cpp/1137/) we've looked at many enlightening typos in the code of the OpenVINO project\. They are enlightening because typos will always happen in a developer's life—you can't escape them—but the issue can be resolved\. Using the static analysis technology that alerts us to such errors, we can create certain statistics about where a programmer makes these mistakes more often\. Once these patterns are identified, fighting typos becomes much more effective\. And if you're wondering where the most common typos are, we have an interesting article worth [reading](https://pvs-studio.com/en/blog/posts/cpp/1064/)\.

However, imagine if, in the blink of an eye we could fully focus on code without getting distracted\. Then our code would be perfect, and typos would disappear completely\. Let's keep on dreaming\.\.\. If we only could foresee all possible scenarios of code usage, consider all interconnections between its components, and anticipate every error\.\.\. The software we developed would've worked perfectly\. However, I know only one "programming language" where all this is possible, and that's HTML\.

We're still talking about C\+\+, though\. When dealing with it, even the most experienced programmer can make a mistake, hmm\.\.\. well, really, anywhere and everywhere\. So, now that we've divided all the warnings into "before" and "after" and dealt with typos, let's look at other, no less interesting and diverse errors\.

<details>
   <summary>Disclaimer</summary>

The article purpose isn't to devalue the work of programmers involved in the development of this product\. Its purpose is to popularize static code analyzers that are useful even for high\-quality and established projects\. Moreover, we offer free licenses for open\-source projects \(and not just for those\)\. You can learn more [here](https://pvs-studio.com/en/order/open-source-license/)\.


</details>
## Check results

In fact, the analyzer didn't find that many errors\. All the interesting and important errors fit into just a couple of articles\. The code is quite beautiful and secure\. The commit I was using to build and test the project is still the same: [2d8ac08](https://github.com/openvinotoolkit/openvino/tree/2d8ac08bf1f87f8ac455eae381213b52e781fe8c)\.

Well, now we're done with the formalities, so let's get back to looking at the errors\. As expected, the bug complexity increases, and the intricacy of the code we have to figure out increases as well\. Some parts of the code are quite strange\. It's not clear how or why it's written that way, but I suggest you start with some simple things, and then\.\.\. Well, you'll see soon enough\.

### From simple to complex\. Not quite clear code

**Fragment N1**

```cpp
template <typename T>
void ROIAlignForward(....) 
{
  int64_t roi_cols = 4;       // <=

  int64_t n_rois = nthreads / channels / pooled_width / pooled_height;
  // (n, c, ph, pw) is an element in the pooled output
  for (int64_t n = 0; n < n_rois; ++n) 
  {
    ....
    if (roi_cols == 5)        // <=
    {
      roi_batch_ind = static_cast<int64_t>(offset_bottom_rois[0]);
      offset_bottom_rois++;
    }
    ....
  }
....
}
```

The analyzer warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'roi\_cols \=\= 5' is always false\. [experimental\_detectron\_roi\_feature\_extractor\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/core/reference/src/op/experimental_detectron_roi_feature_extractor.cpp#L201-L211) 211

Checking _if \(roi\_cols \=\= 5\)_ really always returns _false_, and the code in the body is unreachable\. This is because the value of the _roi\_cols_ variable doesn't change in any way between when it's declared and when it's checked in the condition\.

**Fragment N2**

```cpp
bool is_valid_model(std::istream& model) 
{
  // the model usually starts with a 0x08 byte
  // indicating the ir_version value
  // so this checker expects at least 3 valid ONNX keys
  // to be found in the validated model
  const size_t EXPECTED_FIELDS_FOUND = 3u;
  std::unordered_set<::onnx::Field, std::hash<int>> onnx_fields_found = {};
  try 
  {
    while (!model.eof() && onnx_fields_found.size() <    // <=
           EXPECTED_FIELDS_FOUND                      ) 
    {
      const auto field = ::onnx::decode_next_field(model);

      if (onnx_fields_found.count(field.first) > 0) 
      {
        // if the same field is found twice, this is not a valid ONNX model
        return false;
      }
      else
      {
        onnx_fields_found.insert(field.first);
        ::onnx::skip_payload(model, field.second);
      }
    }

      return onnx_fields_found.size() == EXPECTED_FIELDS_FOUND;
  }
  catch (...)
  {
    return false;
  }
}
```

The analyzer warning: [V663](https://pvs-studio.com/en/docs/warnings/v663/) Infinite loop is possible\. The 'cin\.eof\(\)' condition is insufficient to break from the loop\. Consider adding the 'cin\.fail\(\)' function call to the conditional expression\. [onnx\_model\_validator\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/frontends/onnx/onnx_common/src/onnx_model_validator.cpp#L168) 168

The analyzer detected a rather interesting and rare bug that can cause a loop to become infinite\.

When working with objects of the _std::istream_ class, the _\!model\.eof\(\)_ check may not be enough to exit a _while_ loop\. If a failure occurs while reading data, all subsequent calls to the [_eof_](https://en.cppreference.com/w/cpp/io/basic_ios/eof#Notes) function return only _false_, which may result in an infinite loop\.

To avoid this issue, we can use the overloaded [operator _bool_](https://en.cppreference.com/w/cpp/io/basic_ios/operator_bool) in the loop condition as follows: 

```cpp
while (model && onnx_fields_found.size() < EXPECTED_FIELDS_FOUND) 
{
  ....
}
```

**Fragment N3**

```cpp
NamedOutputs pad3d(const NodeContext& node) 
{
  ....
  // padding of type int feature only supported by paddle 'develop'
  // version(>=2.1.0)
  if (node.has_attribute("paddings"))                            // <=
  {
    auto paddings_vector = node.get_attribute<
                             std::vector<int32_t>
                           >("paddings");
    PADDLE_OP_CHECK(node, paddings_vector.size() == 6, 
                    "paddings Params size should be 6 in pad3d!");
    paddings = paddings_vector;
  
  } 
  else if (node.has_attribute("paddings"))                       // <=
  {
    auto padding_int = node.get_attribute<int32_t>("paddings");
    for (int i = 0; i < 6; i++)
      paddings[i] = padding_int;
  } 
  else 
  {
    PADDLE_OP_CHECK(node, false, "Unsupported paddings attribute!");
  }
  ....
}
```

The analyzer warning: [V517](https://pvs-studio.com/en/docs/warnings/v517/) \[CERT\-MSC01\-C\] The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 22, 26\. [pad3d\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/frontends/paddle/src/op/pad3d.cpp#L22-L26) 22

The first and second conditions of the _if_ constructs are the same, so the code in the _then_\-branch of the second _if_ is always unreachable\. You may notice that the branches have different logic: the first one tries to read the _paddings_ attribute as a vector of the _int32\_t_ \(_paddings\_vector_\) number type, while the second one tries to read the same attribute as a number of _int32\_t type_ \(_padding\_int_\)\.

It's hard to define what exactly the correct code should look like in this case\. However, let's take a guess\. The code is in the [OpenVINO Paddle Frontend](https://github.com/openvinotoolkit/openvino/tree/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/frontends/paddle) module, which parses the model generated by the [PaddlePaddle](https://github.com/PaddlePaddle/Paddle) framework\. If we search for the 'pad3d' name in the project, we can find the following [description](https://github.com/PaddlePaddle/Paddle/blob/cdd1f1641cc7c6ee4112f7be6f5472040ecaf33f/python/paddle/nn/layer/common.py#L1462-L1481):

```cpp
Parameters:
  padding (Tensor|list[int]|tuple[int]|int): The padding size with data type
            ``'int'``. If is ``'int'``, use the same padding in all dimensions.
            Else [len(padding)/2] dimensions of input will be padded.
            The pad has the form (pad_left, pad_right, pad_top,
                                  pad_bottom, pad_front, pad_back).
```

This suggests that _padding_ is an option, and we need to visit two interesting alternatives: _std::vector<int32\_t\>_ and _int32\_t_\. We can do this in the following way:

```cpp
auto paddings = std::vector<int32_t>(6, 0);

if (node.has_attribute("paddings"))
{
  auto paddings_attr = node.get_attribute_as_any("paddings");
  if (paddings_attr.is<std::vector<int32_t>>())
  {
    auto paddings_vector = paddings_attr.as<std::vector<int32_t>>();
    PADDLE_OP_CHECK(node, paddings_vector.size() == 6,
                    "paddings Params size should be 6 in pad3d!");
    paddings = std::move(paddings_vector);
  }
  else if (paddings_attr.is<int32_t>())
  {
    auto padding_int = paddings_attr.as<int32_t>();
    if (padding_int != 0)
    {
      std::fill(paddings.begin(), paddings.end(), padding_int);
    }
  }
}
```

Also, while looking at the PaddlePaddle sources, I got an idea that there's a typo in the attribute\. So, it should be called _padding_, not _paddings_\. But I'm not completely sure :\) In any case, I recommend developers to pay attention to this code\.

**Fragment N4**

```cpp
template <typename T>
std::basic_string<T> get_model_extension() {}
```

The analyzer warning: [V591](https://pvs-studio.com/en/docs/warnings/v591/) \[CERT\-MSC52\-CPP\] Non\-void function should return a value\. [graph\_iterator\_flatbuffer\.hpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/frontends/tensorflow_lite/src/graph_iterator_flatbuffer.hpp#L29) 29 

As we can see, the function has an empty body and returns nothing, even though the return type is specified as _std::basic\_string<T\>_\. Well, hello, undefined behavior\.

This code fragment looks like it came straight from The X\-Files series, but it's actually quite simple\. If we jump a few lines down, we can see the specializations of this function template:

```cpp
template <>
std::basic_string<char> get_model_extension<char>();

#if defined(OPENVINO_ENABLE_UNICODE_PATH_SUPPORT) \
 && defined(_WIN32)

template <>
std::basic_string<wchar_t> get_model_extension<wchar_t>();

#endif
```

And these specializations have specific [bodies](https://github.com/openvinotoolkit/openvino/blob/8c58aa1ee6096806a30222b37e9f6f478eef5595/src/frontends/tensorflow_lite/src/graph_iterator_flatbuffer.cpp#L118-L128) that return specific objects:

```cpp
template <>
std::basic_string<char>
ov::frontend::tensorflow_lite::get_model_extension<char>()
{
  return ::tflite::ModelExtension();
}

#if defined(OPENVINO_ENABLE_UNICODE_PATH_SUPPORT) \
 && defined(_WIN32)

template <>
std::basic_string<wchar_t> ov::frontend::
                            tensorflow_lite::get_model_extension<wchar_t>()
{
  return util::string_to_wstring(::tflite::ModelExtension());
}

#endif
```

So, the program works properly with specializations of _char_ and _wchar\_t_ but does whatever it wants with others\. And there are [three](https://en.cppreference.com/w/cpp/language/types#Character_types) more: _char8\_t_, _char16\_t_, _char32\_t_\.

Yes, the code may not use these template specializations, but we're all for safe and secure code\. And we'd like the compiler to stop us at the compilation stage when we're dealing with such code\. It's very easy to do, we just need to turn the function template definition into a declaration:

```cpp
template <typename T>
std::basic_string<T> get_model_extension();
```

Now, when we try to call a specialization of _char8\_t_, _char16\_t_, or _char32\_t_, the compiler throws an error because it can't do the required instantiation without a body\.

Here are a few more such warnings:

* V591 \[CERT\-MSC52\-CPP\] Non\-void function should return a value\. graph\_iterator\_meta\.hpp 18
* V591 \[CERT\-MSC52\-CPP\] Non\-void function should return a value\. graph\_iterator\_saved\_model\.hpp 19
* V591 \[CERT\-MSC52\-CPP\] Non\-void function should return a value\. graph\_iterator\_saved\_model\.hpp 21

**Fragment N5**

```cpp
template <typename TReg>
int getFree(int requestedIdx)
{
  if (std::is_base_of<Xbyak::Mmx, TReg>::value)
  {
    auto idx = simdSet.getUnused(requestedIdx);
    simdSet.setAsUsed(idx);
    return idx;
  }
  else if (   std::is_same<TReg, Xbyak::Reg8>::value
           || std::is_same<TReg, Xbyak::Reg16>::value
           || std::is_same<TReg, Xbyak::Reg32>::value
           || std::is_same<TReg, Xbyak::Reg64>::value)
  {
    auto idx = generalSet.getUnused(requestedIdx);
    generalSet.setAsUsed(idx);
    return idx;
  }
  else if (std::is_same<TReg, Xbyak::Opmask>::value)
  {
    return getFreeOpmask(requestedIdx);
  }
}
```

The analyzer warning: [V591](https://pvs-studio.com/en/docs/warnings/v591/) \[CERT\-MSC52\-CPP\] Non\-void function should return a value\. [registers\_pool\.hpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/plugins/intel_cpu/src/nodes/kernels/x64/registers_pool.hpp#L229) 229

Here's another snippet with a function that doesn't return a value in all of the execution paths\. If you think that nothing will happen to your program if you forget _return_ in a function, you're wrong\. Even if the return type is built\-in, the code contains undefined behavior\. I also recommend reading this [article](https://pvs-studio.com/en/blog/posts/cpp/0917/) about the cruel joke the compiler can play on you in this situation\.

Let's go back to our example\. Starting with C\+\+17, we can enhance this code\. Firstly, as you can see, conditions are compile\-time expressions\. So, let's use the [_if constexpr_](https://en.cppreference.com/w/cpp/language/if#Constexpr_if) construct\. It discards compiling the code in the branches where the condition is _false_\. Secondly, we can use _static\_assert_ to protect the code from specializations for which no value is returned in the current code:

```cpp
template <typename TReg>
int getFree(int requestedIdx)
{
  if constexpr (std::is_base_of<Xbyak::Mmx, TReg>::value) { .... }
  ....
  else
  {
    // until C++23
    static_assert(sizeof(TReg *) == 0, "Your message.");

    // since C++23
    static_assert(false, "Your message."); 
  }
}
```

The fixed example provides two ways to write _static\_assert_, depending on the version of the standard you are using\. This is because prior to C\+\+23, the _static\_assert\(false, "\.\.\."\)_ option in a function template always [resulted](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2593r0.html) in a compile\-time error\.

If you work with versions prior to C\+\+17, you can fix the code by adding overloads to the _getFree_ function template and using [_std::enable\_if_](https://en.cppreference.com/w/cpp/types/enable_if):

```cpp
template <typename TReg,
          std::enable_if_t< std::is_base_of<Xbyak::Mmx, TReg>::value,
                            int > = 0>
int getFree(int requestedIdx)
{
  auto idx = simdSet.getUnused(requestedIdx);
  simdSet.setAsUsed(idx);
  return idx;
}

template <typename TReg,
          std::enable_if_t< std::is_same<TReg, Xbyak::Reg8>::value
                         || std::is_same<TReg, Xbyak::Reg16>::value
                         || std::is_same<TReg, Xbyak::Reg32>::value
                         || std::is_same<TReg, Xbyak::Reg64>::value,
                            int > = 0>
int getFree(int requestedIdx)
{
  auto idx = generalSet.getUnused(requestedIdx);
  generalSet.setAsUsed(idx);
  return idx;
}

template <typename TReg,
          std::enable_if_t< std::is_same<TReg, Xbyak::Opmask>::value,
                            int > = 0>
int getFree(int requestedIdx)
{
  return getFreeOpmask(requestedIdx);
}
```

**Fragment N6**

```cpp
template <>
void RandomUniform<x64::avx512_core>::initVectors()
{
  ....
  if (m_jcp.out_data_type.size() <= 4)
  {
    static const uint64_t n_inc_arr[8]  = { 0, 1, 2, 3, 4, 5, 6, 7 };
    mov(r64_aux, reinterpret_cast<uintptr_t>(n_inc_arr));
  }
  else
  {
    static const uint64_t n_inc_arr[8]  = 
                                    { 0, 1, 2, 3, 4, 5, 6, 7 }; // TODO: i64
    mov(r64_aux, reinterpret_cast<uintptr_t>(n_inc_arr));
  }
  ....
}
```

The analyzer warning: [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. [random\_uniform\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/plugins/intel_cpu/src/nodes/kernels/x64/random_uniform.cpp#L120) 120

This snippet is simple but unclear\. The same code is contained in the body of both the _if_ and _else_ branches\. Perhaps this is a copy\-paste error, and developers need to pay attention to this code fragment\.

**Fragment N7**

```cpp
inline ParseResult parse_xml(const char* file_path) 
{
  ....
  try
  {
    auto xml = std::unique_ptr<pugi::
                          xml_document>{new pugi::xml_document{}};
    const auto error_msg = [&]() -> std::string {....}();
    ....

    return {std::move(xml), error_msg};
  }
  catch (std::exception& e) 
  {
    return {std::move(nullptr),               // <=
            std::string("Error loading XML file: ") + e.what()};
  }
}
```

The analyzer warning: [V575](https://pvs-studio.com/en/docs/warnings/v575/) \[CERT\-EXP37\-C\] The null pointer is passed into 'move' function\. Inspect the first argument\. [xml\_parse\_utils\.hpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/common/util/include/openvino/util/xml_parse_utils.hpp#L249) 249

To be honest, we've never seen such an interesting code fragment before: _nullptr_ passed to the _std::move_ function\. It's not prohibited to pass _nullptr_ to _std::move_, the function just converts it to _std::nullptr\_t &&_\. However, it's unclear why this was done\.

To better understand what's going on, let's take a look at _ParseResult_:

```cpp
struct ParseResult 
{
  ParseResult(std::unique_ptr<pugi::xml_document>&& xml, std::string error_msg)
        : xml(std::move(xml)),
          error_msg(std::move(error_msg)) {}

  std::unique_ptr<pugi::xml_document> xml;

  std::string error_msg{};
};
```

Let's play detectives and solve the mystery of where such code could've come from\. Most likely, a programmer wrote _return_ in the _try_\-block first: the _ParseResult_ object is constructed there by moving the _xml_ smart pointer and copying _error\_msg_\. Then they handled the situation when an exception would be thrown\. In this case, an object of the _ParseResult_ type should also be returned\. To make their life easier, they copied the previous _return_ and slightly modified the constructor arguments\. When they saw the _xml_ smart pointer moving, they decided that they needed to move something here as well\. The _nullptr_ pointer, for example\.

However, there's no need for that\. According to C\+\+ rules, when you choose an overload of a constructor, the compiler should perform some [implicit conversions](https://timsong-cpp.github.io/cppwp/n4950/over.best.ics#over.ics.ref-2) on the passed arguments\. For example, the _rvalue_ reference to _std::unique\_ptr<pugi::xml\_document\>_ can't be bound to the _rvalue_ reference to _std::nullptr\_t_\. So, the compiler creates a temporary object of the _std::unique\_ptr<pugi::xml\_document\>_ type by calling the appropriate [converting constructor](https://timsong-cpp.github.io/cppwp/n4950/unique.ptr.single.ctor#1)\. Only then a reference \(constructor parameter\) is bound to this temporary object\.

If we omit _std::move_ and pass _nullptr_ to the constructor, the code also [compiles](https://godbolt.org/z/GdbYTcKMx) and becomes more readable:

```cpp
return { nullptr, std::string("Error loading XML file: ") +
         e.what() };
```

### Null pointers and potential memory leaks

And here we seamlessly move on to working with pointers\. And the analyzer helps understand the cases where things went wrong, and how one shouldn't code in general\.

**Fragment N8**

```cpp
void GraphOptimizer::FuseFCAndWeightsDecompression(Graph &graph) 
{
  ....

  // Fusion processing
  auto *multiplyInputNode = dynamic_cast<node::
                                         Input *>(multiplyConstNode.get());
  if (!multiplyInputNode)
  {
    OPENVINO_THROW("Cannot cast ", multiplyInputNode->getName(),   // <=
                   " to Input node.");
  }
  fcNode->fuseDecompressionMultiply(multiplyInputNode->getMemoryPtr());

  if (withSubtract)
  {
    auto *subtractInputNode = dynamic_cast<node::
                                           Input *>(subtractConstNode.get());
    if (!subtractInputNode)
    {
      OPENVINO_THROW("Cannot cast ", subtractInputNode->getName(), // <=
                     " to Input node.");
    }
    fcNode->fuseDecompressionSubtract(subtractInputNode->getMemoryPtr());
  }
  if (withPowerStatic)
  {
    auto *eltwiseNode = dynamic_cast<node::
                                     Eltwise *>(powerStaticNode.get());
    if (!eltwiseNode) 
    {
      OPENVINO_THROW("Cannot cast ", eltwiseNode->getName(),       // <=
                     " to Eltwise node.");
    }
  }
  ....
}
```

The analyzer warnings: 

* V522 Dereferencing of the null pointer 'multiplyInputNode' might take place\. [graph\_optimizer\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/plugins/intel_cpu/src/graph_optimizer.cpp#L451) 452
* V522 Dereferencing of the null pointer 'subtractInputNode' might take place\. [graph\_optimizer\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/plugins/intel_cpu/src/graph_optimizer.cpp#L459) 459
* V522 Dereferencing of the null pointer 'eltwiseNode' might take place\. [graph\_optimizer\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/plugins/intel_cpu/src/graph_optimizer.cpp#L466) 466

I often see such errors in various projects, so I decided to pay attention to them\. The thing is that programmers rarely test error handlers :\)

Let's imagine for a moment that the _dynamic\_cast_ result returns a null pointer\. Then, when an exception is thrown, the same null pointer is used in an attempt to call the _getName_ function\. We get undefined behavior that can cause the exception handling to turn into a critical error for the program\. 

**Fragment N9**

```cpp
void Defer(Task task) 
{
  auto &stream = *(_streams.local());      // <=
  stream._taskQueue.push(std::move(task));
  if (!stream._execute) 
  {
    stream._execute = true;
    try
    {
      while (!stream._taskQueue.empty())
      {
        Execute(stream._taskQueue.front(), stream);
        stream._taskQueue.pop();
      }
    }
    catch (...)
    {
    }

    stream._execute = false;
  }
}
```

The analyzer warning: [V758](https://pvs-studio.com/en/docs/warnings/v758/) The 'stream' reference becomes invalid when smart pointer returned by a function is destroyed\. [cpu\_streams\_executor\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/inference/src/dev/threading/cpu_streams_executor.cpp#L410) 410

In fact, this example is fine and the reference isn't "dangling"\. This happens because the _shared\_ptr_ that the [_local_](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/inference/src/dev/threading/cpu_streams_executor.cpp#L257) function returns is [pre\-stored](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/inference/src/dev/threading/cpu_streams_executor.cpp#L297) in the _\_stream\_map_ container, which has a longer lifetime than the reference:

```cpp
class CustomThreadLocal : public ThreadLocal<std::shared_ptr<Stream>>
{
  ....
public:
  std::shared_ptr<Stream> local()
  {
    ....
    if (stream == nullptr)
    {
      stream = std::make_shared<Impl::Stream>(_impl);
    }

    auto tracker_ptr = std::make_shared<
                         CustomThreadLocal::ThreadTracker
                       >(id);
    t_stream_count_map[(void*)this] = tracker_ptr;
    auto new_tracker_ptr = tracker_ptr->fetch();
    _stream_map[new_tracker_ptr] = stream;                // <=
    return stream;
  }
private:
  ....
  std::map<std::shared_ptr<CustomThreadLocal::ThreadTracker>,
           std::shared_ptr<Impl::Stream>> _stream_map;
  ....
};
```

However, in the analyzer's defense, it's better to store the result of the _local_ function in a variable in the _Defer_ function\. Let's imagine that the code has changed a bit\. The following things begin to happen:

1. The _local_ function now returns _shared\_ptr_ with a reference count of 1\.
1. The _Defer_ function code hasn't changed\.
1. The _stream_ declaration evaluation begins\. The _local_ function is called, it returns a temporary _shared\_ptr_ with a reference count of 1\. The _stream_ reference is then bound to the object under the smart pointer\.
1. Once the declaration is fully evaluated, the destructor of the temporary smart pointer is called\. The reference count becomes 0 and the object under the smart pointer is destroyed\.
1. The _stream_ reference becomes dangling\. Its use results in undefined behavior\.

By storing the result in the variable, we extend the lifetime of the object before it leaves scope\. So, we eliminate a potential issue:

```cpp
void Defer(Task task) 
{
  auto local = _streams.local();
  auto &stream = *local;
  ....
}
```

**Fragment N10**

```cpp
~DIR()
{
  if (!next)
    delete next;
  next = nullptr;
  FindClose(hFind);
}
```

The analyzer warning: [V575](https://pvs-studio.com/en/docs/warnings/v575/) \[CERT\-EXP37\-C\] The null pointer is passed into 'operator delete'\. Inspect the argument\. [w\_dirent\.h](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/samples/cpp/common/utils/include/samples/os/windows/w_dirent.h#L92-L96) 94

When you look at this code, you may not be able to see at first what the error is\. The thing is, the memory at the _next_ pointer isn't freed because the check was written incorrectly\. The analyzer indirectly informs about it when it sees a null pointer being passed to the _operator delete_\. Also, there's no point in checking the pointer at all because the operator _delete_ [correctly](https://timsong-cpp.github.io/cppwp/n4950/expr.delete#2) handles null pointers\.

Here's a code example that is almost exactly the same as the one my colleague gave in the article: "[Simple, yet easy\-to\-miss errors in code](https://pvs-studio.com/en/blog/posts/cpp/1068/)"\. I recommend you to take a look at it\. If you've read the article before and thought that the code given there was synthetic and couldn't happen in real life, here's a direct proof for you :\)

The fixed code:

```cpp
~DIR()
{
  delete next;
  next = nullptr;
  FindClose(hFind);
}
```

**Fragment N11**

```cpp
void ov_available_devices_free(ov_available_devices_t* devices) 
{
  if (!devices) 
  {
    return;
  }
  for (size_t i = 0; i < devices->size; i++) 
  {
    if (devices->devices[i]) 
    {
      delete[] devices->devices[i];
    }
  }
  if (devices->devices)
    delete[] devices->devices;
  devices->devices = nullptr;
  devices->size = 0;
}
```

The analyzer warning: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'devices\-\>devices' pointer was utilized before it was verified against nullptr\. Check lines: 271, 274\. [ov\_core\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/bindings/c/src/ov_core.cpp#L265-L278) 271

First of all, let's see what the _ov\_available\_devices\_t_ type is:

```cpp
typedef struct {
    char** devices;  //!< devices' name
    size_t size;     //!< devices' number
} ov_available_devices_t;
```

As you can see from the name, the function frees an object of the _ov\_available\_devices\_t_ type passed to it\. Except the developer made one mistake in doing so\.

First, each pointer in the _devices\-\>devices_ array is freed in the loop\. It's implied that the pointer to the array is always non\-null\. The developer then doubted this, and after the loop, decided to test it before passing to the operator _delete\[\]_\. Except that he forgot about this assumption in the loop\. As a result, we have dereferencing of a potentially null pointer\.

Here's the fixed code:

```cpp
void ov_available_devices_free(ov_available_devices_t* devices) 
{
  if (!devices || !devices->devices)
  {
    return;
  }

  for (size_t i = 0; i < devices->size; i++) 
  {
    delete[] devices->devices[i];
  }

  delete[] devices->devices;
  devices->devices = nullptr;
  devices->size = 0;
}
```

As you may notice, I've deleted all pointer checks before passing them to the operator _delete\[\]_, since it knows how to handle them\.

The analyzer also detected several other similar fragments:

* V595 The 'versions\-\>versions' pointer was utilized before it was verified against nullptr\. Check lines: 339, 342\. ov\_core\.cpp 339
* V595 The 'profiling\_infos\-\>profiling\_infos' pointer was utilized before it was verified against nullptr\. Check lines: 354, 356\. ov\_infer\_request\.cpp 354

**Fragment N12**

```cpp
char* str_to_char_array(const std::string& str) 
{
  std::unique_ptr<char> _char_array(new char[str.length() + 1]); // <=
  char* char_array = _char_array.release();                      // <=
  std::copy_n(str.c_str(), str.length() + 1, char_array);
  return char_array;
}
```

As promised in the beginning—as the cherry on top—here's the code that may cause a reviewer to ask, "What are you?"\.

We have a converter function that copies _std::string_ to the dynamically allocated _char\*_ buffer\. The buffer is created using the operator _new\[\]_, whose ownership is passed to the smart pointer\. Overall, it's a pretty good strategy to wrap a raw pointer, because if something goes wrong, a smart pointer takes care of everything\.

Note, however, that the _std::unique\_ptr<char\>_ specialization of the smart pointer is used\. Its destructor frees the passed resource using the operator_ delete_\. In fact, that's what the analyzer warns us about:

[V554](https://pvs-studio.com/en/docs/warnings/v554/) Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. [ov\_core\.cpp](https://github.com/openvinotoolkit/openvino/blob/2d8ac08bf1f87f8ac455eae381213b52e781fe8c/src/bindings/c/src/ov_core.cpp#L13-L18) 14

The right thing to do in such cases is to use the _std::unique\_ptr<char\[\]\>_ specialization:

```cpp
char* str_to_char_array(const std::string& str) 
{
  std::unique_ptr<char[]> _char_array(new char[str.length() + 1]);
  ....
}
```

**P\.S\.** The reader may object that there's nothing wrong here, since the next line gives the resource ownership to the returned object\. Indeed, it does\. However, it's still a "code smell", and the issue is that other developers may want to use this smart pointer declaration elsewhere\.

Unfortunately, the multiplication process had already begun:

* V554 Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. ov\_node\.cpp 69
* V554 Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. ov\_partial\_shape\.cpp 25
* V554 Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. ov\_partial\_shape\.cpp 53
* V554 Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. ov\_partial\_shape\.cpp 70
* V554 Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. ov\_partial\_shape\.cpp 125
* V554 Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. ov\_shape\.cpp 23

If an exception\-throwing operation occurs between the resource acquisition and its passing to the raw pointer ownership, we get undefined behavior\.

**P\.P\.S\.** If the project uses the C\+\+14 standard and later, you can replace the code with the following:

```cpp
char* str_to_char_array(const std::string& str) 
{
  auto _char_array = std::make_unique<char[]>(str.length() + 1);
  char* char_array = _char_array.release();
  ....
}
```

Firstly, we eliminate the explicit use of the operator _new\[\]_ in the code, shifting everything to [_std::make\_unique_](https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique)\. Secondly, _auto_ infers the correct specialization of the smart pointer based on the initializer\.

However, such code, in addition to dynamic allocation, also fills the array with zeros\. Since the array is completely overwritten, we can save resources by not initializing it\. Since C\+\+20, the [_std::make\_unique\_for\_overwrite_](https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique) function is available for this purpose:

```cpp
char* str_to_char_array(const std::string& str) 
{
  auto _char_array = std::make_unique_for_overwrite<char[]>(
                       str.length() + 1
                     );
  char* char_array = _char_array.release();
  ....
}
```

## Conclusion

Some of the examples from the OpenVINO project really surprised me\. I think they surprised you just as much\. However, as I've written before, all the important errors that the project devs should've been paying attention to are packed into a couple of articles\. Most of them are typos, which is a common issue among programmers that can occur in almost any project\. OpenVINO is an impressive project, and having so few \(in my opinion\) serious errors only means that the code is quite well written\.

I hope you were as interested as I was to look at these code fragments and review the errors they contain\.

Of course, we've sent all the information to the developers and hopefully they'll fix the bugs in the near future\. 

And as a tradition, I recommend you to [try](https://pvs-studio.com/en/pvs-studio/try-free/) our PVS\-Studio analyzer\. We [offer](https://pvs-studio.com/en/order/open-source-license/) a free license for open\-source projects\.

Take care and have a good day\!