Verification of five open projects with general-purpose static analyzers

The article “Difficulties of comparing code analyzers or do not forget about usability” [ 1 ] says that comparing two tools with each other is not as easy as it seems, because besides the technical characteristics of the analyzers, such a parameter as usability is very important.

But still, the comparison of detectable errors cannot be avoided. Naturally, just count their number - it makes no sense. Therefore, we decided to conduct a practical experiment to detect errors in real projects.
')
The article shows the errors identified using a static code analyzer embedded in Visual Studio 2010. The study was conducted on five open source projects. The same projects were checked using PVS-Studio. The results of the comparison of these two instruments are given.



We tested five random open source projects first using a static analyzer built into Visual Studio 2010 Premium. Viewed the entire list of messages and selected obvious errors. Then they also arrived from PVS-Studio.

Here is a list of projects that participated in the study:

• eMule Plus ;
• Pixie ;
• VirtualDub ;
• WinMerge ;
• XUIFramework .

So let's go!

eMule Plus

Total messages from the static analyzer Visual Studio - 237. Of these, real errors - 4.

Total messages from PVS-Studio are 68. Of these, there are 3 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 string may not end in 0, leading 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 that such a design looks quite harmless, however, such an error is very common.

  Visual Studio: warning C6298: Argument '6': using a read-only string
 as a writable string argument.  This will attempt to write 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 the problem of all static analysis tools. They give out quite correct messages, but not always these are really mistakes. Does this mean that tools and similar messages are useless? No, it does not, because the correction of even such comments 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 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 priority of the operators turned out. How many times have the world been told ... Well, who prevented me from writing this all in a single line (as in the original program), but split 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 you tried to look like the color RGB (60,0,0) and forgot to remove.

Pixie

Total messages from the Visual Studio static analyzer are 18. Of these, there are 0 real errors.

Total messages from PVS-Studio are 65. Of these, 5 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. Mistake? Or not? Knows only the author of the code. 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)) { 

Obviously there must be something else. By the way, a general note on the correction of errors detected by the static analyzer. If in some cases the correction is obvious and anybody can make it, in some only the author of the code can figure out what he wanted to write. This is the question of whether it is possible to make a tool that offers correction of errors "as 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 an 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 <nloops; i ++) {
     ...
     do {
       ...
       do {
         ...
         CTriVertex * snVertex = (CTriVertex *)
           alloca (2 * sizeof (CTriVertex));
         ...
       } while (dVertex! = loops [0]);
       ...
     } while (sVertex! = loops [i]);
     ...
   }
   ...
 } 

Due to the large nesting, the call to alloca can lead to a stack overflow.

Virtualdub

Total messages from the Visual Studio static analyzer are 24. Of these, there are 0 real errors.

Total messages from PVS-Studio are 41. Of these, real errors are 2.

  PVS-Studio: V547 Expression 'c <0' is always false. 
 Unsigned type value is never <0. lexer.cpp 279 

  typedef 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 73 

  struct 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 going beyond the array.

WinMerge

Total messages from the static analyzer Visual Studio - 343. Of these, the real errors - 3.

Total messages from PVS-Studio are 69. 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 1489 

  else if (nFlags & MF_STRING) {
  ASSERT (! (NFlags & MF_OWNERDRAW));
  ModifyMenu (pos, nFlags, nID, mdata-> GetString ());
 } 

Failed to write condition. They wanted, of course, to write 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 1560 

  PVS-Studio: V501 There are identical sub-expressions to the left and
 to the right of the '||'  operator: c == '}' ||  c == '}' 
 ccrystaleditview.cpp 1560 

  bool
 isclosebrace (TCHAR c)
 {
   return c == _T ('}') ||  c == _T ('}') ||  c == _T (']') 
       ||  c == _T ('>');
 } 

