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 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 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 1489


    else 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 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 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 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 mine-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 (" ");
        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 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 ... 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 1852


    if (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 956


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

    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 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 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 216


    if (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 133


    void 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 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 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 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 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 544


    BYTE * 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 244


    m_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 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 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 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 make any specific conclusions. In some projects, one tool showed itself better, in others - another.

    Also popular now: