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

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

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

03.11.2021 17:32

Stolyarrr 2 1400 Источник

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

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 на своём проекте, то вы можете сделать это, скачав его здесь.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Vladislav Stolyarov. How a PVS-Studio developer defended a bug in a checked project.

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

vvviperrr
04.11.2021 15:41
#23667366
по поводу первого варнинга:
```
int main(void)
{
    char buf[128];
    char *ptr = buf;
    free(ptr);
    return 0;
}
```
```
[viper@viper-manjaro ~]$ g++ main.cpp 
main.cpp: In function ‘int main()’:
main.cpp:12:13: warning: ‘void free(void*)’ called on unallocated object ‘buf’ [-Wfree-nonheap-object]
   12 |         free(ptr);
      |         ~~~~^~~~~
main.cpp:8:10: note: declared here
    8 |     char buf[128];
      |  
```
gcc и так понял, что происходит косвенное освобождение non-heap памяти. про явное и подавно ругнется.

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

хз почему, но читается очень тяжело. зачем писать - this is incorrect в ошибках? бывают корректные ошибки? "free() called on stack allocated object (wbuf)".
1. Andrey2008
  04.11.2021 18:56
  #23667966
  +2
  gcc и так понял
  Да. Но нашли ошибку мы. :) Мощь PVS-Studio.

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

Введение

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

Заключение

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

vvviperrr

Andrey2008