Court is in session: Top 10 most notorious C and C++ errors in 2024

• All rise!
• Accused bugs
• The verdict

Every year, we witness the same drama: bugs wreak havoc on our code as if asserting their dominance. But today, the tide turns—it's time for judgment. Let's dive into the most intriguing bugs we've uncovered this year.

All rise!

Over the past twelve months, our team has scrutinized countless errors from open-source C and C++ projects. Heated debates and discussions unfolded in comments to defend each bug's innocence. Now, it's time for reckoning!

We'll delve into 10 of the most notorious bugs from these projects that we've covered in the articles. For eager fans of C and C++, we've picked the most popular C and C++ articles for this year:

• Let's check Blender
• How not to check array size in C++
• What's up with Telegram messenger: dozen errors detected
• An insect is sitting in your compiler and doesn't want to leave for 13 years
• PPSSPP or psp? Uncovering bugs from the future

The full list is at the link.

Accused bugs

Defendant N10

To keep the suspense alive and not reveal the most dangerous villain right away, we'll start from the bottom of the list. Bring out the tenth offender! Ah yes, of course—it's an infamous memory leak. Let's show where it's hiding:

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

We found this bug in the PPSSPP project. The full article is at the link.

Defendant N9

Alright, we've dealt with that perp—now it's time for the next one. Wait, it's not just one? Two? How many? Who are the real offenders here?

Now let's play a mini-game: find typos in the OpenVINO project code. Whoever found them deserve kudos! Whoever didn't find the typos deserve kudos, too! However, we hope it's obvious to everyone. Here's a great example of why developers need a static analyzer.

It's amusing that many devs think it's pointless to look for errors in such code. So, they just scroll down to see an answer right away.

There's also an irony in how, despite weighing up inefficiency and labor costs, developers still monotonously search for errors without using static analyzers.

It's so simple: take the analyzer, run the analysis, look at the list of errors. Profit. No need to waste time looking for bugs—they're already highlighted. Just fix them and that's it.

The PVS-Studio warnings:

• V766 An item with the same key '"Abs"' has already been added. cpu_types.cpp 178
• V766 An item with the same key '"SoftPlus"' has already been added. cpu_types.cpp 198

Here are typos:

static const TypeToNameMap& get_type_to_name_tbl() {
  static const TypeToNameMap type_to_name_tbl = {
    ....,
    {"Abs", Type::Eltwise},                   // <=
    ....,
    {"SoftPlus", Type::Eltwise},              // <=
    ....,
    {"Abs", Type::Math},                      // <=
    ....,
    {"SoftPlus", Type::Math},                 // <=
    ...., 
  };
  return type_to_name_tbl;
}

Obviously, there should not be identical values, and duplicates will never be used.

We found this bug in the OpenVINO project. The full article is at the link.

Defendant N8

It was painful to look at, yeah? Alright, let's move on to the next... Wait, what is this!? Holy molly... Each of bug is awful in its own way. Now, look at this naughty one.

Since this case is tricky and very confusing, take a peek at the warning:

V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'preferred_domain' in derived class 'HandlePositionFieldInput' and base class 'CurvesFieldInput'. node_geo_input_curve_handles.cc 95

So, we can see the issue lies in incorrect overriding of a virtual function in arbitrary classes. What exactly is wrong? Let's find out.

Let's start with the base class:

typedef struct CurvesGeometry { .... };

namespace bke
{
  ....
  class CurvesGeometry : public ::CurvesGeometry { .... };

  class CurvesFieldInput : public fn::FieldInput 
  {
    ....
    virtual std::optional<AttrDomain> preferred_domain(
      const CurvesGeometry &curves) const;
  };
  ....
}

The preferred_domain virtual function takes a parameter of the bke::CurvesGeometry type. We'll keep that in mind.

Now, take a look at the child:

namespace blender::nodes::node_geo_input_curve_handles_cc
{
  class HandlePositionFieldInput final : public bke::CurvesFieldInput 
  {
    ....
    std::optional<AttrDomain> preferred_domain(
      const CurvesGeometry & /*curves*/) const;
  };
}

Found an issue? If not, don't worry, we didn't understand it at first either :)

In the base class, the virtual function accepts a parameter with an unqualified name, CurvesGeometry. When the compiler searches for this type, it starts with the scope of the CurvesFieldInput class and looks in all framed scopes until it finds the type. As a result, the bke::CurvesGeometry type is found.

