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 to check the RPCS3 emulator

PVS-Studio to check the RPCS3 emulator

Nov 15 2021
Author:

RPCS3 is an interesting project that emulates the PS3 console. It is actively evolving. Recently we heard the news that the emulator learned how run all the games from the console's catalog. That's a good excuse to analyze the project. We'll see which errors remained after new fixes were added to the project.

0886_rpcs3/image1.png

Introduction

The project is quite hefty. It contains about 300 thousand lines of C++ code and relies upon many external dependencies that include the following:

  • llvm, a toolkit for writing compilers and utilities. By the way, we've recently checked LLVM 13;
  • ffmpeg, a library for working with media files;
  • curl, helpful in network interactions and for work with the HTTP protocol;
  • zlib, a data compression library that uses the DEFLATE algorithm.

For the GUI part, the project uses Qt - however, that is taken from the system library. The screenshot below demonstrates the full list of dependencies:

0886_rpcs3/image2.png

Note, that the C++ standard used, is the latest one, C++20. PVS-Studio handles checking such modern code very well. This is because we are constantly working to support innovations. Yes, there are some things to improve yet - and we are working on fixing them. Overall, the check was a good test of how the analyzer supports new language constructs.

The RPCS3 project uses the CMake build system. Unfortunately, I experienced some problems during the build - GCC 11.2 refused to compile some constexpr construction. Clang, however, handled the build perfectly. I built the project on Ubuntu's developer version, so the problem I experienced could be related to the distribution.

The entire procedure of building and checking the project on Linux in the intermodular analysis mode looks as follows:

cmake -S. -Bbuild -DCMAKE_EXPORT_COMPILE_COMMANDS=On -DCMAKE_BUILD_TYPE=Debug \
          -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
cmake --build build -j$(nproc)
pvs-studio-analyzer analyze -f ./build/compile_commands.json -j`nproc` \
          -o pvs.log -e 3rdparty/ -e llvm/ --intermodular

Alright, the analysis is all done! Time to look at errors!

Don't code in std, bro

V1061 Extending the 'std' namespace may result in undefined behavior. shared_ptr.hpp 1131

namespace std
{
  template <typename T>
  void swap(stx::single_ptr<T>& lhs, stx::single_ptr<T>& rhs) noexcept
  {
    lhs.swap(rhs);
  }

  template <typename T>
  void swap(stx::shared_ptr<T>& lhs, stx::shared_ptr<T>& rhs) noexcept
  {
    lhs.swap(rhs);
  }
}

The C++ standard explicitly prohibits defining user function templates in the std namespace. C++20 also prohibits defining specializations for function templates. Defining the swap custom function is a frequent error of this kind. In this case, you can do the following:

  • define the swap function in the same namespace where the class is defined (stx);
  • add the using std::swap directive to the block that requires calling the swap function;
  • call swap without specifying the std namespace, i.e. do an unqualified function call: swap(obj1, obj2);

This approach uses the Argument-Dependent Lookup (ADL) mechanism. As a result, the compiler finds the swap function that we defined next to the class. The std namespace remains unchanged.

Deleted memset

V597 The compiler could delete the 'memset' function call, which is used to flush 'cty' object. The memset_s() function should be used to erase the private data. aes.cpp 596

/*
 * AES key schedule (decryption)
 */
int aes_setkey_dec(....)
{
    aes_context cty;

    // ....

done:
    memset( &cty, 0, sizeof( aes_context ) );

    return( 0 );
}

This is a frequent error. When optimizing the code, the compiler removes the memset call, while private data remains in the memory. Yes, in case of the emulator this hardly poses any data leakage threat - but either way, the error is present.

PVS-Studio found more locations with this type of an error:

  • V597 The compiler could delete the 'memset' function call, which is used to flush 'tmpbuf' buffer. The memset_s() function should be used to erase the private data. sha1.cpp 371
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The memset_s() function should be used to erase the private data. sha1.cpp 396

Redundant check

V547 Expression 'rawcode == CELL_KEYC_KPAD_NUMLOCK' is always false. cellKb.cpp 126

