# New Year's Eve show: Top 10 errors in C and C\+\+ projects in 2023
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\.
Before reading, here are a few cozy tips for you
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\.
## Introduction
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](https://en.wikipedia.org/wiki/Ringmaster_(circus)) 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\!"_
### It's showtime\!
_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?"_
### Ninth "act"\. A dubious cycle
_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](https://pvs-studio.com/en/blog/posts/cpp/1067/)"\.
```cpp
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](https://pvs-studio.com/en/docs/warnings/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\.
### Eighth "act"\. The most dangerous function in the C and C\+\+ worlds
_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](https://pvs-studio.com/en/blog/posts/cpp/1078/)"\.
```cpp
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:
* [V579](https://pvs-studio.com/en/docs/warnings/v579/) The memset function receives the pointer and its size as arguments\. It is possibly a mistake\. Inspect the third argument\. KeyboardEventHandlers\.cpp 16
* [V568](https://pvs-studio.com/en/docs/warnings/v568/) It's odd that 'sizeof\(\)' operator evaluates the size of a pointer to a class, but not the size of the 'keyEventList' class object\. KeyboardEventHandlers\.cpp 16
* [V1086](https://pvs-studio.com/en/docs/warnings/v1086/) A call of the 'memset' function will lead to underflow of the buffer 'keyEventList'\. KeyboardEventHandlers\.cpp 16
We even have a separate article about such errors: "[The most dangerous function in the C/C\+\+ world](https://pvs-studio.com/en/blog/posts/cpp/0360/)"\.
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:
```cpp
typedef struct tagINPUT
{
DWORD type;
union
{
MOUSEINPUT mi;
KEYBDINPUT ki;
HARDWAREINPUT hi;
} DUMMYUNIONNAME;
} INPUT, *PINPUT, FAR* LPINPUT;
```
### Seventh "act"\. Double symbol
_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](https://pvs-studio.com/en/blog/posts/cpp/1065/)"\.
```cpp
std::unordered_set delimiters =
{ ':', '@', '.', '!', ' ', '\t', '.', '\\',
'+', '*', '-', '<', '>', '[', ']', '(',
')', '{', '}', '=', '%', '#', '^', '&',
'\'', '"', '/', '|', ',', '~', ';', '`' };
```
The PVS\-Studio warning: [V766](https://pvs-studio.com/en/docs/warnings/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\.
### Sixth "act"\. Incorrect override
_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](https://pvs-studio.com/en/blog/posts/cpp/1065/)"\.
Here's what the function looks like in the base class:
```cpp
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:
```cpp
class clProgressDlg : public wxProgressDialog
{
public:
....
bool Update(int value, const wxString& msg);
....
};
```
The PVS\-Studio warning: [V762](https://pvs-studio.com/en/docs/warnings/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:
```cpp
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:
```cpp
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\.
### Fifth "act"\. The multiverse of madness
_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](https://pvs-studio.com/en/blog/posts/cpp/1066/)"\.
```cpp
// 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](https://pvs-studio.com/en/docs/warnings/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:
```cpp
// 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](https://c-faq.com/aryptr/aryvsadr.html) 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\)\.
### Fourth "act"\. Only truth
_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](https://pvs-studio.com/en/blog/posts/cpp/1058/)"\.
```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/0908/)"\.
### Third "act"\. The typo and undefined behavior
_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](https://pvs-studio.com/en/blog/posts/cpp/1072/)"\.
```cpp
QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
ViewProviderPage* vpp = getViewProviderPage(dView);
if (!vpp) {
return vpp->getQGVPage();
}
return nullptr;
}
```
The PVS\-Studio warning: [V522](https://pvs-studio.com/en/docs/warnings/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:
* If the pointer is non\-null, the function will do nothing and will return _nullptr_\.
* If the pointer is null, dereferencing will occur, and after that, [undefined behavior](https://pvs-studio.com/en/blog/posts/cpp/0306/) will appear\.
The thing is, there's an identical function nearby\.
```cpp
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](https://pvs-studio.com/en/blog/posts/cpp/1068/)"\. I recommend you read it: here is another interesting story from our technical support\.
### Second "act"\! Trust, but verify
_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\)](https://pvs-studio.com/en/blog/posts/cpp/1081/)"\.
File: SDL/src/events/SDL\_mouse\.c \([GitHub permalink](https://github.com/libsdl-org/SDL/blob/bac7eeaaae00b929808ed8efe1a76e45a4e2c54b/src/stdlib/SDL_iconv.c#L789)\)
```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/libsdl-org/SDL/blob/bac7eeaaae00b929808ed8efe1a76e45a4e2c54b/src/stdlib/SDL_iconv.c#L35) function:
```cpp
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](https://www.gnu.org/software/libiconv/documentation/libiconv-1.13/iconv_open.3.html) 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](https://github.com/lattera/glibc/blob/895ef79e04a953cac1493863bcae29ad85657ee1/iconv/iconv_open.c#L32):
```cpp
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](https://github.com/freebsd/freebsd-src/blob/366ef17bb6ce008f79a283ff5dc5e87ab13dbdb3/sys/libkern/iconv.c#L248) and [Musl](https://github.com/kraj/musl/blob/2e6edc1b699851fcbe9f7ab0fbd724624adde88a/src/locale/iconv.c#L145)\. And they all behaved the same way\.
### First "act"\! Many threads, many troubles
_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](https://pvs-studio.com/en/blog/posts/cpp/1077/)"\.
```cpp
TAsyncSignalsHandler *SIGNALS_HANDLER = nullptr;
void SetAsyncSignalHandler(int signum, TAutoPtr 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](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'SIGNALS\_HANDLER \=\= nullptr' is always true\. async\_signals\_handler\.cpp:200
Although the analyzer issued the [V547](https://pvs-studio.com/en/docs/warnings/v547/) warning, I found a curious example of the [double\-checked locking](https://en.wikipedia.org/wiki/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:
```cpp
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:
```cpp
std::atomic SIGNALS_HANDLER { nullptr };
void SetAsyncSignalHandler(int signum, TAutoPtr 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);
}
```
### Zero "act"\!\!\! Captain Blood and his treasures\!
_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?](https://pvs-studio.com/en/blog/posts/cpp/1033/)"\.
Here is the tricky function:
```cpp
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](https://pvs-studio.com/en/docs/warnings/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:
```cpp
fwrite(buf, s, 1, to);
```
### Show is over\!
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](https://pvs-studio.com/en/pvs-studio/download/) the PVS\-Studio analyzer\. We provide [a free license](https://pvs-studio.com/en/order/open-source-license/) for open\-source projects\.
Take care of yourself, and all the best\! Happy New Year\!