Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
GBP
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

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

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
PVS-Studio ищет баги в проекте DuckStat…

PVS-Studio ищет баги в проекте DuckStation

03 Ноя 2021

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

0881_duckstation_ru/image1.png

Введение

DuckStation – это эмулятор консоли Sony PlayStation. Как видно по сайту, он имеет версию как для Windows и Linux, так и для смартфонов на Android. А недавно его запустили на Xbox Series X и S. Сам проект содержит чуть меньше миллиона строк исходного кода на языках С и С++. У DuckStation нет релизов, в него постоянно коммитят изменения, и поэтому для проверки пришлось зафиксировать SHA коммита: 13c5ee8.

Мы проверили проект и обнаружили множество предупреждений (170 уровня High и 434 уровня Medium). Давайте разберём 10 самых интересных из них.

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

Предупреждение N1

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<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 для её очистки – она будет произведена автоматически при уничтожении объекта.

Также в процессе написания статьи, коллега, который проверял её, посчитал срабатывание ложным. Этот интересный случай я описал в виде отдельной статьи, с которой приглашаю вас ознакомиться, она так и называется: История о том, как один разработчик защищал баг в проверяемом проекте от другого разработчика PVS-Studio.

Предупреждение N2

V547 Expression 'i < pathLength' is always true. file_system.cpp 454

void CanonicalizePath(const char *Path, ....)
{
  ....
  u32 pathLength = static_cast<u32>(std::strlen(Path));
  ....
  for (i = 0; i < pathLength;)
  {
    ....
    char nextCh = (i < pathLength) ? Path[i + 1] : '\0';
    ....
  }
  ....
}

Индуктивная переменная i увеличивается уже после инициализации nextCh. Судя по тому, что, для определения длины строки используется функция strlen, строка Path нуль-терминирована. Тогда проверка i < pathLength явно лишняя, и её можно убрать, так как условие будет всегда верным. На последней итерации цикла мы получим нулевой символ и без неё. Тогда код:

char nextCh = (i < pathLength) ? Path[i + 1] : '\0';

эквивалентен:

char nextCh = Path[i + 1];

Однако, даже если бы строка не заканчивалась терминальным нулём, проверка была бы неверной. На завершающей итерации цикла при попытке взять последний символ Path[i + 1] произойдет выход за границу массива. В таком случае надо было бы написать:

char nextCh = ((i + 1) < pathLength) ? Path[i + 1] : '\0';

Предупреждения N3, N4

На данный фрагмент кода анализатор выдал сразу два срабатывания:

  • V547 Expression 'm_value.wSecond <= other.m_value.wSecond' is always true. timestamp.cpp 311
  • V779 Unreachable code detected. It is possible that an error is present. timestamp.cpp 314
bool Timestamp::operator<=(const Timestamp& other) const
{
  ....
  if (m_value.wYear > other.m_value.wYear)
    return false;
  else if (m_value.wYear < other.m_value.wYear)
    return true;
  if (m_value.wMonth > other.m_value.wMonth)
    return false;
  else if (m_value.wMonth < other.m_value.wMonth)
    return true;
  if (m_value.wDay > other.m_value.wDay)
    return false;
  else if (m_value.wDay < other.m_value.wDay)
    return true;
  if (m_value.wHour > other.m_value.wHour)
    return false;
  else if (m_value.wHour < other.m_value.wHour)
    return true;
  if (m_value.wMinute > other.m_value.wMinute)
    return false;
  else if (m_value.wMinute < other.m_value.wMinute)
    return true;
  if (m_value.wSecond > other.m_value.wSecond)
    return false;
  else if (m_value.wSecond <= other.m_value.wSecond) // <=
    return true;
  if (m_value.wMilliseconds > other.m_value.wMilliseconds)
    return false;
  else if (m_value.wMilliseconds < other.m_value.wMilliseconds)
    return true;

  return false;
}

В этом операторе происходит сравнение значений от года до миллисекунд, но ошибка в коде видимо осталась ещё с того момента, когда сравнение заканчивалось на секундах. Забытое (или случайно поставленное) <= в проверке секунд приводит к тому, что последующие операции стали недостижимыми.

Интересно, что данная ошибка была допущена ещё раз. Во второй раз это был подобный operator >=. На него анализатор тоже выдал два срабатывания:

  • V547 Expression 'm_value.wSecond >= other.m_value.wSecond' is always true. timestamp.cpp 427
  • V779 Unreachable code detected. It is possible that an error is present. timestamp.cpp 430

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

Предупреждение N5

V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value. gamelistmodel.cpp 415

bool GameListModel::lessThan(...., int column, bool ascending) const
{
  ....
  const GameListEntry& left  = m_game_list->GetEntries()[left_row];
  const GameListEntry& right = m_game_list->GetEntries()[right_row];
  ....
  switch(column)
  {
    case Column_Type:
    {
      ....
      return ascending ? 
             (static_cast<int>(left.type) 
           <  static_cast<int>(right.type)) 
           :
             (static_cast<int>(right.type) 
           >  static_cast<int>(left.type));
    }
  }
  ....
}

