Errors that static code analysis does not find, because it is not used

From time to time, readers of our articles on checking open projects note that the PVS-Studio static code analyzer reveals a large percentage of errors that are minor or do not affect the application at all. It really is. Most of the important bugs have already been corrected due to manual testing, user feedback and other expensive methods. At the same time, many of these errors could be found at the stage of writing the code and corrected with minimal loss of time, reputation, and money. This article will provide several examples of real errors that would be immediately corrected if the authors of the projects used static code analysis.

The idea is very simple. Let's look at GitHub for examples of pull requests, in the comments to which it is indicated that this is a bug fix. And we will try to find these errors with the help of the PVS-Studio static code analyzer. If the corrected error is in the analyzer, it means that it could be corrected at the stage of writing the code. And the sooner the error is corrected, the cheaper it costs.

Unfortunately, GitHub let us down and did not allow us to make a great posh article on this topic. In GitHub itself there is also a glitch (or feature) that does not allow searching for comments in pull requests in projects written only in certain programming languages. Well, or I do not "know how to cook it." Regardless of what I specify to search for comments in projects in C ++, C #, or Java, results are displayed in all languages, including PHP, Python, JavaScript, and so on. As a result, it turned out to be extremely tedious to look for suitable cases, and I will limit myself to just a few examples. Nevertheless, they are enough to demonstrate the usefulness of static code analysis tools for its regular use.
')
What if the error were caught early? The answer is simple: programmers would not need to wait for its manifestation, and then search for and fix the defective code.
Let's look at the errors that PVS-Studio could immediately detect:

The first example is taken from the SatisfactoryModLoader project. Before fixing the error, the code looked like this:

// gets an API function from the mod handler SML_API PVOID getAPIFunction(std::string name) { bool found = false; for (Registry reg : modHandler.APIRegistry) { if (reg.name == name) { found = true; } } if (!found) { std::string msg = ...; MessageBoxA(NULL, msg.c_str(), "SatisfactoryModLoader Fatal Error", MB_ICONERROR); abort(); } } 

This code contained an error, for which PVS-Studio would immediately issue a warning:

V591 Non-void function should return a value. ModFunctions.cpp 44

The above function has no return , so it will return a formally undefined value. The programmer did not use the code analyzer, so he had to look for the error himself. Function after editing:

 // gets an API function from the mod handler SML_API PVOID getAPIFunction(std::string name) { bool found = false; PVOID func = NULL; for (Registry reg : modHandler.APIRegistry) { if (reg.name == name) { func = reg.func; found = true; } } if (!found) { std::string msg = ...; MessageBoxA(NULL, msg.c_str(), "SatisfactoryModLoader Fatal Error", MB_ICONERROR); abort(); } return func; } 

It is curious that the author indicated a bug as critical in the commit: " fixed critical bug where API functions were not returned ".

In the second commit from the history of the mc6809 project, corrections were made to the following code:

 void mc6809dis_direct( mc6809dis__t *const dis, mc6809__t *const cpu, const char *const op, const bool b16 ) { assert(dis != NULL); assert(op != NULL); addr.b[MSB] = cpu->dp; addr.b[LSB] = (*dis->read)(dis, dis->next++); ... if (cpu != NULL) { ... } } 

The author has corrected only one line. He replaced the expression

 addr.b[MSB] = cpu->dp; 

on expression

 addr.b[MSB] = cpu != NULL ? cpu->dp : 0; 

In the old version of the code, no cpu was checked for equality to the null pointer. If it suddenly turns out that a null pointer is passed to the mc6809dis_direct function as the second argument, it will be de -referenced in the function body. The result is pitiable and unpredictable .

Null pointer dereferencing is one of the most frequent patterns about which we are told: “This is not a critical error. Well, she lives in the code and lives, and if the dereferencing happens - the program will quietly fall and that's all. ” It is strange and sad to hear that from C ++ programmers, but life is life.

In any case, in this project such dereferencing has turned into a bug, as the title of the commit tells us: " Bug fix --- NULL dereference ".

If the project developer had used PVS-Studio, he would have been two and a half months ago (exactly how long since the introduction of the error) could check his code and detect the warning:

