Testing five open projects with general-purpose static analyzers
The article “Difficulties in comparing code analyzers or don't forget about usability” [ 1 ] says that comparing two tools is not as easy as it seems, because in addition to the actual technical characteristics of the analyzers, such a parameter as usability is very important.
But still, there is no escape from comparing the detected errors. Naturally, just counting their number makes no sense. Therefore, we decided to conduct a practical experiment to detect errors in real projects.
The article shows the errors identified using the static code analyzer built into Visual Studio 2010. The study was conducted on five open source projects. These same projects were verified using PVS-Studio. The results of comparing these two tools are presented.
We tested five random open source projects first using the static analyzer built into Visual Studio 2010 Premium. We looked through the entire list of messages and selected obvious errors. Then they also came from PVS-Studio.
Here is the list of projects involved in the study:
So let's go!
eMule Plus
There are 237 messages from the Visual Studio static analyzer. Of these, 4 are real errors.
There are 68 messages from PVS-Studio. 3 of them are real errors.
Visual Studio: warning C6054: String 'szwThemeFile' might not be zero-terminated. c: \ emuleplus \ dialogmintraybtn.hpp 445
WCHAR szwThemeFile [MAX_PATH]; WCHAR szwThemeColor [256]; if (m_themeHelper.GetCurrentThemeName (szwThemeFile, ARRSIZE (szwThemeFile), szwThemeColor, ARRSIZE (szwThemeColor), NULL, 0)! = S_OK) return NULL; WCHAR * p; if ((p = wcsrchr (szwThemeFile, L '\\')) == NULL)
Indeed, the line may not end with 0, which will lead to potential problems. However, in this case, this error will not manifest itself, most likely.
Visual Studio: warning C6269: Possibly incorrect order of operations: dereference ignored. c: \ emuleplus \ customautocomplete.cpp 277
PVS-Studio: V532 Consider inspecting the statement of '* pointer ++' pattern. Probably meant: '(* pointer) ++'. customautocomplete.cpp 277
if (pceltFetched! = NULL) * pceltFetched ++;
Here the programmer "does not know how" to use the expression (* ptr) ++. Although, it would seem, such a design looks harmless enough, nevertheless, such a mistake is very common.
Visual Studio: warning C6298: Argument '6': using a read-only string as a writable string argument. This will attempt to write into static read-only memory and cause random crashes. c: \ emuleplus \ firewallopener.cpp 183
HRESULT hr = pNSC-> AddPortMapping ( riPortRule.m_strRuleName.AllocSysString (), riPortRule.m_byProtocol, riPortRule.m_nPortNumber, riPortRule.m_nPortNumber, 0, L "127.0.0.1", ICSTT_IPADDRESS, & pNSPM);
Although this is not a mistake, the analyzer message is fair. In general, this is a problem for all static analysis tools. They give out completely correct messages, but far from always these are really mistakes. Does this mean that tools and similar messages are useless? No, it doesn’t, because correcting even such remarks generally improves the quality of the code.
Visual Studio: warning C6314: Incorrect order of operations: bitwise- or has higher precedence than the conditional-expression operator. Add parentheses to clarify intent. c: \ emuleplus \ searchlistctrl.cpp 659
PVS-Studio: V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '|' operator. searchlistctrl.cpp 659
menuSearchFile.AppendMenu (MF_STRING | ((iSelectionMark! = -1) && (dwSelectedCount> 0) && g_App.m_pServerConnect-> IsConnected () && ((pCurServer = g_App.m_pServerConnect-> GetCurrentServer ())! = NULL) && (pCurServer-> GetTCPFlags () & SRV_TCPFLG_RELATEDSEARCH))? MF_ENABLED: MF_GRAYED, MP_SEARCHRELATED, GetResString (IDS_SEARCHRELATED));
Here (due to the complexity of the design) the wrong operator priority was obtained. How many times they kept repeating to the world ... Well, who prevented us from writing all this on one line (as in the original program), but on breaking it into several separate expressions? No, programmers often want to "write beautifully."
PVS-Studio: V519 The 'm_clrSample' object is assigned values twice successively. Perhaps this is a mistake. fontpreviewcombo.cpp 61
CFontPreviewCombo :: CFontPreviewCombo () { ... m_clrSample = GetSysColor (COLOR_WINDOWTEXT); m_clrSample = RGB (60,0,0); ... }
Perhaps they tried how the RGB color (60,0,0) will look and forgot to remove it.
Pixie
There are 18 messages from the Visual Studio static analyzer. Of these, 0 are real errors.
There are 65 messages from PVS-Studio. 5 of them are real errors.
PVS-Studio: V519 The 'numRays' object is assigned values twice successively. Perhaps this is a mistake. bundles.cpp 579 void CGatherBundle :: post () { numRays = last; numRays = 0; last = 0; depth ++; }
Oh, how suspicious that numRays is first initialized with one value and then with another. Error? Or not? Only the author of the code knows. But alarming!
PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '|' operator: PARAMETER_DPDU | PARAMETER_DPDU quadrics.cpp 880
if (up & (PARAMETER_DPDU | PARAMETER_DPDU)) {
Apparently there must be something else here. By the way, a general note on fixing errors detected by a static analyzer. If in some cases the correction is obvious and anyone can do it, in some cases only the author of the code can figure out what he wanted to write. This is to the question of whether it is possible to make a tool that offers error correction “like in Word”.
PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '|' operator: SLC_VECTOR | SLC_VECTOR expression.cpp 2604
lock (N, getConversion (SLC_VECTOR | SLC_VECTOR, parameters [2]));
The twice-mentioned SLC_VECTOR in a similar context is, of course, a sign of error.
PVS-Studio: V505 The 'alloca' function is used inside the loop. This can quickly overflow stack. polygons.cpp 1120
inline void triangulatePolygon (...) { ... for (i = 1; i
Due to the large nesting, the allocationa call can lead to a stack overflow.Virtualdub
There are 24 messages from the Visual Studio static analyzer. Of these, 0 are real errors.
There are 41 messages from PVS-Studio. 2 of them are real errors.PVS-Studio: V547 Expression 'c <0' is always false. Unsigned type value is never <0. lexer.cpp 279typedef unsigned short wint_t; wint_t lexgetescape () { wint_t c = lexgetc (); if (c <0) fatal ("Newline found in escape sequence"); ... }
The code will never be called because the expression is always false.PVS-Studio: V557 Array overrun is possible. The '9' index is pointing beyond array bound. f_convolute.cpp 73struct ConvoluteFilterData { long m [9]; long bias; void * dyna_func; DWORD dyna_size; DWORD dyna_old_protect; BOOL fClip; }; static unsigned long __fastcall do_conv ( unsigned long * data, const ConvoluteFilterData * cfd, long sflags, long pit) { long rt0 = cfd-> m [9], gt0 = cfd-> m [9], bt0 = cfd-> m [9]; ... }
Banal exit beyond the boundaries of the array.Winmerge
The total number of messages from the Visual Studio static analyzer is 343. Of these, 3 are real errors.
There are 69 total messages from PVS-Studio. Of these, 12 are real errors.Visual Studio: warning C6313: Incorrect operator: zero-valued flag cannot be tested with bitwise-and. Use an equality test to check for zero-valued flags. c: \ winmerge \ src \ bcmenu.cpp 1489else if (nFlags & MF_STRING) { ASSERT (! (NFlags & MF_OWNERDRAW)); ModifyMenu (pos, nFlags, nID, mdata-> GetString ()); }
Unsuccessfully recorded condition. They wanted, of course, to record something else, but “what happened”.Visual Studio: warning C6287: Redundant code: the left and right sub-expressions are identical. c: \ winmerge \ src \ editlib \ ccrystaleditview.cpp 1560PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '||' operator: c == '}' || c == '}' ccrystaleditview.cpp 1560bool isclosebrace (TCHAR c) { return c == _T ('}') || c == _T ('}') || c == _T (']') || c == _T ('>'); }
Not all types of brackets are checked. Why? Copy-paste-technology, as is often the case, leads to errors.Visual Studio: warning C6287: Redundant code: the left and right sub-expressions are identical. c: \ winmerge \ src \ mergedoc.cpp 1165 PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '||' operator. mergedoc.cpp 1165if ((m_nBufferType [nBuffer] == BUFFER_UNNAMED) || (m_nBufferType [nBuffer] == BUFFER_UNNAMED)) nSaveErrorCode = SAVE_NO_FILENAME;
Again a bad condition. And again, it seems, because of the mine-paste.PVS-Studio: V551 The code under this 'case' label is unreachable. The value range of signed char type: [-128, 127]. ccrystaltextview.cpp 1646TCHAR ch = strText [i]; switch (ch) { case 0xB7: case 0xBB: strHTML + = ch; strHTML + = _T (""); bLastCharSpace = FALSE; nNonbreakChars = 0; break;
And here is an example of code that will never be used. It seems that the case is written, but he never gets control. Because the range of values is too narrow. TCHAR in this case is a type of char.PVS-Studio: V524 It is odd that the body of 'IsValidTextPosX' function is fully equivalent to the body of 'IsValidTextPos' function (ccrystaltextview.cpp, line 3700). ccrystaltextview.cpp 3707bool CCrystalTextView :: IsValidTextPos (const CPoint & point) { return GetLineCount ()> 0 && m_nTopLine> = 0 && m_nOffsetChar> = 0 && point.y> = 0 && point.y <GetLineCount () && point.x> = 0 && point.x <= GetLineLength (point.y); } bool CCrystalTextView :: IsValidTextPosX (const CPoint & point) { return GetLineCount ()> 0 && m_nTopLine> = 0 && m_nOffsetChar> = 0 && point.y> = 0 && point.y <GetLineCount () && point.x> = 0 && point.x <= GetLineLength (point.y); } bool CCrystalTextView :: IsValidTextPosY (const CPoint & point) { return GetLineCount ()> 0 && m_nTopLine> = 0 && m_nOffsetChar> = 0 && point.y> = 0 && point.y <GetLineCount (); }
Extremely similar functions ... Over and over, copy-pastil and forgot to correct the result. The IsValidTextPosX () function does an extra check.PVS-Studio: V563 It is possible that this 'else' branch must apply to the previous 'if' statement. bcmenu.cpp 1852if (IsLunaMenuStyle ()) if (! xp_space_accelerators) return; else if (! original_space_accelerators) return;
Who, looking at the code, will say exactly what if else refers to? Was this what the programmer wanted to do? Are you sure?PVS-Studio: V556 The values of different enum types are compared: switch (ENUM_TYPE_A) {case ENUM_TYPE_B: ...}. diffwrapper.cpp 956enum output_style {} ... enum DiffOutputType m_outputStyle; switch (m_options.m_outputStyle) { case OUTPUT_CONTEXT:
Slightly messed up the enum type in switch. But isn’t it scary? Or scary?PVS-Studio: V530 The return value of function 'empty' is required to be utilized. diractions.cpp 1307void CDirView :: GetItemFileNames (int sel, String & strLeft, String & strRight) const { UINT_PTR diffpos = GetItemKey (sel); if (diffpos == (UINT_PTR) SPECIAL_ITEM_POS) { strLeft.empty (); strRight.empty ();
That case when empty () does not do empty. An example of an extremely unsuccessful method name.PVS-Studio: V524 It is odd that the body of 'OnUpdateLastdiff' function is fully equivalent to the body of 'OnUpdateFirstdiff' function (DirView.cpp, line 2189). dirview.cpp 2220void CDirView :: OnUpdateLastdiff (CCmdUI * pCmdUI) { int firstDiff = GetFirstDifferentItem (); if (firstDiff> -1) pCmdUI-> Enable (TRUE); else pCmdUI-> Enable (FALSE); } void CDirView :: OnUpdateFirstdiff (CCmdUI * pCmdUI) { int firstDiff = GetFirstDifferentItem (); if (firstDiff> -1) pCmdUI-> Enable (TRUE); else pCmdUI-> Enable (FALSE); }
Two other very similar features ...PVS-Studio: V501 There are identical sub-expressions 'pView1-> GetTextBufferEol (line)' to the left and to the right of the '! =' operator. mergedoclinediffs.cpp 216if (pView1-> GetTextBufferEol (line)! = pView1-> GetTextBufferEol (line)) {
Or this, or that ... Or not that? Probably here should be pView2.PVS-Studio: V530 The return value of function 'empty' is required to be utilized. varprop.cpp 133void VariantValue :: Clear () { m_vtype = VT_NULL; m_bvalue = false; m_ivalue = 0; m_fvalue = 0; m_svalue.empty (); m_tvalue = 0; }
And again empty () does not clear the line at all.PVS-Studio: V510 The 'Format' function is not expected to receive class-type variable as 'N' actual argument ". PropShel 105String GetSysError (int nerr); ... CString msg; msg.Format ( _T ("Failed to open registry key HKCU /% s: \ n \ t% d:% s"), f_RegDir, retVal, GetSysError (retVal) );
If various emergencies occur, WinMerge will try to report errors, but in some cases it will fail. At first glance, everything is fine. That's just the type of "String" is nothing more than "std :: wstring". And, therefore, in the best case, we will print an abracadabra, and in the worst case, an Access Violation error will occur. The correct code must contain a call to c_str ().PVS-Studio: V534 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i'. "BinTrans.cpp 357// Get length of translated array of bytes from text. int Text2BinTranslator :: iLengthOfTransToBin ( char * src, int srclen) { ... for (k = i; i') break; } ... }
The analyzer found a suspicious cycle. This code is predisposed for Access Violation. The loop must continue until a '>' character is found or a string with a length of 'srclen' characters ends. It’s just by chance that the variable 'i' was used for comparison, not 'k'. If the '>' character is not found, then everything will probably end sadly.XUIFramework
There are 93 messages from the Visual Studio static analyzer. Of these, 2 are real errors.
There are 30 messages from PVS-Studio. 5 of them are real errors.Visual Studio: warning C6269: Possibly incorrect order of operations: dereference ignored c: \ xui-gui framework \ widgets \ cstatichtml \ ppdrawmanager.cpp 298PVS-Studio: V532 Consider inspecting the statement of '* pointer ++' pattern. Probably meant: '(* pointer) ++'. ppdrawmanager.cpp 298 for (DWORD pixel = 0; pixel <dwWidth * dwHeight; pixel ++, * pBits ++)
Again, someone cannot use * ptr ++. As mentioned above, a common mistake.Visual Studio: warning C6283: 'pBuffer' is allocated with array new [], but deleted with scalar delete. c: \ xui-gui framework \ widgets \ cxstatic \ cxstatic.cpp 544BYTE * pBuffer = new BYTE [nBufferLen]; ... delete pBuffer;
Confused people are delete and delete []. This leads to problems. Sometimes problems appear, sometimes not. But this is not necessary.PVS-Studio: V519 The 'm_xSt' object is assigned values twice successively. Perhaps this is a mistake. resizedlg.cpp 244m_xSt = CST_RESIZE; m_xSt = CST_RESIZE;
Judging by the code, there should be m_ySt there. But you can’t hold back from mine-paste again and again, right?V531 It is odd that a sizeof () operator is multiplied by sizeof (). pphtmldrawer.cpp 258DWORD dwLen = :: LoadString (hInstDll, dwID, szTemp, (sizeof (szTemp) * sizeof (TCHAR)));
Must be sizeof (szTemp) / sizeof (TCHAR).PVS-Studio: V556 The values of different enum types are compared: enuHAlign == Center. cxstatic.cpp 151if (enuHAlign == Center)
Must be enuHAlign == Midle. And in the code nearby there is still if (enuVAlign == Middle), although there should be a Center. Confused enum, in short.PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '||' operator. resizedlg.cpp 157HDWP CItemCtrl :: OnSize (...) { ... if (m_styTop == CST_ZOOM || m_styTop == CST_ZOOM || m_styBottom == CST_DELTA_ZOOM || m_styBottom == CST_DELTA_ZOOM) ... }
The code should probably look like this:HDWP CItemCtrl :: OnSize (...) { ... if (m_styTop == CST_ZOOM || m_styTop == CST_DELTA_ZOOM || m_styBottom == CST_ZOOM || m_styBottom == CST_DELTA_ZOOM) ... }Comparison results
We do not make any specific conclusions. In some projects, one tool showed itself better, in others - another.