# Как один разработчик PVS\-Studio защищал баг в проверяемом проекте

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

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

## Вступление

В данный момент я пишу статью про проверку проекта [DuckStation](https://github.com/stenzek/duckstation/tree/13c5ee8bfb4f0f8fc40f76b39de58b5d9b473dc3)\. Это эмулятор консоли Sony PlayStation\. Проект довольно интересный и активно развивающийся\. Я нашёл там несколько занимательных багов, одним из которых хочу поделиться прямо сейчас\. Он продемонстрирует:

* как всё же невнимателен может быть человек, даже если является опытным специалистом\.
* как статический анализ подстраховывает человека от его рассеянности и невнимательности\.

## Показательный пример ошибки

**Предупреждение анализатора PVS\-Studio:** [V726](https://pvs-studio.ru/ru/docs/warnings/v726/) An attempt to free memory containing the 'wbuf' array by using the 'free' function\. This is incorrect as 'wbuf' was created on stack\. log\.cpp 216

```cpp
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_ и откуда он взялся\. Что же, это мой недочёт и самое время его исправить\. Давайте я покажу полный код функции, фрагмент из которой я уже приводил выше при описании ошибки и который как раз изучил человек, который проверял статью:

```cpp
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\)_, под него создастся новый буфер на куче\.

Может показаться, что пример всё больше и больше начинает походить на ложное срабатывание\. Однако, я посмотрел на код из функции несколько минут и написал такой ответ, со ссылками на репозиторий:

> Категорически не cогласен, давай смотреть на код: [буфер на стеке](https://github.com/stenzek/duckstation/blob/13c5ee8bfb4f0f8fc40f76b39de58b5d9b473dc3/src/common/log.cpp), [динамическая аллокация нового буфера на куче](https://github.com/stenzek/duckstation/blob/13c5ee8bfb4f0f8fc40f76b39de58b5d9b473dc3/src/common/log.cpp), [освобождение не того буфера](https://github.com/stenzek/duckstation/blob/13c5ee8bfb4f0f8fc40f76b39de58b5d9b473dc3/src/common/log.cpp)\.
>
> Если в локальный буфер на стеке не помещается строчка, то мы кладём её в динамически выделенный буфер по указателю _wmessage\_buf_\. И, как видно по коду, ниже есть 2 ветки с освобождением памяти, если динамическая аллокация произошла \(это делают проверками вроде _wmessage\_buf \!\= wbuf_\)\. **Только в первой ветке освобождается не та память, на что мы и ругаемся\.** А уже во второй [освобождается](https://github.com/stenzek/duckstation/blob/13c5ee8bfb4f0f8fc40f76b39de58b5d9b473dc3/src/common/log.cpp) правильный буфер, и на этот код мы как раз не ругаемся\.

Действительно, ошибка всё же есть, и программисту, написавшему этот код, следовало очистить буфер _wmessage\_buf_, по аналогии с тем, как он сделал это ниже\.

Ответ коллеги был краток:

> Согласен\. Поспешил\.
>
> P\.S\. С меня пиво\.

## Заключение

Любой статический анализатор, к сожалению, выдаёт [ложноположительные срабатывания](https://pvs-studio.ru/ru/blog/terms/6461/)\. Это приводит к тому, что иногда программисты слишком спешат посчитать показавшееся им неубедительное сообщение ложным\. Хочется предостеречь от спешки и постараться подходить к изучению сообщений с вниманием\.

Кстати, у нас уже были забавные статьи на подобную тему, например:

1. [Как PVS\-Studio оказался внимательнее, чем три с половиной программиста](https://pvs-studio.ru/ru/blog/posts/cpp/0587/)\.
1. [Один день из жизни разработчика PVS\-Studio, или отладка диагностики, которая оказалась внимательнее трёх программистов](https://pvs-studio.ru/ru/blog/posts/cpp/0842/)\.
1. [Ложные срабатывания в PVS\-Studio: как глубока кроличья нора](https://pvs-studio.ru/ru/blog/posts/cpp/0612/)\.

Приятного прочтения\. И приглашаем [попробовать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/try-free/) в деле\.