Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
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.
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 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:
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.
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 code using the PVS-Studio analyzer on Linux. The analyzer version is 7.32.
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:
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.
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.
template<class T>
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 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
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 behavior of the program.
The authors have already fixed the code as follows:
FastVec &operator=(FastVec &&other)
{
if (this != &other)
{
free(data_); // <=
....
}
return *this;
}The code was pushed in the 956d784 commit (May 23, 2023) and fixed in 36b7875 (April 2, 2024). The bug has been waiting to be fixed for a little over a year.
Fragment N2
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 The function was exited without releasing the 'openFile' pointer. A memory leak is possible. ZipFileReader.cpp 284
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:
....
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):
VFSOpenFile *ZipFileReader::OpenFileForRead
(VFSFileReference *vfsReference, size_t *size)
{
ZipFileReaderFileReference *reference =
(ZipFileReaderFileReference *)vfsReference;
auto openFile = std::make_unique<ZipFileReaderOpenFile>();
....
// 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 commit (March 7, 2023) and fixed in 9c3c23d (April 2, 2024). Another bug has been waiting to be fixed for a little over a year :)
Fragment N3
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 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
The ARM64FloatEmitter::TryMOVI 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 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:
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 commit (September 11, 2023) and fixed in 81ef166 (April 4, 2024). In this case, it's been a little over six months.
Fragment N4
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 Expression 'retval < 0' is always false. Unsigned type value is never < 0. Buffer.cpp 114
The vsnprintf function returns (the int type):
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 commit (May 1, 2021), and fixed in 8e58004 (April 11, 2024). Here's our record-breaker that's been hiding for almost 3 years :)
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.
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:
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 The 'vfb' pointer was utilized before it was verified against nullptr. Check lines: 3155, 3157. 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:
Fragment N6
This is an interesting case of an error when using a function to manage memory.
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 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 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'hello' is lost. Consider assigning realloc() to a temporary pointer. sceNetAdhoc.cpp 5407
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.
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 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
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.
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.
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 Undefined behavior. Check the shift operator '<<'. The left operand is negative ('halfSize' = [-1..2]). ArmEmitter.cpp 2870
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:
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. Unfortunately, the check in _dbg_assert_msg_ won't help as this macro expands to void on release.
Here are some similar warnings:
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:
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 Expression 'press.duration < 0' is always false. Unsigned type value is never < 0. InputSubscriber.cpp 179
Let's see what press.duration looks like:
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
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.
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 Expression 'reg2 != RSI' is always true. ABI.cpp 465
Note that RSI is a value hiding under the ABI_PARAM2 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:
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.
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 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!apctlThreadHackAddr' and 'apctlThreadHackAddr'. sceNet.cpp 257
Let's refresh our memory on Boolean algebra a bit, and imagine that we have an expression:
¬A V (A ∧ B)We'll utilize the distributive property and get the following:
(¬A V A) ∧ (¬A V B)The (¬A V A) expression is always true. So, this is the only thing left:
¬A V BThis means that the second apctlThreadHackAddr check is in fact redundant, and we can simplify the code:
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.
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 Consider inspecting this expression. The expression is excessive or contains a misprint. IRPassSimplify.cpp 1792
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.
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.
Let's take a look at the following code snippet:
Fragment N14
void JsonWriter::pushArray()
{
str_ << arrayComma() << arrayIndent() << "[";
stack_.back().first = false;
stack_.push_back(StackEntry(ARRAY));
}The PVS-Studio warning: V823 Decreased performance. Object may be created in-place in the 'stack_' container. Consider replacing methods: 'push_back' -> 'emplace_back'. JSONWriter.cpp 101
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.
class JsonWriter
{
private:
....
struct StackEntry
{
StackEntry(BlockType t) : type(t), first(true) {}
BlockType type;
bool first;
};
std::vector<StackEntry> 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:
This is what we have if we replace push_back with emplace_back:
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:
Fragment N15
bool LoadRemoteFileList(const Path &url, const std::string &userAgent,
bool *cancel, std::vector<File::FileInfo> &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
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:
The compiler is unlikely to optimize 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:
....
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.
bool ZipFileReader::GetFileListing
(const char *orig_path, std::vector<File::FileInfo> *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 'fpath' object of 'basic_string' type was created but was not utilized. ZipFileReader.cpp 128
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:
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.
Then, substr creates another copy of the string. Only then will everything be placed in the relativePath variable.
The same thing happens in another loop 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.
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.
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 The bool type is implicitly cast to the float type. Inspect the first argument. ControlMappingScreen.cpp 810
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:
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.
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.
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 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
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 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.
To prevent the compiler from deleting the clearing of private data is actually a non-trivial task. This proposal 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 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:
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:
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:
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:
PVS-Studio has several options to use it for free. If you represent a commercial organization, you can get a free trial version of the PVS-Studio analyzer here.
Thank you for reading the article. See you soon!
0
0
0
145
0
0