Андрей Карпов

28 Мар 2012

Теги:

#Cpp

Вторая проверка WinMerge с помощью PVS-Studio

28 Мар 2012

Автор: Андрей Карпов

Введение
Фрагменты очень подозрительного кода
Заключение
Библиографический список

Статья продолжает развивать тему, что статические анализаторы кода, это инструменты постоянного, а не разового применения.

Введение

Анализатор PVS-Studio позволяет выявлять ошибки в приложениях на языке Си/Си++. Однажды, с его помощью был проверен проект WinMerge. Ошибок было найдено немного и с их описанием можно познакомиться в статье "Сравнение статического анализа общего назначения из Visual Studio 2010 и PVS-Studio на примере обнаруженных ошибок в пяти открытых проектах" [1].

С того времени прошёл год и возникло желание проверить новую версию WinMerge, используя новую версию PVS-Studio. Далее будут приведены результаты этой повторной проверки. Но важнее то, какой из этой проверки можно сделать вывод:

Нет никакого смысла один раз проверить проект, используя инструмент статического анализа кода, и успокоиться на этом. Анализ должен выполняться регулярно.

И вот почему:

Каждая новая версия анализатора, как правило, содержит новые диагностические правила, а значит позволят выявить больше ошибок.
В процессе написания нового кода возникают новые ошибки. Наиболее дешёвым способом их обнаружения многих из них является использование статических анализаторов кода [2].

Вернемся теперь к найденным в коде дефектам. Заметим, что многие из описанных ошибок относятся не к самому проекту WinMerge, а к используемым в нем библиотекам. Однако, это не имеет значения. Хотелось показать, что анализатор PVS-Studio быстро развивается и учится выявлять всё новые разновидности дефектов. И это удается продемонстрировать на примерах.

Фрагменты очень подозрительного кода

Фрагмент N1

BOOL CCrystalEditView::
DoDropText (....)
{
  ...
  UINT cbData = (UINT) ::GlobalSize (hData);
  UINT cchText = cbData / sizeof(TCHAR) - 1;
  if (cchText < 0)
    return FALSE;
  ...
}

Диагностика анализатора PVS-Studio: V547 Expression 'cchText < 0' is always false. Unsigned type value is never < 0. Merge ccrystaleditview.cpp 1135

Функция GlobalSize() возвращает в случае ошибки значение 0. Если это произойдёт, данная ситуация будет обработана неправильно. Код построен с использованием беззнаковых типов данных. В том числе и переменная 'cchText' имеет тип 'unsigned'. Это значит, что условие "cchText < 0" всегда ложно. Код можно исправить, переписав его следующим образом:

UINT cbData = (UINT) ::GlobalSize (hData);
if (cbData < sizeof(TCHAR))
  return FALSE;
UINT cchText = cbData / sizeof(TCHAR) - 1;

Фрагмент N2

bool isopenbrace (TCHAR c)
{
  return c == _T ('{') || c == _T ('(') ||
         c == _T ('[') || c == _T ('<');
}

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

Диагностика анализатора PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '||' operator: c == L'}' || c == L'}' Merge ccrystaleditview.cpp 1556

В функции isclosebrace() переменная 'c' два раза сравнивается с символом '}'. Если изучить код функции isopenbrace(), то становится ясно, что во втором случае переменная 'c' должна сравниваться с символом ')'.

Фрагмент N3

static HRESULT safeInvokeA(....)
{
  HRESULT h;
  ...
  // set h to FAILED
  h = -1;
  ...
}

Диагностика анализатора PVS-Studio: V543 It is odd that value '-1' is assigned to the variable 'h' of HRESULT type. Merge plugins.cpp 992

Присваивать значение -1 переменной, имеющей тип HRESULT, это очень некрасиво и неправильно.

HRESULT - это 32-разрядное значение, разделенное на три различных поля: код серьезности ошибки, код устройства и код ошибки. Для работы со значением HRESULT служат специальные константы, такие как S_OK, E_FAIL, E_ABORT и так далее. А для проверки значений тип HRESULT предназначены такие макросы, как SUCCEEDED, FAILED.

Запись значения "-1" некорректна. Если хочется сообщить о какой-то непонятной ошибке, то следует использовать значение 0x80004005L (Unspecified failure). Эта и аналогичные константы, описаны в "WinError.h".

Аналогичная ошибка также имеется здесь:

V543 It is odd that value '-1' is assigned to the variable 'h' of HRESULT type. Merge plugins.cpp 1033

Фрагмент N4

