Top 10 Bugs Found By PVS-Studio Analyzer In C++ Projects For 2017

It’s almost 3 months since 2018, which means that it’s time (albeit a little too late) to make the top 10 errors found by the PVS-Studio analyzer in C ++ projects over the past year. So, let’s begin!

Note. For more interest, we recommend that you first try to find errors in the code snippets yourself, and only after that read the analyzer warning and explanations. We think it will be much more interesting.

Tenth place

The error was detected when checking one of the most famous text editors – Notepad ++.

Source: Notepad ++: Code Verification Five Years Later 

The code fragment containing the error:

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

Warning: PVS-Studio: V560 A part of conditional expression is always true: 0xff. babygrid.cpp 711

The analyzer found the expression (lParam >> 16) && 0xff suspicious. The second argument passed to the ToAscii function will always have a value of 0 or 1, and the resulting value depends only on the left subexpression – (lParam >> 16). Obviously, instead of the && operator, the & operator should be used.

Ninth place

On the ninth place, there is an error from the project ClickHouse, developed by the company Yandex.

Source: We would like to extend our greetings to the developers of Yandex

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

Warning: PVS-Studio V522 Dereferencing of the null pointer ‘cond_col’ might take place. FunctionsConditional.h 765

This code incorrectly handles the error situation when it is necessary to generate an exception. Note the pointer cond_col. For it, the if statement checks that the pointer is nonzero. If the control has reached the else branch where the exception is thrown, the pointer cond_col is exactly zero. However, when an exception message is generated, cond_col is dereferenced in the cond_col-> getName () expression.

Eighth place

The eighth place is located one of the errors found in the MySQL project while comparing the quality of the code Firebird, MySQL and PostgreSQL.

Source: Comparing the quality of Firebird, MySQL and PostgreSQL code

The code of the method containing the error:

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

Warning: PVS-Studio 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 is created and … that’s it. After the creation, it will simply be deleted, while the method will continue to execute. Obviously, the developer forgot the throw keyword to throw an exception.

Seventh Place

How to find 56 potential vulnerabilities in the evening? With the help of static analysis, of course!

Source: How to find 56 potential vulnerabilities in the FreeBSD code in one evening

One of the problems found in the FreeBSD code is:

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

Warning: PVS-Studio 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 expression memset (& dout, 0, sizeof (dout)). The developer wanted to “wipe” the data in the memory block corresponding to dout, setting zero values. Usually, this approach is used when it is necessary to clear some private data so that they are not “hung” in memory.

However, dout is not used further (sizeof (dout) is not counted), which will allow the compiler to remove the above call to the memset function because this optimization does not affect the behavior of the program in terms of C / C ++. As a consequence, the data that should have been cleaned out may remain in memory.

Sixth place

 

The first game engine, to the code of which we turn in this top – CryEngine V.

Source: The long-awaited CryEngine V test

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

Warning: PVS-Studio V529 Odd semicolon ‘;’ after ‘for’ operator. boolean3d.cpp 1314

Agree that if this fragment were not written out like this – being shortened and separated from the rest of the code, it would not be so easy to find in it that suspicious site that the analyzer found – the character ‘;’, which completes the for a loop. In this case, the formatting of the code (shifting the next expression) also suggests that the character ‘;’ This is superfluous, and the expression pmd0-> next = pmd; must be the body of a cycle.

Fifth place

The following error was detected during the work on the correction of errors found by PVS-Studio in the code of the game engine Unreal Engine.

Source: Static analysis as part of the Unreal Engine development process

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

Warning: PVS-Studio V767 Suspicious access to an element of ‘SelectedObjects’ array by a constant index inside a loop. skeletonnotifydetails.cpp 38

In the cycle, we wanted to bypass all the elements and find the first one among them, having the type UEditorSkeletonNotifyObj. But they made an unfortunate typo, using the constant index 0 in the expression SelectedObjects [0].Get () instead of the counter of the cycle i. As a result, only the first element will always be checked.

Fourth place

The error was found when checking the operating system Tizen, as well as third-party components used in it. The article is large, and it contains many interesting examples of errors – we highly recommend that you read it.

