Друзья-разработчики, приглашаем вас продолжить захватывающее путешествие по дебрям кода Intel OpenVINO! Вооружившись статическим анализатором аки детективы, мы разоблачим наиболее коварные, интересные ошибки и опечатки, а также их скрытые тайны, оставшиеся за рамками первой части статьи.

Вступление

Итак, в первой части мы с вами рассмотрели множество поучительных опечаток в коде проекта OpenVINO. Поучительных, потому как опечатки всегда будут иметь место в жизни разработчика и от них никуда не денешься, однако проблема решаема. С помощью технологии статического анализа, который нам подсказывает о наличии подобных ошибок, мы можем составить определённую статистику о том, где программист чаще совершает такие ошибки. Когда выявлены соответствующие паттерны, борьба с опечатками становится в разы эффективнее. А если вам интересно, где чаще всего встречаются опечатки — у нас есть интересная статья, которую я рекомендую вам прочитать.

Однако, представьте, если бы мы умели по щелчку пальцев включать всю свою внимательность и выключать все отвлекающие факторы. Тогда наш код был бы идеален, и опечатки бы исчезли как вид. Но давайте помечтаем дальше... Если бы мы ещё могли предвидеть все возможные сценарии использования кода, учитывать все взаимосвязи между компонентами и предугадывать каждую ошибку... Написанные нами программы работали бы идеально. Однако я знаю только один "язык программирования", где всё это возможно, и это — HTML.

Но мы всё же имеем дело с С++. Здесь даже самый опытный программист может ошибиться, хм... да, собственно, где и как угодно. И вот, разделив все предупреждения на "до" и "после" и разобравшись с опечатками, давайте посмотрим и на другие, не менее интересные и разнообразные ошибки.

Дисклеймер

Написание и публикация этой статьи не имеют цели обесценить труд программистов, занимающихся разработкой данного продукта. Цель: популяризация статических анализаторов кода, которые полезны даже для качественных и состоявшихся проектов. Даже больше, для Open Source проектов (и не только) мы предоставляем бесплатные лицензии. Подробнее можно узнать здесь.

Результаты проверки

Вообще, какого-либо огромного количества ошибок во время анализа найдено не было. Всё интересное и важное уместилось всего в паре статей. Код весьма красивый и защищённый. Всё так же, коммит, на котором я собирал проект для проверки: 2d8ac08.

Ну, с формальностями закончили, можно вернуться к разбору ошибок. Как и ожидалось, сложность багов повышается, а вместе со сложностью возрастает запутанность кода, в котором нам придётся разобраться. Местами код вообще довольно странный — непонятно, как или почему так написан — но предлагаю начать с более-менее простого, а затем... Ну, вы уже и сами всё скоро увидите.

От простого к сложному. Не совсем понятный код

Фрагмент 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++;
    }
    ....
  }
....
}

Предупреждение анализатора: V547 Expression 'roi_cols == 5' is always false. experimental_detectron_roi_feature_extractor.cpp 211

Проверка if (roi_cols == 5) действительно всегда будет возвращать false, а код в теле никогда не будет достижим. Связано это с тем, что значение переменной roi_cols никак не изменяется между её декларацией и проверкой в условии.

Фрагмент 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;
  }
}

Предупреждение анализатора: 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

Обнаружена довольно интересная и редкая ошибка, из-за которой цикл может быть бесконечным.

Работая с объектами класса std::istream, проверки !model.eof() для выхода из цикла while может быть недостаточно. В случае возникновения сбоя при чтении данных, все последующие вызовы функции eof будут возвращать исключительно false, что может привести к зацикливанию.

Для того, чтобы не допустить этой проблемы, можно было бы использовать перегруженный оператор bool в условии цикла следующим образом:

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

Фрагмент 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!");
  }
  ....
}

Предупреждение анализатора: 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

Первое и второе условия конструкций if совпадают, поэтому код в then-ветке второго if всегда недостижим. При этом можно заметить, что ветки выполняют разную логику: первая пытается прочитать атрибут paddings в виде вектора чисел типа int32_t (paddings_vector), вторая – прочитать этот же атрибут в виде числа типа int32_t (padding_int).

