Яндекс.Метрика

    Ни о чём

    Проверка пяти открытых проектов статическими анализаторами общего назначения



    В статье «Трудности сравнения анализаторов кода или не забывайте об удобстве использования» [1] говорится о том, что сравнить между собой два инструмента не так просто как кажется, потому что помимо собственно технических характеристик анализаторов очень большое значение имеет такой параметр как удобство использования.

    Но все-таки от сравнения по обнаруживаемым ошибкам никуда не деться. Естественно, просто посчитать их количество – смысла нет. Поэтому мы решили провести практический эксперимент по обнаружению ошибок в реальных проектах.

    В статье показаны ошибки, выявленные с помощью статического анализатора кода, встроенного в Visual Studio 2010. Исследование проводилось на пяти open source проектах. Эти же проекты были проверены с помощью PVS-Studio. Приведены результаты сравнения этих двух инструментов.


    Мы проверили пять случайных open source проектов сначала c помощью статического анализатора, встроенного в Visual Studio 2010 Premium. Просмотрели весь список сообщений и выбрали явные ошибки. Затем также поступили с PVS-Studio.

    Вот список проектов, участвовавших в исследовании:

    Итак, поехали!

    eMule Plus


    Всего сообщений от статического анализатора Visual Studio – 237. Из них реальных ошибок – 4.

    Всего сообщений от PVS-Studio – 68. Из них реальных ошибок – 3.

    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)

    Действительно строка может не заканчиваться на 0, что приведет к потенциальным проблемам. Однако в данном случае эта ошибка не проявит себя, скорее всего.

    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++;

    Здесь программист «не умеет» пользоваться выражением (*ptr)++. Хотя, казалось бы, такая конструкция выглядит достаточно безобидно, тем не менее, подобная ошибка очень распространена.

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

    Хотя это и не ошибка, но сообщение анализатора справедливое. Вообще это проблема всех инструментов статического анализа. Они выдают совершенно корректные сообщения, но далеко не всегда это действительно ошибки. Означает ли это, что инструменты и подобные сообщения бесполезны? Нет, не означает, поскольку исправление даже подобных замечаний в целом повышает качество кода.

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

    Здесь (из-за сложности конструкции) получился неправильный приоритет операторов. Уж сколько раз твердили миру… Ну вот кто мешал писать это все не в одну строку (как в оригинальной программе), а разбить на несколько отдельных выражений? Нет же, программисты часто хотят «записать красиво».

    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);
      ...
    }

    Возможно, попробовали как будет смотреться цвет RGB(60,0,0) и забыли убрать.

    Pixie


    Всего сообщений от статического анализатора Visual Studio – 18. Из них реальных ошибок – 0.

    Всего сообщений от PVS-Studio – 65. Из них реальных ошибок – 5.

    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++;
    }

    Ой, как подозрительно, что сначала numRays инициализируется одним значением, а затем другим. Ошибка? Или нет? Знает только автор кода. Но настораживает!

    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)) {

    Явно здесь должно быть что-то еще. Кстати общее замечание по исправлению ошибок, обнаруженных статическим анализатором. Если в каких-то случаях исправление очевидно и его может сделать любой, то в каких-то лишь автор кода может разобраться с тем, что же он хотел написать. Это к вопросу о том, а нельзя ли сделать инструмент, который предлагает исправление ошибок «как в Ворде».

    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]));

    Дважды упомянутый SLC_VECTOR в подобном контексте – конечно же, признак ошибки.

    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]);
        ...
      }
      ...
    }

    Из-за большой вложенности вызов alloca может привести к переполнению стека.

    VirtualDub


    Всего сообщений от статического анализатора Visual Studio – 24. Из них реальных ошибок – 0.

    Всего сообщений от PVS-Studio – 41. Из них реальных ошибок – 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");
      ...
    }

    Код никогда не будет вызван, поскольку выражение всегда 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];
      ...
    }

    Банальный выход за границы массива.

    WinMerge


    Всего сообщений от статического анализатора Visual Studio – 343. Из них реальных ошибок – 3.

    Всего сообщений от PVS-Studio – 69. Из них реальных ошибок – 12.

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

    Неудачно записано условие. Хотели, конечно записать другое, но уж «что получилось».

    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 ('>');
    }

    Не все типы скобочек проверяются. Почему? «Copy-paste-technology», как это нередко бывает, приводит к ошибкам.

    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;

    Опять неудачное условие. И опять, похоже, из-за копи-паста.

    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;

    А вот здесь пример кода, который никогда не будет использован. Вроде бы и case написан, да никогда он не получит управление. Потому что диапазон значений слишком узкий. TCHAR в данном случае представляет собой тип 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 ();
    }

    Крайне похожие функции… Снова и снова копи-пастили и забыли поправить результат. Функция IsValidTextPosX() делает лишнюю проверку.

    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;

    Кто, глядя на код, точно скажет, к какому if относится else? А это ли хотел сделать программист? А вы уверены?

    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:

    Немножечко попутали тип enum в switch. Но это ведь не страшно? Или страшно?

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

    Тот случай, когда empty() не делает empty. Пример крайне неудачного названия метода.

    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);
    }

    Еще две крайне похожие функции...

    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))
    {

    Или то, или то… Или не то? Наверное, здесь должно быть 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;
    }

    И опять empty() вовсе не очищает строку.

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

    При возникновении различных аварийных ситуаций WinMerge попробует сообщить об ошибках, но в некоторых случаях у него это плохо получится. На первый взгляд все хорошо. Вот только тип «String» есть не что иное как «std::wstring». А, следовательно, в лучшем случае мы распечатаем абракадабру, а в худшем произойдет ошибка доступа к памяти (Access Violation). Корректный код должен содержать вызов 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<srclen; k++)
      {
        if (src[k]=='>')
          break;
      }
      ...
    }

    Анализатор нашел подозрительный цикл. Этот код предрасположен к Access Violation. Цикл должен продолжаться пока не найдется символ '>' или не закончится строка длиной в 'srclen' символов. Вот только случайно для сравнения использована переменная 'i', а не 'k'. Если символ '>' найден не будет, то все вероятно закончится печально.

    XUIFramework


    Всего сообщений от статического анализатора Visual Studio – 93. Из них реальных ошибок – 2.

    Всего сообщений от PVS-Studio – 30. Из них реальных ошибок – 5.

    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++)

    Опять кто-то не умеет пользоваться *ptr++. Как говорилось выше, ошибка распространенная.

    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;

    Путают люди delete и delete[]. Это приводит к проблемам. Иногда проблемы проявляются, иногда нет. Но так делать не надо.

    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;

    Судя по коду, там должно быть m_ySt. Но от копи-паста ведь снова и снова не удержаться, да?

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

    Должно быть sizeof(szTemp) / sizeof(TCHAR) .

    PVS-Studio: V556 The values of different enum types are compared:
    enuHAlign == Center. cxstatic.cpp 151


    if (enuHAlign == Center)

    Должно быть enuHAlign == Midle. И в коде рядом есть еще if (enuVAlign == Middle), хотя должно быть Center. Попутали enum, короче.

    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)
      ...
    }

    Вероятно, код должен выглядеть так:

    HDWP CItemCtrl::OnSize(...)
    {
      ...
      if (m_styTop == CST_ZOOM ||
          m_styTop == CST_DELTA_ZOOM ||
          m_styBottom == CST_ZOOM ||
          m_styBottom == CST_DELTA_ZOOM)
      ...
    }

    Результаты сравнения



    Мы не делаем никаких конкретных выводов. В каких-то проектах лучше себя показал один инструмент, в каких-то – другой.

    Библиографический список


    1. Андрей Карпов, Евгений Рыжков. Трудности сравнения анализаторов кода или не забывайте об удобстве использования. http://www.viva64.com/ru/a/0071/.