Vladislav Stolyarov

Dec 18 2020

Tags:

#Cpp

Top 10 bugs found in C++ projects in 2020

• No. 10. Modulo division by one
• No 9. Four checks
• No 8. delete instead of delete[]
• No. 7. Buffer overflow
• No. 6. Something super weird
• No. 5. Meaningless overflow protection
• No. 4. Don't expand std, bro
• No. 3. The little scrollbar that could not
• No. 2. Radius and height mixed up
• No. 1. Overwriting the result
• Conclusion

It's winter outside, the year is coming to an end, which means it's time to review the most notable errors the PVS-Studio analyzer detected in 2020.

In the past year, we introduced many new diagnostic rules that detected these errors and placed them at the top. We've also enhanced the analyzer's core and added new use case scenarios. You can learn more about this in our blog. Let me remind you that our analyzer also supports C# and Java. Check out my colleagues' articles for more information on those languages. Now let's move on to the most memorable bugs PVS-Studio found in open source projects over the past year.

No. 10. Modulo division by one

V1063 The modulo by 1 operation is meaningless. The result will always be zero. llvm-stress.cpp 631

void Act() override {
  ....
  // If the value type is a vector, and we allow vector select,
  // then in 50% of the cases generate a vector select.
  if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 1)) {
    unsigned NumElem =
        cast<FixedVectorType>(Val0->getType())->getNumElements();
    CondTy = FixedVectorType::get(CondTy, NumElem);
  }
  ....
}

The developer intended to get a random value between 0 and 1 from a modulo operation. However, the operation of type X%1 always returns 0. In this case, it would be correct to rewrite the condition as follows:

if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 2))

More information on this bug is available in the following article: "Checking Clang 11 with PVS-Studio".

No 9. Four checks

After processing the code snippet below, PVS-Studio generated four warning messages:

• V560 A part of conditional expression is always true: x >= 0. editor.cpp 1137
• V560 A part of conditional expression is always true: y >= 0. editor.cpp 1137
• V560 A part of conditional expression is always true: x < 40. editor.cpp 1137
• V560 A part of conditional expression is always true: y < 30. editor.cpp 1137

int editorclass::at( int x, int y )
{
  if(x<0) return at(0,y);
  if(y<0) return at(x,0);
  if(x>=40) return at(39,y);
  if(y>=30) return at(x,29);

  if(x>=0 && y>=0 && x<40 && y<30)
  {
      return contents[x+(levx*40)+vmult[y+(levy*30)]];
  }
  return 0;
}

The last if statement triggered all four warnings. The problem is that the statement performs four checks that always returns true. I would call this bug amusing rather than major. These checks are redundant, and you can remove them.

This error got here from the following article: VVVVVV??? VVVVVV!!!

No 8. delete instead of delete[]

V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 410

BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type)
{
  ....
  char *poke_data = new char [length + 2*sizeof(int)]; // <=
  ....
  if(DDE_Class->Poke_Server( .... ) == FALSE) {
    CCDebugString("C&C95 - POKE failed!\n");
    DDE_Class->Close_Poke_Connection();
    delete poke_data;                                  // <=
    return (FALSE);
  }

  DDE_Class->Close_Poke_Connection();

  delete poke_data;                                    // <=

  return (TRUE);
}

The analyzer detected that memory is freed in a way that is incompatible with how memory was allocated. To free the memory allocated for the array, use the delete[] operator instead of delete.

For more information on this bug, check out the following article: "The code of the Command & Conquer game: bugs from the 90's. Volume two"

No. 7. Buffer overflow

Let's take a look at the net_hostname_get function.

#if defined(CONFIG_NET_HOSTNAME_ENABLE)
const char *net_hostname_get(void);
#else
static inline const char *net_hostname_get(void)
{
  return "zephyr";
}
#endif

The option from the #else branch is selected during preprocessing. The preprocessed file reflects this as follows:

static inline const char *net_hostname_get(void)
{
  return "zephyr";
}

The function returns a pointer to a 7-byte array that contains the string and a null terminator.

Now let's take a look at the code that produces the buffer overflow.

static int do_net_init(void)
{
  ....
  (void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);
  ....
}

PVS-Studio warning: V512 [CWE-119] A call of the 'memcpy' function will lead to the 'net_hostname_get()' buffer becoming out of range. log_backend_net.c 114

After preprocessing MAX_HOSTNAME_LEN expands as follows:

(void)memcpy(hostname, net_hostname_get(),
    sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx"));

When the data is copied, string literal overflow occurs. This causes undefined behavior.

For more information on this bug, see "Checking the code of Zephyr operating system".

No. 6. Something super weird

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    ((u8_t *)mntpt)[strlen(mntpt)] = '\0';
    memcpy(cpy_mntpt, mntpt, strlen(mntpt));
  }
  return cpy_mntpt;
}

PVS-Studio warning: V575 [CWE-628] The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. shell.c 427

Here someone failed to emulate the strdup function.

Let's start with the analyzer's warning. The analyzer reports that the memcpy function copied the string but didn't copy the null terminator.

The following line of code seems to copy the null terminator:

((u8_t *)mntpt)[strlen(mntpt)] = '\0';

