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

Thunderbolt

Дата 28.3.2012, 10:11 (ссылка)

(нет голосов)

Загрузка ...

DevRel

Профиль
Группа: Участник
Сообщений: 122
Регистрация: 7.11.2007
Где: Тула

Репутация: 10
Всего: 16

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

Введение

Анализатор 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/

--------------------

Карпов Андрей, DevRel в PVS-Studio.

borisbn

Дата 28.3.2012, 16:10 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Завсегдатай
Сообщений: 4875
Регистрация: 6.2.2010
Где: Ростов-на-Дону

Репутация: 22
Всего: 135

Цитата(Thunderbolt @ 28.3.2012, 10:11

)

А вот тут просто MSVC отстал... Такую диагностику и MinGW делает

Код


    std::string str = "world";
    printf( "Hello %s", str );
    printf( "Hello %d", str.c_str() );

Цитата

error: cannot pass objects of non-trivially-copyable type 'std::string {aka struct std::basic_string<char>}' through '...'
warning: format '%s' expects argument of type 'char*', but argument 2 has type 'std::string {aka std::basic_string<char>}' [-Wformat]
warning: format '%d' expects argument of type 'int', but argument 2 has type 'const char*' [-Wformat]

http://liveworkspace.org/code/84d87da38966...9ccb6bf417301b0

хотя, это нисколько не уменьшает ценности PVS для MSVC

--------------------

Женщины отличаются от программистов тем, что у них чары состоят из стрингов

boostcoder

Дата 28.3.2012, 20:40 (ссылка)

(нет голосов)

Загрузка ...

pattern`щик

Профиль
Группа: Завсегдатай
Сообщений: 5458
Регистрация: 1.4.2010

Репутация: 49
Всего: 110

Цитата(borisbn @ 28.3.2012, 16:10

)

это нисколько не уменьшает ценности PVS для MSVC

в точку!

--------------------

компилятор онлайн с поддержкой C++11
сборки компиляторов MinGW

1 Пользователей читают эту тему (1 Гостей и 0 Скрытых Пользователей)
0 Пользователей:
« Предыдущая тема \| C/C++: Общие вопросы \| Следующая тема »