Fixing Bugs in Your AI or Error Analysis in OpenVINO

Fellow developers, we invite you to continue our exciting journey through the depths of Intel OpenVINO code! Armed with a static analyzer like detectives, we will expose the most insidious, interesting errors and typos, as well as their hidden secrets, which remained beyond the scope of the first part of the article.

Introduction

So, in first part we have looked at many instructive typos in the code of the OpenVINO project. Instructive, because typos will always occur in the life of a developer and you can’t get away from them, but the problem is solvable. With the help of static analysis technology, which tells us about the presence of such errors, we can compile certain statistics about where a programmer most often makes such errors. When the corresponding patterns are identified, the fight against typos becomes many times more effective. And if you are interested in where typos are most often found, we have an interesting article that I recommend to you read.

However, imagine if we could turn on all our attentiveness and turn off all distractions with a snap of our fingers. Then our code would be perfect, and typos would disappear as a species. But let's dream further… If we could also foresee all possible scenarios for using the code, take into account all the relationships between components and predict every error… The programs we wrote would work perfectly. However, I know of only one “programming language” where all this is possible, and that is HTML.

But we are still dealing with C++. Here even the most experienced programmer can make a mistake, hmm… well, actually, anywhere and anyhow. And so, having divided all the warnings into “before” and “after” and having dealt with typos, let's look at other, no less interesting and diverse errors.

Test results

In general, no huge number of errors were found during the analysis. Everything interesting and important fit into just a couple of articles. The code is very beautiful and secure. Everything is the same, the commit on which I assembled the project for verification: 2d8ac08.

Well, we're done with the formalities, we can get back to analyzing the errors. As expected, the complexity of the bugs increases, and along with the complexity, the complexity of the code, which we will have to figure out, increases. In some places, the code is quite strange – it is not clear how or why it is written like that – but I suggest starting with something more or less simple, and then… Well, you will see everything for yourself soon.

From Simple to Complex. Not quite clear code

Fragment N1

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

Analyzer warning: V547 Expression 'roi_cols == 5' is always false. experimental_detectron_roi_feature_extractor.cpp 211

Examination if (roi_cols == 5) will really always return falseand the code in the body will never be reachable. This is due to the fact that the value of the variable roi_cols does not change in any way between its declaration and its check in the condition.

Fragment N2

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

Analyzer warning: 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 168

A rather interesting and rare bug has been discovered that can cause the loop to become infinite.

Working with class objects std::istreamchecks !model.eof() to exit the loop while may not be enough. If a failure occurs while reading data, all subsequent calls to the function eof will return exclusively falsewhich can lead to a loop.

To avoid this problem, you could use an overloaded operator bool in the loop condition as follows:

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

Fragment N3

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!");
  }
  ....
}

Analyzer warning: 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 22

The first and second conditions of constructions if match, so the code in then– the second branch if is always unreachable. It can be seen that the branches perform different logic: the first one tries to read the attribute paddings in the form of a vector of numbers of the type int32_t (padding_vector), the second is to read the same attribute as a number of the type int32_t (padding_int).

It's hard for me to determine what exactly the correct code should look like in this situation. However, let's try to guess. The code is located in a module OpenVINO Paddle Frontendwhich parses the model generated by the framework PaddlePaddle. If you search for the name 'pad3d' in this project, you will find the following description:

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).

From this we assume that padding – this is an option, and you need to go through two alternatives of interest – std::vector And int32_t. You can try to do this in the following way:

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, studying the PaddlePaddle source code suggested that there was a typo in the spelling of the attribute, and it should be called paddingbut not paddings. But this is not certain 🙂 In any case, I recommend that developers pay attention to this code.

Fragment N4

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

Analyzer warning: V591 [CERT-MSC52-CPP] Non-void function should return a value. graph_iterator_flatbuffer.hpp 29

As you can see, the function has an empty body and returns nothing, although the return type is specified as std::basic_stringWe have undefined behavior in the program.

This code snippet looks like it came straight out of The X-Files. But it's actually quite simple. If you look just a couple of lines below, you'll notice the specializations of this function template:

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 very specific bodieswhich return very specific objects:

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

This means that the program will work quite correctly with specializations from char And wchar_t and do whatever you want in front of others. And there may be more of them three: char8_t, char16_t, char32_t.

