How programmers with PVS-Studio looked for project errors

Recently, the site Pinguem.ru together with the PVS-Studio team organized a contest in which programmers had to use the static analyzer PVS-Studio for a month to find and correct errors in the code of open-source projects. Thanks to their efforts, the programs in the world have become a little safer and more reliable. In the article, we will look at a couple of the most interesting errors that were found with PVS-Studio.

And how was the competition?

The competition was held from October 23 to November 27, 2017 in two stages for the Russian-speaking audience. At the first stage, the contestants had to send as many Pull Requests as possible to the authors of the projects. The second stage was somewhat more difficult: it was necessary to find an error and describe the sequence of actions in which it would manifest itself in the operation of the application. Nikolay Shalakin coped with the tasks best of all and became the winner of the competition. Congratulations to him!

During the competition, a lot of good Pull Requests were sent, those who wish can familiarize themselves with them via this link . We propose to consider the most interesting errors found by the contestants at the second stage.

QtCreator

How many of you use QtCreator for programming in Python? Like many IDEs, it also highlights some built-in functions and objects. Take QtCreator 4.4.1 and write a few reserved words:
')

What is it? Why are the built-in functions oct and chr not highlighted? Take a look at the code:

// List of python built-in functions and objects static const QSet<QString> builtins = { "range", "xrange", "int", "float", "long", "hex", "oct" "chr", "ord", "len", "abs", "None", "True", "False" }; 

Functions declared, they should be highlighted. And here PVS-Studio helps:

V653 A suspicious string of two parts is used for initialization. It is possible that a comma is missing. Consider inspecting this literal: “oct” “chr”. pythonscanner.cpp 205

Indeed, between the literals “oct” and “chr” they forgot to put a comma, so the two literals merged into one “octchr”, and it will be highlighted by the development environment:

Here you can see the Pull Request for correction.

Conemu

You are working on the ConEmu project and, in the debug version, decided to test the operation of some settings:

Let's take a look at the code, why the warning "ListBox was not processed" crashes:

 INT_PTR CSetPgViews::OnComboBox(HWND hDlg, WORD nCtrlId, WORD code) { switch (code) { .... case CBN_SELCHANGE: { .... UINT val; INT_PTR nSel = SendDlgItemMessage(hDlg, nCtrlId, CB_GETCURSEL, 0, 0); switch (nCtrlId) { .... case tThumbMaxZoom: gpSet->ThSet.nMaxZoom = max(100,((nSel+1)*100)); default: _ASSERTE(FALSE && "ListBox was not processed"); } } } } 

Because of the missed break, the control will go to the default branch after the expressions in the tThumbMaxZoom branch have been processed . PVS-Studio warns about this:

V796 It is possible that 'break' statement is missing in switch statement. setpgviews.cpp 183

Here you can see the Pull Request for correction.

Universalpausebutton

Quite an interesting project, especially useful for gamers. When you click on the Pause button, the program pauses the window in the foreground:



You can reassign the pause / resume button to another using the settings.txt file:

If you enter a key code of at least 20 characters and no more than 30 characters, this will damage the stack:

We will understand why this happens. We are interested in the LoadPauseKeyFromSettingsFile function:

 int LoadPauseKeyFromSettingsFile(_In_ wchar_t* Filename) { HANDLE FileHandle = CreateFile(Filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); if (FileHandle == INVALID_HANDLE_VALUE) { goto Default; } char KeyLine[32] = { 0 }; char Buffer[2] = { 0 }; DWORD ByteRead = 0; do { if (!ReadFile(FileHandle, Buffer, 1, &ByteRead, NULL)) { goto Default; } if (Buffer[0] == '\r' || Buffer[0] == '\n') { break; } size_t Length = strlen(KeyLine); if (Length > 30) // <= { goto Default; } KeyLine[Length] = Buffer[0]; memset(Buffer, 0, sizeof(Buffer)); } while (ByteRead == 1); if (!StringStartsWith_AI(KeyLine, "KEY=")) { goto Default; } char KeyNumberAsString[16] = { 0 }; // <= for (DWORD Counter = 4; Counter < strlen(KeyLine); Counter++) // <= { KeyNumberAsString[Counter - 4] = KeyLine[Counter]; } .... Default: if (FileHandle != INVALID_HANDLE_VALUE && FileHandle != NULL) { CloseHandle(FileHandle); } return(0x13); } 

The loop reads byte-bye the first line. If it exceeds the length of 30 characters, then execution passes by the Default label, thus the resource is released and the character with the code 0x13 is returned. If the reading is successful, and the first line starts with “KEY =”, then the substring after the character “=” is copied into the 16-byte buffer KeyNumberAsString . Entering a key between 20 and 30 characters will result in a buffer overflow. PVS-Studio warns about this:

V557 Array overrun is possible. The value of 'Counter - 4' index could reach 26. main.cpp 146

Here you can see the Pull Request for correction.

Explorer ++

A bug related to sorting selected tabs was found in this project:

Let's look at the sorting code:

 int CALLBACK SortByName(const NBookmarkHelper::variantBookmark_t BookmarkItem1, const NBookmarkHelper::variantBookmark_t BookmarkItem2) { if ( BookmarkItem1.type() == typeid(CBookmarkFolder) && BookmarkItem2.type() == typeid(CBookmarkFolder)) { const CBookmarkFolder &BookmarkFolder1 = boost::get<CBookmarkFolder>(BookmarkItem1); const CBookmarkFolder &BookmarkFolder2 = boost::get<CBookmarkFolder>(BookmarkItem2); return BookmarkFolder1.GetName() .compare(BookmarkFolder2.GetName()); } else { const CBookmark &Bookmark1 = boost::get<CBookmark>(BookmarkItem1); const CBookmark &Bookmark2 = boost::get<CBookmark>(BookmarkItem1); return Bookmark1.GetName().compare(Bookmark2.GetName()); } } 

In the else branch, they made a mistake and used BookmarkItem1 twice, instead of BookmarkItem2 . PVS-Studio warns about this error:

• V537 Consider reviewing the correctness of 'BookmarkItem1' item's usage. bookmarkhelper.cpp 535
• and 5 more additional warnings.

Here you can see the Pull Request for correction.

Conclusion

The PVS-Studio team is very grateful to all the programmers who participated in the competition. You have done a great job in eliminating the bugs in open-source projects, making them safer, safer and better. In the future, we may hold a similar competition for an English-speaking audience.

We also offer everyone else to download and try the PVS-Studio analyzer, it is easy to use and can be useful to you.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Phillip Khandeliants. How developers were checking projects for bugs using PVS-Studio