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

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

![0881_duckstation_ru/image1.png](https://import.viva64.com/docx/blog/0881_duckstation_ru/image1.png)

## Введение

DuckStation – это эмулятор консоли Sony PlayStation\. Как видно по сайту, он имеет версию как для Windows и Linux, так и для смартфонов на Android\. А недавно его запустили на Xbox Series X и S\. Сам проект содержит чуть меньше миллиона строк исходного кода на языках С и С\+\+\. У DuckStation нет релизов, в него постоянно коммитят изменения, и поэтому для проверки пришлось зафиксировать SHA коммита: [_13c5ee8_](https://github.com/stenzek/duckstation/tree/13c5ee8bfb4f0f8fc40f76b39de58b5d9b473dc3)\.

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

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

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

[V726](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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);
  }
  ....
}
```

Здесь анализатор обнаружил ошибочный код, в котором осуществляется попытка удаления массива_, в_ыделенного на стеке\. Так как память не была выделена на куче, не стоит также и вызывать каких\-либо специальных функций, таких как s_td::free_ для её очистки – она будет произведена автоматически при уничтожении объекта\.

Также в процессе написания статьи, коллега, который проверял её, посчитал срабатывание ложным\. Этот интересный случай я описал в виде отдельной статьи, с которой приглашаю вас ознакомиться, она так и называется: [_История о том, как один разработчик защищал баг в проверяемом проекте от другого разработчика PVS\-Studio_](https://pvs-studio.ru/ru/blog/posts/cpp/0880/)\.

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

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'i < pathLength' is always true\. file\_system\.cpp 454

```cpp
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_ [нуль\-терминирована](https://pvs-studio.ru/ru/blog/terms/0088/)\. Тогда проверка _i < pathLength_ явно лишняя, и её можно убрать, так как условие будет всегда верным\. На последней итерации цикла мы получим нулевой символ и без неё\. Тогда код:

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

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

```cpp
char nextCh = Path[i + 1];
```

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

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

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

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

* [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'm\_value\.wSecond <\= other\.m\_value\.wSecond' is always true\. timestamp\.cpp 311
* [V779](https://pvs-studio.ru/ru/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. timestamp\.cpp 314

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'm\_value\.wSecond \>\= other\.m\_value\.wSecond' is always true\. timestamp\.cpp 427
* [V779](https://pvs-studio.ru/ru/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. timestamp\.cpp 430

На тему функций сравнения, кстати, есть отличная [статья](https://pvs-studio.ru/ru/blog/posts/cpp/0509/) моего коллеги, в которой он показывает разные паттерны ошибок такого типа\.

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

[V583](https://pvs-studio.ru/ru/docs/warnings/v583/) The '?:' operator, regardless of its conditional expression, always returns one and the same value\. gamelistmodel\.cpp 415

```cpp
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_ эквивалентен:

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions 'c \!\= ' '' to the left and to the right of the '&&' operator\. file\_system\.cpp 560

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions 'TokenMatch\(command, "CATALOG"\)' to the left and to the right of the '\|\|' operator\. cue\_parser\.cpp 196

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v1065/) Expression can be simplified, check 'm\_display\_texture\_height' and similar operands\. host\_display\.cpp 549

```cpp
....
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);
  ....
}
```

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

```cpp
read_y = m_display_texture_view_y - read_height;
```

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

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

[V614](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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_\. 

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

```cpp
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_ в четыре и код должен работать медленнее, однако я написал похожий синтетический [пример](https://godbolt.org/z/PWPT6P4ve)\. Как можно видеть, под оптимизациями _O2 _компиляторы _Сlang 13_ и _GCC_ _11\.2_ генерируют меньше ассемблерных инструкций для второго примера \(особенно это видно для _GCC_\)\.

## Заключение

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

А если вы хотите попробовать PVS\-Studio на своём проекте, то вы можете сделать это, скачав его [здесь](https://pvs-studio.ru/ru/pvs-studio/download/)\.