We often check retro games. In our company, many developers like to find interesting projects for themselves. They feel nostalgic when they're studying these projects. But we need to run retro games on something, right? This time we checked a project that helps to run old games on modern hardware.
DuckStation is an emulator of the Sony PlayStation console. The emulator, according to its website, has a version for Windows, Linux, and for Android smartphones. And recently it was launched on Xbox Series X and S. The project itself contains slightly less than a million lines of C and C++ code. DuckStation doesn't release updates. Its developers regularly commit changes. So, we had to fixate the SHA of the commit: 13c5ee8.
We checked the project and found a lot of warnings - 170 of the High level and 434 of the Medium level. Let's look at the 10 most exciting of them.
Warning 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);
}
....
}Here the analyzer detected code with an error. In this code fragment, we see an attempt to delete an array allocated on the stack. Since the memory has not been allocated on the heap, you don't need to call any special functions like std::free to clear it. When the object is destroyed, the memory is cleared automatically.
Also, when my colleague was editing this article, he considered this warning a false-positive. I described this interesting case in a separate article. So, I invite you to read it: How a PVS-Studio developer defended a bug in a checked project.
Warning 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';
....
}
....
}The induction variable i increases after the initialization of nextCh. Judging by the fact that the strlen function is used to determine the string length, the Path string is null-terminated. Then the i < pathLength check is clearly redundant. You can skip the check since the condition will always be true. During the last loop iteration, we will get the null character anyway. Then the following code:
char nextCh = (i < pathLength) ? Path[i + 1] : '\0';is the equivalent of:
char nextCh = Path[i + 1];However, even if the string wasn't null-terminated, the check would be incorrect. During the final loop iteration, when trying to take the last character from Path[i + 1], you will get outside the array boundaries. In this case, the following code fragment would be better:
char nextCh = ((i + 1) < pathLength) ? Path[i + 1] : '\0';Warnings N3, N4
For this code snippet, the analyzer issued two warnings at once:
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;
}Here the operator compares values from a year to milliseconds. However, the error apparently occurred already in the code line that compared seconds. The <= sign forgotten (or misprinted) when seconds are checked made subsequent operations unreachable.
The error was repeated. The second time it was a similar operator >=. The analyzer issued two warnings as well:
By the way, my colleague wrote an excellent article on the topic of comparison functions. In his article, he shows various examples of patterns similar to the errors described above.
Warning 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));
}
}
....
}We have two identical comparisons here. The conditional operator's operands, located on both sides of the greater than and less than signs, are simply swapped in two branches of the operator. In fact, the code fragment in the return operator is equivalent to:
return ascending ?
(static_cast<int>(left.type)
< static_cast<int>(right.type))
:
(static_cast<int>(left.type)
< static_cast<int>(right.type));Probably, the code should look as follows:
return ascending ?
(static_cast<int>(left.type)
< static_cast<int>(right.type))
:
(static_cast<int>(right.type)
< static_cast<int>(left.type));Warnings 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 != '.')
{
....
}
....
}In this case, an extra check for space occurs twice. Also, the analyzer issued a few more similar 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
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"}}
};Here we have identical KMOD_LCTRL sub-expressions to the left and to the right of the | operator. It looks suspicious.
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")
|| ....)
{
....
}
....
}Here, the TokenMatch function is called twice.
Intriguingly, in the check below, there is also an error: command is written as a string literal instead of a variable. By the way, we've been meaning to make a diagnostic rule that will allow to monitor such situations. This code fragment is one of the indicators that such diagnostic will be useful.
Maybe, in all these cases, in place of redundant checks there should have been checks for other values. That's why the code fragments do not work as expected by the developers who wrote them.
Warning 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);
....
}Yes, this code fragment doesn't contain an error. But we can slightly shorten the code by simplifying the expression:
read_y = m_display_texture_view_y - read_height;To tell the truth, this is not a serious warning and I should not add it to the article. However, I added, simply because this is the warning of my diagnostic. I am pleased that it worked :)
Warning 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;
}According to the diagnostic, the code contains an uninitialized variable. There is a meaningless smart pointer check going on here. First check: !host_interface will always return true.
It would seem that the error is not very critical, and the redundant code is written to maintain the overall coding style. It's possible to rewrite the code so it is even more readable:
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 {};
}Seems that now we have four return statements instead of one. Code is supposed to work slower, however, I wrote a similar synthetic code example. As you can see, under the O2 optimizations, the Slang 13 and GCC 11.2 compilers generate fewer assembly instructions for the second example (it is especially evident for GCC).
Even though the project is not that big, the analyzer issued some fascinating warnings. I hope this article will help the DuckStation developers fix some bugs. Maybe they will want to double-check their code base using PVS-Studio.
If you want to try PVS-Studio on your project, you can download it here.