Как один разработчик PVS-Studio защищал баг в проверяемом проекте
Мы в PVS-Studio часто проверяем открытые проекты и пишем статьи об этом. Иногда при написании статьи случаются интересные ситуации или попадаются особенно эпичные ошибки, тогда хочется написать про это отдельную небольшую заметку. Сейчас совпали оба случая.
Вступление
В данный момент я пишу статью про проверку проекта DuckStation. Это эмулятор консоли Sony PlayStation. Проект довольно интересный и активно развивающийся. Я нашёл там несколько занимательных багов, одним из которых хочу поделиться прямо сейчас. Он продемонстрирует:
как всё же невнимателен может быть человек, даже если является опытным специалистом.
как статический анализ подстраховывает человека от его рассеянности и невнимательности.
Показательный пример ошибки
Предупреждение анализатора PVS-Studio: 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
template
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
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(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
(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огласен, давай смотреть на код: буфер на стеке, динамическая аллокация нового буфера на куче, освобождение не того буфера. Если в локальный буфер на стеке не помещается строчка, то мы кладём её в динамически выделенный буфер по указателю wmessage_buf. И, как видно по коду, ниже есть 2 ветки с освобождением памяти, если динамическая аллокация произошла (это делают проверками вроде wmessage_buf!= wbuf). Только в первой ветке освобождается не та память, на что мы и ругаемся. А уже во второй освобождается правильный буфер, и на этот код мы как раз не ругаемся.
Действительно, ошибка всё же есть, и программисту, написавшему этот код, следовало очистить буфер wmessage_buf, по аналогии с тем, как он сделал это ниже.
Ответ коллеги был краток:
Согласен. Поспешил.
P.S. С меня пиво.
Заключение
Любой статический анализатор, к сожалению, выдаёт ложно-положительные срабатывания. Это приводит к тому, что иногда программисты слишком спешат посчитать показавшееся им неубедительное сообщение ложным. Хочется предостеречь от спешки и постараться подходить к изучению сообщений с вниманием.
Кстати, у нас уже были забавные статьи на подобную тему, например:
Как PVS-Studio оказался внимательнее, чем три с половиной программиста.
Один день из жизни разработчика PVS-Studio, или отладка диагностики, которая оказалась внимательнее трёх программистов.
Ложные срабатывания в PVS-Studio: как глубока кроличья нора.
Приятного прочтения. И приглашаем попробовать PVS-Studio в деле.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Vladislav Stolyarov. How a PVS-Studio developer defended a bug in a checked project.
