Мы используем куки, чтобы пользоваться сайтом
было удобно.
Вебинар: Go-Go-Gadg...Error? Смотрим, как ошибаются Go разработчики! - 26.08
20 августа исполнилось 136 лет Говарду Филлипсу Лавкрафту, человеку, создавшему свой собственный жанр "лавкрафтовских ужасов". Многие пытались копировать или развивать его идеи, но получалось только у считанных единиц.
Удачным примером можно назвать популярную игру Bloodborne, мир которой вдохновлён творчеством Лавкрафта, но имеет свою уникальную идентичность. Долгое время геймеры лишь мечтали запустить её на ПК, и вот, наконец, это стало возможным. А всё благодаря нашему сегодняшнему герою — эмулятору ShadPS4.
В нашей проверке участвовал код непосредственно ShadPS4 без всяческих сторонних библиотек. За основу взят коммит 24e68ba main-ветки. Ошибки брались из High и Medium уровней группы общего назначения. Комментарии вида //! добавлены мной.
Приведённые ссылки на конкретные коммиты добавлены для упрощения верифицирования и исправления найденных ошибок и не имеют цели скомпрометировать или каким-то образом принизить их авторов. Всем контрибуторам проекта большая личная благодарность от фаната Лавкрафта и игр Миядзаки.
В отличие от моей прошлой проверки llvm здесь не будет примеров "кривого мёржа", наложения двух реализаций и прочих артефактов совместной разработки. Охотник никогда не одинок, но всё же их всегда немного. Начнём же охоту, о, я не могу дождаться... хи-хи...
Фрагмент N1. Fear your blindness
Предупреждение PVS-Studio: V557 Array overrun is possible. The 3 index is pointing beyond array bound. input_handler.h 397
class InputBinding {
public:
InputID keys[3];
InputBinding(....) {
....
if (k1 <= k2 && k1 <= k3) {
....
} else if (k2 <= k1 && k2 <= k3) {
....
} else {
keys[0] = k3;
if (k1 <= k2) {
keys[1] = k1;
keys[2] = k2;
} else {
keys[1] = k2;
keys[3] = k1;
}
}
}
}Выход за границу буфера в чистейшем виде. Весь файл добавили в огромном коммите с горой разных фич и, скорее всего, глаз автора просто замылился.
Так вышло, что пока писалась статья, эту ошибку нашли и исправили, заменив keys[3] = k1 на keys[2] = k1. Но полтора года зловреду всё же удалось прожить в проекте.
Фрагмент N2. No mercy for "Liar"
Предупреждение PVS-Studio: V547 Expression m_streams.size() >= 0 is always true. Unsigned type value is always >= 0. avplayer_source.cpp 132
class AvPlayerSource{
....
std::vector<Stream> m_streams;
}
bool AvPlayerSource::FindStreams() {
....
return m_streams.size() >= 0;
}Давайте попробуем разобраться, присутствует ли здесь ошибка. Сразу замечу, что m_streams всегда был вектором, а значит его size всегда был беззнаковым.
Фрагмент менялся в этом коммите: функция AvPlayerSource::FindStreams заменила собой AvPlayerSource::HasStreams, утащив также часть кода из AvPlayerSource::Init.
bool AvPlayerSource::HasStreams() {
return m_streams.size() >= 0;
}Если копнуть ещё глубже, то у AvPlayerSource::HasStreams тоже был предшественник:
bool AvPlayerSource::FindStreamInfo() {
if (m_avformat_context == nullptr) {
LOG_ERROR(Lib_AvPlayer, "Could not find stream info. NULL context.");
return false;
}
if (m_avformat_context->nb_streams > 0) {
return true;
}
return avformat_find_stream_info(m_avformat_context.get(), nullptr) == 0;
}m_avformat_context — умный указатель на тип AVFormatContext, а поле nb_streams объявлено так:
unsigned int nb_streams;Как видим, в прошлом действия были менее тавтологичны.
Если же поискать использование AvPlayerSource::FindStreams, то оно присутствует только в функции AvPlayerState::ProcessEvent:
void AvPlayerState::ProcessEvent() {
....
case AvEventType::AddSource: {
std::shared_lock lock(m_source_mutex);
if (m_up_source->FindStreams()) { //!HasStreams() ранее,
//!FindStreamInfo() ещё ранее
SetState(AvState::Ready);
OnPlaybackStateChanged(AvState::Ready);
} else {
OnWarning(ORBIS_AVPLAYER_ERROR_NOT_SUPPORTED);
SetState(AvState::Error);
}
break;
}
....
}Как итог, всё указывает на то, что разработчик сделал опечатку >= при создании функции AvPlayerSource::HasStreams, а потом скопипастил её в функцию AvPlayerSource::FindStreams.
Фрагмент N3. Let us cleanse these foul streets
Предупреждения PVS-Studio:
V568 It's odd that the argument of sizeof() operator is the sizeof (int) * 512 expression. aio.cpp 318
V1086 A call of the memset function will lead to underflow of the buffer id_state. aio.cpp 318
namespace Libraries::Kernel {
#define MAX_QUEUE 512
static s32* id_state;
static s32 id_index;
....
}
void RegisterAio(Core::Loader::SymbolsResolver* sym) {
id_index = 1;
id_state = (int*)malloc(sizeof(int) * MAX_QUEUE);
memset(id_state, 0, sizeof(sizeof(int) * MAX_QUEUE));
....
}Файл добавлен целиком.
Выражение sizeof(int) * 512 имеет тип size_t, соответственно sizeof(sizeof(int) * MAX_QUEUE)) будет равно размеру size_t. Поскольку его размер обычно 4 или 8 байт, обнулится в итоге только часть массива. С большой долей уверенности можно предположить, что внешний sizeof лишний и предполагалось заполнить нулями весь буфер. Однако найти примеры использования с конкретными данными сложно, т. к. читаются элементы массива только в семействе функций sceKernelAio***, которые сохраняются и вызывается как указатели. Надеюсь, "охотники" этого мира обратят внимание и либо очистят всё целиком, либо вставят чёткий знак, почему это не нужно.
Фрагмент N4. Beware of "Attack from behind"
Предупреждение PVS-Studio: V595 The chunkIds pointer was utilized before it was verified against nullptr. Check lines: 173, 179.
s32 PS4_SYSV_ABI scePlayGoGetProgress(....,
const OrbisPlayGoChunkId* chunkIds, ....)
{
LOG_DEBUG(Lib_PlayGo, "called handle = {}, chunkIds = {},
numberOfEntries = {}", handle,
*chunkIds, numberOfEntries);
if (handle != PlaygoHandle) {
return ORBIS_PLAYGO_ERROR_BAD_HANDLE;
}
if (chunkIds == nullptr || outProgress == nullptr) {
return ORBIS_PLAYGO_ERROR_BAD_POINTER;
}
....
}Макрос LOG_DEBUG определён в файле log.h,и после подстановки код выглядит так:
do {
if (auto logger = Common::Log::ALL_LOGGERS[Common::Log::Class::Lib_PlayGo]){
logger->log(
spdlog::level::debug,
"[{}] <{}> ({}) {}:{} {}: "
"called handle = {}, chunkIds = {}, numberOfEntries = {}",
Common::Log::Class::Lib_PlayGo,
Common::Log::to_string_view(spdlog::level::debug),
Common::GetCurrentThreadName(),
spdlog::source_loc::basename(
"path to playgo.cpp"),
174, std::string_view(__func__) == "operator()" ? "lambda" : __func__,
handle,
*chunkIds, numberOfEntries);
}
} while (false);Как видим, при chunkIds == nullptr получаем классическое UB, от которого может спасти только отсутствие в таком случае логера для Common::Log::Class::Lib_PlayGo. Но, увы, все логеры создаются в функции main на старте приложения и живут вместе с ним.
Несмотря на то, что логгируется только debug информация, вызов функции logger::log, а с ним и потенциальное UB, произойдёт и в release — излишние записи отсекаются уже внутри.
Сам макрос — немного разный по форме, но с разыменованием во всех версиях, — был всегда, а вот код после него добавлен позже.
Судя по тексту коммита, разработчик расширил поддержку системы PlayGo, той самой, что позволяет начать охоту, не дожидаясь скачивания игры целиком. Функция перестала быть просто заглушкой, получив и необходимые проверки, а безусловное разыменование chunkIds, видимо, выпало из поля зрения автора. Возможных исправлений тут множество, и они довольно очевидны: всё зависит от стилистики и формата логов, принятых в проекте. Я бы предложил такой вариант:
LOG_DEBUG(Lib_PlayGo, "called handle = {}, chunkIds = {},
numberOfEntries = {}", handle,
chunkIds ? *chunkIds : static_cast<32>(-1),
numberOfEntries);Фрагмент N5. Oh, Yourself, please, carry on in my stead
Предупреждение PVS-Studio: V570 The same value is assigned twice to the inComment variable. text_editor.cpp 2109
void TextEditor::ColorizeInternal()
{
....
if (mCheckComments) {
....
while (currentLine < endLine || currentIndex < endIndex) {
....
//!~50 насыщенных строк кода, в которых могут измениться
//!commentStartLine и commentStartIndex
....
bool inComment =
(commentStartLine < currentLine ||
(commentStartLine == currentLine &&
commentStartIndex <= currentIndex));
.....
inComment = inComment =
(commentStartLine < currentLine ||
(commentStartLine == currentLine &&
commentStartIndex <= currentIndex));
....
}
}
}Добавлено единым огромным коммитом.
Очевидно, что присваивание inComment самой себе здесь бессмысленно, но как это произошло?
Как мне кажется, автор написал декларацию inComment, затем скопировал декларатор с инициализатором из верхней строки целиком и всё вставил ниже, не заметив подвоха. Поэтому повторное присваивание здесь просто лишнее. Другой возможный вариант: вместо = должно было быть == или !=, но это не особо вяжется с логикой алгоритма. Хотя, конечно, последнее слово может сказать только сам разработчик или его "боевые товарищи".
Фрагмент N6. Treat "The unseen" with care
Предупреждение PVS-Studio: V634 The priority of the * operation is higher than that of the << operation. Its possible that parentheses should be used in the expression. liverpool_to_vk.cpp 759
// Table 8.13 Data and Image Formats
//[Sea Islands Series Instruction Set Architecture]
//All values are under 64
static const size_t amd_gpu_data_format_bit_size = 6;
//All values are under 16
static const size_t amd_gpu_number_format_bit_size = 4;
static auto surface_format_table = []() constexpr {
std::array<vk::Format,
1 << amd_gpu_data_format_bit_size * 1 << amd_gpu_number_format_bit_size>
result;
for (auto& entry : result) {
entry = vk::Format::eUndefined;
}
for (const auto& supported_format : SurfaceFormats()) {
result[GetSurfaceFormatTableIndex(supported_format.data_format,
supported_format.number_format)] =
supported_format.vk_format;
}
return result;
}();Строка:
1 << amd_gpu_data_format_bit_size * 1 << amd_gpu_number_format_bit_sizeРавносильна:
1 << 6 * 1 << 4Что из-за приоритета операций будет интерпретировано как:
(1 << (6 * 1)) << 4Что равно 1024.
Судя по функции GetSurfaceFormatTableIndex, ожидается, что максимальный индекс в result как раз меньше 1024:
static size_t GetSurfaceFormatTableIndex(AmdGpu::DataFormat data_format,
AmdGpu::NumberFormat num_format) {
DEBUG_ASSERT(u32(data_format) < 1 << amd_gpu_data_format_bit_size);
DEBUG_ASSERT(u32(num_format) < 1 << amd_gpu_number_format_bit_size);
size_t result = static_cast<size_t>(num_format) |
(static_cast<size_t>(data_format) <<
amd_gpu_number_format_bit_size);
return result;
}Что вполне согласуется с документацией AMD из комментария: 64 формата данных и 14 способов их интерпретации шейдерами.
Но смущает * 1, как будто хотели написать:
(1 << amd_gpu_data_format_bit_size) * ( 1 << amd_gpu_number_format_bit_size)Что тоже равно 1024, однако забыли про приоритет операции и при этом всё равно получили правильный результат.
Все функции и константы добавлены единым коммитом, поэтому проблемы кооперации исключаем.
В итоге ошибки вроде бы и нет, но код получился хрупким, и какой-нибудь неискушённый разработчик в будущем может здесь легко оступиться и пасть перед ужасами приоритета операций.
Фрагмент N7. "Strong foe" waits ahead but "don't give up"
Предупреждение PVS-Studio: V579 The ZydisDecoderDecodeFull function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. decoder.cpp 29
Ранее файл назывался Disassembler.cpp:
void DecoderImpl::printInstruction(void* code, u64 address) {
ZydisDecodedInstruction instruction;
ZydisDecodedOperand operands[ZYDIS_MAX_OPERAND_COUNT_VISIBLE];
ZyanStatus status =
ZydisDecoderDecodeFull(&m_decoder, code, sizeof(code),
&instruction, operands);
if (!ZYAN_SUCCESS(status)) {
fmt::print("decode instruction failed at {}\n", fmt::ptr(code));
} else {
printInst(instruction, operands, address);
}
}Для функции ZydisDecoderDecodeFull есть поясняющий комментарий:
/**
* @param buffer A pointer to the input buffer.
* @param length The length of the input buffer.
* Note that this can be bigger than the
* actual size of the instruction -- you don`t have to know
* the size up front. This length is merely used to prevent
* Zydis from doing out-of-bounds reads on your buffer.
**/Довольно подозрительно выглядит передача в функцию указателя на некий буфер и размера указателя в качестве длины этого буфера.
Путь параметра length довольно извилистый, но в какой-то момент он сохраняется в поле структуры:
state.buffer_len = length;И далее используется для ограничения парсинга инструкции, например:
static ZyanStatus ZydisInputPeek(ZydisDecoderState* state,
ZydisDecodedInstruction* instruction, ZyanU8* value)
{
....
if (state->buffer_len > 0)
{
*value = state->buffer[0];
return ZYAN_STATUS_SUCCESS;
}
return ZYDIS_STATUS_NO_MORE_DATA;
}
static ZyanStatus ZydisInputNext(ZydisDecoderState* state,
ZydisDecodedInstruction* instruction, ZyanU8* value)
{
....
if (state->buffer_len > 0)
{
*value = state->buffer++[0];
++instruction->length;
--state->buffer_len;
return ZYAN_STATUS_SUCCESS;
}
return ZYDIS_STATUS_NO_MORE_DATA;
}Ранее вместо sizeof(code) была константа:
#define ZYDIS_MAX_INSTRUCTION_LENGTH 15Что соответствует максимальной длине инструкции в байтах на х86.
Чтобы текущий код был корректным, должна быть гарантия, что нет инструкций длиннее, чем размер указателя. И мне такую найтись не удалось.
Также в пользу ошибки косвенно говорит пример в проекте Zydis — фреймворке для дисассемблирования, используемом в эмуляторе:
ZyanU8 data[] =
{
0x51, 0x8D, 0x45, 0xFF, 0x50, 0xFF, 0x75, 0x0C, 0xFF, 0x75,
0x08, 0xFF, 0x15, 0xA0, 0xA5, 0x48, 0x76, 0x85, 0xC0, 0x0F,
0x88, 0xFC, 0xDA, 0x02, 0x00
};
// The runtime address (instruction pointer) was chosen
// arbitrarily here in order to better
// visualize relative addressing. In your actual program,
// set this to e.g. the memory address
// that the code being disassembled was read from.
ZyanU64 runtime_address = 0x007FFFFFFF400000;
// Loop over the instructions in our buffer.
ZyanUSize offset = 0;
ZydisDisassembledInstruction instruction;
while (ZYAN_SUCCESS(ZydisDisassembleIntel(
/* machine_mode: */ ZYDIS_MACHINE_MODE_LONG_64,
/* runtime_address: */ runtime_address,
/* buffer: */ data + offset,
/* length: */ sizeof(data) - offset,
/* instruction: */ &instruction
))) {
printf("%016" PRIX64 " %s\n", runtime_address, instruction.text);
offset += instruction.info.length;
runtime_address += instruction.info.length;
}
output:
007FFFFFFF400000 push rcx
007FFFFFFF400001 lea eax, [rbp-0x01]
007FFFFFFF400004 push rax
007FFFFFFF400005 push qword ptr [rbp+0x0C]
007FFFFFFF400008 push qword ptr [rbp+0x08]
007FFFFFFF40000B call [0x008000007588A5B1]
007FFFFFFF400011 test eax, eax
007FFFFFFF400013 js 0x007FFFFFFF42DB15Здесь тоже используется sizeof, но для массива с известными во время компиляции границами он действительно даст размер буфера в байтах.
Также можно заметить почти такой же код в истории коммитов и в самом эмуляторе. Его выпилили в 2023 году:
void Linker::LoadModuleToMemory(Module* m){
....
auto* rt1 = reinterpret_cast<uint8_t*>
(m->elf.GetElfEntry() + m->base_virtual_addr);
ZyanU64 runtime_address = m->elf.GetElfEntry() + m->base_virtual_addr;
// Loop over the instructions in our buffer.
ZyanUSize offset = 0;
ZydisDisassembledInstruction instruction;
while (ZYAN_SUCCESS(ZydisDisassembleIntel(
/* machine_mode: */ ZYDIS_MACHINE_MODE_LONG_64,
/* runtime_address: */ runtime_address,
/* buffer: */ rt1 + offset,
/* length: */ sizeof(rt1) - offset,
/* instruction: */ &instruction
))) {
fmt::print("{:#x}" PRIX64 " {}\n", runtime_address, instruction.text);
offset += instruction.info.length;
runtime_address += instruction.info.length;
}
....
}Сразу бросается в глаза точно такая же структура sizeof(***) - offset для length, что и в Zydis, и такое же взятие размера указателя, что и в текущем коде, что выглядит как чёткий сигнал о реальности ошибки.
Вдобавок в другом, уже почившем эмуляторе PS4, тоже использовался Zydis и функция ZydisDecoderDecodeFull с длиной инструкции, равной 15:
ZyanStatus status =
ZydisDecoderDecodeFull(&m_decoder, code,
ZYDIS_MAX_INSTRUCTION_LENGTH,
&instruction, operands,
ZYDIS_MAX_OPERAND_COUNT_VISIBLE,
ZYDIS_DFLAG_VISIBLE_OPERANDS_ONLY);Но для полной определённости, конечно же, необходимы пояснения от опытных разработчиков проекта.
Фрагмент N8. Despicable "Metamorphosis" therefore fear "Moon"
Предупреждение PVS-Studio: V610 Undefined behavior. Check the shift operator <<. The right operand ((64 - systemLang - 1) = [16..63]) is greater than or equal to the length in bits of the promoted left operand. playgo.cpp 233
int scePlayGoConvertLanguage(int systemLang) {
if (systemLang >= 0 && systemLang < 48) { //! systemLang: [0..47]
return (1 << (64 - systemLang - 1)); //! 1 << [16..63]
} else {
return 0;
}
}Функция добавлена целиком вместе с нижележащим куском scePlayGoInitialize.
С учётом того, что сейчас эмулятор собирается для архитектуры x86-64, где int занимает 4 байта, то при значениях systemLang меньше 32 получим переполнение и UB.
Но происходит ли такое в действительности? Отследим происхождение параметра systemLang.
Наша функция вызывается только из scePlayGoInitialize:
s32 PS4_SYSV_ABI scePlayGoInitialize(OrbisPlayGoInitParams* param) {
....
s32 system_lang = 0;
sceSystemServiceParamGetInt(OrbisSystemServiceParamId::Lang, &system_lang);
playgo->langMask = scePlayGoConvertLanguage(system_lang);
return ORBIS_OK;
}А system_lang туда попадает после функции sceSystemServiceParamGetInt:
s32 PS4_SYSV_ABI sceSystemServiceParamGetInt(
OrbisSystemServiceParamId param_id,
int* value
)
{
// TODO this probably should be stored in config for UI configuration
LOG_DEBUG(Lib_SystemService, "called param_id {}", u32(param_id));
if (value == nullptr) {
LOG_ERROR(Lib_SystemService, "value is null");
return ORBIS_SYSTEM_SERVICE_ERROR_PARAMETER;
}
switch (param_id) {
case OrbisSystemServiceParamId::Lang: {
s32 lang = EmulatorSettings.GetConsoleLanguage();
if (lang == 0x15 && g_sdk_version < Common::ElfInfo::FW_200) {
lang = 0x12;
}
if (lang == 0x16 && g_sdk_version < Common::ElfInfo::FW_250) {
lang = 2;
}
if ((lang >= 0x17 && lang <= 0x1a) &&
g_sdk_version < Common::ElfInfo::FW_500) {
lang = 0x12;
}
if ((lang >= 0x1b && lang <= 0x1d) &&
g_sdk_version < Common::ElfInfo::FW_500) {
lang = 1;
}
if (lang == 0x1e && g_sdk_version < Common::ElfInfo::FW_1000) {
lang = 0x12;
}
*value = lang;
break;
}
....
}
return ORBIS_OK;
}Как видим, небольшие значения system_lang вполне возможны. Кроме того, они чаще всего и будут! Ведь по документации каждому поддерживаемому языку соответствует свой номер от 0 до 47. Например, японский — 0 (ожидаемо от Sony), английский — 1, а русский — 8. Так что любая игра, скажем, на японском, гарантировано вызовет переполнение.
Заодно теперь стали понятны условия выше. Вот тут, например, если sdk довольно старой версии, , то вместо канадского французского используется обычный:
if (lang == 0x16 && g_sdk_version < Common::ElfInfo::FW_250)
lang = 2;Кроме вышеперечисленного, дополнительным аргументом в пользу ошибки служит реализация этой же функции в другой имплементации PlayGo, которая практически идентична, но лишена переполнения:
static inline ScePlayGoLanguageMask scePlayGoConvertLanguage(int32_t systemLang)
{
return (systemLang >= 0 && systemLang < 48) ?
(1ULL << (64 - systemLang - 1)) :
0ULL;
}Фрагмент N9. Time for "Hidden path"
Предупреждение PVS-Studio: V796 It is possible that break statement is missing in switch statement. hull_shader_transform.cpp 247
void WalkUsersOfTessConstantHelper(IR::Use use, u32 inc,bool propagateError){
IR::Inst* inst = use.user;
switch (use.user->GetOpcode()) {
case IR::Opcode::LoadSharedU32:
case IR::Opcode::LoadSharedU64:
case IR::Opcode::WriteSharedU32:
case IR::Opcode::WriteSharedU64: {
bool is_addr_operand = use.operand == 0;
if (is_addr_operand) {
u32 counter = inst->Flags<u32>();
inst->SetFlags<u32>(counter + inc);
ASSERT_MSG(!propagateError,
"LDS instruction {} accesses ambiguous attribute type",
fmt::ptr(use.user));
// Stop here
return;
}
}
case IR::Opcode::Phi: {
auto it = phi_infos.find(use.user);
....
}
....
}Ранее в этой ветке был безусловный выход:
switch (use.user->GetOpcode()) {
case IR::Opcode::LoadSharedU32:
case IR::Opcode::LoadSharedU64:
case IR::Opcode::WriteSharedU32:
case IR::Opcode::WriteSharedU64: {
u32 counter = inst->Flags<u32>();
inst->SetFlags<u32>(counter + inc);
// Stop here
return;
}В тексте коммита говорится:
Ignore when a user contributes to the wrong operand of an LDS inst, for
example the data operand of WriteShared* instead of the address operand.
This can mistakenly happen due to phi nodes.
Из "ignore when a user contributes to the wrong operand" можно сделать вывод, что нежелательно продолжать выполнение, когда use.operand != 0, и здесь действительно пропущен break. Если же наши предположения неверны, то разработчику стоило воспользоваться блокнотом и оставить подсказку в виде [[fallthrough]] своим менее благословлённым озарением коллегам.
Фрагмент N10. Treat "Lever" with care or you must accept "Ignoring"
Предупреждение PVS-Studio: V547 Expression compare < 0 is always false. Unsigned type value is never < 0. np_common.cpp 49
s32 PS4_SYSV_ABI sceNpCmpNpIdInOrder(OrbisNpId* np_id1, OrbisNpId* np_id2,
u32* out_result) {
....
// Compare data
u32 compare =
std::strncmp(np_id1->handle.data, np_id2->handle.data,
ORBIS_NP_ONLINEID_MAX_LENGTH);
if (compare < 0) {
*out_result = -1;
return ORBIS_OK;
} else if (compare > 0) {
*out_result = 1;
return ORBIS_OK;
}
....
}Весь файл был добавлен целиком (ранее он располагался в другой папке).
Напомню семантику функции strncmp:
int strncmp( const char* lhs, const char* rhs, std::size_t count );The sign of the result is the sign of the difference between the values of the first pair of characters (both interpreted as unsigned char) that differ in the arrays being compared.
Похоже, автор просто опечатался в типе, написав беззнаковый вместо знакового.
Прервав кооперацию в одном мире, заглянем ненадолго в другой. Кроме основного репозитория у проекта ShadPS4 существует довольно популярный форк, заточенный конкретно под Bloodborne: diegolix29/shadPS4.
Он ушёл от upstream на пару тысяч коммитов и, помимо общих болячек, имеет и свои уникальные.
Фрагмент N11. The sky and the cosmos are one
Предупреждение PVS-Studio: V590 Consider inspecting the 'deltaTime <= 0.0f || deltaTime < 0.0001f' expression. The expression is excessive or contains a misprint. controller.cpp 201
void GameController::CalculateOrientation(/*....*/
float deltaTime,
Libraries::Pad::OrbisFQuaternion& lastOrientation,
Libraries::Pad::OrbisFQuaternion& orientation) {
constexpr float MAX_DELTA_TIME = 0.1f;
if (deltaTime > MAX_DELTA_TIME) {
deltaTime = MAX_DELTA_TIME;
}
if (deltaTime <= 0.0f || deltaTime < 0.0001f) {
orientation = lastOrientation;
return;
}
....
}Кусок с MAX_DELTA_TIME и orientation добавлен отдельно.
На первый взгляд кажется, будто вместо deltaTime < 0.0001f должно было deltaTime > 0.0001f, и это косвенно подтверждается тем, что в upstream репозитории есть похожий фрагмент:
if (delta_time > 1.0f) {
orientation = last_orientation;
return;
}Но тогда бессмысленным становится кусок с MAX_DELTA_TIME. Что ж, придётся углубиться в лор вычисления ориентации контролёра.
Сперва посмотрим, что же такое delta_time. И, неожиданно, это дельта по времени с момента последнего обновления, например:
const float delta_time = static_cast<float>
(timestamp - m_last_orientation_update) / 1'000'000.f;Поискав, удалось понять, что вариант из upstream с константой 1.0f служит защитой от пролагов и пауз эмулятора, при которых в результате получалась бы ерунда.
В форке поступили немного иначе и в таком случае просто обновляют ориентацию, но с фиксированной дельтой 0.1f. Отсюда вытекает, что проблемное место:
if (deltaTime <= 0.0f || deltaTime < 0.0001f) {
orientation = lastOrientation;
return;
}видимо, служит другой цели — параноидальной защите от невалидных данных, поскольку и при отрицательных, и при крайне малых дельтах мы получаем некорректные значения.
В итоге со значительной долей уверенности можно сказать, что левая часть проверки deltaTime <= 0.0f — лишняя.
Фрагмент N12. Time for "jump"
Предупреждение PVS-Studio: V1082 Function marked as 'noreturn' may return control. This will result in undefined behavior. ir_emitter.cpp 15
[[noreturn]] void ThrowInvalidType(Type type,
std::source_location loc = std::source_location::current()) {
const std::string functionName = loc.function_name();
const int lineNumber = loc.line();
// UNREACHABLE_MSG("Invalid type = {}, functionName = {}, line = {}",
// u32(type), functionName,
// lineNumber);
}Как видим, UNREACHABLE_MSG действительно делал функцию noreturn:
#define UNREACHABLE_MSG(...) \
do { \
LOG_CRITICAL(Debug, "Unreachable code!\n" __VA_ARGS__);\
unreachable_impl(); \
} while (0)
[[noreturn]] void unreachable_impl() {
Common::Log::Stop();
std::fflush(stdout);
Crash();
throw std::runtime_error("Unreachable code");
}Закомментировали эти строки с сообщением "SOTC hacks" (вероятно, SOTC — Shadow Of The Colossus).
В upstream этого изменения нет, соответственно, и анализатору негде было срабатывать.
Здесь особо вопросов нет: очевидно, атрибут просто забыли удалить. Согласно стандарту C++, это — UB, и [[noreturn]] всё же нужно убрать.
Фрагмент N13. Treat hunter with care and don't be fooled
Предупреждение PVS-Studio: V501 There are identical sub-expressions 'serial == "CUSA03014"' to the left and to the right of the '||' operator. storage_image_sync.cpp 42
void StorageImageSync::Sync(VideoCore::ImageId image_id) {
const auto& serial = Common::ElfInfo::Instance().GameSerial();
if (serial == "CUSA11227" || serial == "CUSA12982" || serial == "CUSA00093" ||
serial == "CUSA03173" || serial == "CUSA00900" || serial == "CUSA00208" ||
serial == "CUSA01363" || serial == "CUSA01322" || serial == "CUSA003027" ||
serial == "CUSA00299" || serial == "CUSA00207" || serial == "CUSA03014" ||
serial == "CUSA03023" || serial == "CUSA03014" || serial == "CUSA00900" ||
serial == "CUSA00003" || serial == "CUSA01627" || serial == "CUSA01778" ||
serial == "CUSA03388" || serial == "CUSA01589" || serial == "CUSA01760" ||
serial == "CUSA07439" || serial == "CUSA07339" || serial == "CUSA08692" ||
serial == "CUSA08495" || serial == "CUSA50617" || serial == "CUSA18723" ||
serial == "CUSA28863" || serial == "CUSA00093" || serial == "CUSA00003") {
return;
}
....
}Как видим, сравнение с CUSA03014 дублируется, и обе копии добавили одновременно.
В upstream вообще нет этого файла, поэтому там нет и срабатывания.
Символично, что CUSA ID — это пятизначный серийный номер игр для PS4 и наш дубль CUSA03014 соответствует "Bloodborne: The Old Hunters Edition".
Также можно заметить, что CUSA003027 визуально выделяется большей длиной, ломая стройный ряд линий, и не имеет смысла, т. к. шестизначен.
Но просто ли удалить излишние сравнения или заменить ID на другие, может сказать только сам разработчик.
Фрагмент N14. Remember key but fear Malformed thing
Предупреждение PVS-Studio: V766 An item with the same key 'vk::Format::eR8G8B8A8Srgb' has already been added. host_compatibility.cpp 202
/**
* @brief The format compatibility class according to the Vulkan specification
* @url
* https://registry.khronos.org/vulkan/specs/
.* 1.3-extensions/html/vkspec.html#formats-compatibility-classes
* @url
* https://github.com/KhronosGroup/VulkanValidationLayers/
* blob/d37c676f/layers/generated/vk_format_utils.cpp#L70-L812
**/
static const std::unordered_map<vk::Format,CompatibilityClass> FORMAT_TABLE ={
....
{vk::Format::eR64G64Uint, CompatibilityClass::_128BIT},
{vk::Format::eR64Sfloat, CompatibilityClass::_64BIT},
{vk::Format::eR64Sint, CompatibilityClass::_64BIT},
{vk::Format::eR64Uint, CompatibilityClass::_64BIT},
{vk::Format::eR8G8B8A8Sint, CompatibilityClass::_32BIT},
{vk::Format::eR8G8B8A8Snorm, CompatibilityClass::_32BIT},
{vk::Format::eR8G8B8A8Srgb, CompatibilityClass::_32BIT}, // <= 32BIT
{vk::Format::eR8G8B8A8Sscaled, CompatibilityClass::_32BIT},
{vk::Format::eR8G8B8A8Uint, CompatibilityClass::_32BIT},
{vk::Format::eR8G8B8A8Unorm, CompatibilityClass::_32BIT},
{vk::Format::eR8G8B8A8Uscaled, CompatibilityClass::_32BIT},
....
{vk::Format::eR8G8Uscaled, CompatibilityClass::_16BIT},
{vk::Format::eR8Sint, CompatibilityClass::_8BIT},
{vk::Format::eR8Snorm, CompatibilityClass::_8BIT},
{vk::Format::eR8Srgb, CompatibilityClass::_8BIT},
{vk::Format::eR8G8B8A8Srgb, CompatibilityClass::_8BIT}, // <= 8BIT
{vk::Format::eR8Sscaled, CompatibilityClass::_8BIT},
{vk::Format::eR8Uint, CompatibilityClass::_8BIT},
....
};Ключ eR8G8B8A8Srgb повторяется дважды с разными значениями: CompatibilityClass::_8BIT и CompatibilityClass::_32BIT. По стандарту C++ не специфицировано, какое из них останется в итоге.
Если посмотреть по ссылке из комментария, то можно увидеть запись:
{VK_FORMAT_R8G8B8A8_SRGB,
{FORMAT_COMPATIBILITY_CLASS::_32BIT, ....} }Т. е. значение 8BIT не совпадает с образцом, да и просто, похоже, лишнее, с учётом того, что в референсной мапе между значениями VK_FORMAT_R8_SRGB и VK_FORMAT_R8_SSCALED ничего нет.В upstream есть эта мапа, но, естественно, нет второго варианта с 8BIT, который в форке был добавлен позже отдельно от других.
Какой из двух вариантов в итоге нужно оставить, могут уверенно сказать только разработчики.
Вот и конец ночи охоты, и пора увидеть свет дня. Пусть и наделённые силой C++, но разработчики остаются людьми и тоже могут совершать ошибки, пропускать опечатки и утрачивать бдительность. В помощь им создано множество инструментов, и в нашей мастерской в том числе. Например, бесплатная версия PVS-Studio для open source проектов.
Если у вас не open source проект, то вы все равно можете попробовать анализатор бесплатно. Вы ведь в курсе, да?
Ссылки на открытые по итогам проверки issue на GitHub: upstream, fork.
0
0
0
0