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.
The project is quite hefty. It contains about 300 thousand lines of C++ code and relies upon many external dependencies that include the following:
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:
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/ --intermodularAlright, the analysis is all done! Time to look at errors!
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:
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.
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:
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.
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.
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.
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:
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.
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.
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:
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;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.
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.
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.