Yes, the code may not use such template specializations, but we are for reliable and safe code. And we would like the compiler to stop us at the compilation stage if someone writes such code. It is very easy to do this, you just need to turn the definition of the function template into a declaration:

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

Now when trying to call a specialization from char8_t, char16_t or char32_t the compiler will generate an error because without the body it will not be able to do the required instantiation.

Here are a few more similar 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

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

Analyzer warning: V591 [CERT-MSC52-CPP] Non-void function should return a value. registers_pool.hpp 229

Another fragment with a function that does not return a value on all execution paths. If you think that nothing terrible will happen to your program if you forget return in the function, then you are wrong. Even if the return type is built-in, the code contains undefined behavior. I also recommend reading this article about what a cruel joke the compiler can play on you in this situation.

Let's get back to our example. Starting with C++17, this code can be improved. First, as you can see, conditions are expressions evaluated at compile time. Therefore, you should use the construct if const. It will allow you to avoid compiling the code in those branches in which the condition is calculated as false. Secondly, with the help of static_assert you can protect your code from those specializations for which the current code does not return a value:

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 corrected example shows two ways to write static_assert depending on the version of the standard used. The fact is that before C++23 the version static_assert(false, “…”) in the function template always brought to a compile-time error.

Before C++17, you can fix the code by adding function template overloads getFree and taking advantage of std::enable_if:

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

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

Analyzer warning: V523 The 'then' statement is equivalent to the 'else' statement. random_uniform.cpp 120

Everything is simple here, but unclear. What's in the branch ifwhat in elsethe body contains the same code. This may be a copy-paste error, and developers should pay attention to this piece of code.

Fragment N7

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

Analyzer warning: V575 [CERT-EXP37-C] The null pointer is passed into 'move' function. Inspect the first argument. xml_parse_utils.hpp 249

To be honest, this is the first time in our lives that we have seen such interesting code: in the function std::move handed over nullptr. In general, in std::move it is not forbidden to transfer nullptr – it simply transforms into std::nullptr_t &&However, it is not clear why this should be done.

To better understand what is happening, let's take a look at ParseResult:

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 try to play detective and solve the riddle of where such code could have come from. Most likely, the programmer first wrote return V try-block: the object is constructed in it ParseResultmoving the smart pointer xml and copying error_msg. Then handled the situation when an exception would be thrown. In this case, you also need to return some object of the type ParseResultTo make his life easier, he copied the previous one. returnslightly modifying the constructor arguments. At the same time, seeing the movement of the smart pointer xmlhe decided that something needed to be moved here too. For example, nullptr.

However, there is no need for this. According to the rules of C++, when choosing a constructor overload, the compiler will be required to perform some implicit conversions arguments passed. For example, an r-value reference to std::unique_ptrpugi::xml\_document\ cannot be bound to an r-value reference to std::nullptr_t. Therefore, the compiler will create a temporary object of type std::unique_ptrpugi::xml\_document\calling the appropriate converting constructor. And only then will a reference (constructor parameter) be attached to this temporary object.

If we omit std::move and we will pass it on nullptr in the constructor, the code will also be compiled and will become more readable:

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

Null pointers and possible memory leaks

And now we smoothly move on to working with pointers. And the analyzer will help to understand situations in which something went wrong, and in general, how not to write code.

Fragment N8

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.");
    }
  }
  ....
}

Analyzer warnings:

• V522 Dereferencing of the null pointer 'multiplyInputNode' might take place. graph_optimizer.cpp 452

• V522 Dereferencing of the null pointer 'subtractInputNode' might take place. graph_optimizer.cpp 459

• V522 Dereferencing of the null pointer 'eltwiseNode' might take place. graph_optimizer.cpp 466

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

Let's imagine for a second that the result dynamic_cast will return a null pointer. Then when an exception is thrown, that same null pointer will be used in an attempt to call the function getName. We get undefined behavior, due to which the handling of an exceptional situation can turn into a fatal error for the program.

Fragment N9

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

Analyzer warning: V758 The 'stream' reference becomes invalid when smart pointer returned by a function is destroyed. cpu_streams_executor.cpp 410

In fact, in this example everything is fine, and the link will not be “dangling”. This happens because shared_ptrwhich the function returns localpreliminarily is saved into the container _stream_mapwhose lifetime is longer than that of the link:

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, I will speak out in defense of the analyzer: it is better to save the result of the function. local into the function variable Defer. Let's imagine that the code has changed a little. The following will start to happen:

