Analyzing the source code with cppcheck

In the light of many recent articles devoted to static analysis of C ++ code, users have repeatedly been interested in the cppcheck analyzer. This is a relatively young project of open source static analysis, aimed primarily at finding real errors in the code with a minimum number of false positives.

More recently, cppcheck helped find a vulnerability in the Xorg project that has existed there for almost 23 years! He has already helped thousands of programmers around the world, on the official website you can find information about vulnerabilities found with cppcheck in programs, and this list is constantly growing. So, if you want to know why you need to use cppcheck anytime, anywhere, please request cat.

Cppcat and cppcheck

I will begin by comparing these utilities, since this request was voiced repeatedly in the comments. The CppCat developers have already independently conducted such a comparison (with PVS-Studio), but since then much water has flowed, and the comparison is not very objective, since PVS-Studio (as far as I understand the whole intricacy of the phrase “Please write us to get a price for PVS "Studio. Please specify interesting license type.") Is not intended for single programmers. CppCat, like cppcheck, is available to everyone (with the reservation to bind to VisualStudio certain versions and a license for a year).

It will not be easy to compare these analyzers: I do not have a single version of Visual Studio under Linux at hand. Therefore, at first, I will limit myself to analyzing the already analyzed code in a recent review of CppCat : I set cppcheck on Notepad ++, I’ll give errors / warnings statistics that can be compared with the ready-made CppCat analysis.
')
In parallel, I try to install CppCat in a virtual machine. I must say that after Visual Studio 2010 was installed, the installer, without thinking twice, issued the following:



Because of this, testing has become more difficult with a quest to find-put-Visual-Studio-2013-reinstall-IE-11-reboot-update, which on the virtual machine not burdened with updates took exactly half a day.

What is the result? When you open the project Notepad ++ Visual Studio hung up proudly. An attempt to create a new project resulted in errors in the analysis of CppCat about header files not found. By this we have to compare with what we have in the previous article. Although I put Visual Studio almost for the first time, the “usability” effect is obvious.

Training

Since cppcheck is an open source project, no one bothers to download the latest version from the git and compile it yourself. Cppcheck is designed for developers, so compiling a program from source code should not cause any problems:

unzip cppcheck-master.zip cd cppcheck-master make 

Is done. I deliberately took a fresh version of the gita and put it in a separate folder - it will be easier to work with it, since in the future cppcheck can be greatly improved and customized "for yourself."

Test configuration: RHEL 6.1, i5-2400 @ 3.10GHz processor (for estimating the analyzer operation time).

Convenience for the sake of action will be made on the command line - if desired, they can be repeated (in Linux :). Of course, cppcheck has several plugins for popular IDEs, but this is not what is today.

Analysis Notepad ++

cppcheck is designed so that all warnings are sorted by category. By default, only one type of analysis is enabled — error. Errors cannot be ignored, since if cppcheck issued an error, this place will have to be rewritten in 99% of cases. The main catch of cppcheck is memory leaks and buffer overflow, and this is already worth something.

A reasonable question may arise - how to analyze notepad ++ in the Linux operating system, if notepad ++ uses only WInAPI? The answer is simple - cppcheck in my memory is the only analyzer that is not tied to the build environment or operating system . It uses its lexical analyzer, does not require the presence of all header files, loyally refers to the intricacies of classes, etc. This remarkable property allows you to use cppcheck anywhere and for anything, which cannot be said about CppCat (see the installation quest above) .

Analysis using cppcheck is simple to ugliness:

 ./cppcheck-master/cppcheck -q -j4 npp.6.5.3.src/ 

While the simplest out-of-the-box analysis is being conducted. The command defines two parameters -q (“silent” mode - do not display progress on the screen) and -j4 - multi-stream analysis in 4 threads by the number of processor cores.

The result of the previous command:

 [npp.6.5.3.src/PowerEditor/src/tools/ChangeIcon/ChangeIcon.cpp:214]: (error) Mismatching allocation and deallocation: resData [npp.6.5.3.src/PowerEditor/src/tools/ChangeIcon/ChangeIcon.cpp:216]: (error) Mismatching allocation and deallocation: resData [npp.6.5.3.src/scintilla/lexers/LexBash.cxx] -> [npp.6.5.3.src/scintilla/lexers/LexBash.cxx:194]: (error) Internal error. Token::Match called with varid 0. Please report this to Cppcheck developers 

Opening hours - 5 minutes. The error "(error) Internal error. Token :: Match called with varid 0 immediately catches your eye. Please report this to Cppcheck developers". This means that instead of a bug in the program being analyzed, there was a bug in the analyzer itself :) Let's make a discount on the fact that the project is sharpened for Win, and cppcheck does not suspect what DWORD, LPTR, etc. means. In Win, it will show itself differently .