int TimeSizeCompare::CompareFiles(....)
{
  UINT code = DIFFCODE::SAME;
  ...
  if (di.left.size != di.right.size)
  {
    code &= ~DIFFCODE::SAME;
    code = DIFFCODE::DIFF;
  }
  ...
}

Диагностика анализатора PVS-Studio: V519 The 'code' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 79, 80. Merge timesizecompare.cpp 80

Возможно, здесь есть ошибка, а возможно нет. Будучи не знакомым с внутренним устройством проекта WinMerge мне сложно делать выводы.

Варианты:

Код содержит ошибку и вторая строка должна выглядеть, например, так: "code |= DIFFCODE::DIFF;".
Код корректен. Первая строчка лишняя.

Фрагмент N5

BOOL CEditDropTargetImpl::
OnDrop (....)
{
  bool bDataSupported = false;

  m_pOwner->HideDropIndicator ();

  if ((!m_pOwner) ||
      (!(m_pOwner->QueryEditable ())) ||
      (m_pOwner->GetDisableDragAndDrop ()))
  ...
}

Диагностика анализатора PVS-Studio: V595 The 'm_pOwner' pointer was utilized before it was verified against nullptr. Check lines: 1033, 1035. Merge ccrystaleditview.cpp 1033

Как видно из условия "if ((!m_pOwner) ....)" указатель 'm_pOwner' может быть равен нулю. Однако, прежде чем проверка будет выполнена, этот указатель уже используется в высказывании 'm_pOwner->HideDropIndicator()'. Таким образом, вместо штатной обработки нулевого указателя возникнет ошибка сегментации.

Фрагмент N6

BCMenu *BCMenu::FindMenuOption(int nId, UINT& nLoc)
{
  ...
  nLoc = -1;
  ...
}

BOOL BCMenu::ModifyODMenuW(....)
{
  UINT nLoc;
  ...
  BCMenu *psubmenu = FindMenuOption(nID,nLoc);
  ...
  if (psubmenu && nLoc>=0)
    mdata = psubmenu->m_MenuList[nLoc];
  ...
}

Диагностика анализатора PVS-Studio: V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1232

При определённых обстоятельствах функция FindMenuOption() возвращает в переменной 'nLoc' значение -1. Так как переменная 'nLoc' имеет беззнаковый тип, то на самом деле функция вернёт 0xFFFFFFFFu.

Теперь рассмотрим код в функции ModifyODMenuW(). Условие "nLoc>=0" всегда истинно. Это значит, что ситуация, когда функция FindMenuOption() возвращает -1, будет обработана неверно.

Аналогичные ошибки:

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1263

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1285

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1309

V547 Expression 'loc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1561

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 2409

Фрагмент N7

В программе существует класс CompareOptions, который имеет виртуальные методы, но не имеет виртуального деструктора. От него наследуются другие классы, например, DiffutilsOptions. Соответственно, отсутствие виртуального деструктора, хотя может и не приводить к катастрофическому результату, тем не менее, является ошибкой.

Диагностика анализатора PVS-Studio: V599 The virtual destructor is not present, although the 'CompareOptions' class contains virtual functions. Merge diffcontext.cpp 90

Приводить соответствующие участки кода здесь нет смысла, так как они займут много места.

Следует отметить, что анализатор PVS-Studio достаточно аккуратен в диагностике и не ругается на все подряд классы, в которых отсутствует виртуальный деструктор. Понять, как анализатор диагностирует этот тип ошибки, можно познакомившись с её описанием: V599. The virtual destructor is not present, although the 'Foo' class contains virtual functions.

Фрагмент N8

static void StoreDiffData(....)
{
  ...
  GetLog()->Write
  (
    CLogFile::LCOMPAREDATA,
    _T("name=<%s>, leftdir=<%s>, rightdir=<%s>, code=%d"),
    di.left.filename.c_str(),
    di.left.path.c_str(),
    di.right.path.c_str(), di.diffcode
  );
  pCtxt->m_pCompareStats->AddItem(di.diffcode.diffcode);
  ...
}

Диагностика анализатора PVS-Studio: V510 The 'Write' function is not expected to receive class-type variable as sixth actual argument. Merge dirscan.cpp 565

Переменная 'di.diffcode' представляет собой структуру типа DIFFCODE. По всей видимости, корректный код должен был выглядеть так:

CLogFile::LCOMPAREDATA, _T(...., di.diffcode.diffcode);

Фрагмент N9

