# PVS\-Studio to check the RPCS3 emulator

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](https://import.viva64.com/docx/blog/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](https://pvs-studio.com/en/blog/posts/cpp/0871/);
* 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](https://import.viva64.com/docx/blog/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](https://pvs-studio.com/en/blog/posts/cpp/0851/) mode looks as follows:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v1061/) Extending the 'std' namespace may result in undefined behavior\. shared\_ptr\.hpp 1131

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

```cpp
/*
 * 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'rawcode \=\= CELL\_KEYC\_KPAD\_NUMLOCK' is always false\. cellKb\.cpp 126

```cpp
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](https://pvs-studio.com/en/docs/warnings/v557/) Array underrun is possible\. The value of 'month \+ \- 1' index could reach \-1\. cellRtc\.cpp 1470

```cpp
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](https://pvs-studio.com/en/blog/posts/cpp/0550/)\.

## Copy\-paste error

[V519](https://pvs-studio.com/en/docs/warnings/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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v581/) The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 4225, 4226\. PPUTranslator\.cpp 4226

```cpp
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](https://pvs-studio.com/en/docs/warnings/v581/) The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 758, 759\. RSXThread\.cpp 759

## Loop error

[V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: i \!\= 1\. PPUTranslator\.cpp 4252

```cpp
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](https://pvs-studio.com/en/docs/warnings/v595/) The 'cached\_dest' pointer was utilized before it was verified against nullptr\. Check lines: 3059, 3064\. texture\_cache\.h 3059

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

```cpp
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](https://en.cppreference.com/w/cpp/memory/new/nothrow) construction \- in this case the null pointer will be returned\.

Here are some more locations with this error:

* [V668](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'buffer' pointer\. A memory leak is possible\. rsx\_debugger\.cpp 380

```cpp
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:

```cpp
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:

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

## Incorrect size check

[V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The 'pad' index is pointing beyond array bound\. pad\_thread\.cpp 191

```cpp
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](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'current\_version < threshold\_version' is always false\. Unsigned type value is never < 0\. device\.cpp 91

```cpp
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](https://pvs-studio.com/en/pvs-studio/try-free/) 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](https://pvs-studio.com/en/blog/posts/0614/)\.