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

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

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

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

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

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

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


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

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

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

03 Ноя 2021

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

0881_duckstation_ru/image1-9457d13e2871bab688b48f09623abc39.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 для проверки лабораторных работ на C и C++

Дата: 06 Июл 2022

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

Встретил очередной вопрос на Stack Overflow от человека, изучающего язык C++. Количество подобных вопросов можно сократить, используя PVS-Studio. Человек сразу может получить ответ, не отвлекая други…
Игра: найди ошибку в C++ коде

Дата: 29 Июн 2022

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

Авторы анализатора PVS-Studio предлагают вам проверить свою внимательность и развлечься. Попробуйте быстро отыскать баг в фрагменте исходного кода и ткнуть в него мышкой.
Тем, кто учится программировать и решил написать вопрос на Stack Overflow: "Почему код не работает?"

Дата: 28 Июн 2022

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

На сайте Stack Overflow много вопросов от людей, ещё только изучающих языки программирования. Лайфхак: ответы на многие эти вопросы можно получить сразу, запустив анализатор кода. Получится быстрее.
В мире антропоморфных животных: PVS-Studio проверил Overgrowth

Дата: 23 Июн 2022

Автор: Владислав Столяров

Недавно в сети появилась новость о том, что был открыт исходный код игры Overgrowth. Мы не смогли пройти мимо и проверили его качество с помощью PVS-Studio. Давайте же вместе посмотрим, где больше ин…
Как написать рефлексию для C++

Дата: 21 Июн 2022

Автор: Гость

C++ поистине противоречивый язык. Старый добрый С существует аж с 1972 года, С++ появился в 1985 и сохранил с ним обратную совместимость. За это время его не раз хоронили: сперва Java, теперь его пот…

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

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