It's December, the first snow has already fallen, and it means New Year's Eve is around the corner. Ahead of the holidays, we'll show you the most interesting errors found in the code of the popular open-source projects. Our authors have written a lot of informative articles about bugs, and now we'll recap them.
Sit comfortably in a warm, soft armchair next to the fireplace after a hard workday. If you don't have a fireplace, just sit in the cushioned armchair. +10 points to coziness.
You're holding a mug of hot chocolate or tea. Drink carefully so as not to burn yourself and prolong the enjoyment of the flavor. +15 points to your good mood.
Grab your tablet or laptop, and let's read. Let yourself chill out: +9 points to intelligence and +5 points to programming skills.
Evening. The snow glistens under the streetlights. The frost softly pinches your face. Silence surrounds you. Snowflakes are waltzing out of the sky. The New Year is coming, and everything is frozen in anticipation of a miracle. I think that everybody is feeling the looming holiday. I warmly recall of how my family went to the circus in the winter. It was amazing! Numerous colorful lamps were twinkling, and the holiday spirit was filling you! Fun music, amusing clowns, and the popcorn smell. A mysterious mustachioed ringmaster — wearing a black cylinder and carrying an antique cane — set the mysterious atmosphere. He announces the act, a little adventure begins in the ring, miracles happen, and the circus tent freezes until the end of the show. Then comes the applause, the storm of applause, and everything coalesces in a single burst. Although we have grown up, we still can create our own miracles for our loved ones, friends, and everybody we want to make happy.
So, today you'll not only read an article about how someone makes mistakes somewhere in the code but also the magic of coding. You'll find 10 sweeping and spectacular cases of error debugging — all for you! So, *puts on the Sprechstallmeister mask (S:)*, let's take a look at our show program:
S: "Our upcoming New Year's show is ready for your surprise and admiration with the new program! A show that will leave you in awe; here you'll see multiple death-defying stunts, from memory leaks to pointer dereferencing without any harness (i.e. check)! Ridiculous typos!
You will witness a world-class show with the amazing performances of experienced programmers on open-source projects!"
S: "The Show for programmers!" Well, well, well... If we're going to start our magic... from 9 to 0! Truly magical numbers, why would we need more?"
S: "Sirs and Mesdames! Ladies and Gentlemen! Programmers of all stripes! It's time for magic and wizardry! The code will make you doubt the very concept whether an effective code review even exists."
Article: "Checking the GCC 13 compiler with the help of PVS-Studio".
static bool
combine_reaching_defs (ext_cand *cand,
const_rtx set_pat,
ext_state *state)
{
....
while ( REG_P (SET_SRC (*dest_sub_rtx))
&& (REGNO (SET_SRC (*dest_sub_rtx)) == REGNO (SET_DEST (set))))
{
....
if (....)
break;
if (....)
break;
....
break;
}
}The PVS-Studio warning: V612 An unconditional 'break' within a loop. ree.cc 985
The PVS-Studio analyzer has detected an unconditional break within the loop at the first iteration. If we look further, we can also find many other break's that are conditional. Perhaps this is a way to avoid writing the goto statement. It looks odd, though, since the statement is used explicitly throughout the project.
S: "My honorable audience! The King himself visited us today! However, something very dangerous and forbidden in the world of programming came to us... Don't be afraid! We can defeat it. We just need to find out how!"
Article: "Microsoft PowerToys: the GitHub king among C# projects with C++ errors".
void SetNumLockToPreviousState(....)
{
int key_count = 2;
LPINPUT keyEventList = new INPUT[size_t(key_count)]();
memset(keyEventList, 0, sizeof(keyEventList));
....
}PVS-Studio issues three warnings for this code at once:
We even have a separate article about such errors: "The most dangerous function in the C/C++ world".
In this case, the developers wanted to fill the keyEventList array with zeros. Pay attention to the third parameter — the number of bytes the developers wanted to fill with zeros. In this case, sizeof(keyEventList) evaluates the pointer size instead of the array size. It depends on the target platform, but most often it's 4 or 8 bytes. However, the size of the structure is clearly larger than 4 or 8 bytes:
typedef struct tagINPUT
{
DWORD type;
union
{
MOUSEINPUT mi;
KEYBDINPUT ki;
HARDWAREINPUT hi;
} DUMMYUNIONNAME;
} INPUT, *PINPUT, FAR* LPINPUT;S: "Mysteries, mysteries, mysteries... Now we're going to fight! Take a look at the code! Can you quickly find the small but very important symbol here? The symbol often means the end of everything! However, sometimes it means the beginning of something new and wonderful. Well, it can also denote the access to the data members of the object..."
Article: "PVS-Studio vs CodeLite: a battle for the perfect code".
std::unordered_set<wxChar> delimiters =
{ ':', '@', '.', '!', ' ', '\t', '.', '\\',
'+', '*', '-', '<', '>', '[', ']', '(',
')', '{', '}', '=', '%', '#', '^', '&',
'\'', '"', '/', '|', ',', '~', ';', '`' };The PVS-Studio warning: V766 An item with the same key ''.'' has already been added. wxCodeCompletionBoxManager.cpp:19
Can you see what's wrong here? Because of all these single quotation marks, an eye may not notice that the '.' character has been added twice. It's possible that they forgot to add some other character here, or it's just a random duplicate that can be removed.
S: "Do you play to win? Or not to lose? Or is it an illusion of choice? The act will answer these questions and masterfully show the illusion of overriding a virtual function!"
Article: "PVS-Studio vs CodeLite: a battle for the perfect code".
Here's what the function looks like in the base class:
class WXDLLIMPEXP_CORE wxGenericProgressDialog : public wxDialog
{
public:
....
virtual bool Update(int value,
const wxString& newmsg = wxEmptyString,
bool *skip = NULL);
....
};And here's what the function looks like in the derived class:
class clProgressDlg : public wxProgressDialog
{
public:
....
bool Update(int value, const wxString& msg);
....
};The PVS-Studio warning: V762 It is possible a virtual function was overridden incorrectly. See third argument of function 'Update' in derived class 'clProgressDlg' and base class 'wxGenericProgressDialog'. progress_dialog.h:47, progdlgg.h:44
Since the first two function parameters are the same, we can conclude that the developers wanted to override the virtual function. However, parameters with default arguments are also part of the function signature. Therefore, the clProgressDlg::Update function doesn't actually override the wxGenericProgressDialog::Update virtual function but shadows it.
The correct way to declare a virtual function is as follows:
class clProgressDlg : public wxProgressDialog
{
public:
....
bool Update(int value, const wxString& msg, bool *skip);
....
};Since C++11, it's possible (and even necessary) to use the override specifier to avoid such errors:
class clProgressDlg : public wxProgressDialog
{
public:
....
bool Update(int value, const wxString& msg, bool *skip) override;
....
};Now the compiler generates an error if the virtual function doesn't override anything in the base class. That's the answer.
S: "Kings, illusions, magic, and mysteries. Indeed, the universe is mad today, but now you'll see something truly shocking! Don't watch this if you're faint-hearted!"
Article: "Oh my C! How they wrote code back in the Quake days".
// PVS-Studio: void Mod_LoadTexinfo (lump_t *l)
// PVS-Studio: client/model.c
mtexinfo_t *out;
// ...
for (j=0 ; j<8 ; j++)
{
ut->vecs[0][j] = LittleFloat (in->vecs[0][j]);
}The PVS-Studio warning: V557 Array overrun is possible. The value of 'j' index could reach 7.
We have added the declaration of mtexinfo_t *out variable for better understanding:
// PVS-Studio: client/model.c
typedef struct
{
float vecs[2][4]; // PVS-Studio: see what is wrong here?
float mipadjust;
texture_t *texture;
int flags;
} mtexinfo_t;
// PVS-Studio: client/common.h
float (*LittleFloat) (float l);So, there is a 2d array that is treated as a 1d one. Few people will see the issue. The programmers can suppose that the array sequentially lies in memory. True enough, according to C99, 6.2.5 Types p20.
"An array type describes a contiguously allocated nonempty set of objects with a particular member object type, called the element type. The element type shall be complete whenever the array type is specified."
What if we remind you that C language has a pointer to array-of-T type? While accessing the array inside the loop, j certainly exceeds the value of 3 and the operator[] returns the variable of the float[4] type. As a result, the array element will be read outside its bounds, which is a classic example of undefined behavior (UB).
S: "Please step right up, Sirs and Mesdames! You won't believe your eyes! Imagine that conditions are just conventions, and the truth can deceive your expectations..."
Article: "Heroes of Code and Magic: VCMI game engine analysis".
void deallocate_segment(....)
{
....
if (seg_index >= first_block)
{
segment_element_allocator_traits::deallocate(....);
}
else
if (seg_index == 0)
{
elements_to_deallocate = first_block > 0
? this->segment_size(first_block)
: this->segment_size(0);
....
}
}The PVS-Studio warning: V547 Expression 'first_block > 0' is always true. concurrent_vector.h 669
Let's figure out what's going on here. If the first check fails, then seg_index < first_block. Now, if seg_index == 0, then first_block > 0. After that, we again check that first_block > 0, and the analyzer indicates that this condition is always true. Therefore, the this->segment_size(0) expression will never be executed.
It's good that we have technology for static analysis to detect such errors! All this is possible due to the use of symbolic execution technology in the analyzer. Looks like magic, isn't it? And that's probably how average person see it. Now, imagine this:
Once upon a time, there was some kind of high-tech race. While colonizing another planet, technology was irretrievably lost during a war for power... Several hundred years passed, and the technological race traveled through a time with wise and shrewd kings, valiant knights, and mysterious magical ruins. Various rituals and artifacts provided the energy for the technology to work. And, without understanding how it actually works, these creatures call upon magic. By the way, this is the plot of Heroes of Might and Magic.
If you're interested in learning about the symbolic execution technology, I recommend you read this article: "PVS-Studio: static code analysis technology".
S: "Oh, those programmer jesters. If you don't let them get enough sleep once, they start fiddling with code! Now our ring will feature funny typos and their tricky consequences!"
Article: "FreeCAD and undefined behavior in C++ code: meditation for developers".
QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
ViewProviderPage* vpp = getViewProviderPage(dView);
if (!vpp) {
return vpp->getQGVPage();
}
return nullptr;
}The PVS-Studio warning: V522 [CWE-476, CERT-EXP34-C] Dereferencing of the null pointer 'vpp' might take place. QGIView.cpp 592
The code author made a typo while writing conditions:
The thing is, there's an identical function nearby.
QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
ViewProviderPage* vpp = getViewProviderPage(dView);
if (!vpp) {
return vpp->getQGVPage();
}
return nullptr;
}
QGSPage* QGIView::getQGSPage(TechDraw::DrawView* dView)
{
ViewProviderPage* vpp = getViewProviderPage(dView);
if (vpp) {
return vpp->getQGSPage();
}
return nullptr;
}It's unclear how that happened, but it's very likely that a developer copied and pasted the second function. It's strange that a developer fixed the error in the second function but left it in the first one. Then, someone noticed that the getQGSPage function wasn't working correctly and fixed it.
It's also interesting because the author of this article encountered it just after his previous article was released: "Simple, yet easy-to-miss errors in code". I recommend you read it: here is another interesting story from our technical support.
S: "There's another mystery! Once again, here is a code covered with mysteries and illusions of how to execute it correctly! Is it inevitable fate or someone's bad joke? Try to guess what's wrong here, will you?"
Article: "Games! How they write code for SDL (+ interview with the creator)".
File: SDL/src/events/SDL_mouse.c (GitHub permalink)
char *SDL_iconv_string(const char *tocode, const char *fromcode,
const char *inbuf, size_t inbytesleft)
{
SDL_iconv_t cd;
....
cd = SDL_iconv_open(tocode, fromcode);
if (cd == (SDL_iconv_t)-1) {
/* See if we can recover here (fixes iconv on Solaris 11) */
if (tocode == NULL || !*tocode) {
tocode = "UTF-8";
}
if (fromcode == NULL || !*fromcode) {
fromcode = "UTF-8";
}
cd = SDL_iconv_open(tocode, fromcode);
}
....
}The PVS-Studio warning: V595 The 'tocode' pointer was utilized before it was verified against nullptr. Check lines: 37, 789, 792. SDL/src/stdlib/SDL_iconv.c:792:1
To understand the issue, let's take the plunge into the implementation of the SDL_iconv_open function:
SDL_iconv_t SDL_iconv_open(const char *tocode, const char *fromcode)
{
return (SDL_iconv_t)((uintptr_t)iconv_open(tocode, fromcode));
}Null pointers in the tocode and fromcode arguments can be passed to the input of the SDL_iconv_string function. If so, they will be passed in iconv_open function without checking.
When we look at the man page of this function from the GNU project, we don't see any mention of the fact that NULL can't be pushed into it. But we're not fooled! So, let's take a look at the C library sources:
iconv_t
iconv_open (const char *tocode, const char *fromcode)
{
/* Normalize the name. We remove all characters beside alpha-numeric,
'_', '-', '/', '.', and ':'. */
size_t tocode_len = strlen (tocode) + 3;
....
}Well... indeed, there is no pointer dereferencing, but a call to strlen is without checking for NULL. We all know what strlen does in this case, and hardly anybody anywhere has ever enjoyed it!
However, the author didn't stop at one implementation — he also checked BSD and Musl. And they all behaved the same way.
S: "Now the acrobat-processor will show you how to juggle threads! A baby compiler will be his assistant! Look at his accurate acrobatic moves with the threads. Can anyone in the audience trick that again?"
Article: "A deep look into YTsaurus. Availability, reliability, open source".
TAsyncSignalsHandler *SIGNALS_HANDLER = nullptr;
void SetAsyncSignalHandler(int signum, TAutoPtr<TEventHandler> handler)
{
static TAdaptiveLock lock;
if (Y_UNLIKELY(SIGNALS_HANDLER == nullptr)) // N1
{
TGuard dnd(lock);
if (SIGNALS_HANDLER == nullptr) // N2
{
// NEVERS GETS DESTROYED
SIGNALS_HANDLER = new TAsyncSignalsHandler(); // N3
}
}
SIGNALS_HANDLER->Install(signum, handler); // N4
}The PVS-Studio warning: V547 Expression 'SIGNALS_HANDLER == nullptr' is always true. async_signals_handler.cpp:200
Although the analyzer issued the V547 warning, I found a curious example of the double-checked locking pattern.
What can go wrong? Firstly, we have a small chance that the compiler may cache the value of the SIGNALS_HANDLER variable in a register and not read the new value again. At least if unusual locking is used. The best solution is to declare the volatile variable or use std::atomic.
That's not all. Let's figure out what's wrong in this case. Let us assume that the A and B threads operate with this code.
The A thread is the first to reach the N1 point. The pointer is null, so the control flow reaches the first if and catches the lock object.
The A thread reaches the N2 point. The pointer is also null, so the control flow reaches the second if.
Then, a little magic can happen at the N3 point. The pointer initialization can be roughly represented as follows:
auto tmp = (TAsyncSignalsHandler *) malloc(sizeof(TAsyncSignalsHandler));
new (tmp) TAsyncSignalsHandler();
SIGNALS_HANDLER = tmp;A clever processor can reorder the instructions, and SIGNALS_HANDLER will be initialized before placement new is called.
And then the B thread joins the battle — exactly when the A thread hasn't yet called placement new. The control flow enters the N1 point, detects the initialized pointer, and moves to the N4 point.
In the B thread, the Install member function is called on the object that has not yet started its lifetime. Undefined behavior...
How to fix this pattern? We need to make reading and writing to SIGNALS_HANDLER be executed as atomic operations:
std::atomic<TAsyncSignalsHandler *> SIGNALS_HANDLER { nullptr };
void SetAsyncSignalHandler(int signum, TAutoPtr<TEventHandler> handler)
{
static TAdaptiveLock lock;
auto tmp = SIGNALS_HANDLER.load(std::memory_order_acquire); // <=
if (Y_UNLIKELY(tmp == nullptr))
{
TGuard dnd(lock);
tmp = SIGNALS_HANDLER.load(std::memory_order_relaxed); // <=
if (tmp == nullptr)
{
// NEVERS GETS DESTROYED
tmp = new TAsyncSignalsHandler();
SIGNALS_HANDLER.store(tmp, std::memory_order_release); // <=
}
}
tmp->Install(signum, handler);
}S: "That's what hat you've all been waiting for! Ruthless Pirates! A treasure chest has been left for our edification, as well as for those who will come after.... It's carefully stored in our hold, which is full of other rare treasures. Today we're going to open it just for you! Yo-ho-ho and a bottle of rum!"
Article: "Captain Blood's adventures: would Arabella sink?".
Here is the tricky function:
void appDebuger::CopyDump(const char * src, DWORD from, const char * dst)
{
FILE * file = fopen(src, "rb");
if (file)
{
fseek(file, 0, SEEK_END);
DWORD size = ftell(file);
fseek(file, from, SEEK_SET);
if(size > from)
{
FILE * to = fopen(dst, "a+b");
if(to)
{
char buf[128];
while (from < size)
{
DWORD s = size - from;
if (s > sizeof(buf)) s = sizeof(buf);
memset(buf, ' ', sizeof(buf));
fread(buf, s, 1, file);
fwrite(buf, s, 1, file); // <=
from += s;
}
fclose(to);
}
}
fclose(file);
}
}The PVS-Studio warning: V1075 The function expects the file to be opened for writing, but it was opened for reading. Debugger.cpp 172
Since the error was rather rare, we didn't think that the diagnostic rule would ever be issued. However, the first trophy warning occurred after the PVS-Studio 7.15 release. The diagnostic rule was issued on writing to files that were opened for reading and vice versa.
So, the function is to read data from one file on the src path, starting from the from position, and write it to another file on the dst path. The file variable is responsible for the source file, and the variable to is responsible for the resulting file.
Unfortunately, the code reads from the file variable and writes to the file variable. We can fix the code in this way:
fwrite(buf, s, 1, to);This is the end of our performance. I was happy to demonstrate to you all of our rarities. It turned out to be a pretty interesting list of bugs and their descriptions. I hope that among analyzing code fragments, warnings, and errors, you managed to get an interesting or even useful experience.
Most importantly, don't forget that you and I — all of us — can do miracles every day. The main things are tenacity and self-belief, no matter what you and I do.
Special thanks to the authors of the articles mentioned here, because without them there wouldn't be this one.
Traditionally, I suggest you try the PVS-Studio analyzer. We provide a free license for open-source projects.
Take care of yourself, and all the best! Happy New Year!