RPCS3 – интересный проект, который эмулирует консоль PS3. Он активно развивается: недавно была новость о том, что он научился запускать все игры из своего каталога. Это хороший повод для проверки – посмотрим, какие ошибки остались после исправлений.
Проект довольно увесистый. Он насчитывает около 300 тысяч строк кода на C++ и к тому же тянет за собой большое количество внешних зависимостей, в числе которых есть:
Для GUI части также понадобится Qt, однако он берётся из системы. Полный список зависимостей можно посмотреть на скриншоте:
Что примечательно, используемый стандарт языка C++ – свежий C++20. PVS-Studio хорошо справился с проверкой такого современного кода, ведь мы активно работаем над поддержкой нововведений. Да, есть некоторые недоработки, но мы их постепенно исправим. В целом проверка проекта послужила хорошим тестом поддержки новых языковых конструкций.
Проект использует сборочную систему CMake. К сожалению, во время сборки у меня возникли проблемы – GCC 11.2 отказывался компилировать какую-то constexpr-конструкцию, однако Clang 12 справился со сборкой отлично. Сборку я производил под devel-версией Ubuntu, так что, возможно, это проблема дистрибутива.
Полная процедура сборки и проверки под Linux в режиме межмодульного анализа выглядит так:
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Что же, проект проверен – время перейти к ошибкам!
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);
}
}Стандарт C++ явно запрещает определение пользовательских шаблонов функций в пространстве имён std, а C++20 также запрещает определение специализаций шаблонов функций. Определение пользовательской функции swap – частый случай такой ошибки. В этой ситуации следует поступать таким образом:
При таком подходе работает механизм Argument-Dependent Lookup (ADL), в результате чего находится функция swap, которую мы определили рядом с классом. А пространство имён std не изменено.
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 );
}Частая ошибка – вызов memset будет удалён компилятором во время оптимизаций, а приватные данные останутся лежать в памяти. Да, в случае кода эмулятора вряд ли это представляет какую-либо угрозу утечки данных, но тем не менее ошибка присутствует.
Также были найдены ещё случаи такой ошибки:
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;
}
}Здесь ошибка кроется в первом условии: оно перекрывает условие ниже, которое проверяет значение переменной rawcode на равенство константе CELL_KEYC_KPAD_NUMLOCK. Значение CELL_KEYC_KPAD_NUMLOCK соответствует числу 0x53 – это число попадает под первое условие, которое в результате выполнения выходит из функции, так что нижний if никогда не выполнится.
Ошибка может быть в двух местах – либо в первом условии не учтено значение константы, либо сама константа определена неверно.
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]); // <=
}В этом случае в операторе return обращение к массиву DAYS_IN_MONTH может произойти по индексу -1, т.к. значение аргумента month может быть равно 0.
Скорее всего, ошибка находится в первом условии – судя по коду, отсчет месяцев ведётся с единицы, а условие проверяет на то, что month меньше 0, когда надо month < 1.
Эта ошибка напомнила мне интересный случай из проекта protobuf: 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;
// ....
{
};Классическая ошибка: при написании функции скопировали строку и забыли в ней поменять нужную переменную. Глазами заметить ошибку здесь сложно, но статический анализатор отлично справляется в таких случаях.
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);
}Похоже на ещё одну ошибку копипасты – видимо, скопировали условие и забыли его поменять, однако then-часть теперь другая.
Что интересно, это не единственный случай такой ошибки. В коде было ещё одно срабатывание:
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);
}Проверка переменной i на равенство 1 и 2 никогда не выполнится – цикл работает с числами от 16 до 20. Возможно, этот код переписывался и индексы забыли поменять на корректные.
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;
}Ещё один частый паттерн – сначала используется указатель, а затем проверяется. Опять же, возможно, что ошибка появилась в процессе изменений этого фрагмента кода.
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);
}Проверка на nullptr здесь бессмысленна: при вызове new в случае ошибки будет брошено исключение std::bad_alloc. Если бросать исключение не требуется, то следует использовать конструкцию std::nothrow – тогда как раз вернётся нулевой указатель.
Ещё места с подобной ошибкой:
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;
}В начале функции видим выделение памяти через malloc, и если вернулся nullptr, то выходим. Пока всё хорошо. Но дальше идут проверки параметров width и height – они происходят уже после выделения памяти, и в случае успеха из функции также возвращается nullptr. Да, при равенстве этих переменных нулю malloc будет выделять 0 байт, однако стандарт говорит, что в этом случае может вернуться как nullptr, так и валидный указатель, который нельзя разыменовывать. Но освободить его все равно нужно, к тому же, free способна принимать и нулевой указатель. Так что исправление может выглядеть таким образом:
if (!buffer || width == 0 || height == 0)
{
std::free(buffer)
return nullptr;
}Либо проверки на 0 можно вообще убрать – цикл в этом случае не выполнится:
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;
}
}При проверке входных данных использован оператор > вместо >=, в результате чего значение pad может быть равно размеру контейнера m_pads, поэтому при дальнейшем обращении к контейнеру есть вероятность выхода за границу.
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(....);
}
}
}Константа threshold_version всегда будет равна 0, т.к. вместо сдвига влево используется сдвиг вправо. Сдвиг вправо эквивалентен делению на степень двойки – в нашем случае на 2^22 и 2^14 соответственно. Очевидно, что числа из выражений меньше этих степеней, поэтому результат будет равен 0.
Похоже, что это место было скопировано из кода декодирования значения версии, но при копировании поменять операторы просто забыли.
В результате проверки анализатор нашёл в проекте ошибки разного рода: традиционно были выявлены опечатки, а также есть логические ошибки, где код просто не тестировался. Надеемся, что эта проверка поможет исправить пару багов, а также желаем разработчикам эмулятора хорошей поддержки игр и отличной производительности. А вы можете попробовать триальную версию анализатора PVS-Studio и посмотреть, какие ошибки вы найдёте у себя. А если вы разрабатываете какую-то открытую игру, эмулятор либо просто open-source проект, то рассмотрите вариант бесплатного лицензирования.