Now let's look at the derived classes. They're defined in a different namespace from the one in which the base class is located. The compiler also starts searching for the needed CurvesGeometry name, doesn't find it in the framed scopes, and reaches the global one. Yet, the global namespace also has CurvesGeometry, just not the one we need to override the function :)

We can fix it by specifying a qualified type name—just use the C++11 (override) capabilities and protect future generations from errors:

namespace blender::nodes::node_geo_input_curve_handles_cc
{
  class HandlePositionFieldInput final : public bke::CurvesFieldInput 
  {
    ....
    std::optional<AttrDomain> preferred_domain(
      const bke::CurvesGeometry & /*curves*/) const override;
  };
}

We found this bug in the Blender project. The full article is at the link.

Defendant N7

The seventh defendant, what are you draggin' behind your back? Show us quickly! Ah! A little thief! Decided to pocket a juicy chunk of code, huh? No chance, we're bringing this to the light.

Honestly, we really wanted to paste the full code of the function here—which has almost 400 code lines—to make searching for errors more interesting. However, we don't want to abuse your mouse wheel, so we've included just the most intriguing part.

The file: symbolgroupvalue.cpp:1196

static KnownType knownClassTypeHelper(const std::string &type,
                                      std::string::size_type pos,
                                      std::string::size_type endPos)
{
  ....
  // Remaining non-template types
  switch (endPos - qPos)
  {
    ....
  case 30:
    if (!type.compare(qPos, 30, "QPatternist::SequenceType::Ptr"))
      return KT_QPatternist_SequenceType_Ptr;
    if (!type.compare(qPos, 30, "QXmlStreamNamespaceDeclaration"))
      return KT_QXmlStreamNamespaceDeclaration;
    break;
  case 32:
    break;                                                                // <=
    if (!type.compare(qPos, 32, "QPatternist::Item::Iterator::Ptr"))
      return KT_QPatternist_Item_Iterator_Ptr;
  case 34:
    break;                                                                // <=
    if (!type.compare(qPos, 34, "QPatternist::ItemSequenceCacheCell"))
      return KT_QPatternist_ItemSequenceCacheCell;
  case 37:
    break;                                                                // <=
    if (!type.compare(qPos, 37, "QNetworkHeadersPrivate::RawHeaderPair"))
      return KT_QNetworkHeadersPrivate_RawHeaderPair;
    if (!type.compare(qPos, 37, "QPatternist::AccelTree::BasicNodeData"))
      return KT_QPatternist_AccelTree_BasicNodeData;
    break;
  }

  return KT_Unknown;
}

Please note the 32, 34, and 37 branches of the switch statement above. Honestly, we can't think of any reason for using break; right before the executable code.

We thought that they were just temporary kludges. But no, they were added at the same time as this code. According to the history, this code is about 14 years old. If any readers can suggest what exactly is going on here—you're welcome in the comments.

The analyzer points you to this code fragment with the warning:

V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. symbolgroupvalue.cpp 1565

We found this bug in the Qt Creator project. The full article is at the link.

To easily view the analysis results, you can use the PVS-Studio extension (plugin) for Qt Creator. For more details on its installation and usage, refer to the documentation: Using the PVS-Studio extension for Qt Creator.

Defendant N6

Ha-ha-ha-ha! Copy-paste errors, we've been expecting you! These bugs lurk in almost every codebase, and they're not always easy to spot. Now, one of them is on our trial. Step forward and reveal itself!

void WebViewInstance::show(const QString &url, uint64 queryId) 
{
  ....
  const auto allowClipboardRead = v::is<WebViewSourceAttachMenu>(_source)
       || v::is<WebViewSourceAttachMenu>(_source)
       || (attached != end(bots)
         && (attached->inAttachMenu || attached->inMainMenu));
  ....
}

The analyzer warning:

V501 There are identical sub-expressions 'v::is<WebViewSourceAttachMenu > (_source)' to the left and to the right of the '||' operator. bot_attach_web_view.cpp 1129

The same v::is<WebViewSourceAttachMenu>(_source) expressions are checked in the condition. Developers may need to replace one of them with something else—but with what?

Take a look at the last condition line and see how the values of the following expressions are checked:

(attached->inAttachMenu || attached->inMainMenu)

Let's consider the variable names and identical expressions that the analyzer addresses. Perhaps the developers should fix the condition as follows:

const auto allowClipboardRead = 
      v::is<WebViewSourceMainMenu>(_source)
   || v::is<WebViewSourceAttachMenu>(_source)
   ....

