Pinguem.ru and the PVS-Studio team recently held a contest where programmers were to use PVS-Studio static analyzer for one month to find and fix bugs in the source code of open-source projects. Their efforts have helped make a great many applications a tiny bit safer and more reliable. In this article, we will discuss a few of the most interesting bugs found with the help of PVS-Studio.
The contest was held for the Russian-speaking community from October 23 to November 27, 2017, and was split into two stages. At the first stage, the contestants were to submit as many pull requests to the project authors as possible. The second stage was a bit more challenging: they were asked to find a bug and describe the sequence of steps to reproduce it. Nikolay Shalakin was the one who scored the most points and won the contest. Congratulations, Nikolay!
During the contest, the participants submitted a lot of really useful pull requests, all of which are listed here. As for this article, we invite you to take a look at the most interesting bugs found by the contestants at the second stage.
How many of you use QtCreator when coding in Python? Like many other IDEs, it highlights some of the built-in functions and objects. Let's run QtCreator 4.4.1 and write a few keywords:
What's that? Why doesn't it highlight built-in functions oct and chr? Let's look into their 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"
};The function declarations are fine; what's wrong then? PVS-Studio clarifies the issue:
V653 A suspicious string consisting 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, the programmer forgot to write a comma between literals "oct" and "chr", so the two have merged into one, "octchr", and it is this literal that QtCreator highlights:
The bug-fix pull request can be found here.
Suppose you are working on a ConEmu project and want to check on some of the settings in the debug version (click on the animation to enlarge):
Let's look into the code to find out why we get the "ListBox was not processed" message:
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 missing break statement, the control will pass to the default branch after executing the expressions in the tThumbMaxZoom branch. That's just what PVS-Studio warns us about:
V796 It is possible that 'break' statement is missing in switch statement. setpgviews.cpp 183
The bug-fix pull request can be found here.
This project is quite an interesting one and is especially useful for gamers. When you click on the Pause key, the program pauses the foreground window's operation:
You can reassign the pause/resume function to another key by tweaking the settings.txt file:
If you enter a key code whose length is no less than 20 characters and no greater than 30 characters, this will result in a stack buffer overflow (click on the animation to enlarge):
Let's find out why it happens. We are interested in function LoadPauseKeyFromSettingsFile:
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);
}In the loop above, the first string is read byte by byte. If its length is greater than 30 characters, control passes to the Default label, releasing the resource and returning character code 0x13. If the string has been read successfully and the first string starts with "KEY=", the substring following the "=" character is copied into 16-byte buffer KeyNumberAsString. Entering a key code from 20 to 30 characters long would result in a buffer overflow. That's just what PVS-Studio warns us about:
V557 Array overrun is possible. The value of 'Counter - 4' index could reach 26. main.cpp 146
The bug-fix pull request can be found here.
The bug found in this project has to do with bookmark sort (click on the animation to enlarge):
Let's examine the code performing the sort:
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());
}
}The programmer made a mistake in the else branch and used BookmarkItem1 twice instead of using BookmarkItem2 in the second case. That's just what PVS-Studio warns us about:
The bug-fix pull request can be found here.
The PVS-Studio team is very grateful to all the participants. You did a tremendous job eliminating bugs in open-source projects, making them better, safer, and more reliable. Perhaps someday we'll have a similar contest for the English-speaking community as well.
All the rest are welcome to download and try PVS-Studio analyzer. It is very easy to use and could help you a lot.