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

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

В статье «Трудности сравнения анализаторов кода или не забывайте об удобстве использования» [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)
  ...
}

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

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

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

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

06.05.2011 15:54+0400

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