Мне тяжело определить, как именно должен выглядеть корректный код в этой ситуации. Однако давайте попробуем предположить. Код располагается в модуле OpenVINO Paddle Frontend, который парсит модель, сформированную фреймворком PaddlePaddle. Если поискать имя 'pad3d' в этом проекте, то можно найти следующее описание:

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

Из этого предположим, что padding — это вариант, и нужно перебрать две интересующие альтернативы — std::vector<int32_t> и int32_t. Это можно попробовать сделать следующим способом:

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

Также изучение исходников PaddlePaddle навеяло мысль о том, что в написании атрибута закралась опечатка, и он должен называться padding, а не paddings. Но это не точно :) В любом случае рекомендую разработчиками обратить внимание на этот код.

Фрагмент N4

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

Предупреждение анализатора: V591 [CERT-MSC52-CPP] Non-void function should return a value. graph_iterator_flatbuffer.hpp 29

Как можно заметить, функция имеет пустое тело и ничего не возвращает, хотя возвращаемый тип указан как std::basic_string<T>. Имеем неопределённое поведение в программе.

Данный фрагмент кода как будто бы появился из сериала "Секретные материалы". Но на самом деле всё достаточно просто. Если посмотреть буквально пару строк ниже, то можно заметить специализации этого шаблона функции:

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

И у этих специализаций есть вполне конкретные тела, которые возвращают вполне конкретные объекты:

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

Значит, программа будет вполне корректно работать со специализациями от char и wchar_t и творить что угодно при других. А их может быть еще три: char8_t, char16_t, char32_t.

Да, в коде может не быть использования таких специализаций шаблона, но мы же с вами за надёжный и безопасный код. И хотелось бы, чтобы компилятор остановил нас ещё на этапе компиляции, если кто-то напишет такой код. Сделать это очень просто, нужно всего лишь превратить определение шаблона функции в декларацию:

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

Теперь при попытке вызова специализации от char8_t, char16_t или char32_t компилятор выдаст ошибку, потому что без тела он не сможет сделать нужное инстанцирование.

Ещё несколько подобных предупреждений:

• 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

Фрагмент 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);
  }
}

Предупреждение анализатора: V591 [CERT-MSC52-CPP] Non-void function should return a value. registers_pool.hpp 229

Ещё один фрагмент с функцией, которая не возвращает значение на всех путях исполнения. Если вы считаете, что c вашей программой не произойдёт ничего страшного, если вы забыли return в функции, то вы ошибаетесь. Даже если возвращаемый тип встроенный, код содержит неопределённое поведение. Советую также к прочтению эту статью о том, какую злую шутку может сыграть с вами компилятор в этой ситуации.

Вернёмся к нашему примеру. Начиная с C++17, этот код можно сделать лучше. Во-первых, как можно заметить, условия представляют из себя выражения, вычисляемые на этапе компиляции. Поэтому следует воспользоваться конструкцией if constexpr. Она позволит не компилировать код в тех ветках, в которых условие вычисляется как false. Во-вторых, с помощью static_assert можно защитить код от тех специализаций, для которых в текущем коде не возвращается значение:

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

В исправленном примере приведены два способа написать static_assert в зависимости от используемой версии стандарта. Дело в том, что до C++23 вариант static_assert(false, "...") в шаблоне функции всегда приводил к ошибке этапа компиляции.

До C++17 можно поправить код, добавив перегрузки шаблона функции getFree и воспользовавшись 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);
}

Фрагмент 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));
  }
  ....
}

Предупреждение анализатора: V523 The 'then' statement is equivalent to the 'else' statement. random_uniform.cpp 120

Здесь всё просто, но непонятно. Что в ветке if, что в else, в теле содержится один и тот же код. Возможно, это ошибка, связанная с копипастом, и разработчикам стоит обратить на этот фрагмент кода внимание.

Фрагмент 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()};
  }
}

Предупреждение анализатора: V575 [CERT-EXP37-C] The null pointer is passed into 'move' function. Inspect the first argument. xml_parse_utils.hpp 249