We found this bug in the Telegram project. The full article is at the link.

Defendant N5

Halfway through, already we've seen plenty of the perps' antennae. There's danger in the courtroom air. Yes, all the guilty buggies should be eradicated in future releases. Especially those we'll look at next.

Here comes the fifth defendant: a seemingly tiny bug, but what the damage it can cause! This one definitely deserves a spot behind bars.

const qdGameObjectStateWalk* qdGameObjectMoving::current_walk_state() const
{
  const qdGameObjectState* st = get_cur_state();
  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){
#ifndef _QUEST_EDITOR
    st = last_walk_state_;
    if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
      st = get_default_state();
else
    st = get_default_state();
    if(!st) st = get_state(0);
#endif
  }
  ....
}

The PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. qd_game_object_moving.cpp 2781

The analyzer points out that the same action is performed regardless of the condition:

  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
    st = get_default_state();
else
  st = get_default_state();

Note that the code is formatted in a strange way. The else keyword is at the beginning of the line.

If we look at the code, we can see that the developers wanted to write the #else preprocessor directive but not the else statement. However, they made a typo and forgot to write the hash symbol (#).

The fixed code:

const qdGameObjectState* st = get_cur_state();
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){
#ifndef _QUEST_EDITOR
  st = last_walk_state_;
  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
    st = get_default_state();
#else
  st = get_default_state();
  if(!st) st = get_state(0);
#endif

We found this bug in the qdEngine project. The full article is at the link.

Defendant N4

Alright, where's the fourth one, does anyone see it? Where has that bug vanished to? Alright, show us the crime scene—we'll find this villain.

static const char *script_list[][2] = {
  ....
  { "Myanmar / Burmese", "Mymr" },
  { "​Nag Mundari", "Nagm" },
  { "Nandinagari", "Nand" },
  ....
}

The reader may ask, "What's wrong here?" We wouldn't have figured it out ourselves if it weren't for the V1076 diagnostic rule. Interestingly enough, this is the first warning we've written out. The diagnostic rule checks the program code for invisible characters. Such characters are like backdoors that a programmer may not see because of the text display settings in the development environment, but the compiler sees and correctly processes them.

The analyzer warning:

V1076 Code contains invisible characters that may alter its logic. Consider enabling the display of invisible characters in the code editor. locales.h 1114

Let's take a closer look at the next line:

{ "​Nag Mundari", "Nagm" },

It contains the backdoor with an invisible character. If we use a hex editor, we can see the following:

There are 3 bytes sandwiched between the double quotation mark and the N character: E2, 80, and 8B. They correspond to the ZERO WIDTH SPACE (U+200B) Unicode character.

Most likely, the backdoor was added accidentally by copying the name from some website. We can simply delete this character from the string literal.

We found this bug in the Godot Engine project. The full article is at the link.

Defendant N3

The three vicious perps are on the court stand. Which one of them is the most dangerous criminal after all?

The third bug, are you trying to be clever? Computing complex equations on which important variables depend? Write a full confession! You broke it.

void StfsContainerDevice::BlockToOffsetSVOD(size_t block, ....)
{
  ....
  const size_t BLOCK_SIZE = 0x800;
  const size_t HASH_BLOCK_SIZE = 0x1000;
  const size_t BLOCKS_PER_L0_HASH = 0x198;
  const size_t HASHES_PER_L1_HASH = 0xA1C4;
  const size_t BLOCKS_PER_FILE = 0x14388;
  const size_t MAX_FILE_SIZE = 0xA290000;
  const size_t BLOCK_OFFSET =
      header_.metadata.volume_descriptor.svod.start_data_block();
  ....

  // Resolve the true block address and file index
  size_t true_block = block - (BLOCK_OFFSET * 2);
  ....
  size_t file_block = true_block % BLOCKS_PER_FILE;
  size_t file_index = true_block / BLOCKS_PER_FILE;
  size_t offset = 0;

  // Calculate offset caused by Level0 Hash Tables
  size_t level0_table_count = (file_block / BLOCKS_PER_L0_HASH) + 1;
  offset += level0_table_count * HASH_BLOCK_SIZE;

  // Calculate offset caused by Level1 Hash Tables
  size_t level1_table_count = (level0_table_count / HASHES_PER_L1_HASH) + 1;
  offset += level1_table_count * HASH_BLOCK_SIZE;
  ....
}

The PVS-Studio warning:

V1064 The 'level0_table_count' operand of integer division is less than the 'HASHES_PER_L1_HASH' one. The result will always be zero. stfs_container_device.cc 500

The analyzer warns that the level1_table_count value always equals 0 because the level0_table_count left operand is less than the HASHES_PER_L1_HASH right operand during an integer division operation. The value of HASHES_PER_L1_HASH is 41412, so to determine the level0_table_count value, take a look at the code above.

The file_block variable is computed by dividing the true_block by BLOCKS_PER_FILE, so it's within the range of [0 .. 82823].

The BLOCKS_PER_L0_HASH variable divides the value by 408, and the 1 is added to the result. When file_block reaches its maximum value, we'll get 202, so the level0_table_count variable value is within the range of [1 .. 203].

So, the level1_table_count variable is computed as 203/41412+1, and equals 1 for any true_block values.

Could we get it wrong somewhere? It appears not, it's not just our analyzer that warns about it.

It's an interesting case where we can't even spot the error at a glance. A reviewer might easily miss it because it's time-consuming and tedious to calculate manually.

There is a code comment that might shed light on this mystery. Perhaps someone has already got some thoughts on the matter?

We found this bug in the Xenia project. The full article is at the link.

Defendant N2

What's this? A null pointer dereference? It's the same story every year... No project review is without this bad guy. We can spot its tiny footprints in almost every codebase. Await harsh sentences. Now, let's see the crime scene!

Take a look at the following structure:

struct ForceCodegenLinking {
    ForceCodegenLinking() {
      // We must reference the passes in such a way that compilers will not
      // delete it all as dead code, even with whole program optimization,
      // yet is effectively a NO-OP. As the compiler isn't smart enough
      // to know that getenv() never returns -1, this will do the job.
      // This is so that globals in the translation units where these functions
      // are defined are forced to be initialized, populating various
      // registries.
      if (std::getenv("bar") != (char*) -1)
        return;

      (void) llvm::createFastRegisterAllocator();
      (void) llvm::createBasicRegisterAllocator();
      (void) llvm::createGreedyRegisterAllocator();
      (void) llvm::createDefaultPBQPRegisterAllocator();

      (void)llvm::createBURRListDAGScheduler(nullptr,
                                             llvm::CodeGenOptLevel::Default);
      (void)llvm::createSourceListDAGScheduler(nullptr,
                                               llvm::CodeGenOptLevel::Default);
      (void)llvm::createHybridListDAGScheduler(nullptr,
                                               llvm::CodeGenOptLevel::Default);
      (void)llvm::createFastDAGScheduler(nullptr,
                                         llvm::CodeGenOptLevel::Default);
      (void)llvm::createDefaultScheduler(nullptr,
                                         llvm::CodeGenOptLevel::Default);
      (void)llvm::createVLIWDAGScheduler(nullptr,
                                         llvm::CodeGenOptLevel::Default);
    }
  } ForceCodegenLinking; // Force link by creating a global definition.
}