enum Keys
{
  // ....
  CELL_KEYC_KPAD_NUMLOCK          = 0x53,
  // ....
};

u16 cellKbCnvRawCode(u32 arrange, u32 mkey, u32 led, u16 rawcode)
{
  // ....

  // CELL_KB_RAWDAT
  if (rawcode <= 0x03
      || rawcode == 0x29
      || rawcode == 0x35
      || (rawcode >= 0x39 && rawcode <= 0x53)    // <=
      || rawcode == 0x65
      || rawcode == 0x88
      || rawcode == 0x8A
      || rawcode == 0x8B)
  {
    return rawcode | 0x8000;
  }

  const bool is_alt = mkey & (CELL_KB_MKEY_L_ALT | CELL_KB_MKEY_R_ALT);
  const bool is_shift = mkey & (CELL_KB_MKEY_L_SHIFT | CELL_KB_MKEY_R_SHIFT);
  const bool is_caps_lock = led & (CELL_KB_LED_CAPS_LOCK);
  const bool is_num_lock = led & (CELL_KB_LED_NUM_LOCK);

  // CELL_KB_NUMPAD

  if (is_num_lock)
  {
    if (rawcode == CELL_KEYC_KPAD_NUMLOCK)  return 0x00 | 0x4000; // <=
    if (rawcode == CELL_KEYC_KPAD_SLASH)    return 0x2F | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_ASTERISK) return 0x2A | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_MINUS)    return 0x2D | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_PLUS)     return 0x2B | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_ENTER)    return 0x0A | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_0)        return 0x30 | 0x4000;
    if (rawcode >= CELL_KEYC_KPAD_1 && rawcode <= CELL_KEYC_KPAD_9)
      return (rawcode - 0x28) | 0x4000;
  }
}

Here the error is hidden in the first condition: this condition blocks the condition below that checks whether the rawcode variable value equals the CELL_KEYC_KPAD_NUMLOCK constant value. The CELL_KEYC_KPAD_NUMLOCK value corresponds to 0x53 - this number meets the first condition, so the function exits there. Consequently, the lower if block is never executed.

The error could have been caused by one of the two things - either the first condition does not take the constant's value into account, or the constant is defined incorrectly.

Array overflow

V557 Array underrun is possible. The value of 'month + - 1' index could reach -1. cellRtc.cpp 1470

error_code cellRtcGetDaysInMonth(s32 year, s32 month)
{
  cellRtc.todo("cellRtcGetDaysInMonth(year=%d, month=%d)", year, month);

  if ((year < 0) || (month < 0) || (month > 12))
  {
    return CELL_RTC_ERROR_INVALID_ARG;
  }

  if (is_leap_year(year))
  {
    return not_an_error(DAYS_IN_MONTH[month + 11]);
  }

  return not_an_error(DAYS_IN_MONTH[month + -1]); // <=
}

In the code above, the month argument value can be 0. Consequently, the return operator may attempt to access the DAYS_IN_MONTH array's element that has the -1 index.

Most likely, the error is in the first condition. The code above counts months from one, while the condition makes sure that month is no less than zero. The correct condition would be month < 1.

This error reminded me of an interesting case from the protobuf project: February 31.

Copy-paste error

V519 The 'evnt->color.white_x' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 51, 52. sys_uart.cpp 52