Получилось два одинаковых сравнения. Операнды условного оператора, находящиеся по обе стороны от знаков больше и меньше, просто заменены местами в двух его ветвлениях. По сути, фрагмент кода в операторе return эквивалентен:

return ascending ? 
             (static_cast<int>(left.type) 
           <  static_cast<int>(right.type)) 
           :
             (static_cast<int>(left.type) 
           <  static_cast<int>(right.type));

Возможно, код должен был выглядеть так:

return ascending ? 
             (static_cast<int>(left.type) 
           <  static_cast<int>(right.type))
           :
             (static_cast<int>(right.type) 
           <  static_cast<int>(left.type));

Предупреждения N6, N7, N8

V501 There are identical sub-expressions 'c != ' '' to the left and to the right of the '&&' operator. file_system.cpp 560

static inline bool FileSystemCharacterIsSane(char c, ....)
{
  if    (!(c >= 'a' && c <= 'z') 
     && !(c >= 'A' && c <= 'Z') 
     && !(c >= '0' && c <= '9') 
     &&   c != ' ' 
     &&   c != ' ' 
     &&   c != '_' 
     &&   c != '-' 
     &&   c != '.')
  {
    ....
  }
  ....
}

В данном месте два раза происходит лишняя проверка на пробел. Также, было обнаружено ещё несколько подобных предупреждений:

V501 There are identical sub-expressions to the left and to the right of the '|' operator: KMOD_LCTRL | KMOD_LCTRL sdl_key_names.h 271

typedef enum
{
  KMOD_NONE   = 0x0000,
  KMOD_LSHIFT = 0x0001,
  KMOD_RSHIFT = 0x0002,
  KMOD_LCTRL  = 0x0040,
  ....
}
....
static const std::array<SDLKeyModifierEntry, 4> s_sdl_key_modifiers = 
{
  {{KMOD_LSHIFT, static_cast<SDL_Keymod>(KMOD_LSHIFT | KMOD_RSHIFT),
    SDLK_LSHIFT, SDLK_RSHIFT, "Shift"},
  {KMOD_LCTRL, static_cast<SDL_Keymod>(KMOD_LCTRL | KMOD_LCTRL), // <=
    SDLK_LCTRL, SDLK_RCTRL, "Control"},
  {KMOD_LALT, static_cast<SDL_Keymod>(KMOD_LALT | KMOD_RALT),
    SDLK_LALT, SDLK_RALT, "Alt"},
  {KMOD_LGUI, static_cast<SDL_Keymod>(KMOD_LGUI | KMOD_RGUI),
    SDLK_LGUI, SDLK_RGUI, "Meta"}}
};

Здесь, по обе стороны от знака | стоит одинаковое значение KMOD_LCTRL, выглядит подозрительно.

V501 There are identical sub-expressions 'TokenMatch(command, "CATALOG")' to the left and to the right of the '||' operator. cue_parser.cpp 196

bool File::ParseLine(const char* line, ....)
{
  const std::string_view command(GetToken(line));
  ....
  if (   TokenMatch(command, "CATALOG") // <=
      || TokenMatch(command, "CDTEXTFILE") 
      || TokenMatch(command, "CATALOG") // <=
      || TokenMatch(command, "ISRC") 
      || TokenMatch("command", "TRACK_ISRC") 
      || TokenMatch(command, "TITLE")
      ||  ....)
  {
    ....
  }
  ....
}

Тут происходит два одинаковых вызова функции TokenMatch.

Интересно, что, в проверке ниже, также есть ошибка: command записан как строковый литерал вместо переменной. Кстати, мы давно собирались сделать диагностическое правило, которое позволит отслеживать такие ситуации, а этот фрагмент кода – один из показателей, что оно будет полезно.

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

Предупреждение N9

V1065 Expression can be simplified, check 'm_display_texture_height' and similar operands. host_display.cpp 549

....
s32 m_display_texture_height = ....;
s32 m_display_texture_view_y = ....;
....
bool HostDisplay::WriteDisplayTextureToFile(....)
{
  s32 read_y = m_display_texture_view_y;
  s32 read_height = m_display_texture_view_height; 
  ....
  read_y = (m_display_texture_height - read_height) –
           (m_display_texture_height - m_display_texture_view_y);
  ....
}

Да, тут нет ошибки, просто код можно немного сократить, упростив выражение:

read_y = m_display_texture_view_y - read_height;

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

Предупреждение N10

V614 The 'host_interface' smart pointer is utilized immediately after being declared or reset. It is suspicious that no value was assigned to it. main.cpp 45

static std::unique_ptr<NoGUIHostInterface> CreateHostInterface()
{
  const char* platform = std::getenv("DUCKSTATION_NOGUI_PLATFORM");
  std::unique_ptr<NoGUIHostInterface> host_interface;

#ifdef WITH_SDL2
  if (   !host_interface && (!platform 
      || StringUtil::Strcasecmp(platform, "sdl") == 0) 
      && IsSDLHostInterfaceAvailable())
  {
    host_interface = SDLHostInterface::Create();   }
  }
