Разработчики PVS-Studio часто проверяют проекты с открытым исходным кодом и пишут об этом статьи. Иногда при написании статьи мы сталкиваемся с интересными ситуациями или эпичными ошибками. Конечно же, мы хотим написать об этом небольшую заметку. Это один из таких случаев.

Введение

В данный момент пишу статью о проверке проекта DuckStation. Это эмулятор консоли Sony PlayStation. Проект достаточно интересный и активно развивается. Я нашел несколько интересных ошибок и хочу поделиться с вами историей об одной из них. Эта статья демонстрирует:

  • что даже специалисты могут ошибаться.
  • что статический анализ может уберечь человека от подобных ошибок.

Пример ошибки

PVS-Studio выдал предупреждение: V726 Попытка освободить память, содержащую массив wbuf, с помощью функции free. Это неверно, так как wbuf был создан в стеке. лог.cpp 216

template<typename T>
static ALWAYS_INLINE void FormatLogMessageAndPrintW(....)
{
  ....
  wchar_t wbuf[512];
  wchar_t* wmessage_buf = wbuf;
  ....
  if (wmessage_buf != wbuf)
  {
    std::free(wbuf); // <=
  }
  if (message_buf != buf)
  {
    std::free(message_buf);
  }
  ....
}

В оригинальной версии статьи я описал этот баг следующим образом:

Здесь анализатор обнаружил код с ошибкой. В этом фрагменте кода мы видим попытку удалить массив, выделенный в стеке. Поскольку память не была выделена в куче, вам не нужно вызывать какие-либо специальные функции, такие как std::free, для ее очистки. Когда объект уничтожается, память очищается автоматически.

Это может показаться большой ошибкой для статьи — статический буфер и динамическое освобождение памяти. Что могло пойти не так? Я скажу вам сейчас.

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

Здесь нет ошибки. Это ложная тревога; анализатор не освоил его. Посередине находится динамическое выделение памяти для сообщения функцией malloc. Проверка if (wmessage_buf != wbuf) используется для определения, следует ли вызывать std::free или нет.

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

template<typename T>
static ALWAYS_INLINE void FormatLogMessageAndPrintW(
                                             const char* channelName, 
                                             const char* functionName, 
                                             LOGLEVEL level,
                                             const char* message, 
                                             bool timestamp, 
                                             bool ansi_color_code,
                                             bool newline, 
                                             const T& callback)
{
  char buf[512];
  char* message_buf = buf;
  int message_len;
  if ((message_len = FormatLogMessageForDisplay(message_buf,
                       sizeof(buf), channelName, functionName, level, 
                       message, timestamp,
                       ansi_color_code, newline)) > (sizeof(buf) - 1))
  {
    message_buf = static_cast<char*>(std::malloc(message_len + 1));
    message_len = FormatLogMessageForDisplay(message_buf, 
                 message_len + 1, channelName, functionName, 
                 level, message, timestamp, ansi_color_code, newline);
  }
  if (message_len <= 0)
    return;
  // Convert to UTF-16 first so unicode characters display correctly.
  // NT is going to do it anyway...
  wchar_t wbuf[512];
  wchar_t* wmessage_buf = wbuf;
  int wmessage_buflen = countof(wbuf) - 1;
  if (message_len >= countof(wbuf))
  {
    wmessage_buflen = message_len;
    wmessage_buf = static_cast<wchar_t*>
               (std::malloc((wmessage_buflen + 1) * sizeof(wchar_t)));
  }
  wmessage_buflen = MultiByteToWideChar(CP_UTF8, 0, message_buf,
                    message_len, wmessage_buf, wmessage_buflen);
  if (wmessage_buflen <= 0)
    return;
  wmessage_buf[wmessage_buflen] = '\0';
  callback(wmessage_buf, wmessage_buflen);
  if (wmessage_buf != wbuf)
  {
    std::free(wbuf);                        // <=
  }
  if (message_buf != buf)
  {
    std::free(message_buf);
  }
}

Действительно, если длина сообщения больше или равна countof(wbuf), для него будет создан новый буфер в куче.

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

Категорически не согласен. Посмотрим на код: буфер в стеке, динамическое выделение нового буфера в куче, освобождение неправильного буфера.

Если строка не помещается в локальный буфер на стеке, то мы помещаем ее в динамически выделяемый буфер по указателю wmessage_buf. Как видно из кода, ниже 2 ветки с освобождением памяти, если было динамическое выделение. Мы можем проверить это с помощью wmessage_buf != wbuf. Однако в первой ветке освобождается не та память. Поэтому предупреждение здесь. Во второй ветке освобождается правый буфер. Здесь нет предупреждений.

Действительно, есть ошибка. Разработчик должен был очистить wmessage_buf так же, как это было сделано ниже.

Ответ моего товарища по команде был коротким:

Соглашаться. Я ошибался.

P.S. Я должен тебе пиво.

Вывод

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

Кстати, вы можете прочитать аналогичные занимательные статьи. Например:

Наслаждайтесь чтением. Приходите и попробуйте PVS-Studio на своих проектах.