PVS-Studio ricerca i bug nel progetto DuckStation

PVS-Studio ricerca i bug nel progetto DuckStation

Controlliamo spesso i giochi retrò. Nella nostra azienda, molti sviluppatori amano trovare progetti interessanti per se stessi. Si sentono nostalgici quando studiano questi progetti. Ma dobbiamo eseguire giochi retrò su qualcosa, giusto? Questa volta abbiamo verificato un progetto che aiuta a eseguire vecchi giochi su hardware moderno.

Introduzione

DuckStation è un emulatore della console Sony PlayStation. L'emulatore, secondo il suo sito Web, ha una versione per Windows, Linux e per smartphone Android. E di recente è stato lanciato su Xbox Series X e S. Il progetto stesso contiene poco meno di un milione di righe di codice C e C++. DuckStation non rilascia aggiornamenti. I suoi sviluppatori effettuano regolarmente le modifiche. Quindi, abbiamo dovuto correggere lo SHA del commit:13c5ee8 .

Abbiamo controllato il progetto e trovato molti avvisi:170 di livello Alto e 434 di livello Medio. Diamo un'occhiata ai 10 più eccitanti.

Controlla i risultati

Avviso N1

V726 Un tentativo di liberare memoria contenente l'array 'wbuf' utilizzando la funzione 'free'. Questo non è corretto poiché "wbuf" è stato creato sullo 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);
  }
  ....
}

Qui l'analizzatore ha rilevato il codice con un errore. In questo frammento di codice, vediamo un tentativo di eliminare un array allocato nello stack. Poiché la memoria non è stata allocata nell'heap, non è necessario chiamare alcuna funzione speciale come std::free per cancellarla. Quando l'oggetto viene distrutto, la memoria viene cancellata automaticamente.

Inoltre, quando il mio collega stava modificando questo articolo, ha considerato questo avviso un falso positivo. Ho descritto questo caso interessante in un articolo separato. Quindi, ti invito a leggerlo:Come uno sviluppatore di PVS-Studio ha difeso un bug in un progetto verificato .

Avviso N2

V547 L'espressione 'i

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';
    ....
  }
  ....
}

La variabile di induzione i aumenta dopo l'inizializzazione di nextCh . A giudicare dal fatto che il strlen viene utilizzata per determinare la lunghezza della stringa, il Percorso la stringa è con terminazione null. Quindi i il controllo è chiaramente ridondante. Puoi saltare il controllo poiché la condizione sarà sempre vera. Durante l'ultima iterazione del ciclo, otterremo comunque il carattere nullo. Quindi il seguente codice:

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

è l'equivalente di:

char nextCh = Path[i + 1];

Tuttavia, anche se la stringa non fosse terminata con null, il controllo non sarebbe corretto. Durante l'iterazione finale del ciclo, quando si tenta di prendere l'ultimo carattere da Percorso[i + 1] , andrai al di fuori dei limiti dell'array. In questo caso, sarebbe meglio il seguente frammento di codice:

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

Avvertenze N3, N4

Per questo frammento di codice, l'analizzatore ha emesso due avvisi contemporaneamente:

  • L'espressione V547 'm_value.wSecond <=other.m_value.wSecond' è sempre vera. timestamp.cpp 311
  • V779 Rilevato codice irraggiungibile. È possibile che sia presente un errore. 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;
}

Qui l'operatore confronta i valori da un anno a millisecondi. Tuttavia, l'errore apparentemente si è già verificato nella riga di codice che ha confrontato i secondi. Il <= segno dimenticato (o stampato in modo errato) quando i secondi vengono controllati rende le operazioni successive irraggiungibili.

L'errore è stato ripetuto. La seconda volta era un operatore>= simile . L'analizzatore ha emesso anche due avvisi:

  • L'espressione V547 'm_value.wSecond>=other.m_value.wSecond' è sempre vera. timestamp.cpp 427
  • V779 Rilevato codice irraggiungibile. È possibile che sia presente un errore. timestamp.cpp 430