Честно говоря, мы впервые в жизни увидели такой интересный код: в функцию std::move передали nullptr. Вообще в std::move не возбраняется передавать nullptr — он просто преобразуется в std::nullptr_t &&. Однако непонятно, зачем так делать.

Для лучшего понимания происходящего давайте заглянем в 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{};
};

Попробуем поиграть в детектива и разгадаем загадку, откуда мог появиться такой код. Скорее всего, программист сначала написал return в try-блоке: в нём конструируется объект ParseResult, перемещая умный указатель xml и копируя error_msg. Затем обработал ситуацию, когда будет выброшено исключение. В этом случае также надо вернуть какой-то объект типа ParseResult. Чтобы упростить себе жизнь, он скопировал предыдущий return, слегка модифицировав аргументы конструктора. При этом, увидев перемещение умного указателя xml, он принял решение, что и здесь надо что-то переместить. Например, nullptr.

Однако нужды в этом никакой нет. Согласно правилам C++, при выборе перегрузки конструктора компилятор будет обязан произвести некоторые неявные преобразования переданных аргументов. Например, r-value ссылка на std::unique_ptrpugi::xml\_document\ не может быть привязана к r-value ссылке на std::nullptr_t. Поэтому компилятор создаст временный объект типа std::unique_ptrpugi::xml\_document\, позвав соответствующий конвертирующий конструктор. И лишь затем на этот временный объект будет привязана ссылка (параметр конструктора).

Если мы опустим std::move и передадим nullptr в конструктор, код также будет компилируемым и станет более читаемым:

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

Нулевые указатели и возможные утечки памяти

И вот мы плавно переходим к работе с указателями. А анализатор поможет разобраться в ситуациях, в которых что-то пошло не так, да и в целом, как не стоит писать код.

Фрагмент 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.");
    }
  }
  ....
}

Предупреждения анализатора:

• 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

Часто вижу подобные ошибки в различных проектах, поэтому и решил обратить на них внимание. Суть в том, что программисты редко тестируют обработчики ошибок :)

Представим на секунду, что результат dynamic_cast вернёт нулевой указатель. Тогда при формировании исключения будет использован тот самый нулевой указатель в попытке вызвать функцию getName. Получаем неопределённое поведение, из-за которого обработка исключительной ситуации может превратиться в фатальную для программы ошибку.

Фрагмент 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;
  }
}

Предупреждение анализатора: V758 The 'stream' reference becomes invalid when smart pointer returned by a function is destroyed. cpu_streams_executor.cpp 410

На самом деле, в этом примере всё хорошо, и ссылка не будет "висячей". Происходит это потому, что shared_ptr, который возвращает функция local, предварительно сохраняется в контейнер _stream_map, время жизни которого больше, чем у ссылки:

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

Однако выступлю в защиту анализатора: лучше всё же сохранить результат функции local в переменную функции Defer. Давайте представим, что код немного поменялся. Начнёт происходить следующее:

1. Функция local теперь возвращает shared_ptr со счётчиком ссылок, который равен 1.

2. Код функции Defer не изменился.

3. Начинается обработка декларации stream. Происходит вызов функции local, она возвращает временный shared_ptr со счётчиком ссылок, который равен 1. Далее ссылка stream привязывается на объект под умным указателем.

4. Как только декларация будет полностью обработана, вызовется деструктор временного умного указателя. Счётчик ссылок становится равным 0, и объект под умным указателем также уничтожается.

5. Ссылка stream становится висячей, и её использование ведёт к неопределенному поведению.

Сохранив результат в переменную, мы продлим время жизни объекта до выхода из области видимости и избавимся от потенциальной проблемы:

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

Фрагмент N10

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

Предупреждение анализатора: V575 [CERT-EXP37-C] The null pointer is passed into 'operator delete'. Inspect the argument. w_dirent.h 94

С первого взгляда на этот код может быть непонятно, а в чём именно заключается ошибка. А дело в том, что память по указателю next не освободится, так как проверка была написана неверно. Анализатор косвенно сообщает об этом, увидев передачу нулевого указателя в оператор delete. Более того, проверять указатель вообще не имеет смысла, т.к. оператор delete корректно обрабатывает нулевые указатели.

