To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (an extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
PVS-Studio searches for bugs in the Duc…

PVS-Studio searches for bugs in the DuckStation project

Nov 03 2021

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.

0881_duckstation/image1.png

Introduction

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.

Check results

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:

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

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:

  • 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

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).

Conclusion

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.

Popular related articles
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept