# Top 10 Bugs in the C\+\+ Projects of 2017

We're almost three months into 2018, which means the time has come \(albeit with some delay\) to make a top\-10 list of bugs found by the PVS\-Studio analyzer in C\+\+ projects over the last year\. Here we go\!

![0565_Top_Cpp_Bugs_2017/image1.png](https://import.viva64.com/docx/blog/0565_Top_Cpp_Bugs_2017/image1.png)

**Note\.** To make it more entertaining, try to find the bugs in the code fragments that follow on your own first and only then go on reading the warning and my comments\. I guess you'll enjoy it more that way\.

**Tenth place**

Source: [Checking Notepad\+\+: five years later](https://pvs-studio.com/en/blog/posts/cpp/0511/)

The error was found in one of the most popular text editors, Notepad\+\+\.

Here's the code:

```cpp
TCHAR GetASCII(WPARAM wParam, LPARAM lParam)
{
  int returnvalue;
  TCHAR mbuffer[100];
  int result;
  BYTE keys[256];
  WORD dwReturnedValue;
  GetKeyboardState(keys);
  result = ToAscii(static_cast<UINT>(wParam),
    (lParam >> 16) && 0xff, keys, &dwReturnedValue, 0);
  returnvalue = (TCHAR) dwReturnedValue;
  if(returnvalue < 0){returnvalue = 0;}
  wsprintf(mbuffer, TEXT("return value = %d"), returnvalue);
  if(result!=1){returnvalue = 0;}
  return (TCHAR)returnvalue;
}
```

**PVS\-Studio warning:** [V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: 0xff\. babygrid\.cpp 711

The analyzer didn't like the _\(lParam \>\> 16\) && 0xff_ expression\. The second argument passed to the _ToAscii _function will always evaluate to 0 or 1, which will depend solely on the left subexpression, _\(lParam \>\> 16\)_\. It's obvious that the & operator should be used in place of &&\.

**Ninth place**

Source: [Give my Best Regards to Yandex Developers](https://pvs-studio.com/en/blog/posts/cpp/0529/)

This error was found in the ClickHouse project developed by Yandex\.

```cpp
bool executeForNullThenElse(....)
{
  ....
  const ColumnUInt8 * cond_col =
    typeid_cast<const ColumnUInt8 *>(arg_cond.column.get());
  ....
  if (cond_col)
  {
    ....
  }
  else if (cond_const_col)
  {
    ....
  }
  else
    throw Exception(
      "Illegal column " + cond_col->getName() +
      " of first argument of function " + getName() +
      ". Must be ColumnUInt8 or ColumnConstUInt8.",
      ErrorCodes::ILLEGAL_COLUMN);
  ....
}
```

**PVS\-Studio warning:** [V522](https://pvs-studio.com/en/docs/warnings/v522/) Dereferencing of the null pointer 'cond\_col' might take place\. FunctionsConditional\.h 765

This code is an example of incorrect handling of an error that requires throwing an exception\. Note the check of the _cond\_col_ pointer in the _if_ statement\. If control reaches the _else_ branch, where the exception is to be thrown, the _cond\_col_ pointer will definitely be null, yet it will be dereferenced in the _cond\_col\-\>getName\(\)_ expression when forming the error message text\.

**Eighth place**

Source: [Code Quality Comparison of Firebird, MySQL, and PostgreSQL](https://pvs-studio.com/en/blog/posts/cpp/0542/)

This is one of the bugs that we discovered in the MySQL project when comparing the code quality of Firebird, MySQL, and PostgreSQL\.

Here's the code fragment with the error:

```cpp
mysqlx::XProtocol* active()
{
  if (!active_connection)
    std::runtime_error("no active session");
  return active_connection.get();
}
```

**PVS\-Studio warning:** [V596](https://pvs-studio.com/en/docs/warnings/v596/) The object was created but it is not being used\. The 'throw' keyword could be missing: throw runtime\_error\(FOO\); mysqlxtest\.cc 509

If there is no active connection \(_\!active\_connection_\), an exception object of type _std::runtime\_error_ will be created and\.\.\. that's all\. Once created, it will simply be deleted and the method will run on\. The programmer obviously forgot to add the _throw_ keyword for the exception to be thrown\.

**Seventh place**

Source: [How to find 56 potential vulnerabilities in FreeBSD code in one evening](https://pvs-studio.com/en/blog/posts/cpp/0496/) 

How to find 56 potential vulnerabilities in one evening? Using static analysis, of course\! 

Here's one of the defects caught in the code of FreeBSD:

```cpp
int mlx5_core_create_qp(struct mlx5_core_dev *dev,
      struct mlx5_core_qp *qp,
      struct mlx5_create_qp_mbox_in *in,
      int inlen)
{
  ....
  struct mlx5_destroy_qp_mbox_out dout;
  ....
err_cmd:
  memset(&din, 0, sizeof(din));
  memset(&dout, 0, sizeof(dout));
  din.hdr.opcode = cpu_to_be16(MLX5_CMD_OP_DESTROY_QP);
  din.qpn = cpu_to_be32(qp->qpn);
  mlx5_cmd_exec(dev, &din, sizeof(din), &out, sizeof(dout));

  return err;
}
```

**PVS\-Studio warning:** [V597](https://pvs-studio.com/en/docs/warnings/v597/) The compiler could delete the 'memset' function call, which is used to flush 'dout' object\. The memset\_s\(\) function should be used to erase the private data\. mlx5\_qp\.c 159

Note the _memset\(&dout, 0, sizeof\(dout\)\) _expression\. The programmer wanted to erase the data in the memory block allocated for _dout_ by filling that block with zeroes\. This technique is typically used when you need to erase some private data to prevent it from "lingering" in the memory\.

However, _dout_ is not used anywhere after that \(_sizeof\(dout\)_ doesn't count\), allowing the compiler to delete this call to _memset_ since such an optimization won't affect the program's behavior from the viewpoint of C/C\+\+\. As a result, the data intended to be erased may still be there\.

Here's some more reading on the subject:

* [Safe Clearing of Private Data](https://pvs-studio.com/en/blog/posts/cpp/0388/)\.
* [Documentation for the diagnostic rule V597](https://pvs-studio.com/en/docs/warnings/v597/)\.
* [The most dangerous function in the C/C\+\+ world](https://pvs-studio.com/en/blog/posts/cpp/0360/)\.

**Sixth place**

Source: [Long\-Awaited Check of CryEngine V](https://pvs-studio.com/en/blog/posts/cpp/0417/)

CryEngine V, the first game engine on this top\-list\.

```cpp
int CTriMesh::Slice(....)
{
  ....
  bop_meshupdate *pmd = new bop_meshupdate, *pmd0;
  pmd->pMesh[0]=pmd->pMesh[1] = this;  AddRef();AddRef();
  for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next);
    pmd0->next = pmd;
  ....
}
```

**PVS\-Studio warning:** [V529](https://pvs-studio.com/en/docs/warnings/v529/) Odd semicolon ';' after 'for' operator\. boolean3d\.cpp 1314

If I hadn't cited this code fragment as I did \- abridged and isolated from the rest of the code \- would you have noticed the error as easily \- that suspicious ';' after the _for_ loop pointed out by the analyzer? Note how the code formatting \(the indentation before the next expression\) also suggests that the ';' character is unnecessary and that the _pmd0\-\>next \= pmd;_ expression is meant to be the loop body\. But, according to the logic of the loop 'for', in this place a wrong code formatting takes place, which confuses, not a logical error\. By the way, in the CryEngine the code formatting was corrected\.

**Fifth place**

Source: [Static analysis as part of the development process in Unreal Engine](https://pvs-studio.com/en/blog/posts/cpp/0517/)

This defect was found while fixing the bugs detected earlier by PVS\-Studio in the code of the Unreal Engine game engine\.

```cpp
for(int i = 0; i < SelectedObjects.Num(); ++i)
{
  UObject* Obj = SelectedObjects[0].Get();
  EdObj = Cast<UEditorSkeletonNotifyObj>(Obj);
  if(EdObj)
  {
    break;
  }
}
```

**PVS\-Studio warning:** [V767](https://pvs-studio.com/en/docs/warnings/v767/) Suspicious access to element of 'SelectedObjects' array by a constant index inside a loop\. skeletonnotifydetails\.cpp 38

The programmer intended the loop to iterate through all the elements to find the first element of type _UEditorSkeletonNotifyObj_ but made an unfortunate typo using the constant index 0 instead of the loop counter _i _in the _SelectedObjects\[0\]\.Get\(\)_ expression\. This will make the loop check only the first element\.

**Fourth place**

Source: [27 000 Errors in the Tizen Operating System](https://pvs-studio.com/en/blog/posts/cpp/0519/)

This error was discovered when checking the Tizen operating system along with the third\-party components used by it\. The article is a lengthy one; it contains a lot of nice examples of bugs, so I do recommend checking it out\.

But let's get back to this particular case:

```cpp
int _read_request_body(http_transaction_h http_transaction,
                       char **body)
{
  ....
  *body = realloc(*body, new_len + 1);
  ....
  memcpy(*body + curr_len, ptr, body_size);
  body[new_len] = '\0';
  curr_len = new_len;
  ....
}
```

**PVS\-Studio warning:** [V527](https://pvs-studio.com/en/docs/warnings/v527/) It is odd that the '\\0' value is assigned to 'char' type pointer\. Probably meant: \*body\[new\_len\] \= '\\0'\. http\_request\.c 370

The error hides in the _body\[new\_len\] \= '\\0'_ expression\. Note that the _body_ parameter is of type _char\*\*_, so the result of the _body\[new\_len\]_ expression is of type _char\*_\. But the developer made a mistake, forgetting to dereference the pointer one more time, and attempted to write to the pointer the value '\\0' \(which will be interpreted as a null pointer\)\.

This leads us to these two problems:

* The null pointer will be written in the middle of nowhere\.
* No null character will be added to the end of the string\.

Correct code:

```cpp
(*body)[new_len] = '\0';
```

**Third place**

Source: [How Can PVS\-Studio Help in the Detection of Vulnerabilities?](https://pvs-studio.com/en/blog/posts/cpp/0514/)

We have reached the top three leaders\. The code snippet shown below attracted our attention while we were looking for the answer to the question, "How good is PVS\-Studio at searching CVE's?" \(check the article above for the answer\)\. The code is taken from the illumos\-gate project\.

```cpp
static int devzvol_readdir(....)
{
  ....
  char *ptr;
  ....
  ptr = strchr(ptr + 1, '/') + 1;
  rw_exit(&sdvp->sdev_contents);
  sdev_iter_datasets(dvp, ZFS_IOC_DATASET_LIST_NEXT, ptr);
  ....
}
```

**PVS\-Studio warning:** [V769](https://pvs-studio.com/en/docs/warnings/v769/) The 'strchr\(ptr \+ 1, '/'\)' pointer in the 'strchr\(ptr \+ 1, '/'\) \+ 1' expression could be nullptr\. In such case, resulting value will be senseless and it should not be used\.

The _strchr_ function returns a pointer to the first occurrence of the character specified by the second argument in the string specified by the first argument\. If no such character is found, _strchr_ will return _NULL_\. The programmer, however, doesn't take this possibility into account and adds the value 1 to whatever value is returned\. As a result, the _ptr_ pointer will always be non\-null, which means any further _ptr \!\= NULL_ checks won't actually be able to determine if it's valid\. Under certain circumstances, this will eventually end up with a kernel panic\. 

This error was classified as CVE\-2014\-9491: The _devzvol\_readdir_ function in illumos does not check the return value of a _strchr_ call, which allows remote attackers to cause a denial of service \(_NULL_ pointer dereference and panic\) via unspecified vectors\.

Although this CVE was originally discovered in 2014, we discovered it during our own research in 2017, and that's why it's here on this top\-list\.

**Second place**

Source: [Static analysis as part of the development process in Unreal Engine](https://pvs-studio.com/en/blog/posts/cpp/0517/)

The bug that placed second was found in\.\.\. yes, Unreal Engine again\. I like it too much to leave it out\.

**Note**\. I actually considered including a couple more examples from the article about Unreal Engine, but there would be too many bugs from one project then, which I didn't want\. So, I do recommend that you check out the article above for yourself, particularly the warnings [V714](https://pvs-studio.com/en/docs/warnings/v714/) and [V709](https://pvs-studio.com/en/docs/warnings/v709/)\.

This example is a lengthy one, but you need all this code to figure out what the problem is about\.

```cpp
bool FCreateBPTemplateProjectAutomationTests::RunTest(
  const FString& Parameters)
{
  TSharedPtr<SNewProjectWizard> NewProjectWizard;
  NewProjectWizard = SNew(SNewProjectWizard);

  TMap<FName, TArray<TSharedPtr<FTemplateItem>> >& Templates =
    NewProjectWizard->FindTemplateProjects();
  int32 OutMatchedProjectsDesk = 0;
  int32 OutCreatedProjectsDesk = 0;
  GameProjectAutomationUtils::CreateProjectSet(Templates, 
    EHardwareClass::Desktop, 
    EGraphicsPreset::Maximum, 
    EContentSourceCategory::BlueprintFeature,
    false,
    OutMatchedProjectsDesk,
    OutCreatedProjectsDesk);

  int32 OutMatchedProjectsMob = 0;
  int32 OutCreatedProjectsMob = 0;
  GameProjectAutomationUtils::CreateProjectSet(Templates, 
    EHardwareClass::Mobile,
    EGraphicsPreset::Maximum,
    EContentSourceCategory::BlueprintFeature,
    false,
    OutMatchedProjectsMob,
    OutCreatedProjectsMob);

  return ( OutMatchedProjectsDesk == OutCreatedProjectsDesk ) &&
         ( OutMatchedProjectsMob  == OutCreatedProjectsMob  );
}
```

Note one thing essential for understanding the problem\. The pairs of the variables _OutMatchedProjectsDesk_, _OutCreatedProjectsDesk_ and _OutMatchedProjectsMob_, _OutCreatedProjectsMob_ are initialized to zero at declaration and are then passed as arguments to the _CreateProjectSet_ method\.

After that, they are compared in the expression within the _return_ statement\. Therefore, the _CreateProjectSet_ method must initialize the last two arguments\.

Now let's look at the _CreateProjectSet_ method, which is where the mistakes were made\.

```cpp
static void CreateProjectSet(.... int32 OutCreatedProjects,
                                  int32 OutMatchedProjects)
{
  ....
  OutCreatedProjects = 0;
  OutMatchedProjects = 0;
  ....
  OutMatchedProjects++;
  ....
  OutCreatedProjects++;
  ....
}
```

**PVS\-Studio warnings**:

* [V763](https://pvs-studio.com/en/docs/warnings/v763/) Parameter 'OutCreatedProjects' is always rewritten in function body before being used\. gameprojectautomationtests\.cpp 88
* [V763](https://pvs-studio.com/en/docs/warnings/v763/) Parameter 'OutMatchedProjects' is always rewritten in function body before being used\. gameprojectautomationtests\.cpp 89

The programmer forgot to declare the _OutCreatedProjects_ and _OutMatchedProjects_ parameters as references, which results in simply copying the values of their respective arguments\. As a result, the _RunTest_ method shown earlier returns _true_ all the time since all the variables being compared store the same value assigned at initialization \- 0\.

This is the correct version:

```cpp
static void CreateProjectSet(.... int32 &OutCreatedProjects,
                                  int32 &OutMatchedProjects)
```

**First place**

Source: [Appreciate Static Code Analysis\!](https://pvs-studio.com/en/blog/posts/cpp/0535/)

Once I saw this bug, I had no doubt regarding the leader of this top\. Well, see for yourself\. Please, don't read on until you find the error in the code below yourself\. By the way, StarEngine is a game engine too\.

```cpp
PUGI__FN bool set_value_convert(
  char_t*& dest,
  uintptr_t& header,
  uintptr_t header_mask,
  int value)
{
  char buf[128];
  sprintf(buf, "%d", value);

  return set_value_buffer(dest, header, header_mask, buf);
}
```

So, any luck finding the bug? :\)

**PVS\-Studio warning:** [V614](https://pvs-studio.com/en/docs/warnings/v614/) Uninitialized buffer 'buf' used\. Consider checking the first actual argument of the 'printf' function\. pugixml\.cpp 3362

You must have wondered, "_printf_? Why does the analyzer mention _printf_ when there's only the call to _sprint_?"

That's it\! _sprintf_ is a macro expanding into \(\!\) _std::printf_\!

```cpp
#define sprintf std::printf
```

As a result, the uninitialized buffer _buf_ is used as a format string\. That's cool, isn't it? I believe this error deserves the first place\.

[The link to the header file with a macro declaration](https://github.com/StarEngine/engine/blob/061a1fa1b6e4cbe6a90698b08d6a88d2ef705542/src/definesTypes.h)\.

## Conclusion

I hope you liked the bugs on this list\. Personally, I found them pretty interesting\. You may have a different opinion, of course, so feel free to draw up your own "Top 10\.\.\." list based on the articles on our [blog](https://pvs-studio.com/en/blog/posts/) or the list of defects found by PVS\-Studio in open\-source projects\.

![0565_Top_Cpp_Bugs_2017/image2.png](https://import.viva64.com/docx/blog/0565_Top_Cpp_Bugs_2017/image2.png)

As a reminder, all the defects mentioned here \([as well as many others](https://pvs-studio.com/en/blog/examples/)\) were found by the PVS\-Studio analyzer, which I recommend trying with your own projects as well \- [download here](https://pvs-studio.com/en/pvs-studio/download/)\.