Actually there was only one error (with a difference of 2 lines). Not bad, it is possible that the author of notepad ++ uses cppcheck itself. The section of code that caused suspicion in cppcheck:

  BYTE* resData = new BYTE[cbRes]; LPBYTE writePtr = resData; ... if(!UpdateResource(hUpdate, RT_GROUP_ICON, lpResName, resLangId, resData, cbRes)) { _tprintf(_T("Unable to update icon group\n")); delete resData; return false; } IFDEBUG( _tprintf(_T("Updated group %d (lang %d)\n"), lpResName, resLangId); ) delete resData; } 

This looks like a false positive, although the source is, frankly, shocking. UPD: it’s still undefined behavior, the array needs to be freed with the help of the delete [] operator.

But that is not all. The fact is that the motto of cppcheck is the absence of false positives, that is, by default the scanner only looks for very critical errors - buffer overflow, memory leaks. After all errors are found, you can re-scan by turning on warning flags:

 ./cppcheck-master/cppcheck -q -j4 --enable=performance,portability,warning,style npp.6.5.3.src/ 2> npp.out 

The --enable parameter is used, which includes the categories of checks:

- performance - performance problems;
- portability - compatibility issues;
- warning - warnings - suspicious places of the program;
- style - programming style errors.

In this mode, the lion's share of stylistic / logical errors and potential bugs is caught (i.e., errors in which cppcheck is “not sure”). Scanning time - 5 minutes. I immediately sent the result to a file to collect statistics.

Total messages:

 wc -l < npp.out 379 

Small statistics on the type of errors found:

 tr '()' '*' < npp.out | cut -d* -f2 | sort | uniq -c 3 error 39 performance 14 portability 211 style 112 warning 

Only 28 unique messages.

The messages “The scope of the variable% {VAR} can be reduced”, “C-style pointer casting”, “Variable% {VAR} is assigned in constructor body.” Can be not considered - these are stylistic recommendations that are very characteristic of many projects written on old pros.

A sea of ​​variables is not initialized in the constructor: ( warning ) Member variable% {VAR} is not initialized in the constructor. This error cppcheck considers a warning. Perhaps the behavior of such code depends on the compiler, because npp works by some miracle.



( style ) Same expression on both sides of '||'. Check the same condition. CppCat produced the same error, however, either the version of npp is old in the article, or the error has already been fixed, but now that code looks like this:

 while (closeFound.success && (styleAt == SCE_H_DOUBLESTRING || styleAt == SCE_H_SINGLESTRING) && searchStartPoint <= caret); 

And this is the new booty cppcheck:

 if (!(!commentLineSybol || !commentLineSybol[0] || commentLineSybol == NULL)) 

( performance ) Variable 'lineIndent' is reassigned a value before the old one has been used. In fact - double assignment. This is usually a consequence of copy-paste, but cppckeck characterizes such an error as a performance error. This code is worth checking, since it is not known what the author of the program meant. There are a lot of such double assignments, as well as unused values ​​of variables by code:

  int lineIndent = lineStart; ... lineIndent = _pEditView->execute(SCI_GETLINEINDENTPOSITION, i); _pEditView->getGenericText(linebuf, linebufferSize, lineIndent, lineEnd); 

This warning is usually useless - rarely, when there is an error in this part of the code, just forgot to delete the old value during refactoring.

( portability ) The extra qualification 'FunctionListPanel ::' is unnecessary. A useful warning that is not physically present and is not planned in CppCat: portability errors between different platforms (portability). This piece of code will not work in all compilers:

 virtual BOOL CALLBACK FunctionListPanel::run_dlgProc(UINT message, WPARAM wParam, LPARAM lParam); 

This “politically correct” message actually means that the code will not gather without crutches in the gcc compiler. If you plan to run the application on more than one platform - cppcheck will be a good help.

( style ) Exception should not be caught by reference. An interesting warning is to catch an exception by reference , not by value:

  catch(std::exception e) { ::MessageBoxA(NULL, e.what(), "Exception", MB_OK); return -1; } 

( style ) Consecutive return, break, continue, goto or throw statements are unnecessary. Dead code: break after return:

  switch(lpnm->wID) { case REBAR_BAR_TOOLBAR: { ... return TRUE; break; } } 

( warning ) There is no effect outside the function. Usually this is a useful warning signaling a typo - the value assigned inside the function is not transmitted anywhere. However, here this is obviously a false positive, since value is a class variable:

 void SetValue( const TCHAR* _value ) { value = _value; } 

It can be used for data. Such a habit of using scanf can lead to a dangerous buffer overflow . In the case of numeric variables, this is a trivial undefined behavior:

 if ( sscanf( value.c_str(), "%d", ival ) == 1 ) 

To convert numbers it is better to use a safe strtol.

Another representative:

 sscanf( wordBuffer, "%[^.<>|&=\\/]", sKeywordBuffer ); 

There is no buffer overflow just because the wordBuffer and sKeywordBuffer are the same size.