Not all types of parentheses are checked. Why? "Copy-paste-technology", as often happens, 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 1165 

  if ((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 copy-paste.

  PVS-Studio: V551 The code under this 'case' label is unreachable. 
 The value range of signed char type: [-128, 127]. 
 ccrystaltextview.cpp 1646 

  TCHAR ch = strText [i];
 switch (ch)
 {
   case 0xB7:
   case 0xBB:
     strHTML + = ch;
     strHTML + = _T ("<wbr>");
     bLastCharSpace = FALSE;
     nNonbreakChars = 0;
     break; 

And here is an example of code that will never be used. It seems to be the case is written, but he will never get control. Because the range of values ​​is too narrow. TCHAR in this case is a type of char.

  PVS-Studio: V524 It’s odd that the body of 'IsValidTextPosX' function
 is fully equivalent to the body of 'IsValidTextPos' function 
 (ccrystaltextview.cpp, line 3700).  ccrystaltextview.cpp 3707 

  bool 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 ... Again and again, copy-paste and forget 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 1852 

  if (IsLunaMenuStyle ())
   if (! xp_space_accelerators) return;
 else
   if (! original_space_accelerators) return; 

Who, looking at the code, will say for sure if to what else belongs? Is 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 956 

  enum output_style {}
 ...
 enum DiffOutputType m_outputStyle;

 switch (m_options.m_outputStyle)
 {
   case OUTPUT_CONTEXT: 

A little bit the type enum in the switch. But this is not scary? Or scary?

  PVS-Studio: V530 'empty' is required to
 be utilized.  diractions.cpp 1307 

  void 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 (); 

The case when empty () does not empty. An example of a very unfortunate 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 2220 

  void 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 more extremely 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 216 

  if (pView1-> GetTextBufferEol (line)! = 
     pView1-> GetTextBufferEol (line))
 { 

Either that, or that ... Or not? Probably there should be pView2.

  PVS-Studio: V530 'empty' is required to
 be utilized.  varprop.cpp 133 

  void VariantValue :: Clear ()
 {
   m_vtype = VT_NULL;
   m_bvalue = false;
   m_ivalue = 0;
   m_fvalue = 0;
   m_svalue.empty ();
   m_tvalue = 0;
 } 

Again, empty () does not clear the string at all.

  PVS-Studio: V510 The 'Format' function is not expected to receive
 class-type variable as 'N' actual argument ". PropShel 105 

  String 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 "String" is nothing more than "std :: wstring". And, therefore, at best, we will print abracadabra, and at worst, there will be a memory access error (Access Violation). A valid code must contain a call to c_str ().

  PVS-Studio: V534 It is likely that a variable is being compared inside the 'for' operator.  Consider reviewing 'i'. "BinTrans.cpp 357 

  // Get the length of the translated array of bytes from text.
 int Text2BinTranslator :: iLengthOfTransToBin (
   char * src, int srclen)
 {
   ...
   for (k = i; i <srclen; k ++)
   {
     if (src [k] == '>>)
       break;
   }
   ...
 } 

The analyzer found a suspicious loop. This code is predisposed to Access Violation. The loop must continue until a '>' character is found or a string of 'srclen' characters in length ends. That's just by chance for comparison used the variable 'i', and not 'k'. If the '>' character is not found, then everything is likely to end sadly.

XUIFramework

Total messages from the static analyzer Visual Studio - 93. Of these, real errors - 2.

Total messages from PVS-Studio are 30. Of these, 5 are real errors.

  Visual Studio: warning C6269: Possibly incorrect order of operations:
 dereference ignored 
 c: \ xui-gui framework \ widgets \ cstatichtml \ ppdrawmanager.cpp 298 

  PVS-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 does not know how to use * ptr ++. As mentioned above, the error is common.

  Visual Studio: warning C6283: 'pBuffer' is allocated with array new [],
 but deleted with scalar delete.
 c: \ xui-gui framework \ widgets \ cxstatic \ cxstatic.cpp 544 

  BYTE * pBuffer = new BYTE [nBufferLen];
 ...
 delete pBuffer; 

Confused people delete and delete []. This leads to problems. Sometimes problems appear, sometimes not. But do not do that.

  PVS-Studio: V519 The 'm_xSt' object is assigned values ​​twice
 successively.  Perhaps this is a mistake.  resizedlg.cpp 244 

  m_xSt = CST_RESIZE;
 m_xSt = CST_RESIZE; 

Judging by the code, there should be m_ySt. But you can't resist from copy-paste again and again, right?

  V531 It is odd that a sizeof () operator is multiplied by sizeof ().
 pphtmldrawer.cpp 258 

  DWORD 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 151 

  if (enuHAlign == Center) 

Must be enuHAlign == Midle. And in the code next to there is still if (enuVAlign == Middle), although it should be Center. Lured enum, in short.

  PVS-Studio: V501 There are identical sub-expressions to the left and
 to the right of the '||'  operator.  resizedlg.cpp 157 

  HDWP 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 draw any concrete conclusions. In some projects, one tool showed itself better; in others, another.