In its constructor, a number of functions are called to create some entities. We'll look only at the functions to which arguments are passed (there are six). For example, here are some of them:

ScheduleDAGSDNodes *llvm::createBURRListDAGScheduler(SelectionDAGISel *IS,
                                                    CodeGenOptLevel OptLevel)
{
  const TargetSubtargetInfo &STI = IS->MF->getSubtarget();
  ....
}

Or

ScheduleDAGSDNodes* createDefaultScheduler(SelectionDAGISel *IS,
                                           CodeGenOpt::Level OptLevel) 
{
  const TargetLowering *TLI = IS->TLI;
  const TargetSubtargetInfo &ST = IS->MF->getSubtarget();
  ....
}

If we look at the function call in the constructor, we see that nullptr is passed as its first argument. It's that first IS parameter that's dereferenced in the very first line of the function.

It's a strange code. Perhaps the LLVM developers know something and can actually control UB.

Every function with the first zero argument gets dereferenced.

So, here are the analyzer warnings:

• V522 Dereferencing of the null pointer might take place. The null pointer is passed into 'createBURRListDAGScheduler' function. Inspect the first argument. Check lines: 'ScheduleDAGRRList.cpp:3147', 'LinkAllCodegenComponents.h:40'.
• V522 Dereferencing of the null pointer might take place. The null pointer is passed into 'createSourceListDAGScheduler' function. Inspect the first argument. Check lines: 'ScheduleDAGRRList.cpp:3161', 'LinkAllCodegenComponents.h:42'.
• V522 Dereferencing of the null pointer might take place. The null pointer is passed into 'createHybridListDAGScheduler' function. Inspect the first argument. Check lines: 'ScheduleDAGRRList.cpp:3175', 'LinkAllCodegenComponents.h:44'.
• ... (the other three are similar)