Перед вами пример кода, который практически полностью совпадает с примером, приведённым моим коллегой в статье: "Ошибка настолько проста, что программисты её не замечают". Рекомендую вам ознакомиться с ней. Если вы читали её ранее и думали, что приведённый там код синтетический, и в реальной жизни такое встречаться не может — то вот вам прямое доказательство :)

Исправленный код:

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

Фрагмент 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;
}

Предупреждение анализатора: V595 The 'devices->devices' pointer was utilized before it was verified against nullptr. Check lines: 271, 274. ov_core.cpp 271

Для начала разберёмся, что из себя представляет тип ov_available_devices_t:

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

Как можно понять из названия, функция освобождает переданный в неё объект типа ov_available_devices_t. Только разработчик допустил при этом одну ошибку.

Сначала в цикле происходит освобождение каждого указателя в массиве devices->devices. При этом подразумевается, что указатель на массив всегда ненулевой. Затем разработчик усомнился в этом и после цикла решил проверить его перед передачей в оператор delete[]. Только в цикле он забыл о таком предположении, в результате имеем разыменование потенциально нулевого указателя.

Поправленный код:

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

Как вы можете заметить, я убрал все проверки указателей перед передачей в оператор delete[], т.к. последний умеет с ними работать.

Анализатор также обнаружил еще несколько похожих мест:

• 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

Фрагмент 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;
}

Как и было обещано в начале, — как вишенка на торте, — тот самый код, при прочтении которого у ревьюера может возникнуть вопрос: "Что ты такое?".

Перед нами функция-конвертер, копирующая строку std::string в динамически аллоцированный буфер char*. Буфер создается при помощи оператора new[], владение которым передаётся умному указателю. В целом, очень верная стратегия – обернуть сырой указатель, ведь если что-то пойдёт не так, то умный указатель обо всём позаботится.

Однако обратите внимание, какую именно специализацию умного указателя используют — std::unique_ptr<char>. Её деструктор освобождает переданный ресурс с помощью оператора delete. Собственно, об этом нас и предупреждает анализатор:

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

Правильно использовать в таких ситуациях специализацию std::unique_ptr<char[]>:

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

P.S. Читатель может возразить, что в этой ситуации ничего страшного не произойдёт, т.к. владение ресурсом отдаётся возвращаемому объекту прямо следующей строкой. Действительно, это так. Но всё же это "код с запахом", и его проблема в том, что у других разработчиков может возникнуть желание использовать такое объявление умного указателя в других местах.

И, к сожалению, процесс размножения уже был запущен:

• 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

И если между захватом ресурса и его передачей во владение сырому указателю произойдёт бросающая исключение операция, то получим неопределённое поведение.

P.P.S. Если в проекте используется стандарт C++14 и выше, можно заменить код на следующий:

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

Во-первых, мы избавились от явного использования оператора new[] в коде, переложив всё на std::make_unique. Во-вторых, auto выведет за нас корректную специализацию умного указателя на основе инициализатора.

Однако такой код, помимо динамической аллокации, также заполнит массив нулями. Раз уж далее происходит полная перезапись массива, то можно сэкономить ресурсы и не выполнять его инициализацию. Начиная с C++20, для этого доступна функция 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();
  ....
}

Заключение

Некоторые примеры из проекта OpenVINO меня сильно удивили. Думаю, и вас тоже. Однако, как я уже писал ранее, все важные ошибки, на которые стоило бы обратить внимание разработчикам данного проекта, уместились в паре статьей. Чаще всего это были опечатки, что является распространённой проблемой среди программистов и встречаются почти в любом проекте. OpenVINO — проект внушительный и столько малое (на мой взгляд) количество серьёзных ошибок говорит только о том, что код довольно качественно написан.

Надеюсь, вам так же, как и мне, было интересно разглядывать все эти примеры кода, а разбор некоторых ошибок оказался полезным опытом.

Всю информацию мы, конечно же, передали разработчикам и будем надеяться, что баги в ближайшем будущем исправят.

И, как у нас уже исторически сложилось, предлагаю попробовать наш анализатор PVS-Studio. Для Open Source проектов у нас предоставляется бесплатная лицензия.

Берегите себя и всего доброго!