Top 10 errors in C ++ projects in 2020

Winter is outside the window, the year is striving to end, which means that the time has come to consider the most interesting errors detected by the PVS-Studio analyzer in 2020.



It is worth noting that the past year was marked by a large number of new diagnostic rules, the triggering of which allowed them to get into this top. We also continue to improve the analyzer core and add new scenarios for its use, you can read about all this in our blog . If you are interested in other languages ​​supported by our analyzer (C, C # and Java), take a look at the articles of my colleagues. Now let's go directly to the most memorable bugs found by PVS-Studio over the past year.

Tenth place: 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 wanted to get a random value between 0 and 1 by using modulo division. However, an operation like X% 1 will always return 0. In this case, it would be correct to rewrite the condition as follows:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

This error was included in the top from the article: " Checking Clang 11 with PVS-Studio ".

Ninth place: Four checks

PVS-Studio issued four warnings for the next section of the code:

• 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;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

All warnings are for the last if statement. The problem is that all four checks that are performed in it will always return true . I wouldn't say that this is a serious mistake, but it turned out quite funny. In general, these checks are redundant and can be removed.



This error entered the top from the article: " VVVVVV ??? VVVVVV !!! ".

Eighth place: 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 has detected an error related to the fact that memory has been allocated and freed in incompatible ways. To free memory allocated for an array, use the delete [] operator , not delete .



This error made it to the top from the article: " Command & Conquer Game Code: Bugs from the 90s. Volume Two ".

Seventh place: Buffer out of bounds

Let's look at the net_hostname_get function that will be used next.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

In this case, during preprocessing, the option related to the #else branch was selected . That is, in the preprocessed file, the function is implemented as follows:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

The function returns a pointer to an array of 7 bytes (take into account the terminal zero at the end of the string).



Now let's look at the code that leads to the overflow of the array.

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"));
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Accordingly, when copying data, an overrun of the string literal occurs. How this will affect program execution is difficult to predict, as it leads to undefined behavior.



This bug made it to the top of the article: " Investigating the quality of the Zephyr operating system code ".

Sixth place: Something very strange

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



Someone tried to make an analogue of the strdup function , but they failed.



Let's start with the analyzer warning. It says that the memcpy function copies the line, but it won't copy the terminal zero, which is very suspicious.



This terminal 0 seems to be copied here:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

But no! Here's a typo that causes terminal zero to be copied into itself! Note that the writing is going to the mntpt array , not to cpy_mntpt . As a result, the mntpt_prepare function returns an incomplete terminal null string.



In fact, the programmer wanted to write like this:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

However, it is still not clear why it was done so difficult! This code can be simplified to the following:

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;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

This bug made it to the top of the aforementioned article: " Examining the quality of Zephyr operating system code ".

Fifth place: Incorrect 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 this case, the rel_wait variable is of unsigned DWORD type . This means that the comparison rel_wait <0 does not make sense, since the result is always true.



The error itself is not very interesting. But it turned out to be interesting with how they tried to fix it. It turned out that the changes were not fixed, but only simplified the code. You can read more about this story in the article by my colleague: " Why PVS-Studio doesn't offer automatic code edits ".



The error entered the top from the article: " Static analysis of the code of the collection of PMDK libraries from Intel and errors that are not errors ".

Fourth place: Don't write to std, brother

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
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

The article from which the trigger is taken: " Analyzing the code of the DeepSpeech project or why you shouldn't write in namespace std " describes in detail why you should not do this.

Third place: Scrollbar, which failed

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 is called "triggering with history". In this case, due to an error, the scrollbar in Windows Terminal did not work. A whole article was written based on this bug, in which my colleague conducted research and figured out why this happened. Are you interested? Here it is: "The Scrollbar That Couldn't ".

Second place: confused radius and height

And again we will talk about several analyzer 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

Here are the function calls:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

And this is how her ad looks like:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Arguments were reversed in function calls.



This error made it to the top from the article: " Re-checking Newton Game Dynamics with the PVS-Studio static analyzer ".

First place: Erasing 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;
  ....
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

This function should parse the color name with the transparency parameter and return its hexadecimal code. Depending on the result of checking the condition, either the result of the line splitting or a copy of the function argument is passed to the color_name variable .



However, then in the lowercase () function, not the resulting string itself is converted to lower case, but the original function argument. As a result, we will simply lose the color that parseNamedColorString () should have returned .

color_name = lowercase(color_name);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

This error made it to the top from the article: " PVS-Studio: Analysis of pull requests in Azure DevOps using self-hosted agents ".

Conclusion

Over the past year, we have found many bugs in open source projects. These were common copy-paste errors, constant errors, memory leaks, and many other problems. Our analyzer does not stand still, and in the top there are several positives of new diagnostics written this year.



I hope you enjoyed the collected errors. Personally, I found them quite interesting. But, of course, your vision may differ from mine, so you can create your "Top 10 ..." by reading articles from our blog or looking at the list of errors found by PVS-Studio in open source projects.



I also bring to your attention articles with the top 10 C ++ errors of past years: 2016 , 2017 , 2018 , 2019 .

If you want to share this article with an English-speaking audience, please use the translation link: Vladislav Stolyarov. Top 10 Bugs Found in C ++ Projects in 2020 .