We found this bug in the LLVM project. The full article is at the link.

Defendant N1

You've been waiting for it. We show it! Beware! Here it is—the leader of the bug gang! This is the day of reckoning! We've been waiting for it all year. Step forward and unveil yourself!

Let's examine the list of named constants:

Now note the string array:

And now look at the code that turns the named constant into the string:

const char *qed_dbg_get_status_str(enum dbg_status status)
{
  return (status < MAX_DBG_STATUS) ?
    s_status_str[status] : "Invalid debug status";
}

Where's the error?

Found it?

There's a hint that you could theoretically catch on to. In the array, a single empty string separates all rows except for one place. Let's start with the PVS-Studio analyzer warning:

V557 Array overrun is possible. The value of 'status' index could reach 58. qede_debug.c 7149

It points to the code fragment, where the index array returns the string pointer:

return (status < MAX_DBG_STATUS) ?
  s_status_str[status] : "Invalid debug status";

Initially, we diagnosed it incorrectly and unpretentiously. We inadvertently thought we found an off-by-one error here. It looked like a typical incorrect check, which we encountered many times in various projects.

Here is the essence of a classic antipattern. There is an enum whose last component determines the number of elements.

enum Efoo {
  A,
  B,
  C,
  COUNT
};

Constants in enum are assigned values starting from 0. So, the COUNT auxiliary constant will be equal to 3, which corresponds to the number of the main named constants.

There's an array where each named constant corresponds to something:

char Cfoo[] = { 'A', 'B', 'C' };

It's a common bug when developers check that the index doesn't overrun the array bounds:

char Convert(unsigned id)
{
  return (id > COUNT) ? 0 : Cfoo[id];
}

The check doesn't operate correctly: if id is equal to COUNT, the array will be out of bounds. PVS-Studio gives a similar warning about these kinds of errors.

The correct option would be one of the following:

return (id >= COUNT) ? 0 : Cfoo[id];  // OK
return (id < COUNT) ? Cfoo[id] : 0;   // OK

Nothing special here. Let's move on. We can write out a warning about array overrun in the article but don't analyze the bug in the DPDK check article.

Then at the last minute—STOP!

THE CHECK IS ACTUALLY CORRECT!

return (status < MAX_DBG_STATUS) ?
  s_status_str[status] : "Invalid debug status";

Now it's less clear and more intriguing! Why did the analyzer issue a warning? A false positive? I don't think so. There's no way it could get tangled up.

Rolling up my sleeves, I start looking over the code. Here it is!

A missing line for the DBG_STATUS_NO_MATCHING_FRAMING_MODE constant.

In the enum:

DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS,
DBG_STATUS_NO_MATCHING_FRAMING_MODE,
DBG_STATUS_VFC_READ_ERROR,

In the array:

/* DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS */
"The filter/trigger constraint dword offsets are not "
"enabled for recording",


/* DBG_STATUS_VFC_READ_ERROR */
"Error reading from VFC",

Look at a separator with two empty lines. Something went wrong. Maybe the array was filled incorrectly, or the row was accidentally deleted, or a branch merge failed, or there's another reason.

However, once we've already found the bug, two empty lines look suspicious. Well, developers will hardly pay attention to them in code reviews. Even if they do, they'll just remove one of them for beauty's sake. No one's going to say, "We might have missed something. Let's check it" :)

Results of the error:

• possible array overrun;
• the function returns incorrect strings for all constants starting with DBG_STATUS_NO_MATCHING_FRAMING_MODE.

We found this bug in the DPDK project. The full article is at the link.

The verdict

The court is silent. Even the judge is speechless. The most dangerous criminals of the year have been apprehended. The only thing left is to destroy them, but that's another story... However, we wouldn't have been able to find these errors without the help of our trusty detective—the PVS-Studio analyzer.

In 2024, we've tackled quite a few bugs, but there are still countless projects worth checking. Our detective is ready to help you hunt down dangerous code fragments lurking in your project:

Bring your own license with you:

• for open-source projects
• for educational purposes
• a 30-day trial.

And if you'd like to read about all the most fascinating bugs for this year, I invite you to check out the top 10 bugs from projects written in Java and C#:

• Top 10 most intriguing Java errors in 2024
• Top 10 errors found in C# projects in 2024.