Source: 27000 errors in the Tizen operating system

But back to a specific warning:

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

Warning: PVS-Studio 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 lies in the body expression [new_len] = ‘\ 0’. Note that the body parameter is of type char **, respectively, the type of the expression body [new_len] is char *. But the developer gave a crash, and, forgetting one more dereferencing, tried to write the value ‘\ 0’ into the pointer (it will be converted to a null pointer).

This leads to two problems:

• Somewhere (body [new_len]) a null pointer will be written.
• Terminal zero will not be written to the end of the line.

The correct code is:

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

Third place

So we got to the top three. The code below was found during the search for the answer to the question: “How does PVS-Studio cope with CVE search?” (See the answer in the article above). The code from the project illumos-gate.

Source: How can PVS-Studio help in the search for vulnerabilities?

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

Warning: PVS-Studio V769 The ‘strchr (ptr + 1,’ / ‘)’ pointer in the ‘strchr (ptr + 1,’ / ‘) + 1’ expression could be nullptr. In such a case, the resulting value will be senseless and should not be used.

The function strchr returns a pointer to the first occurrence of the character specified by the second argument in the string specified by the first argument. If there is no such character, strchr returns NULL. However, this fact is not taken into account, and the value ‘1’ is always added to the return value. As a result, the ptr pointer will always be non-zero, which means that further checks of the form ptr! = NULL will not give information about the validity of the pointer. As a result, under certain conditions, this code resulted in a kernel panic.

• This error has been assigned the identifier CVE-2014-9491: The devzvol_readdir function in illos 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.

Despite the fact that the CVE itself was discovered in 2014, during our research, we found this error in 2017, so it hit this top.

Second place

An error located in the second place was detected … yes, again in the Unreal Engine. It seemed very interesting to me, so we could not resist and not write it out.

Source: Static analysis as part of the Unreal Engine development process

Note. In fact, we would write a couple more errors from the above article about the Unreal Engine, but still, do not want to refer to the same project so often. Therefore, we strongly recommend that you look at the above-mentioned article yourself, in particular, warnings V714 and V709.

Further, there will be a lot of code, but it is necessary to understand the essence of the problem.

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

We note the following important point necessary for understanding the problem. The variables OutMatchedProjectsDesk, OutCreatedProjectsDesk and OutMatchedProjectsMob, OutCreatedProjectsMob are initialized with zeros on the declaration, and then passed as arguments to the CreateProjectSet method.

After that, the variables are compared in the expression of the return statement. Therefore, the CreateProjectSet method must initialize the last two arguments.

Now let’s turn to the CreateProjectSet method, which contains errors.

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

• PVS-Studio warnings:
  V763 Parameter ‘OutCreatedProjects’ is always rewritten in function body before being used. gameprojectautomationtests.cpp 88
• V763 Parameter ‘OutMatchedProjects’ is always rewritten in function body before being used. gameprojectautomationtests.cpp 89

Parameters OutCreatedProjects and OutMatchedProjects forgot to make references, and as a result, the values of the corresponding arguments are simply copied. As a result, the return value of the RunTest method above is always true, since all compared variables have the same value specified at initialization – 0.

Correct code: 

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

First place

As soon as we find out this error, we did not have a single doubt about who should lead the top. In general, see for yourself. Never go to the problem description until you find an error in the code snippet. By the way, the project – StarEngine – is again a game engine.

Source: Love static code analysis!

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

Warning: PVS-Studio V614 Uninitialized buffer ‘buf’ used. Consider checking the first actual argument of the ‘printf’ function. pugixml.cpp 3362

Surely you have a question: “printf? Where in the analyzer printf warning, if the code only calls sprintf?”

This is the essence! sprintf is a macro that is expanded in (!) std:: printf!

#define sprintf std::printf

As a result, an uninitialized buffer buf is used as a format string. That’s great, is not it? we think this mistake was quite rightly won first place.

Conclusion

We hope you liked the errors you collected. Personally, they seemed to be quite interesting. But, of course, your vision may be different from mine. Also, we remind you that all the errors listed in the article were found using the PVS-Studio analyzer, which we advise you to try on your own project.