However, it does not. There is a typo here, and the null terminator is assigned to itself. Note that the value is recorded to the mntpt array instead of cpy_mntpt. As a result, the mntpt_prepare function returns a string that lacks the null terminator.

We see that the programmer intended to write the statement below:

((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';

However, there is still no reason to make the line so complex. Let's simplify the code:

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    strcpy(cpy_mntpt, mntpt);
  }
  return cpy_mntpt;
}

See "Checking the code of Zephyr operating system" for more details.

No. 5. Meaningless overflow protection

V547 [CWE-570] Expression 'rel_wait < 0' is always false. Unsigned type value is never < 0. os_thread_windows.c 359

static DWORD
get_rel_wait(const struct timespec *abstime)
{
  struct __timeb64 t;
  _ftime64_s(&t);
  time_t now_ms = t.time * 1000 + t.millitm;
  time_t ms = (time_t)(abstime->tv_sec * 1000 +
    abstime->tv_nsec / 1000000);

  DWORD rel_wait = (DWORD)(ms - now_ms);

  return rel_wait < 0 ? 0 : rel_wait;
}

In the code above, take a look at the rel_wait variable. It is of the unsigned DWORD type. This means that the rel_wait < 0 statement always returns FALSE and has no practical value.

The error itself is ordinary. However, its fix is more intriguing. The developers simplified the code but failed to fix the bug. You can read the entire case in my colleague's article: "Why PVS-Studio doesn't offer automatic fixes".

For more details on this error, see the following article: "Static code analysis of the PMDK library collection by Intel and errors that are not actual errors".

No. 4. Don't expand std, bro

V1061 Extending the 'std' namespace may result in undefined behavior. sized_iterator.hh 210

// Dirty hack because g++ 4.6 at least wants
// to do a bunch of copy operations.
namespace std {
inline void iter_swap(util::SizedIterator first,
                      util::SizedIterator second)
{
  util::swap(*first, *second);
}
} // namespace std

You can read more on this example and why this is a poor practice in the following article: "Checking the code of DeepSpeech, or why you shouldn't write in namespace std".

No. 3. The little scrollbar that could not

V501 There are identical sub-expressions to the left and to the right of the '-' operator: bufferHeight - bufferHeight TermControl.cpp 592

bool TermControl::_InitializeTerminal()
{
  ....
  auto bottom = _terminal->GetViewport().BottomExclusive();
  auto bufferHeight = bottom;

  ScrollBar().Maximum(bufferHeight - bufferHeight);
  ScrollBar().Minimum(0);
  ScrollBar().Value(0);
  ScrollBar().ViewportSize(bufferHeight);
  ....
}

This is what's called "history-dependent activation". In this case, the Windows Terminal failed to show its scrollbar because of an error. My colleague researched the bug and figured out what happened. Curious? Here's his article: "The little scrollbar that could not".

No. 2. Radius and height mixed up

And once again we'll talk about the analyzer's several warnings:

• V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 791
• V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 833
• V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 884

This is how the function is called:

NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);

And this is its definition:

static NewtonBody* CreateWheel (DemoEntityManager* const scene,
  const dVector& location, dFloat radius, dFloat height)

You can see that when the developer called the function, the arguments were mixed up.

Read more on this error in the following article: "A second check of Newton Game Dynamics with PVS-Studio"

No. 1. Overwriting the result

V519 The 'color_name' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 621, 627. string.cpp 627

static bool parseNamedColorString(const std::string &value,
                                  video::SColor &color)
{
  std::string color_name;
  std::string alpha_string;

  size_t alpha_pos = value.find('#');
  if (alpha_pos != std::string::npos) {
    color_name = value.substr(0, alpha_pos);
    alpha_string = value.substr(alpha_pos + 1);
  } else {
    color_name = value;
  }

  color_name = lowercase(value); // <=

  std::map<const std::string, unsigned>::const_iterator it;
  it = named_colors.colors.find(color_name);
  if (it == named_colors.colors.end())
    return false;
  ....
}

The function above analyzes the color name with its transparency parameter and returns the color's hexadecimal code. If the string contains the transparency parameter, this parameter is split from the string and the color is recorded to the color_name variable. Otherwise, the color_name variable is assigned the original color string.

The problem arises when the lowercase() function is called. The programmer passed the wrong parameter into this function. If the color_name variable contains a substring of value, then this substring will always be rewritten. Thus, we won't get what we expected from parseNamedColorString() function.

This is how we can fix this line:

color_name = lowercase(color_name);

For more details on this error see: "PVS-Studio: analyzing pull requests in Azure DevOps using self-hosted agents".

Conclusion

Over the past year, we found many errors in open source projects. These were ordinary copy-paste bugs, incorrect constants, memory leaks, and many other problems. This year's Top 10 bugs include several ones detected by our new algorithms and prove that our analyzer keeps evolving.

I hope you enjoyed reading my selection of memorable bugs as much as I enjoyed assembling this list. Of course, if you read our blog or looked through warning lists PVS-Studio produced after scanning open source projects, you may have your own Top-10.

Here are the Top 10 bugs we found in C++ projects over the previous years: 2016, 2017, 2018, 2019.

#Cpp

0

0

0

0

SHARE

Tags:

#Cpp

We can email you a selection of our best articles once a month