In the world of video games, emulators have become a real boon for fans of classic projects. Among these, PPSSPP stands out as one of the most popular PlayStation Portable (PSP) emulators, enabling gamers to relive iconic moments on modern devices. However, developers must create well-written emulator code to ensure seamless gaming experience.
This is a sequel to this article. I'd like to give kudos to the PPSSPP developers, as almost ALL of the errors I wrote about in the previous article have been fixed within a month and didn't get into the 1.18.1 release. Unfortunately, when writing the previous article, I didn't have the energy and time to describe all the detected errors, I confess. Maybe that's why the bugs I'm talking about today are still in the project code base.
There might have been no errors in the project as well as this article if the developers had used PVS-Studio. By the way, this seems like a good timing to remind that you can get a free PVS-Studio license for open-source projects. You can learn more about it here.
As I've already mentioned that this is a continuation of another article. So, I found the errors by checking the PPSSPP 1.17.1 code using the PVS-Studio analyzer on Linux. The analyzer version is 7.32. However, those bugs got into the 1.18.1 release, therefore all links to warnings in the code below lead to 1.18.1. I'd also like to point out that at the moment of writing this article, the version of PVS-Studio analyzer is 7.34. You can learn more about the changes in this version here.
Fragment N1
We'll start with this fragment:
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)
{
....
}
....
}
}The PVS-Studio warning: V595 The 'nickName' pointer was utilized before it was verified against nullptr. Check lines: 6143, 6152. sceNetAdhoc.cpp 6143
As we can see, the nickname pointer is passed to memcpy as a source buffer. The pointer is checked for null below in the code. So, developers assumed that the formal parameter could be null. It also doesn't change beyond the points where I've hidden the intermediate code (you can check this by clicking the warning line number).
What does memcpy think of this? Here's a quote from cpperefence:
If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero.
Additional warning:
Fragment N2
Now let's look at the following fragment:
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) // <=
{
....
}
}The PVS-Studio warning: V595 The 'category_name' pointer was utilized before it was verified against nullptr. Check lines: 103, 109. Profiler.cpp 103
We have the category_name pointer check, but one might not immediately notice where it's used.
If the UNIFIED_CONST_STR macro isn't defined, the category_name null pointer may get to strcmp. This leads to UB.
Fragment 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];
....
}The PVS-Studio warning: V557 Array underrun is possible. The value of 'gameModeSocket - 1' index could reach -1. sceNetAdhoc.cpp 191
Note the initialization of the sock variable. We can see that the devs tried to initialize it with the adHocSockets array element that has the gameModeSocket - 1 index.
Now let's look at the check above. It discards all negative values for the gameModeSocket variable.
The issue is that they forgot to consider the case when gameModeSocket is zero. Then the gameModeSocket - 1 index is negative, which leads to UB.
The fixed code:
if (gameModeSocket <= 0)Fragment N4
SoftGPU::SoftGPU(GraphicsContext *gfxCtx, Draw::DrawContext *draw)
: GPUCommon(gfxCtx, draw)
{
....
drawEngine_ = new SoftwareDrawEngine();
if (!drawEngine_)
return;
....
}The PVS-Studio warning: 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
As the warning message has already pointed out, it makes no sense to check the pointer obtained as a result of the new expression. It throws an exception if system runs out of memory or other failures occur.
This fragment is probably the result of refactoring, and malloc was used here before.
Fragment 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;
}
}The PVS-Studio warnings:
This is one of the most unusual ways to shoot yourself in the foot that I've ever seen.
Let's take a look at the waitingThreads.erase(waitingThreads.begin() + count); code. It appears twice depending on which branch we jump to.
A number of issues can arise in this kind of code. After such a deletion operation, all iterators and references at or after the deletion point, including the end() iterator become invalid. This means that the invalid iterator will be processed in the next iteration of the loop.
The solution is a so-called erase-remove idiom. Also, std::erase/std::erase_if have been introduced in C++20. However, the latter requires good and proper refactoring.
Fragment N6
void Int_VecDo3(MIPSOpcode op)
{
....
u32 lastsat = (currentMIPS->vfpuCtrl[VFPU_CTRL_DPREFIX] & 3) << (n + n - 2);
....
}The PVS-Studio warning: V610 Undefined behavior. Check the shift operator '<<'. The right operand is negative ('(n + n - 2)' = [-2..6]). MIPSIntVFPU.cpp 2155
To understand what the error is, we need to look at the n variable declaration.
Here it is:
u32 n = GetNumVectorElements(sz);So, why did the analyzer issue a warning?
The thing is, the interprocedural analysis has worked out. Having examined the GetNumVectorElements function definition, the analyzer learned that it can return values in the [0 ... 4] range.
Then, seeing that n doesn't change in any way in the code, it concluded that the (n + n - 2) expression has a possible range of [-2..6] result values.
Now let's look at what vpfuCtrl is. It's an array of 32-bit unsigned numbers. Thus, the operator here performs the usual bitwise shift to the left by -2 elements. What is it? It could be anything.
Identical warnings:
Fragment N7
inline float Float16ToFloat(float16 ix)
{
float x;
memcpy(&x, &ix, sizeof(float));
return x;
}The PVS-Studio warning: V512 A call of the 'memcpy' function will lead to the '& ix' buffer becoming out of range. math_util.h 23
We're trying to copy a value from the float16 type to the float type. The float16 type is unsigned short:
typedef unsigned short float16;The sizes of unsigned short and float are implementation-defined, but on many platforms, they're 16 and 32 bits respectively.
Now let's look at the third argument in the memcpy call. It says how many bytes we need to copy from the ix variable. If the float size is 32 bits (4 bytes) and the unsigned short size is 16 bits (2 bytes), the ix buffer overflow will occur.
Fragment 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;
....
}The PVS-Studio warning: V523 [CWE-691] The 'then' statement is equivalent to the subsequent code fragment. CompVFPU.cpp 313
Note that in the then branch of the if statement, there's the same code as after the statement. This is what the DISABLE macro looks like:
#define DISABLE { fpr.ReleaseSpillLocks(); Comp_Generic(op); return; }Of course, nothing too serious will happen here, since we'll exit the function anyway. Here's a question, though: is the condition redundant here, or should there have been some other code in the body that would have been executed before the function exit, or is it just a leftover TODO?
We can't say anything for sure about the code, as there are no comments either. This is why the code isn't good.
Similar warnings:
Fragment 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));
}The PVS-Studio warning: V547 Expression 'tagLen != sizeof (tag)' is always true. BlockAllocator.cpp 508
This is an interesting check. Let's look at the tag array declaration:
char tag[32];The sizeof(tag) expression equals 32. The tagLen != sizeof(tag) expression is false only when the result of strlen(tag) is 32. However, the maximum value that strlen can return for tag is 31, since the last character must be a null terminal. So, the only way the condition could fail is if tag was a non-null-terminated string. However, there would be a nuance in this case—the strlen behavior would be undefined. Thus, the expression always evaluates to true, and memset always zeroes sizeof(tag) - tagLen bytes.
More V547 warnings:
Fragment 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))
{
....
}The PVS-Studio warning: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!dummyThreadHackAddr' and 'dummyThreadHackAddr'. sceNetAdhoc.cpp 1121
I think this is a pretty obvious warning, and we can simplify this fragment.
Let's imagine that we're a compiler analyzing this condition. Since we must execute it from left to right, let's look at the left side of the || operator. The !dummyThreadHackAddr condition can be true or false. If true, we immediately go in. If false, the dummyThreadHackAddr subexpression is always true.
So, we can simplify the check:
if ( !dummyThreadHackAddr
|| strcmp("dummythreadhack",
kernelMemory.GetBlockTag(dummyThreadHackAddr)) != 0))Identical warnings:
Fragment 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;
}
}
}The PVS-Studio warning: V823 Decreased performance. Object may be created in-place in the 'compileQueue_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. VulkanRenderManager.cpp 883
First, let's look what the compileQueue_ container looks like:
std::vector<CompileQueueEntry> compileQueue_;Now we'll look what the CompileQueueEntry container looks like:
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;
};The compileQueue_ variable is a regular std::vector, and CompileQueueueEntry is a struct that has a user-defined constructor. So, we can enhance the code performance if we replace push_back with emplace_back, as I have written in the previous article.
In our case, we can eliminate the extra copying of the five data members within the CompileQueueueEntry struct when moving a temporary object into a vector. Yes, this kind of copying isn't heavyweight, but the program can have a lot of such code fragments. We have no guarantee that this struct won't become heavyweight later, and developers won't forget to change the insertion operation to emplace_back.
By the way, starting with C++20, emplace_back works with aggregate types as well.
Additional warnings:
Fragment 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);
}The PVS-Studio warning: V808 'pspKeys' object of 'vector' type was created but was not utilized. MainScreen.cpp 159
Obviously, an empty vector requires little memory. Such errors are dangerous because one variable should have been used, but another variable was accidentally used instead. This could happen due to a typo, for example.
As we can see, this snippet is fine. Most likely, they stopped using the vector after refactoring the code, and the analyzer can highlight such code fragments.
Fragment 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;
....
}The PVS-Studio warning: 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
This type of errors is quite similar to the previous fragment. Developers may have intended to check a different flag, but forgot to change it due to a copy-paste error, for example. Such errors are usually hard to spot with the naked eye, but static analysis easily handles them.
Imagine stepping aboard a spaceship that takes you to a world of nostalgia, where every button press reignites memories of thrilling PSP adventures. However, for this spaceship to take off, it needs a reliable control system—just as an emulator needs solid code. And by solid code, I mean not only that it should be well-written—it must also be thoroughly tested and meticulously debugged.
This is why one should remember to test the code using various methods, including static analysis. You can get a trial version of the PVS-Studio analyzer here.