static DIFFITEM *AddToList(....,
 const DirItem * lent, const DirItem * rent,
 ....)
{
  ...
  if (lent)
  {
    ...
  }
  else
  {
    di->left.filename = rent->filename;
  }

  if (rent)
  {
  ...
}

Диагностика анализатора PVS-Studio: V595 The 'rent' pointer was utilized before it was verified against nullptr. Check lines: 608, 611. Merge dirscan.cpp 608

Указатель 'rent' используется без проверки на равенство нулю. Возможно на практике такая ситуация никогда не возникает. Тем не менее, проверка "if (rent)" намекает, что такое теоретически возможно.

Фрагмент N10

String FileFilterHelper::ParseExtensions(....) const
{
  String strParsed;
  String strPattern;
  ...
  strParsed = _T("^");
  strPattern = string_makelower(strPattern);
  strParsed = strPattern;
  ...
}

Диагностика анализатора PVS-Studio: V519 The 'strParsed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 339, 342. Merge filefilterhelper.cpp 342

Переменной 'strParsed' два раза подряд присваиваются различные значения. В этом коде присутствует ошибка или лишнее присваивании. Похожий случай уже рассматривался ранее.

Фрагмент N11

void CLogFile::EnableLogging(BOOL bEnable)
{
  ...
  Write(_T("Path: %s\n*******\n"), m_strLogPath);
  ...
}

Диагностика анализатора PVS-Studio: V510 The 'Write' function is not expected to receive class-type variable as second actual argument. Merge logfile.cpp 85

Переменная 'm_strLogPath' имеет тип std::wstring. Это значит, что в лог запишется мусор. Корректный вариант кода:

Write(_T("Path: %s\n*******\n"), m_strLogPath.c_str());

Фрагмент N12

void CMergeDoc::Computelinediff(
  CCrystalTextView * pView1, CCrystalTextView * pView2, 
  ....)
{
  ...
  if (pView1->GetTextBufferEol(line) !=
      pView1->GetTextBufferEol(line))
  ...
}

Диагностика анализатора PVS-Studio: V501 There are identical sub-expressions 'pView1->GetTextBufferEol(line)' to the left and to the right of the '!=' operator. Merge mergedoclinediffs.cpp 216

Два раза используется переменная 'pView1'. Скорее всего, в этом коде имеется опечатка, и корректный код должен был выглядеть так:

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

Фрагмент N13

void CSplashWnd::OnPaint()
{
  ...
  String text = LoadResString(IDS_SPLASH_DEVELOPERS);

  // avoid dereference of empty strings and
  // the NULL termiated character
  if (text.length() >= 0)
  {
  ...
}

Диагностика анализатора PVS-Studio: V547 Expression 'text.length() >= 0' is always true. Unsigned type value is always >= 0. Merge splash.cpp 262

Проверка "text.length() >= 0" не имеет смысла. Тип 'String' представляет из себя 'std::wstring'. Функция 'std::wstring::length()' всегда возвращает значение большее или равное 0.

Фрагмент N14

void CPreferencesDlg::AddPage(CPropertyPage* pPage, ....)
{
  ...
  m_tcPages.SetItemData(hti, (DWORD)pPage);
  ...
}

Диагностика анализатора PVS-Studio: V205 Explicit conversion of pointer type to 32-bit integer type: (DWORD) pPage Merge preferencesdlg.cpp 200

Теоретически (но вряд ли практически) в 64-битной программе, объект, на который указывает 'pPage', может находиться за пределами младших четырёх гигабайт. Это несет потенциальную опасность, так как указатель явно приводится к 32-битному типу 'DWORD'. Безопасный код должен выглядеть так:

m_tcPages.SetItemData(hti, (DWORD_PTR)pPage);

Заключение

Обнаружены и другие подозрительные места в коде. Но заявить о наличии в них ошибок я затрудняюсь. Главное, что прогресс в развитии анализатора PVS-Studio очевиден.

Если вы хотите скачать пробную полнофункциональную версию анализатора, то перейдите по этой ссылке: http://www.viva64.com/ru/pvs-studio/download/. С новой моделью триала вы можете получить пользу от анализатора без его покупки

Если у вас возникли вопросы касательно этой статьи или анализатора, то предлагаю познакомиться с заметкой "Ответы на вопросы, которые часто задают после прочтения наших статей" [3]. Также вы можете написать и задать вопросы лично мне и моим коллегам, используя форму обратной связи.

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

Евгений Рыжков. Сравнение статического анализа общего назначения из Visual Studio 2010 и PVS-Studio на примере обнаруженных ошибок в пяти открытых проектах. http://www.viva64.com/ru/a/0073/
Андрей Карпов. Лев Толстой и статический анализ кода. http://www.viva64.com/ru/b/0105/
Андрей Карпов. Ответы на вопросы, которые часто задают после прочтения наших статей. http://www.viva64.com/ru/b/0132/

#Cpp