V595 The "cpu" pointer was used before it was verified against nullptr. Check lines: 1814, 1821. mc6809dis.c 1814

Thus, the error would have been eliminated at the time of its appearance, which would save time and, perhaps, the developer's nerves :).

An example of another interesting edit was found in the libmorton project.

Corrected code:

 template<typename morton> inline bool findFirstSetBitZeroIdx(const morton x, unsigned long* firstbit_location) { #if _MSC_VER && !_WIN64 // 32 BIT on 32 BIT if (sizeof(morton) <= 4) { return _BitScanReverse(firstbit_location, x) != 0; } // 64 BIT on 32 BIT else { *firstbit_location = 0; if (_BitScanReverse(firstbit_location, (x >> 32))) { // check first part firstbit_location += 32; return true; } return _BitScanReverse(firstbit_location, (x & 0xFFFFFFFF)) != 0; } #elif _MSC_VER && _WIN64 .... #elif __GNUC__ .... #endif } 

In his edit, the programmer replaces the expression " firstbit_location + = 32 " with " * firstbit_location + = 32 ". The programmer expected that the number 32 would be added to the value of the variable to which the firstbit_location pointer is attached , but it was added directly to the pointer. The changed pointer value was not used anywhere else, and the expected value of the variable remained unchanged.

PVS-Studio would issue a warning to this code:

V1001 The 'firstbit_location' variable is assigned. morton_common.h 22

It would seem, what could be terrible in a modified, but not further used expression? Diagnostics V1001 clearly does not look like it is designed to identify particularly dangerous bugs. Despite this, she discovered an important error that affected the logic of the program.

Moreover, it turned out that this error was not so easy to find! Not only was she in the program from the very moment the file was created, she also experienced many edits in the next lines and existed in the project for as many as 3 ( ! ) Years! All this time, the logic of the program was broken, and it did not work exactly as the developers had expected. If they used PVS-Studio, the error would have been detected much earlier.

At the end, consider another beautiful example. While I was collecting bug fixes on GitHub, I stumbled upon a fix with this content several times. The corrected error was here:

 int kvm_arch_prepare_memory_region(...) { ... do { struct vm_area_struct *vma = find_vma(current->mm, hva); hva_t vm_start, vm_end; ... if (vma->vm_flags & VM_PFNMAP) { ... phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) + vm_start - vma->vm_start; ... } ... } while (hva < reg_end); ... } 

For this piece of code, PVS-Studio issued a warning:

V629 Consider inspecting the 'vma-> vm_pgoff << 12' expression. Bit shifting of the 32-bit type. mmu.c 1795

I looked at the variable declarations used in the expression " phys_addr_t pa = (vma-> vm_pgoff << PAGE_SHIFT) + vm_start - vma-> vm_start; ", and found that the above code is similar to the following synthetic example:

 void foo(unsigned long a, unsigned long b) { unsigned long long x = (a << 12) + b; } 

If the value of the 32-bit variable a is greater than 0xFFFFF , then the 12 most significant bits will have at least one nonzero value. After applying the left shift operation to this variable, these significant bits will be lost, as a result of which incorrect information will be written to x .

To eliminate the loss of high-order bits, you must first cast a to the unsigned long long type, and only after that perform a shift operation:

 pa = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT; pa += vm_start - vma->vm_start; 

Then pa will always write the correct value.

Everything would be fine, but this bug, like the first example from the article, also turned out to be critical, as the author of the commit wrote separately in his commentary. Moreover, this error has hit well, just a huge number of projects. To fully appreciate the scale of the tragedy, I suggest looking at the number of results when searching for this bugfix on GitHub. Scary, is not it?

So, I applied a new approach to demonstrate the benefits of regular use of a static code analyzer. Hope you enjoyed it. Download and try the PVS-Studio static code analyzer to check your own projects. At the time of this writing, it has implemented about 700 diagnostic rules for detecting a variety of error patterns. C, C ++, C # and Java languages ​​are supported.

If you want to share this article with an English-speaking audience, then please use the link to the translation: George Gribkov. Errors that it doesn’t find