A proposito, il mio collega ha scritto un eccellente articolo sull'argomento delle funzioni di confronto. Nel suo articolo, mostra vari esempi di schemi simili agli errori descritti sopra.

Avviso N5

V583 L'operatore '?:', indipendentemente dalla sua espressione condizionale, restituisce sempre lo stesso valore. 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));
    }
  }
  ....
}

Abbiamo due confronti identici qui. Gli operandi dell'operatore condizionale, situati su entrambi i lati del segno maggiore e minore di, vengono semplicemente scambiati in due rami dell'operatore. In effetti, il frammento di codice nel ritorno operatore è equivalente a:

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

Probabilmente, il codice dovrebbe apparire come segue:

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

Avvisi N6, N7, N8

V501 Esistono sottoespressioni identiche 'c !='' a sinistra ea destra dell'operatore '&&'. 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 != '.')
  {
    ....
  }
  ....
}

In questo caso, si verifica due volte un ulteriore controllo dello spazio. Inoltre, l'analizzatore ha emesso alcuni avvisi simili:

V501 Sono presenti sottoespressioni identiche a sinistra ea destra di '|' operatore:KMOD_LCTRL | KMOD_LCTRL nomi_chiavi_sdl.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"}}
};

Qui abbiamo KMOD_LCTRL identici sottoespressioni a sinistra ea destra di | operatore. Sembra sospetto.

V501 Sono presenti sottoespressioni identiche 'TokenMatch(command, "CATALOG")' a sinistra ea destra di '||' operatore. 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")
      ||  ....)
  {
    ....
  }
  ....
}

Qui, il TokenMatch la funzione viene chiamata due volte.

Curiosamente, nel controllo sottostante, c'è anche un errore:comando viene scritto come una stringa letterale anziché una variabile. A proposito, volevamo stabilire una regola diagnostica che consentirà di monitorare tali situazioni. Questo frammento di codice è uno degli indicatori dell'utilità di tale diagnostica.

Forse, in tutti questi casi, al posto dei controlli ridondanti avrebbero dovuto esserci controlli per altri valori. Ecco perché i frammenti di codice non funzionano come previsto dagli sviluppatori che li hanno scritti.

Avviso N9

L'espressione V1065 può essere semplificata, controlla 'm_display_texture_height' e operandi simili. 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);
  ....
}

Sì, questo frammento di codice non contiene un errore. Ma possiamo abbreviare leggermente il codice semplificando l'espressione:

read_y = m_display_texture_view_y - read_height;

A dire il vero, questo non è un avvertimento serio e non dovrei aggiungerlo all'articolo. Tuttavia, ho aggiunto, semplicemente perché questo è l'avvertimento della mia diagnostica. Sono contento che abbia funzionato :)

Avviso N10

V614 Il puntatore intelligente 'host_interface' viene utilizzato subito dopo essere stato dichiarato o resettato. È sospetto che non gli sia stato assegnato alcun valore. 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;
}

Secondo la diagnostica, il codice contiene una variabile non inizializzata. C'è un controllo del puntatore intelligente senza senso in corso qui. Primo controllo:!host_interface restituirà sempre vero .

Sembrerebbe che l'errore non sia molto critico e il codice ridondante viene scritto per mantenere lo stile di codifica generale. È possibile riscrivere il codice in modo che sia ancora più leggibile:

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 {};
}

Sembra che ora abbiamo quattro ritorno affermazioni invece di una. Il codice dovrebbe funzionare più lentamente, tuttavia, ho scritto un esempio di codice sintetico simile. Come puoi vedere, sotto O2 ottimizzazioni, lo Slang 13 e GCC 11.2 i compilatori generano meno istruzioni di assembly per il secondo esempio (è particolarmente evidente per GCC ).

Conclusione

Anche se il progetto non è così grande, l'analizzatore ha emesso alcuni avvertimenti affascinanti. Spero che questo articolo aiuti gli sviluppatori di DuckStation a correggere alcuni bug. Forse vorranno ricontrollare la loro base di codice usando PVS-Studio.

Se vuoi provare PVS-Studio sul tuo progetto, puoi scaricarlo qui.