struct av_get_monitor_info_cmd : public ps3av_cmd
{
  bool execute(....) override
  {
    // ....
    evnt->color.blue_x = 0xFFFF;
    evnt->color.blue_y = 0xFFFF;
    evnt->color.green_x = 0xFFFF;
    evnt->color.green_y = 0xFFFF;
    evnt->color.red_x = 0xFFFF;
    evnt->color.red_y = 0xFFFF;
    evnt->color.white_x = 0xFFFF;
    evnt->color.white_x = 0xFFFF; // <=
    evnt->color.gamma = 100;
    // ....
  {
};

That's a common error: when writing a function, a developer copied a line and forgot to change the required variable. And it's quite a challenge to spot this error by just reading the code - while the static analyzer does an excellent job in these cases.

Repeated checks

V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 4225, 4226. PPUTranslator.cpp 4226

void PPUTranslator::MTFSFI(ppu_opcode_t op)
{
  SetFPSCRBit(op.crfd * 4 + 0, m_ir->getInt1((op.i & 8) != 0), false);
  if (op.crfd != 0) SetFPSCRBit(op.crfd * 4 + 1,
                                m_ir->getInt1((op.i & 4) != 0), false);
  if (op.crfd != 0) SetFPSCRBit(op.crfd * 4 + 2,
                                m_ir->getInt1((op.i & 2) != 0), false);
  SetFPSCRBit(op.crfd * 4 + 3, m_ir->getInt1((op.i & 1) != 0), false);

  if (op.rc) SetCrFieldFPCC(1);
}

This looks like another copy-paste error. Most likely, someone copied the condition and forgot to change it. However, the then part is now different.

Interestingly, this is not the only case of such an error. The analyzer found one more error of this kind:

  • V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 758, 759. RSXThread.cpp 759

Loop error

V560 A part of conditional expression is always true: i != 1. PPUTranslator.cpp 4252

void PPUTranslator::MTFSF(ppu_opcode_t op)
{
  const auto value = GetFpr(op.frb, 32, true);

  for (u32 i = 16; i < 20; i++)
  {
    if (i != 1 && i != 2 && (op.flm & (128 >> (i / 4))) != 0)
    {
      SetFPSCRBit(i, Trunc(m_ir->CreateLShr(value, i ^ 31),
                  GetType<bool>()), false);
    }
  }

  if (op.rc) SetCrFieldFPCC(1);
}

The for-loop above works with numbers from 16 to 20, which means that the condition of the if-block inside the loop is never met and the i variable value is never evaluated against 1 and 2. Maybe someone refactored this code and forgot to change the indexes to the correct ones.

Pointer dereferencing before check

V595 The 'cached_dest' pointer was utilized before it was verified against nullptr. Check lines: 3059, 3064. texture_cache.h 3059

template <typename surface_store_type, typename blitter_type, typename ...Args>
blit_op_result upload_scaled_image(....)
{
  // ....

  if (!use_null_region) [[likely]]
  {
    // Do preliminary analysis
    typeless_info.analyse();

    blitter.scale_image(cmd, vram_texture, dest_texture, src_area, dst_area,
                        interpolate, typeless_info);
  }
  else
  {
    cached_dest->dma_transfer(cmd, vram_texture, src_area, // <=
                              dst_range, dst.pitch);
  }

  blit_op_result result = true;

  if (cached_dest) // <=
  {
    result.real_dst_address = cached_dest->get_section_base();
    result.real_dst_size = cached_dest->get_section_size();
  }
  else
  {
    result.real_dst_address = dst_base_address;
    result.real_dst_size = dst.pitch * dst_dimensions.height;
  }

  return result;
}

We can see one more frequent pattern here - first, a pointer is used, and only then is it checked. Again, someone could have unknowingly created this error when modifying the code.

Checking 'new' result for null

V668 There is no sense in testing the 'movie' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. movie_item.h 56

void init_movie(const QString& path)
{
  if (path.isEmpty() || !m_icon_callback) return;

  if (QMovie* movie = new QMovie(path); movie && movie->isValid())
  {
    m_movie = movie;
  }
  else
  {
    delete movie;
    return;
  }

  QObject::connect(m_movie, &QMovie::frameChanged, m_movie, m_icon_callback);
}

Checking for nullptr is pointless here: if the new call causes an error, the std::bad_alloc exception is thrown. If there's no need to throw an exception, one can use the std::nothrow construction - in this case the null pointer will be returned.

Here are some more locations with this error:

  • V668 There is no sense in testing the 'm_render_creator' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. emu_settings.cpp 75
  • V668 There is no sense in testing the 'trophy_slider_label' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. trophy_manager_dialog.cpp 216

Memory leak

V773 The function was exited without releasing the 'buffer' pointer. A memory leak is possible. rsx_debugger.cpp 380

u8* convert_to_QImage_buffer(rsx::surface_color_format format,
                             std::span<const std::byte> orig_buffer,
                             usz width, usz height) noexcept
{
  u8* buffer = static_cast<u8*>(std::malloc(width * height * 4));
  if (!buffer || width == 0 || height == 0)
  {
    return nullptr;
  }
  for (u32 i = 0; i < width * height; i++)
  {
    // depending on original buffer, the colors may need to be reversed
    const auto &colors = get_value(orig_buffer, format, i);
    buffer[0 + i * 4] = colors[0];
    buffer[1 + i * 4] = colors[1];
    buffer[2 + i * 4] = colors[2];
    buffer[3 + i * 4] = 255;
  }
  return buffer;
}

At the beginning, the function uses malloc to allocate memory. If nullptr is returned, the function exits. So far so good. Then the width and height parameters are checked - this takes place after the memory has been allocated. In case of success, the function also returns nullptr. Yes, if these variables equal zero, malloc returns 0 bytes. However, the standard states that in this case the function may return either nullptr or a valid pointer that cannot be dereferenced. But either way, it needs to be freed. Besides, free is also capable of accepting a null pointer. So the fix may look like this:

if (!buffer || width == 0 || height == 0)
{
  std::free(buffer)
  return nullptr;
}

Alternatively, you can remove checks for 0 altogether - the loop will not be executed in this case:

if (!buffer)
{
  return nullptr;
}
for (u32 i = 0; i < width * height; i++)
{
  // ....
}
return buffer;

Incorrect size check

V557 Array overrun is possible. The 'pad' index is pointing beyond array bound. pad_thread.cpp 191

void pad_thread::SetRumble(const u32 pad, u8 largeMotor, bool smallMotor)
{
  if (pad > m_pads.size())
    return;

  if (m_pads[pad]->m_vibrateMotors.size() >= 2)
  {
    m_pads[pad]->m_vibrateMotors[0].m_value = largeMotor;
    m_pads[pad]->m_vibrateMotors[1].m_value = smallMotor ? 255 : 0;
  }
}

The code above uses the > operator instead of >= to check input data. As a result, the pad value can be equal to the m_pads container size. This may cause an overflow when the container is accessed the next time.

Shift in wrong direction

V547 Expression 'current_version < threshold_version' is always false. Unsigned type value is never < 0. device.cpp 91

void physical_device::create(VkInstance context,
                             VkPhysicalDevice pdev,
                             bool allow_extensions)
{
  else if (get_driver_vendor() == driver_vendor::NVIDIA)
  {
#ifdef _WIN32
    // SPIRV bugs were fixed in 452.28 for windows
    const u32 threshold_version = (452u >> 22) | (28 >> 14);
#else
    // SPIRV bugs were fixed in 450.56 for linux/BSD
    const u32 threshold_version = (450u >> 22) | (56 >> 14);
#endif
    // Clear patch and revision fields
    const auto current_version = props.driverVersion & ~0x3fffu;
    if (current_version < threshold_version)
    {
      rsx_log.error(....);
    }
  }
}

The threshold_version constant is always 0, because the right shift is used instead of the left shift. The right shift is equivalent to dividing by a power of two - in our case, by 2^22 and 2^14 respectively. It is obvious that the values from the expressions above are less than these powers. This means the result is always zero.

Looks like someone copied this snippet from the code that decoded version values and forgot to change the operators.

Conclusion

The analyzer checked the project and found various errors: from traditional ones - like typos - to more intricate issues like logical errors caused by the fact that some parts of the code were not tested. We hope that this check will help fix a couple of bugs. We also hope the emulator's developers keep up the great work supporting games and we wish their emulator excellent performance. Got curious? You can download the PVS-Studio analyzer's trial version and see what errors it finds in your code. And if you are developing an open-source game or project, we invite you to consider our free license.

Popular related articles
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…
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…
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…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
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 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 Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
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…
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…
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…

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