Разработчики 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 оказалась внимательнее трех с половиной программистов.
- Один день из жизни разработчика PVS-Studio, или как я отлаживал диагностику, которая превзошла трех программистов.
- Ложные срабатывания в PVS-Studio: насколько глубока кроличья нора.
Наслаждайтесь чтением. Приходите и попробуйте PVS-Studio на своих проектах.