( style ) 'TiXmlStringA :: operator =' should return 'TiXmlStringA &'. Operator = returns void:

 void operator = (const TiXmlStringA & copy); 

With such an operator, you cannot use the standard C ++ chain:

 a = b = c; 

( warning ) The class 'ControlsTab' defines a member variable with name '_isVertical' also defined in its parent class 'TabBar'. Error of double definition of a variable in a class:

 class ControlsTab : public TabBar { public : ... private : ... bool _isVertical; }; 

which is already defined in the parent class:

 class TabBar : public Window { ... protected: ... bool _isVertical; }; 

Not being an expert in the pros, I can not immediately answer whether it is possible to do so (protected / private).

( style ) Found duplicate branches for 'if' and 'else'. Similar errors found and CppCat. Extra condition:

  if(eol_mode == SC_EOL_CRLF) extraEOLLength = 2; else if(eol_mode == SC_EOL_LF) extraEOLLength = 1; else // SC_EOL_CR extraEOLLength = 1; 

( style ) Checking if unsigned variable 'lenFile' is less than zero. A similar message was issued by CppCat, except that cppcheck, without finding the windows.h file, did not begin to build hypotheses about types like WPARAM. There is a lack of non-orientation exclusively on Windows.

  size_t lenFile = 0; ... if (lenFile <= 0) break; 

I think if I had the Windows header files, you can specify the path to them through the -I parameter, then there will be much more errors.

( style ) Array index 'j' is used before limits check. Despite the fact that the warning is of low priority, the error found represents the danger of going beyond the array:

  int j; int ReturnValue; j=startcol; if(direction == 1){j++;} if(direction != 1){j--;} while((BGHS[SI].columnwidths[j] == 0)&&(j<=BGHS[SI].cols)&&(j>0)) 

Given that the startcol parameter is external, you can fly out of the array, not to mention the index -1.

( style ) Unsigned variable "i can't be negative so it is unnecessary to test it. The condition in the loop is always positive => infinite loop:

 for(unsigned int i = position_of_click; i >= 0; --i) 

This error could have been avoided when building a project with the gcc compiler and -Wall -Wextra flags. I think that such an error often appears when a project is refactored by another compiler error - a type mismatch. It was int - it became unsigned, and that's the result

Minor bugs

( style ) Unused variable: ent. Compilers are able to issue this warning, nothing interesting.

( warning )% d in format string (no. 2) requires' int 'but this argument is a DWORD {aka unsigned long}' is a very popular error, programmers in printf write a type that does not match the type of the variable. This is also shot by most compilers.

Class without constructor:

  class CachedValue { generic_string fullname; int index; }; 

( performance ) Function parameter 'range' should not be passed by reference. cppcheck recommends passing a parameter by reference to avoid copying the argument:

 XYScrollPosition XYScrollToMakeVisible(const SelectionRange range, const XYScrollOptions options); 

( warning ) Ineffective call of function 'empty ()'. Did you intend to call 'clear ()' instead? The empty method only makes sense inside the condition and does not clear the string. This is a false positive, cppcheck did not realize that the author would make his String class :) You should check the logic of naming methods.

( style ) 'class ByteArray' doesn’t have to be a copy constructor. cppcheck simply recommends creating a missing copy constructor in a class in case the programmer forgot to implement it.

Conclusion

Both analyzers picked up a decent amount of errors, and many of them are unique to each analyzer. In general, it would be nice to have cppcheck on hand always, since it is open, cross-platform, it really finds errors and helps to improve the programming style. Using cppcheck is usually not a problem.

From this analysis, we can conclude that the tools complement each other well. For someone, the main disadvantage of cppcheck is the lack of a plug-in under Visual Studio, so the authors of cppcheck kindly suggest trying PVS-Studio. Despite the command line interface, using cppcheck is very convenient. No compiler, no IDE, no header files are required - this is the easiest to use static analyzer I've ever seen. In addition, I specifically installed the cppcheck for Windows assembly in a virtual machine - it has a nice graphical interface, it installs quickly and performs analysis without any problems:



The result of the analysis can be exported to XML and viewed in the browser.

Both projects should wish success in development - these are really very necessary programs. And in the development of cppcheck, you can take part on your own right now: check your projects, write to the cppcheck developer about errors found or not found, report bugs, send useful patches to github . If cppcheck could not yet find the error if (malloc ()), now it just pours messages about memory leaks - the result of the competition is obvious.

This analysis could be greatly improved if we explicitly tell cppcheck where to look for header files, which functions allocate and free memory. Since the article turned out to be big, how to set up cppcheck for a specific project, improve the quality of analysis and write your own rules for cppcheck next time.

PS Please forgive one nonsense. Cppcheck has the ability to analyze the code specifically for Windows in the settings, which is why a lot of interesting errors were missed. It was necessary to analyze the npp with the --platform = win32A flag.