#endif

#ifdef WITH_VTY
  if (  !host_interface && (!platform 
      || StringUtil::Strcasecmp(platform, "vty") == 0))
  {
    host_interface = VTYHostInterface::Create();
  }
#endif

#ifdef _WIN32
  if (  !host_interface && (!platform 
      || StringUtil::Strcasecmp(platform, "win32") == 0))
  {
    host_interface = Win32HostInterface::Create();
  }
    
#endif

  return host_interface;
}

Диагностика говорит об использовании неинициализированной переменной. Здесь происходит бессмысленная проверка умного указателя. Первая проверка: !host_interface всегда будет возвращать true.

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

static std::unique_ptr<NoGUIHostInterface> CreateHostInterface()
{
  const char* platform = std::getenv("DUCKSTATION_NOGUI_PLATFORM");
#ifdef WITH_SDL2
  if (   (!platform 
      ||  StringUtil::Strcasecmp(platform, "sdl") == 0) 
      &&  IsSDLHostInterfaceAvailable())
  {
    return SDLHostInterface::Create();
  }
#endif

#ifdef WITH_VTY
  if (   !platform 
      || StringUtil::Strcasecmp(platform, "vty") == 0)
  {
    return VTYHostInterface::Create();
  }
#endif

#ifdef _WIN32
  if (   !platform 
      || StringUtil::Strcasecmp(platform, "win32") == 0)
  {
    return Win32HostInterface::Create();
  }
#endif

  return {};
}

Кажется, что мы превратили один return в четыре и код должен работать медленнее, однако я написал похожий синтетический пример. Как можно видеть, под оптимизациями O2 компиляторы Сlang 13 и GCC 11.2 генерируют меньше ассемблерных инструкций для второго примера (особенно это видно для GCC).

Заключение

Несмотря на то, что проект небольшой, мы смогли найти несколько интересных срабатываний. Надеюсь, что благодаря статье, разработчики DuckStation смогут исправить некоторые недочёты, а может быть захотят самостоятельно перепроверить свою кодовую базу PVS-Studio.

А если вы хотите попробовать PVS-Studio на своём проекте, то вы можете сделать это, скачав его здесь.

Популярные статьи по теме
Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей

Дата: 21 Ноя 2018

Автор: Андрей Карпов

Краткое описание технологий, используемых в инструменте PVS-Studio, которые позволяют эффективно обнаруживать большое количество паттернов ошибок и потенциальных уязвимостей. Статья описывает реализа…
Статический анализ как часть процесса разработки Unreal Engine

Дата: 27 Июн 2017

Автор: Андрей Карпов

Проект Unreal Engine развивается - добавляется новый код и изменятся уже написанный. Неизбежное следствие развития проекта - появление в коде новых ошибок, которые желательно выявлять как можно раньш…
Бесплатный PVS-Studio для тех, кто развивает открытые проекты

Дата: 22 Дек 2018

Автор: Андрей Карпов

В канун празднования нового 2019 года команда PVS-Studio решила сделать приятный подарок всем контрибьюторам open-source проектов, хостящихся на GitHub, GitLab или Bitbucket. Им предоставляется возмо…
PVS-Studio ROI

Дата: 30 Янв 2019

Автор: Андрей Карпов

Время от времени нам задают вопрос, какую пользу в денежном эквиваленте получит компания от использования анализатора PVS-Studio. Мы решили оформить ответ в виде статьи и привести таблицы, которые по…
Как и почему статические анализаторы борются с ложными срабатываниями

Дата: 20 Мар 2017

Автор: Андрей Карпов

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

Дата: 19 Май 2017

Автор: Андрей Карпов

Возможно, читатели помнят мою статью под названием "Эффект последней строки". В ней идёт речь о замеченной мной закономерности: ошибка чаще всего допускается в последней строке однотипных блоков текс…
Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний

Дата: 31 Июл 2017

Автор: Андрей Карпов

После большой статьи про проверку операционной системы Tizen мне было задано много вопросов о проценте ложных срабатываний и о плотности ошибок (сколько ошибок PVS-Studio выявляет на 1000 строк кода)…
Эффект последней строки

Дата: 31 Май 2014

Автор: Андрей Карпов

Я изучил множество ошибок, возникающих в результате копирования кода. И утверждаю, что чаще всего ошибки допускают в последнем фрагменте однотипного кода. Ранее я не встречал в книгах описания этого …
PVS-Studio для Java

Дата: 17 Янв 2019

Автор: Андрей Карпов

В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальн…
Любите статический анализ кода!

Дата: 16 Окт 2017

Автор: Андрей Карпов

Я в шоке от возможностей статического анализа кода, хотя сам участвую в разработке инструмента PVS-Studio. На днях я был искренне удивлён тому, что анализатор оказался умнее и внимательнее меня.

Комментарии (0)

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