1. Function local now returns shared_ptr with a reference count of 1.

2. Function code Defer did not change.

3. Declaration processing begins stream. A function call occurs. localit returns temporary shared_ptr with a link counter equal to 1. Next link stream binds to the object under the smart pointer.

4. Once the declaration is fully processed, the temporary smart pointer's destructor is called. The reference count is reset to 0, and the object under the smart pointer is also destroyed.

5. Link stream becomes dangling, and its use leads to undefined behavior.

By storing the result in a variable, we extend the lifetime of the object until it goes out of scope and get rid of a potential problem:

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

Fragment N10

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

Analyzer warning: V575 [CERT-EXP37-C] The null pointer is passed into 'operator delete'. Inspect the argument. w_dirent.h 94

At first glance at this code it may not be clear what exactly the error is. The thing is that the memory by pointer next will not be freed because the check was written incorrectly. The analyzer indirectly reports this by seeing a null pointer being passed to the operator delete. Moreover, there is no point in checking the pointer at all, since the operator delete correctly handles null pointers.

Here is a code example that is almost identical to the example given by my colleague in the article: “The error is so simple that programmers don't notice it.“. I recommend you read it. If you read it before and thought that the code given there is synthetic, and in real life such things cannot be found – then here is direct proof for you 🙂

Corrected code:

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

Fragment N11

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

Analyzer warning: V595 The 'devices->devices' pointer was utilized before it was verified against nullptr. Check lines: 271, 274. ov_core.cpp 271

First, let's figure out what a type is. ov_available_devices_t:

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

As the name suggests, the function frees the object of type passed to it. ov_available_devices_t. Only the developer made one mistake.

First, the loop frees each pointer in the array. devices->devices. This implies that the pointer to the array is always non-zero. Then the developer doubted this and after the loop decided to check it before passing it to the operator delete[]. Only in the loop he forgot about this assumption, as a result we have a dereference of a potentially null pointer.

Corrected code:

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 can see, I removed all the pointer checks before passing them to the operator delete[]because the latter knows how to work with them.

The analyzer also found several other similar places:

• V595 The 'versions->versions' pointer was used 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

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 at the beginning, the cherry on top is the very code that, upon reading, may make the reviewer ask: “What are you?”

Here we have a converter function that copies a string. std::string into a dynamically allocated buffer char*. The buffer is created using the operator new[]the ownership of which is transferred to the smart pointer. In general, it is a very good strategy to wrap the raw pointer, because if something goes wrong, the smart pointer will take care of everything.

However, pay attention to what specific smart pointer specialization is used – std::unique_ptr. Its destructor frees the passed resource using the operator delete. Actually, this is what the analyzer warns us about:

V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. ov_core.cpp 14

It is correct to use specialization in such situations std::unique_ptr:

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 nothing terrible will happen in this situation, since the ownership of the resource is given to the returned object right on the next line. Indeed, this is true. But it is still a “code smell”, and the problem with it is that other developers may want to use such a smart pointer declaration in other places.

And, unfortunately, the reproduction 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

And if an exception-throwing operation occurs between the acquisition of the resource and its transfer to the raw pointer, then we get undefined behavior.

P.P.S. If your project uses C++14 or higher, you can replace the code with the following:

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();
  ....
}

First, we got rid of the explicit use of the operator new[] in the code, transferring everything to std::make_unique. Secondly, auto will infer the correct specialization of a smart pointer based on the initializer for us.

However, such code, in addition to dynamic allocation, will also fill the array with zeros. Since a complete rewriting of the array occurs later, you can save resources and not initialize it. Starting with C++20, the function std::make_unique_for_overwrite:

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 examples from the OpenVINO project surprised me a lot. I think you did too. However, as I wrote earlier, all the important errors that the developers of this project should pay attention to fit into a couple of articles. Most often, these were typos, which is a common problem among programmers and occurs in almost any project. OpenVINO is an impressive project and such a small (in my opinion) number of serious errors only indicates that the code is written quite well.

I hope you found all these code examples as interesting as I did, and that reviewing some of the bugs was a rewarding experience.

We, of course, passed on all the information to the developers and we hope that the bugs will be fixed in the near future.

And, as has already happened historically, I propose try our analyzer PVS-Studio. For Open Source projects we have provided free license.

Take care of yourself and all the best!