# PPSSPP or psp? Uncovering bugs from the future Many of us get a sense of warm nostalgia when it comes to the PSP\. Released in 2004, this portable console was a real breakthrough for its time\. It introduced us to the possibility of enjoying full\-fledged game projects on the go\. It was incredibly groundbreaking at the time\. As if you had a little piece of home console in your pocket\. ![1167_ppsspp/image1.png](https://import.viva64.com/docx/blog/1167_ppsspp/image1.png) ## Introduction PSP games were diverse and exciting, from iconic series like God of War and Final Fantasy to unique projects like Patapon and LocoRoco\. Each game introduced a new world that players could explore wherever they were: on the bus, during a school break, or just sitting in a park\. The PSP was also Sony's first console to feature multimedia playback, making it a really versatile entertainment device\. The rise of emulators for the PSP has breathed a new life into this nostalgia\. Emulators enable today's gamers to relive those unforgettable times or discover games they missed\. They make it possible to run classic projects on modern devices equipped with enhanced graphics and performance\. This is especially important in the age of digital distribution, where physical game copies are becoming increasingly rare\. Emulators also play an important role in preserving gaming history\. They help us maintain access to games that might otherwise be unavailable due to obsolete hardware or a limited number of copies\. Thanks to the community of enthusiasts and emulator developers, we can be sure that the PSP legacy will live on for a long time\. [PPSSPP](https://www.ppsspp.org/) is an emulator that enables you to run games for the PlayStation Portable \(PSP\) console on a variety of devices, including PCs, smartphones, and tablets\. It was created by Henrik Rydgård and first released in 2012\. Since then, PPSSPP has become one of the most popular PSP emulators due to its compatibility and performance\. Here are the key features of PPSSPP: * Cross\-platform compatibility: it's available for Windows, macOS, Linux, Android, and iOS, which makes it an all\-in\-one solution for gamers across platforms\. * High performance: PPSSPP is optimized to work even on resource\-constrained devices\. It supports various graphics settings, enabling users to improve image quality or reduce system load\. * HD resolution support: unlike the original PSP, which has a screen resolution of 480x272 pixels, PPSSPP makes it possible to increase the resolution to HD and above, which greatly enhances the visual experience when playing games\. * Data saving: users can save their progress at any point in the game thanks to data saving\. This is particularly useful for challenging games, or when a player needs to take a break from the game\. * Multi\-platform saves: users can transfer saved games between devices, which makes it easy to continue playing on the go\. * High\-resolution texture support: some enthusiasts create high\-resolution texture packs for popular games\. Players can use them with PPSSPP to improve the graphics quality\. * A large library of compatible games: the emulator supports most PSP games, including popular franchises such as God of War, Final Fantasy, Monster Hunter, and many more\. * An active community: thanks to its open\-source code, PPSSPP has an active community of developers and users who are constantly working to enhance the emulator and add new features\. * Controller support: the emulator supports external controllers via Bluetooth or USB, making the gaming experience more enjoyable\. * Control settings: users can customize the controls to suit their preferences, whether using touch screens or physical buttons on the controllers\. So, PPSSPP is a powerful tool that enables fans of classic PSP games to enjoy them with improved graphics and the convenience of modern devices\. Since PPSSPP is an open\-source project, we were eager to check it using [our tool](https://pvs-studio.com/en/pvs-studio/)\. ## Check results Before we get to breaking down errors, I'd like to mention that the code is of pretty high quality\. The developers even took extra care in some of its parts\. However, plenty of errors have crept into the code\. I checked the PPSSPP [1\.17\.1](https://github.com/hrydgard/ppsspp/tree/v1.17.1) code using the [PVS\-Studio](https://pvs-studio.com/en/) analyzer on [Linux](https://pvs-studio.com/en/docs/manual/6615/)\. The analyzer version is [7\.32](https://pvs-studio.com/en/docs/manual/0010/#ID6F50F00D02)\. ### Bugs of the last release It's been a while since the last release on February 4, 2024\. The code has already changed quite a lot\. As I was copying and pasting the GitHub links for the code snippets described in the article, I noticed that some bugs in the master branch had already been fixed\. However, the relevance of these fragments remains strong\. On the contrary, it confirms the following: * those were definitely errors; * the developers took the time to find and fix them\. I'll elaborate on each point in more detail\. **Those were definitely errors**\. It's unlikely that anyone would go to the trouble of fixing the code just for fun\. As our architect says, "If it works, don't touch it\." Most likely, someone reported a real bug, so developers fixed it\. **The devs took the time to find and fix them**\. It's perfect if your project is open source: someone can notice a bug there, reproduce it, find where the issue is, fix it, and submit a PR\. However, even in open\-source projects, developers most often get an issue and have to debug errors instead of developing new features\. Sometimes debugging can take up to [50 hours](https://pvs-studio.com/en/code-inspection/)\. The worst case is when your project is commercial\. You sell it, and then the customer rejects it because the bug has somehow put a spanner in the works and spoiled the impression of the product\. In this case, time is your worst enemy\. The longer the bug exists in your code, the more damage it can do to your business\. All of these points strongly suggest that projects using static analysis have an advantage: early error detection at the code\-writing stage\. In the case of PPSSPP, I can't say whether the project authors or good Samaritans fixed all these bugs, or how long it took to fix them once they were discovered\. But the fact is that some of them have been lurking in the project for years, only to surface and be addressed much later :\) Finally, before we get to the fun part\. Even though the developers fixed errors in the fragments below, the analyzer found similar patterns in others \(we'll mention them in the article\)\. **Fragment N1** Let's look at the first fragment, which the developers have already fixed\. ```cpp template class FastVec { .... public: FastVec &operator=(FastVec &&other) { if (this != &other) { delete[] data_; // <= data_ = other.data_; size_ = other.size_; capacity_ = other.capacity_; other.data_ = nullptr; other.size_ = 0; other.capacity_ = 0; } return *this; } .... private: void IncreaseCapacityTo(size_t newCapacity) { .... T *oldData = data_; data_ = (T *)malloc(sizeof(T) * newCapacity); // <= _assert_msg_(data_ != nullptr, "%d", (int)newCapacity); if (capacity_ != 0) { memcpy(data_, oldData, sizeof(T) * size_); free(oldData); } capacity_ = newCapacity; } .... } ``` The PVS\-Studio warning: [V611](https://pvs-studio.com/en/docs/warnings/v611/) The memory was allocated using 'malloc/realloc' function but was released using the 'delete' operator\. Consider inspecting operation logics behind the 'data\_' variable\. Check lines: 51, 158\. [FastVec\.h 51](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/Data/Collections/FastVec.h#L51) The project has a fast \(judging by the name\) custom vector\. It has an overloaded move operator that frees the buffer using the operator _delete\[\]_\. Next, we see the _IncreaseCapacityTo_ function that increases the capacity of the vector to the required value\. It uses the _malloc_ and _free_ functions: a new memory block is allocated, data is copied from the old memory block, and then it is freed\. Now for the plot twist\. If someone wants to move one vector to another, the memory that was allocated via _malloc_ is deleted using the operator _delete\[\]_\. Such inconsistent use of memory management functions results in [undefined](https://timsong-cpp.github.io/cppwp/n4950/expr.delete#2) behavior of the program\. The authors have already [fixed](https://github.com/hrydgard/ppsspp/commit/36b7875644da2f9286455afe3b199a86dd9f8577) the code as follows: ```cpp FastVec &operator=(FastVec &&other) { if (this != &other) { free(data_); // <= .... } return *this; } ``` The code was pushed in the [956d784](https://github.com/hrydgard/ppsspp/commit/956d784bde4bcbc3b017ceb3f7bee03d119f0ada) commit \(May 23, 2023\) and fixed in [36b7875](https://github.com/hrydgard/ppsspp/commit/36b7875644da2f9286455afe3b199a86dd9f8577) \(April 2, 2024\)\. The bug has been waiting to be fixed for a little over a year\. **Fragment N2** ```cpp VFSOpenFile *ZipFileReader::OpenFileForRead(VFSFileReference *vfsReference, size_t *size) { ZipFileReaderFileReference *reference = (ZipFileReaderFileReference *)vfsReference; ZipFileReaderOpenFile *openFile = new ZipFileReaderOpenFile(); .... if (zip_stat_index(zip_file_, reference->zi, 0, &zstat) != 0) { lock_.unlock(); delete openFile; return nullptr; } openFile->zf = zip_fopen_index(zip_file_, reference->zi, 0); if (!openFile->zf) { WARN_LOG(G3D, "File with index %d not found in zip", reference->zi); lock_.unlock(); return nullptr; // <= } *size = zstat.size; // Intentionally leaving the mutex locked, will be closed in CloseFile. return openFile; } ``` The PVS\-Studio warning [V773](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'openFile' pointer\. A memory leak is possible\. [ZipFileReader\.cpp 284](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/File/VFS/ZipFileReader.cpp#L284) The analyzer points to the second _return_ of the function\. Indeed, we can see that in the body of the second condition, when exiting the function, they forgot to release the allocated memory fragment \(unlike in the body of the first condition\)\. Here's what the project authors did, which is the easiest way to fix it: ```cpp .... openFile->zf = zip_fopen_index(zip_file_, reference->zi, 0); if (!openFile->zf) { WARN_LOG(G3D, "File with index %d not found in zip", reference->zi); lock_.unlock(); delete openFile; return nullptr; } .... ``` However, the main drawback of this approach is that one needs to always think about early returns from the function\. So, I'd recommend using smart pointers \(_std::unique\_ptr_\): ```cpp VFSOpenFile *ZipFileReader::OpenFileForRead (VFSFileReference *vfsReference, size_t *size) { ZipFileReaderFileReference *reference = (ZipFileReaderFileReference *)vfsReference; auto openFile = std::make_unique(); .... // Intentionally leaving the mutex locked, will be closed in CloseFile. return openFile.release(); } ``` Using _std::unique\_ptr_ saves us from having to think about manual memory release and makes the code more readable and concise\. We also don't have to pay for it\. The code was pushed in the [97cf5f8](https://github.com/hrydgard/ppsspp/commit/97cf5f8d81ec8221e2c074d2dee553efe4e48e40) commit \(March 7, 2023\) and fixed in [9c3c23d](https://github.com/hrydgard/ppsspp/commit/9c3c23daa6d3a70921f3aeb3560c81b497065eb4) \(April 2, 2024\)\. Another bug has been waiting to be fixed for a little over a year :\) **Fragment N3** ```cpp bool ARM64FloatEmitter::TryAnyMOVI(u8 size, ARM64Reg Rd, uint64_t elementValue) { // Try the original size first in case that's more optimal. if (TryMOVI(size, Rd, elementValue)) return true; if (size != 64) { uint64_t masked = elementValue & ((1 << size) - 1); for (int i = size; i < 64; ++i) { value |= masked << i; } } .... } ``` The PVS\-Studio warning: [V629](https://pvs-studio.com/en/docs/warnings/v629/) Consider inspecting the '1 << size' expression\. Bit shifting of the 32\-bit value with a subsequent expansion to the 64\-bit type\. [Arm64Emitter\.cpp 4298](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/Arm64Emitter.cpp#L4298) The [_ARM64FloatEmitter::TryMOVI_](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/Arm64Emitter.cpp#L4225) function limits the _size_ variable to the following values: 16, 32, or 64\. We also have a check that _size_ isn't equal to 64\. Great, that leaves only 16 or 32\. Now let's look at how the _masked_ variable is declared\. This line has two issues\. Let's start with the bitwise shift\. If _size \=\= 16_ nothing bad can happen, but if _size \=\= 32_, it can\. We're shifting the '1' literal, which has the _int_ type\. The _int_ size is implementation\-defined, but it's equal to 32 bits on many platforms\. We get the _1 << 32_ expression\. Unfortunately, the behavior is [undefined](https://timsong-cpp.github.io/cppwp/n4950/expr.shift#1) in this case\. To fix it, all we need to do is change the literal type when performing the shift operation, which is what the project authors did: ```cpp uint64_t masked = elementValue & ((1ULL << size) – 1ULL); ``` But there's more\. We'll take a closer look at the bitwise AND operation\. Let's say that shifting one to the left works fine and all 32 bits are filled correctly\. However, we are doing a bitwise AND with a 64\-bit variable\. According to the standard, the right operand should be sign\-extended to 64 bits\. If the shift results in a positive number, the most significant bits will be zero when extended\. So, a bitwise AND zeroes the most significant part of the _elementValue_ variable\. I doubt this is what the developer had in mind\. The code was pushed in the [00e691d](https://github.com/hrydgard/ppsspp/commit/00e691d6339b035b089f63004801ee92bbfe9c80) commit \(September 11, 2023\) and fixed in [81ef166](https://github.com/hrydgard/ppsspp/commit/81ef1667720732278872dc46aa5633dd7c1d9327) \(April 4, 2024\)\. In this case, it's been a little over six months\. **Fragment N4** ```cpp void Buffer::Printf(const char *fmt, ...) { .... size_t retval = vsnprintf(buffer, sizeof(buffer), fmt, vl); if ((int)retval >= (int)sizeof(buffer)) { // Output was truncated. TODO: Do something. ERROR_LOG(IO, "Buffer::Printf truncated output"); } if (retval < 0) // <= { ERROR_LOG(IO, "Buffer::Printf failed"); } .... } ``` The PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'retval < 0' is always false\. Unsigned type value is never < 0\. [Buffer\.cpp 114](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/Buffer.cpp#L114) The _vsnprintf_ function returns \(the _int_ type\): * the number of characters that would've been written if there hadn't been a constraint passed by the second parameter; * a negative value in case of failure\. The execution result is stored in the _retval_ unsigned variable\. In the first _if_ condition, the developers decided to check that the resulting string was written correctly\. They did this by converting the operands to _int_\. However, they forgot to do the same in the second check, which is why _ERROR\_LOG_ is never executed\. The code was pushed in the [1e22966](https://github.com/hrydgard/ppsspp/commit/1e22966984a606aa9e565bdf792fd607bec80682) commit \(May 1, 2021\), and fixed in [8e58004](https://github.com/hrydgard/ppsspp/commit/8e580045cd423b6cfff0a2c30e53e32f675cf9e8) \(April 11, 2024\)\. Here's our record\-breaker that's been hiding for almost 3 years :\) ### Possible bugs of the upcoming release Now we'll take a look at the bugs that still remain in the project\. We'll surely report them to the developers, and hopefully they'll have time to fix them before the next release\. #### Incorrect memory handling We'd like to start with errors related to memory management, one of the main language features, which is both its advantage and disadvantage\. **Fragments N5** A potential null\-pointer dereference: ```cpp void FramebufferManagerCommon::ReadFramebufferToMemory (VirtualFramebuffer *vfb, int x, int y, int w, int h, RasterChannel channel, Draw::ReadbackMode mode) { // Clamp to bufferWidth. Sometimes block transfers can cause this to hit. if (x + w >= vfb->bufferWidth) { w = vfb->bufferWidth - x; } if (vfb && vfb->fbo) { .... } .... } ``` The PVS\-Studio warning: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'vfb' pointer was utilized before it was verified against nullptr\. Check lines: [3155](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/GPU/Common/FramebufferManagerCommon.cpp#L3155), [3157](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/GPU/Common/FramebufferManagerCommon.cpp#L3157)\. FramebufferManagerCommon\.cpp 3155 We dereference the _vfb_ pointer in the conditional statement, and then a few lines below, we check whether it's null\. I think even if the developers would've asked, "Isn't the pointer null?", it's possible that the pointer can indeed be null\. So, it makes sense to bring the null check before the pointer dereference\. Another option is to remove the check if the pointer can never be null\. Here's another one of these warnings: * V595 The 'dst' pointer was utilized before it was verified against nullptr\. Check lines: 1331, 1346\. [VulkanRenderManager\.cpp 1331](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/GPU/Vulkan/VulkanRenderManager.cpp#L1331) **Fragment N6** This is an interesting case of an error when using a function to manage memory\. ```cpp int sceNetAdhocMatchingSetHelloOpt(int matchingId, int optLenAddr, u32 optDataAddr) { .... if (optLenAddr > context->hellolen) { hello = realloc(hello, optLenAddr); } .... } ``` At first glance, the code is fine, and the program works correctly 99% of the time\. Until _realloc_ can't fulfill the programmer's request\. Let's take a look at the [documentation](https://en.cppreference.com/w/c/memory/realloc) for the _realloc_ function: > If there is not enough memory, the old memory block is not freed and null pointer is returned\. So, if there's not enough memory during reallocation, we'll write a null pointer to the _hello_ pointer, and there'll be a leak\. This is what the analyzer reports: The PVS\-Studio warning: [V701](https://pvs-studio.com/en/docs/warnings/v701/) realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'hello' is lost\. Consider assigning realloc\(\) to a temporary pointer\. [sceNetAdhoc\.cpp 5407](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Core/HLE/sceNetAdhoc.cpp#L5407) **Fragment N7** This fragment could've been discussed in the section on optional condition checks\. However, there's a mishandling of the memory allocation function here, so I put it in here\. ```cpp u32 __MicInput(u32 maxSamples, u32 sampleRate, u32 bufAddr, MICTYPE type, bool block) { .... if (!audioBuf) { audioBuf = new QueueBuf(size); } else { audioBuf->resize(size); } if (!audioBuf) return 0; .... } ``` The PVS\-Studio warning: [V668](https://pvs-studio.com/en/docs/warnings/v668/) There is no sense in testing the 'audioBuf' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. [sceUsbMic\.cpp 432](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Core/HLE/sceUsbMic.cpp#L432) I think the message of the V668 diagnostic rule is quite informative, but still: the operator _new_ can't return a null pointer, so there's no point in checking it\. Most likely, _malloc_ was used here first, and when the devs replaced it with _new_, they simply didn't remove the check\. #### Bitwise shifts In this section of the article, let's look at errors related to bitwise shifts\. **Fragment N8** Here's a fragment that potentially contains UB\. ```cpp void ARMXEmitter::VMOVN(u32 Size, ARMReg Vd, ARMReg Vm) { .... _dbg_assert_msg_((Size & I_8) == 0, "%s cannot narrow from I_8", __FUNCTION__); // For consistency with assembler syntax and VMOVL - encode one size down. int halfSize = encodedSize(Size) - 1; Write32( (0xF3B << 20) | (halfSize << 18) | (1 << 17) | EncodeVd(Vd) | (1 << 9) | EncodeVm(Vm)); } ``` The PVS\-Studio warning: [V610](https://pvs-studio.com/en/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<'\. The left operand is negative \('halfSize' \= \[\-1\.\.2\]\)\. [ArmEmitter\.cpp 2870](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/ArmEmitter.cpp#L2870) Let's look at the possible values of the _halfSize_ variable that the analyzer refers to\. We can see that its value is the result of the _encodedSize_ function minus one\. Here's the _encodedSize_ function: ```cpp u32 encodedSize(u32 value) { if (value & I_8) return 0; else if (value & I_16) return 1; else if ((value & I_32) || (value & F_32)) return 2; else if (value & I_64) return 3; else _dbg_assert_msg_(false, "Passed invalid size to integer NEON instruction"); return 0; } ``` As we can see, the possible value of _halfSize_ has a range of \[\-1\.\.2\]\. This means that a shift to the left by a value of \-1 is possible, resulting in [undefined behavior](https://timsong-cpp.github.io/cppwp/n4950/expr.shift#1)\. Unfortunately, the check in _\_dbg\_assert\_msg\__ won't help as this macro [expands](https://github.com/hrydgard/ppsspp/blob/9d48e5120541c77d992f96849753a31802166b53/Common/Log.h#L146-L151) to void on release\. Here are some similar warnings: * V610 Undefined behavior\. Check the shift operator '<<'\. The left operand is negative \('halfSize' \= \[\-1\.\.2\]\)\. ArmEmitter\.cpp 2884 * V610 Undefined behavior\. Check the shift operator '<<'\. The left operand is negative \('halfSize' \= \[\-1\.\.2\]\)\. ArmEmitter\.cpp 2897 * V610 Undefined behavior\. Check the shift operator '<<'\. The right operand is negative \('\(n \- 1\)' \= \[\-1\.\.3\]\)\. MIPSIntVFPU\.cpp 2149 #### Optional condition checks Let's move on to cases where the developers have added redundant checks, either as a result of incorrect calculation of logical expressions, or as a safety precaution\. **Fragments N9\-10** I promised to show bugs of the same pattern that have already been fixed \(see fragment N4\)\. Here's the first of them: ```cpp void WebSocketInputState::ButtonsPress(DebuggerRequest &req) { .... PressInfo press; press.duration = 1; if (!req.ParamU32("duration", &press.duration, false, DebuggerParamType::OPTIONAL)) return; if (press.duration < 0) return req.Fail("Parameter 'duration' must not be negative"); .... } ``` The PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'press\.duration < 0' is always false\. Unsigned type value is never < 0\. [InputSubscriber\.cpp 179](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Core/Debugger/WebSocket/InputSubscriber.cpp#L179) Let's see what _press\.duration_ looks like: ```cpp struct WebSocketInputState : public DebuggerSubscriber { .... protected: struct PressInfo { std::string ticket; uint32_t button; uint32_t duration; // <= std::string Event(); }; .... }; ``` The gist is roughly the same as before\. This is an unsigned integer variable\. Some value that shouldn't be negative is written to it\. It's possible that the variable used to be signed, and the programmer forgot to fix the check\. Here's another one: V547 Expression 'press\.duration < 0' is always false\. Unsigned type value is never < 0\. [InputSubscriber\.cpp 208](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Core/Debugger/WebSocket/InputSubscriber.cpp#L208) **Fragment N11** The code snippet perfectly illustrates another advantage of static analyzers\. Seeing the code below, one can realize that it's impossible to look at it for more than a few seconds\. The eye has nothing to catch on, let alone find an error\.\.\. However, this is a trivial task for analyzers\. ```cpp void XEmitter::ABI_CallFunctionRR(const void *func, X64Reg reg1, X64Reg reg2) { if (reg2 != ABI_PARAM1) { if (reg1 != ABI_PARAM1) MOV(64, R(ABI_PARAM1), R(reg1)); if (reg2 != ABI_PARAM2) MOV(64, R(ABI_PARAM2), R(reg2)); } else { if (reg2 != ABI_PARAM2) MOV(64, R(ABI_PARAM2), R(reg2)); if (reg1 != ABI_PARAM1) MOV(64, R(ABI_PARAM1), R(reg1)); } } ``` The PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'reg2 \!\= RSI' is always true\. [ABI\.cpp 465](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/ABI.cpp#L465) Note that _RSI_ is a value hiding under the [_ABI\_PARAM2_](https://github.com/hrydgard/ppsspp/blob/9d48e5120541c77d992f96849753a31802166b53/Common/ABI.h#L47-L76) macro\. We get to the _else_ branch if _reg2 \=\= ABI\_PARAM1_\. If so, the first check in the _else_ branch is always true, and we can simplify the code: ```cpp else { MOV(64, R(ABI_PARAM2), R(reg2)); if (reg1 != ABI_PARAM1) MOV(64, R(ABI_PARAM1), R(reg1)); } ``` **Fragment N12** Here's a slightly different case of a redundant check\. ```cpp void netValidateLoopMemory() { // Allocate Memory if it wasn't valid/allocated // after loaded from old SaveState if ( !apctlThreadHackAddr || ( apctlThreadHackAddr && strcmp("apctlThreadHack", kernelMemory.GetBlockTag(apctlThreadHackAddr)) != 0)) { .... } } ``` The PVS\-Studio warning: [V728](https://pvs-studio.com/en/docs/warnings/v728/) An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite expressions '\!apctlThreadHackAddr' and 'apctlThreadHackAddr'\. [sceNet\.cpp 257](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Core/HLE/sceNet.cpp#L257) Let's refresh our memory on Boolean algebra a bit, and imagine that we have an expression: ```cpp ¬A V (A ∧ B) ``` We'll utilize the distributive property and get the following: ```cpp (¬A V A) ∧ (¬A V B) ``` The _\(¬A_ _V_ _A\)_ expression is always true\. So, this is the only thing left: ```cpp ¬A V B ``` This means that the second _apctlThreadHackAddr_ check is in fact redundant, and we can simplify the code: ```cpp void netValidateLoopMemory() { // Allocate Memory if it wasn't valid/allocated // after loaded from old SaveState if ( !apctlThreadHackAddr || strcmp("apctlThreadHack", kernelMemory.GetBlockTag(apctlThreadHackAddr)) != 0) { .... } } ``` **Fragment N13** Here's another interesting example of an overcomplicated check\. ```cpp bool ApplyMemoryValidation(const IRWriter &in, IRWriter &out, const IROptions &opts) { .... for (IRInst inst : in.GetInstructions()) { IRMemoryOpInfo info = IROpMemoryAccessSize(inst.op); // Note: we only combine word aligned accesses. if (info.size != 0 && inst.src1 == MIPS_REG_SP && info.size == 4) // <= { .... } .... } .... } ``` The PVS\-Studio warning: [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. [IRPassSimplify\.cpp 1792](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Core/MIPS/IR/IRPassSimplify.cpp#L1792) Something strange is going on in the check\. If _info\.size \=\= 4_, then the _info\.size \!\= 0_ check is obviously redundant\. The developers may have needed to check something else, or they could've simplified the check\. #### Decreased performance Of course, I could also put the warnings from the previous section here\. However, here are the warnings from the [micro\-optimization group of diagnostic rules](https://pvs-studio.com/en/docs/warnings/#MicroOptimizationsCPP)\. Let's take a look at the following code snippet: **Fragment N14** ```cpp void JsonWriter::pushArray() { str_ << arrayComma() << arrayIndent() << "["; stack_.back().first = false; stack_.push_back(StackEntry(ARRAY)); } ``` The PVS\-Studio warning: [V823](https://pvs-studio.com/en/docs/warnings/v823/) Decreased performance\. Object may be created in\-place in the 'stack\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. JSONWriter\.cpp [101](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/Data/Format/JSONWriter.cpp#L101) The analyzer suggests replacing the call to the _push\_back_ function with _emplace\_back_ in the _stack\__ container\. Well, let's see what the _stack\_ variable_ and the _StackEntry_ type are\. ```cpp class JsonWriter { private: .... struct StackEntry { StackEntry(BlockType t) : type(t), first(true) {} BlockType type; bool first; }; std::vector stack_; .... }; ``` As we can see, the _stack\__ variable is a normal _std::vector_, and _StackEntry_ is a structure that has a converting constructor\. So, we can make the code a bit more performance efficient if we replace _push\_back_ with _emplace\_back_\. This is what we have in the case of _push\_back_: 1. The _StackEntry_ constructor is called, creating a temporary object\. 1. The temporary object is passed via the _rvalue_ reference to _push\_back_\. 1. The address in the vector at which the object is constructed using _placement new_ is calculated\. 1. The temporary object passed by the _rvalue_ reference is moved to _placement new_\. 1. An implicit move constructor that copies 2 data members is called\. This is what we have if we replace _push\_back_ with _emplace\_back_: 1. Parameters of the _StackEntry_ constructor are perfect\-forwarded to _emplace\_back_\. No temporary object is created\. 1. The address in the vector at which the object is constructed using _placement new_ is calculated\. 1. The arguments of _emplace\_back_ are perfect\-forwarded to _placement new_, the object is constructed right in the vector\. As we can see, in this case, _emplace\_back_ helps avoid calling the move constructor\. Some might call it a drop in the ocean, but hey, we're C\+\+ developers, aren't we? The battle for performance never ends :\) Here are the similar warnings: * V823 Decreased performance\. Object may be created in\-place in the 'stack\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. JSONWriter\.cpp 22 * V823 Decreased performance\. Object may be created in\-place in the 'stack\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. JSONWriter\.cpp 27 * V823 Decreased performance\. Object may be created in\-place in the 'stack\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. JSONWriter\.cpp 32 * V823 Decreased performance\. Object may be created in\-place in the 'stack\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. JSONWriter\.cpp 87 * V823 Decreased performance\. Object may be created in\-place in the 'stack\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. JSONWriter\.cpp 108 * V823 Decreased performance\. Object may be created in\-place in the 'callbacks\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. VulkanContext\.h 123 * V823 Decreased performance\. Object may be created in\-place in the 'compileQueue\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. VulkanRenderManager\.cpp 876 * V823 Decreased performance\. Object may be created in\-place in the 'threads\_' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. HTTPServer\.cpp 48 * V823 Decreased performance\. Object may be created in\-place in the 'ip\_ranges' container\. Consider replacing methods: 'push\_back' \-\> 'emplace\_back'\. proAdhoc\.cpp 1896 **Fragment N15** ```cpp bool LoadRemoteFileList(const Path &url, const std::string &userAgent, bool *cancel, std::vector &files) { .... std::string contentType; for (const std::string &header : responseHeaders) { if (startsWithNoCase(header, "Content-Type:")) { contentType = header.substr(strlen("Content-Type:")); // Strip any whitespace (TODO: maybe move this to stringutil?) contentType.erase(0, contentType.find_first_not_of(" \t\r\n")); contentType.erase(contentType.find_last_not_of(" \t\r\n") + 1); } } .... } ``` The PVS\-Studio warning: V814 Decreased performance\. The 'strlen' function was called multiple times inside the body of a loop\. [PathBrowser\.cpp 58](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/File/PathBrowser.cpp#L58) After seeing the warning, readers may say, "Compilers are perfectly capable of optimizing multiple calls to _strlen_ in a loop"\. Yes and no\. In fact, I'd like to focus on another point\. This warning helped uncover rather strange code\. This is what it does: 1. It iterates over all lines in the _responseHeaders_ vector\. 1. It checks whether each line starts with the "Content\-Type:" header\. 1. If so, the substring without a header is stored in the _contentType_ external variable of the loop, and whitespace characters are removed\. The compiler is unlikely to [optimize](https://godbolt.org/z/5Poezdv7M) this by removing the loop :\) Maybe the loop should've been terminated as soon as the first header was encountered\. Let's not forget about _strlen_\. You can put this string literal in _std::string\_view_, and a constant value is guaranteed to be substituted at compile time: ```cpp .... constexpr std::string_view ContentTypeHeader = "Content-Type:"; std::string contentType; for (const std::string &header : responseHeaders) { if (startsWithNoCase(header, ContentTypeHeader)) { contentType = header.substr(strlen(ContentTypeHeader.size())); .... } } .... ``` **Fragment N16** This code snippet is much worse than in the previous ones\. ```cpp bool ZipFileReader::GetFileListing (const char *orig_path, std::vector *listing, const char *filter = 0) { .... for (auto fiter = files.begin(); fiter != files.end(); ++fiter) { std::string fpath = path; File::FileInfo info; info.name = *fiter; std::string relativePath = std::string(path).substr(inZipPath_.size()); info.fullName = Path(relativePath + *fiter); info.exists = true; info.isWritable = false; info.isDirectory = false; std::string ext = info.fullName.GetFileExtension(); if (filter) { if (filters.find(ext) == filters.end()) continue; } listing->push_back(info); } .... } ``` The PVS\-Studio warning: [V808](https://pvs-studio.com/en/docs/warnings/v808/) 'fpath' object of 'basic\_string' type was created but was not utilized\. [ZipFileReader\.cpp 128](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/File/VFS/ZipFileReader.cpp#L128) After seeing the warning text, I started looking for where the _fpath_ variable was supposed to be used\. What I found rather "surprised" me, to put it mildly\. Indeed, at each loop iteration we create an object of the _std::string_ type that we don't use \(the _fpath_ variable\)\. Next, let's look at the intended code fragment for using _fpath_: ```cpp std::string relativePath = std::string(path).substr(inZipPath_.size()); ``` Most likely the _substr_ function should've been called for _fpath_\. Instead, we create another temporary object of the _std::string_ type\. ![1167_ppsspp/image2.png](https://import.viva64.com/docx/blog/1167_ppsspp/image2.png) Then, _substr_ creates another copy of the string\. Only then will everything be placed in the _relativePath_ variable\. ![1167_ppsspp/image3.png](https://import.viva64.com/docx/blog/1167_ppsspp/image3.png) The same thing happens in another [loop](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/File/VFS/ZipFileReader.cpp#L118) a little higher up the code\. And as an icing on the cake, I should mention that all the calculations could've been done once, because the variables used for these calculations don't change in the loop\. ![1167_ppsspp/image4.png](https://import.viva64.com/docx/blog/1167_ppsspp/image4.png) #### Miscellaneous This section contains all the other analyzer warnings that are difficult to fit into the above sections\. **Fragment N17** After the last code snippet, it may be hard to surprise you, but I'll try anyway\. ```cpp MockPSP::MockPSP(UI::LayoutParams *layoutParams) : AnchorLayout(layoutParams) { .... AddButton(CTRL_RTRIGGER, ImageID("I_R"), ImageID("I_SHOULDER_LINE"), 0.0f, LayoutSize(50.0f, 16.0f, 397.0f, 0.0f))->SetFlipHBG(true); .... } ``` The PVS\-Studio warning: [V601](https://pvs-studio.com/en/docs/warnings/v601/) The bool type is implicitly cast to the float type\. Inspect the first argument\. [ControlMappingScreen\.cpp 810](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/UI/ControlMappingScreen.cpp#L810) This is the _AddButton_ function call, but we don't care about it\. Take a look at the _SetFlipHBG\(true\)_ function call inside\. The thing is that it actually takes a parameter of the _float_ type: ```cpp MockButton *SetFlipHBG(float f); ``` How did the _true_ flag get in it? A mystery\. **Fragment N18** I'd like to end the article on a serious note by highlighting a security\-related error in a function\. You might wonder, "What's there to protect in a game console emulator?"—and this may be a valid question\. However, we still have a function called _sha1\_hmac_ here\.
Here's some info for those who aren't very familiar with cryptographic algorithms: HMAC \(hash\-based message authentication code\) is a mechanism for verifying the information integrity to ensure that data hasn't been altered by someone else\. The HMAC implementation is required for the IPsec protocol\. Other internet protocols, such as TLS, also use HMAC\.
```cpp void sha1_hmac( unsigned char *key, int keylen, unsigned char *input, int ilen, unsigned char output[20] ) { sha1_context ctx; sha1_hmac_starts( &ctx, key, keylen ); sha1_hmac_update( &ctx, input, ilen ); sha1_hmac_finish( &ctx, output ); memset( &ctx, 0, sizeof( sha1_context ) ); } ``` The PVS\-Studio warning: [V597](https://pvs-studio.com/en/docs/warnings/v597/) The compiler could delete the 'memset' function call, which is used to flush 'ctx' object\. The memset\_s\(\) function should be used to erase the private data\. [md5\.cp p 290](https://github.com/hrydgard/ppsspp/blob/d479b74ed9c3e321bc3735da29bc125a2ac3b9b2/Common/Crypto/md5.cpp#L290) The analyzer highlights the issue that has been has been known for a very long time\. In short, during the optimization process, compilers [can delete](https://godbolt.org/z/zYhss55xE) memory writes without subsequent reads \(Dead Store Elimination\)\. In our case, such a write would be a call to _memset_, which should clear the private information in the _ctx_ object\. You can learn more about the issue [here](https://pvs-studio.com/en/blog/posts/cpp/0388/)\. To prevent the compiler from deleting the clearing of private data is actually a non\-trivial task\. This [proposal](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1315r7.html) reflects all currently available ways to explicitly clear memory \(see the "The problem" section\)\. Soon, we'll be resolving the issue via the [_memset\_explicit_](https://en.cppreference.com/w/c/string/byte/memset) function that has been introduced in C23\. In C\+\+26, there's also a good chance to see this function implemented but in the form of a function template\. The fixed code looks as follows: ```cpp void sha1_hmac( unsigned char *key, int keylen, unsigned char *input, int ilen, unsigned char output[20] ) { sha1_context ctx; sha1_hmac_starts( &ctx, key, keylen ); sha1_hmac_update( &ctx, input, ilen ); sha1_hmac_finish( &ctx, output ); // won't be optimized out #ifdef __cplusplus memset_explicit( ctx ); #else memset_explicit( &ctx, 0, sizeof( sha1_context ) ); #endif } ``` Here are the similar warnings: * V597 The compiler could delete the 'memset' function call, which is used to flush 'tmpbuf' buffer\. The memset\_s\(\) function should be used to erase the private data\. sha1\.cpp 379 * V597 The compiler could delete the 'memset' function call, which is used to flush 'sum' buffer\. The memset\_s\(\) function should be used to erase the private data\. sha1\.cpp 355 * V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object\. The memset\_s\(\) function should be used to erase the private data\. sha1\.cpp 325 * V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object\. The memset\_s\(\) function should be used to erase the private data\. md5\.cpp 359 * V597 The compiler could delete the 'memset' function call, which is used to flush 'tmpbuf' buffer\. The memset\_s\(\) function should be used to erase the private data\. md5\.cpp 344 * V597 The compiler could delete the 'memset' function call, which is used to flush 'sum' buffer\. The memset\_s\(\) function should be used to erase the private data\. md5\.cpp 320 * V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object\. The memset\_s\(\) function should be used to erase the private data\. md5\.cpp 290 ## Conclusion This article doesn't contain the whole list of detected errors\. So, we'll post a separate article about the rest of them\. Still, even from what we've looked at here, we can draw the following conclusions: * Once in the project, errors can stay there for several years \(we've even seen a 3\-year\-old error\)\. * You'll be lucky if someone reports those errors and they can be fixed immediately\. However, even this doesn't guarantee that you'll fix all fragments containing the same error\. * [PVS\-Studio](https://pvs-studio.com/en/) has issued genuinely helpful warnings, and the fact that some of them have already been reported and fixed confirms it\. Finally, I'd like to emphasize that static analyzers enable us to detect bugs at early development stages, before the code gets released\. This is particularly important for bigger projects such as PPSSPP, where having a large number of developers involved can lead to complex and difficult\-to\-detect issues\. However, finding errors at early stages of development isn't the only benefit of static analyzers\. They can also: * reduce the time and cost of troubleshooting; * free up resources, so you can focus on addressing business challenges; * ensure quality control; * support a wide range of coding standards; * and so on\. [PVS\-Studio](https://pvs-studio.com/en/) has several [options to use it for free](https://pvs-studio.com/en/blog/posts/0614/)\. If you represent a commercial organization, you can get a free trial version of the PVS\-Studio analyzer [here](https://pvs-studio.com/en/pvs-studio/try-free/)\. Thank you for reading the article\. See you soon\!