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!
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
The error was found in one of the most popular text editors, Notepad++.
Here's the code:
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 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
This error was found in 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 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
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:
mysqlx::XProtocol* active()
{
if (!active_connection)
std::runtime_error("no active session");
return active_connection.get();
}PVS-Studio warning: 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
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:
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 '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:
Sixth place
Source: Long-Awaited Check of CryEngine V
CryEngine V, the first game engine on this top-list.
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
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
This defect was found while fixing the bugs detected earlier by PVS-Studio in the code of the Unreal Engine game 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
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
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:
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 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:
Correct code:
(*body)[new_len] = '\0';Third place
Source: How Can PVS-Studio Help in the Detection of Vulnerabilities?
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.
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 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
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 and V709.
This example is a lengthy one, but you need all this code to figure out what the problem is about.
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.
static void CreateProjectSet(.... int32 OutCreatedProjects,
int32 OutMatchedProjects)
{
....
OutCreatedProjects = 0;
OutMatchedProjects = 0;
....
OutMatchedProjects++;
....
OutCreatedProjects++;
....
}PVS-Studio warnings:
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:
static void CreateProjectSet(.... int32 &OutCreatedProjects,
int32 &OutMatchedProjects)First place
Source: Appreciate Static Code Analysis!
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.
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 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!
#define sprintf std::printfAs 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.
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 or the list of defects found by PVS-Studio in open-source projects.
As a reminder, all the defects mentioned here (as well as many others) were found by the PVS-Studio analyzer, which I recommend trying with your own projects as well - download here.