В мире видеоигр эмуляторы стали настоящей находкой для поклонников классических проектов. PPSSPP — один из самых популярных эмуляторов для PlayStation Portable (PSP), который позволяет нам вновь окунуться в атмосферу любимых игр, но уже на современных устройствах. Однако чтобы играть без сбоев и лагов, нужно убедиться, что код эмулятора хорошо написан.
Эта статья является сиквелом вот этой статьи. Хочется похвалить разработчиков PPSSPP, так как практически ВСЕ ошибки из предыдущей части, о которых я написал, были исправлены в течение месяца и в релиз 1.18.1 не вошли. К сожалению, все ошибки, найденные на момент написания предыдущей статьи, расписать мне не удалось из-за нехватки сил и времени. Каюсь. Возможно, поэтому ошибки, о которых пишу сегодня, всё так же находятся в кодовой базе проекта :)
Ошибок в проекте, как и этой статьи, могло бы не быть, если бы разработчики воспользовались PVS-Studio. К слову, кажется, это хороший момент напомнить о том, что в PVS-Studio есть возможность получить бесплатную лицензию для open source проектов. Подробнее об этом можно прочитать на специальной странице.
Как я уже сказал выше, это продолжение другой статьи, поэтому ошибки найдены при проверке кода PPSSPP версии 1.17.1 при помощи анализатора PVS-Studio на Linux. Версия анализатора — 7.32. Но эти ошибки попали в релиз 1.18.1, поэтому все ссылки на срабатывания в коде ниже ведут на версию 1.18.1. Также отмечу, что на момент написания статьи актуальная версия анализатора PVS-Studio — 7.34. Об изменениях в этой версии можно почитать тут.
Фрагмент N1
А начнём мы с такого фрагмента:
static int sceNetAdhocctlGetAddrByName(const char *nickName,
u32 sizeAddr, u32 bufAddr)
{
....
// Copied to null-terminated var to prevent unexpected behaviour on Logs
memcpy(nckName, nickName, ADHOCCTL_NICKNAME_LEN);
....
if (netAdhocctlInited)
{
// Valid Arguments
if (nickName != NULL && buflen != NULL)
{
....
}
....
}
}Предупреждение PVS-Studio: V595 The 'nickName' pointer was utilized before it was verified against nullptr. Check lines: 6143, 6152. sceNetAdhoc.cpp 6143
Как мы видим, в memcpy передаётся указатель nickname в качестве исходного буфера. Чуть ниже по коду проверяется, не нулевой ли этот указатель. Значит, разработчик предполагает, что формальный параметр может быть нулевым. За точками, в которых я скрыл промежуточный код, он тоже не изменяется (можете проверить, ткнув на номер строки предупреждения).
А что по этому поводу думает memcpy? Цитирую cpperefence:
If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero.
Дополнительное срабатывание:
Фрагмент N2
Теперь рассмотрим следующий фрагмент:
int internal_profiler_find_cat(const char *category_name, bool create_missing)
{
int i;
for (i = 0; i < MAX_CATEGORIES; i++)
{
const char *catname = categories[i].name;
if (!catname)
break;
#ifdef UNIFIED_CONST_STR
if (catname == category_name)
{
#else
if (!strcmp(catname, category_name)) // <=
{
#endif
return i;
}
}
if (i < MAX_CATEGORIES && category_name && create_missing) // <=
{
....
}
}Предупреждение PVS-Studio: V595 The 'category_name' pointer was utilized before it was verified against nullptr. Check lines: 103, 109. Profiler.cpp 103
У нас есть проверка указателя category_name, но сходу можно не заметить, где он используется.
В случае, если не определена директива UNIFIED_CONST_STR, нулевой указатель category_name может прилететь в strcmp. А это приводит к UB.
Фрагмент N3
static void __GameModeNotify(u64 userdata, int cyclesLate)
{
....
if (gameModeSocket < 0)
{
// ReSchedule
CoreTiming::ScheduleEvent(usToCycles(GAMEMODE_UPDATE_INTERVAL) - cyclesLate,
gameModeNotifyEvent, userdata);
return;
}
auto sock = adhocSockets[gameModeSocket - 1];
....
}Предупреждение PVS-Studio: V557 Array underrun is possible. The value of 'gameModeSocket - 1' index could reach -1. sceNetAdhoc.cpp 191
Обратим внимание на инициализацию переменной sock. Мы видим, что её пытаются проинициализировать элементом массива adHocSockets с индексом gameModeSocket - 1.
Теперь обратим внимание на проверку выше. Она отсеивает все отрицательные значения для переменной gameModeSocket.
Проблема в том, что забыли учесть случай, когда gameModeSocket будет равен нулю. Тогда индекс gameModeSocket - 1 получится отрицательным, и это приведёт к UB.
Исправленный код:
if (gameModeSocket <= 0)Фрагмент N4
SoftGPU::SoftGPU(GraphicsContext *gfxCtx, Draw::DrawContext *draw)
: GPUCommon(gfxCtx, draw)
{
....
drawEngine_ = new SoftwareDrawEngine();
if (!drawEngine_)
return;
....
}Предупреждение PVS-Studio: V668 There is no sense in testing the 'drawEngine_' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SoftGpu.cpp 436
Как уже красноречиво подметило сообщение предупреждения, нет смысла проверять указатель, полученный в результате работы оператора new. В случае нехватки памяти или какой-либо другой ошибки он бросит исключение.
Вероятно, такой фрагмент возник в результате рефакторинга, а раньше здесь использовался malloc.
Фрагмент N5
static std::vector<MicWaitInfo> waitingThreads;
....
static void __MicBlockingResume(u64 userdata, int cyclesLate)
{
....
int count = 0;
for (auto waitingThread : waitingThreads)
{
if (waitingThread.threadID == threadID)
{
....
if (Microphone::isHaveDevice())
{
if (Microphone::getReadMicDataLength() >= waitingThread.needSize)
{
....
waitingThreads.erase(waitingThreads.begin() + count); // <=
}
else
{
....
}
}
else
{
....
waitingThreads.erase(waitingThreads.begin() + count); // <=
readMicDataLength += waitingThread.needSize;
}
}
++count;
}
}Предупреждения PVS-Studio:
Один из самых необычных способов прострелить себе ногу, который я видел.
Обратим внимание на код waitingThreads.erase(waitingThreads.begin() + count);. Он появляется в двух местах в зависимости от того, по какой ветке условий мы пойдём.
В подобном коде может возникнуть ряд проблем. После такого удаления инвалидируются все итераторы и ссылки в точке удаления или после неё, включая итератор end(). То есть при следующем проходе цикла будет обрабатываться уже невалидный итератор.
В качестве решения можно воспользоваться так называемой erase-remove идиомой, а начиная с C++20, появились std::erase/std::erase_if. Но для этого придётся хорошенько отрефакторить код.
Фрагмент N6
void Int_VecDo3(MIPSOpcode op)
{
....
u32 lastsat = (currentMIPS->vfpuCtrl[VFPU_CTRL_DPREFIX] & 3) << (n + n - 2);
....
}Предупреждениe PVS-Studio: V610 Undefined behavior. Check the shift operator '<<'. The right operand is negative ('(n + n - 2)' = [-2..6]). MIPSIntVFPU.cpp 2155
Чтобы понять в чём тут ошибка, необходимо посмотреть на объявление переменной n.
Вот оно:
u32 n = GetNumVectorElements(sz);Так почему же ругается анализатор?
Всё дело в том, что отработал межпроцедурный анализ. Просмотрев определение функции GetNumVectorElements, анализатор узнал, что она может возвращать значения в диапазоне [0 .. 4].
Дальше, увидев, что n никак не изменяется по коду, он может сделать вывод, что в выражении (n + n - 2) возможный диапазон значений результата [-2..6].
Теперь посмотрим, что такое vpfuCtrl. Это массив 32-битных беззнаковых чисел. Таким образом, оператор здесь производит обычный побитовый сдвиг влево на -2 элемента. Как это? А как получится.
Аналогичные срабатывания:
Фрагмент N7
inline float Float16ToFloat(float16 ix)
{
float x;
memcpy(&x, &ix, sizeof(float));
return x;
}Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to the '& ix' buffer becoming out of range. math_util.h 23
Мы пытаемся скопировать значение из типа float16 в тип float. Тип float16 это unsigned short:
typedef unsigned short float16;Размеры unsigned short и float определяются реализацией, однако на многих платформах они равны 16 и 32 битам соответственно.
Дальше обратим внимание на третий аргумент в вызове memcpy. Он говорит, какое количество байтов надо скопировать из переменной ix. В случае, когда размер float равен 32 битам (4 байта), а размер unsigned short 16 битам (2 байта), произойдёт выход за границы буфера ix.
Фрагмент N8
void Jit::Comp_SVQ(MIPSOpcode op)
{
CONDITIONAL_DISABLE(LSU_VFPU);
int imm = (signed short)(op&0xFFFC);
int vt = (((op >> 16) & 0x1f)) | ((op&1) << 5);
MIPSGPReg rs = _RS;
CheckMemoryBreakpoint(0, rs, imm);
switch (op >> 26)
{
case 53: //lvl.q/lvr.q
{
if (!g_Config.bFastMemory)
{
DISABLE;
}
DISABLE;
....
}Предупреждение PVS-Studio: V523 [CWE-691] The 'then' statement is equivalent to the subsequent code fragment. CompVFPU.cpp 313
Обратим внимание, что в then-ветке конструкции if написан такой же код, как и после самой конструкции. А вот что из себя представляет сам макрос DISABLE:
#define DISABLE { fpr.ReleaseSpillLocks(); Comp_Generic(op); return; }Понятно, что ничего серьёзного здесь не произойдёт, так как мы всё равно выйдем из функции. Вопрос в том, является ли здесь лишним условие, или в теле должен был быть другой код, который выполнится перед выходом из функции? Или это банально оставленная TODO?
Однозначно по такому коду сказать ничего нельзя, так как комментариев тоже нет. Именно поэтому этот код и плох.
Похожие срабатывания:
Фрагмент N9
void BlockAllocator::Block::DoState(PointerWrap &p)
{
....
size_t tagLen = strlen(tag);
if (tagLen != sizeof(tag))
memset(tag + tagLen, 0, sizeof(tag) - tagLen);
DoArray(p, tag, sizeof(tag));
}Предупреждение PVS-Studio: V547 Expression 'tagLen != sizeof (tag)' is always true. BlockAllocator.cpp 508
Интересная проверка. Взглянем на декларацию массива tag:
char tag[32];Выражение sizeof(tag) будет равно 32. Выражение tagLen != sizeof(tag) окажется false, только когда результат strlen(tag) будет равен 32. Однако максимальное значение, которое может вернуть strlen для tag, это 31, так как последний символ должен быть нуль-терминалом. То есть единственный вариант, когда условие не выполнится, это если tag будет не нуль-терминированной строкой. Но в этом случае всплыл бы один нюанс — поведение strlen оказалось бы неопределённым. Поэтому выражение всегда вычисляется как true и memset всегда зануляет sizeof(tag) - tagLen байт.
Ещё срабатывания V547:
Фрагмент N10
void netAdhocValidateLoopMemory()
{
// Allocate Memory if it wasn't valid/allocated
// after loaded from old SaveState
if ( !dummyThreadHackAddr
|| ( dummyThreadHackAddr
&& strcmp("dummythreadhack",
kernelMemory.GetBlockTag(dummyThreadHackAddr)) != 0))
{
....
}Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!dummyThreadHackAddr' and 'dummyThreadHackAddr'. sceNetAdhoc.cpp 1121
Думаю, вполне очевидное срабатывание и место, которое можно упростить.
Представим, что мы — компилятор и разбираем это условие. Так как мы обязаны выполнять его слева направо, мы посмотрим на левую часть от оператора ||. Условие !dummyThreadHackAddr может быть true или false. Если true, то мы сразу заходим внутрь. Если false, то это значит, что подвыражение dummyThreadHackAddr — это всегда true.
Поэтому проверку можно упростить:
if ( !dummyThreadHackAddr
|| strcmp("dummythreadhack",
kernelMemory.GetBlockTag(dummyThreadHackAddr)) != 0))Аналогичные срабатывания:
Фрагмент N11
void QueueCallback(void (*func)(VulkanContext *vulkan, void *userdata),
void *userdata)
{
callbacks_.push_back(Callback(func, userdata));
}
void VulkanRenderManager::EndCurRenderStep()
{
for (VKRGraphicsPipeline *pipeline : pipelinesToCheck_)
{
if (!pipeline)
{
// Not good, but let's try not to crash.
continue;
}
if (!pipeline->pipeline[(size_t)rpType])
{
pipeline->pipeline[(size_t)rpType] = Promise<VkPipeline>::CreateEmpty();
_assert_(renderPass);
compileQueue_.push_back(CompileQueueEntry(pipeline,
renderPass->Get(vulkan_, rpType, sampleCount),rpType, sampleCount));
needsCompile = true;
}
}
}Предупреждение PVS-Studio: V823 Decreased performance. Object may be created in-place in the 'compileQueue_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. VulkanRenderManager.cpp 883
Для начала посмотрим, что из себя представляет контейнер compileQueue_:
std::vector<CompileQueueEntry> compileQueue_;Теперь посмотрим, что такое CompileQueueEntry:
struct CompileQueueEntry
{
CompileQueueEntry(VKRGraphicsPipeline *p,
VkRenderPass _compatibleRenderPass,
RenderPassType _renderPassType,
VkSampleCountFlagBits _sampleCount)
: type(Type::GRAPHICS), graphics(p),
compatibleRenderPass(_compatibleRenderPass),
renderPassType(_renderPassType), sampleCount(_sampleCount){}
enum class Type
{
GRAPHICS,
};
Type type;
VkRenderPass compatibleRenderPass;
RenderPassType renderPassType;
VKRGraphicsPipeline* graphics = nullptr;
VkSampleCountFlagBits sampleCount;
};Переменная compileQueue_ — это обычный std::vector, а CompileQueueEntry — это структура, которая имеет пользовательский конструктор. Соответственно, код можно сделать немного эффективнее, если заменить push_back на emplace_back, о чём я писал в предыдущей статье.
В нашем случае можно убрать дополнительное копирование пяти полей структуры CompileQueueEntry при перемещении временного объекта в вектор. Да, такое копирование не тяжеловесное. Но таких мест в программе может быть много. Да и нет гарантии, что потом эта структура не станет тяжеловесной, а вставку не забудут поменять на emplace_back.
Кстати, начиная с C++20, emplace_back будет работать также и с агрегатными типами.
Дополнительные срабатывания:
Фрагмент N12
bool Key(const KeyInput &key) override
{
std::vector<int> pspKeys; // <=
bool showInfo = false;
if (HasFocus() && UI::IsInfoKey(key))
{
// If the button mapped to triangle, then show the info.
if (key.flags & KEY_UP)
{
showInfo = true;
}
}
else if (hovering_ && key.deviceId == DEVICE_ID_MOUSE
&& key.keyCode == NKCODE_EXT_MOUSEBUTTON_2)
{
// If it's the right mouse button, and it's not otherwise mapped,
// show the info also.
if (key.flags & KEY_DOWN)
{
showInfoPressed_ = true;
}
if ((key.flags & KEY_UP) && showInfoPressed_)
{
showInfo = true;
showInfoPressed_ = false;
}
}
if (showInfo)
{
TriggerOnHoldClick();
return true;
}
return Clickable::Key(key);
}Предупреждение PVS-Studio: V808 'pspKeys' object of 'vector' type was created but was not utilized. MainScreen.cpp 159
Понятно, что пустой вектор не занимает много места в памяти. Ошибки такого типа опасны тем, что предполагалось использование одной переменной, а вместо неё случайно воспользовались другой. Такое может случиться, например, при опечатке.
В этом фрагменте, как видно, всё в порядке. Скорее всего, вектор перестали использовать после рефакторинга кода, и анализатор может подсвечивать такие места.
Фрагмент N13
VkResult VulkanContext::InitDebugUtilsCallback()
{
// We're intentionally skipping
// VK_DEBUG_UTILS_MESSAGE_SEVERITY_VERBOSE_BIT_EXT
// and VK_DEBUG_UTILS_MESSAGE_SEVERITY_INFO_BIT_EXT,
// just too spammy.
int bits = VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT
| VK_DEBUG_UTILS_MESSAGE_SEVERITY_WARNING_BIT_EXT
| VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT;
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT' to the left and to the right of the '|' operator. VulkanContext.cpp 867
Ошибки такого паттерна очень похожи на предыдущий фрагмент. Возможно, предполагалось проверить другой флаг, но в результате, например, copy-paste, его могли забыть поменять. Такие ошибки, как правило, тяжело ищутся глазами, но статический анализ с ними справляется на раз-два.
Представьте, что вы садитесь в космический корабль, который перенесёт вас в мир ностальгии, где каждое нажатие кнопки пробуждает в вас воспоминания о захватывающих приключениях на PSP. Но для того, чтобы этот космический корабль мог взлететь, важно, чтобы у него была надёжная система управления. Под надёжной системой управления я подразумеваю хороший код. А под хорошим кодом я подразумеваю не только то, что он должен быть качественно написан, но и то, что он должен быть протестирован и отлажен.
Поэтому важно не забывать тестировать код различными методиками, в том числе и статическими анализаторами. Получить пробную версию анализатора PVS-Studio можно здесь.