Top 10 errors in C ++ projects for 2017

Outside the window, the year 2018 is almost like 3 months, which means that the time has come (albeit a bit late) to compile 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, I recommend that you first try to independently find errors in the above code snippets, and only then read the analyzer warning and explanations. I think it will be so much more interesting.

Tenth place
')
Source: Notepad ++: Code Review Five Years Later

The error was found when checking one of the most famous text editors - Notepad ++.

Error code snippet:

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 A part of conditional expression is always true: 0xff. babygrid.cpp 711

The analyzer found the expression suspicious (lParam >> 16) && 0xff . The second argument passed to the ToAscii function will always have the value 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

Source: Greetings to Yandex developers

In ninth place is the error from the ClickHouse project developed by 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); .... } 

PVS-Studio warning : 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 cond_col pointer. For it, in the if statement it checks that the pointer is nonzero. If control reaches the else branch where an exception is generated, the cond_col pointer is exactly zero. However, when generating an exception message, cond_col is dereferenced in the expression cond_col-> getName () .

Eighth place

Source: Firebird, MySQL and PostgreSQL Code Quality Comparison

In eighth place is one of the errors found in the MySQL project when comparing the quality of the 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(); } 

PVS-Studio warning : V596 The object was not created. 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 the type std :: runtime_error is created and ... that's all. Once created, it will simply be deleted, and the method will continue. Obviously, the developer forgot the throw keyword to generate an exception.

Seventh place

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

How to find 56 potential vulnerabilities per evening? With static analysis, of course!

One of the problems found in FreeBSD code:

 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 The compiler could delete the memset function call, which is used to flush the '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 it does not “hang” in memory.

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

To dive deeper into the topic, I recommend reading the following articles:

• Secure cleaning of private data .
• Documentation for the V597 diagnostic rule .
• The most dangerous feature in the C / C ++ world .

Sixth place

Source: CryEngine V Welcome Check

The first game engine, to which code we will address in this top - CryEngine V.

 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 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 section that the analyzer found - the symbol ';', the final for loop. At the same time, the formatting of the code (shift of the next expression) also suggests that the symbol ';' there is an extra one, and the expression pmd0-> next = pmd; must be a loop body. But judging by the logic of the for loop, it is precisely the wrong formatting of the code, which is confusing, and not a logical error. In the CryEngine code, the formatting of the code, by the way, was corrected.

Fifth place

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

The next error was discovered during the work on correcting errors found by PVS-Studio in the code of the Unreal Engine.

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

PVS-Studio warning : V767 Suspicious access to element of SelectedObjects' array by a constant index inside a loop. skeletonnotifydetails.cpp 38

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

Fourth place

Source: 27,000 errors in Tizen operating system

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

But back to the 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; .... } 

PVS-Studio warning : 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 expression body [new_len] = '\ 0' . Notice that the body parameter has type char ** , respectively, the type of the expression [new_len] is char * . But the developer made a blunder, and, forgetting one more dereference, 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.
• The terminal zero will not be written to the end of the line.

Correct code:

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

Third place

Source: How can PVS-Studio help finding vulnerabilities?

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

 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 The 'strchr (ptr + 1,' / ')' pointer in the 'strchr (ptr + 1,' / ') + 1' expression could be nullptr. In such a case, it will 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 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 like ptr! = NULL will not give information about the pointer's validity. As a result, under certain conditions, this code led to the emergence of a kernel panic.

This error was assigned the following identifier: CVE-2014-9491: The devzvol_readdir function is not subject to the null pointer dereference and panic via unspecified vectors.

Despite the fact that CVE itself was discovered in 2014, during our own research, we discovered this error in 2017, so she got to this top.

Second place

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

The error, located in second place, was discovered ... yes, again in the Unreal Engine. It seemed to me very interesting, so I could not resist and did not write it out.

Note Actually, I would have written a couple more mistakes from the above article about the Unreal Engine, but still I don’t want to refer so often to the same project. Therefore, I strongly recommend that you look at the above article yourself, in particular, the V714 and V709 warnings .

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 needed to understand the problem. The variables OutMatchedProjectsDesk , OutCreatedProjectsDesk and OutMatchedProjectsMob , OutCreatedProjectsMob when declared are initialized with zeros, and then passed as arguments to the CreateProjectSet method.

After that, the variables are compared in 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

The OutCreatedProjects and OutMatchedProjects parameters were forgotten to make links, and as a result, the values ​​of the corresponding arguments are simply copied. As a result, the return value of the RunTest method given above is always true , since all compared variables have the same value specified during initialization — 0.

The correct code is:

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

First place

Source: Love static code analysis!

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

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

Well, how are you doing with finding a bug? :)

PVS-Studio warning : V614 Uninitialized buffer 'buf' used. Consider checking the printf function. pugixml.cpp 3362

Surely you have a question: " printf ? Where in the warning of the printf analyzer, if the code contains a call to the sprintf function only?"

This is the essence! A sprintf is a macro that expands into (!) std :: printf !

 #define sprintf std::printf 

As a result, the uninitialized buffer buf is used as a format string. Great, isn't it? I think this mistake quite deservedly won first place.

Link to the header file with the macro declaration .

Conclusion

Hope you liked the bugs you have collected. Personally, they seemed to me 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 by looking at the list of errors found by PVS-Studio in open source projects.

I also remind you that all the errors in the article ( as well as many others ) were found with the help of the PVS-Studio analyzer, which I advise you to try on your project: link to the download page .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Top 10 Bugs in the C ++ Projects of 2017