Статья продолжает развивать тему, что статические анализаторы кода, это инструменты постоянного, а не разового применения.
Введение
Анализатор http://www.viva64.com/ru/pvs-studio/ позволяет выявлять ошибки в приложениях на языке Си/Си++. Однажды, с его помощью был проверен проект http://winmerge.org/. Ошибок было найдено немного и с их описанием можно познакомиться в статье "Сравнение статического анализа общего назначения из Visual Studio 2010 и PVS-Studio на примере обнаруженных ошибок в пяти открытых проектах" [http://www.viva64.com/ru/a/0073/].
С того времени прошёл год и возникло желание проверить новую версию WinMerge, используя новую версию PVS-Studio. Далее будут приведены результаты этой повторной проверки. Но важнее то, какой из этой проверки можно сделать вывод:
Нет никакого смысла один раз проверить проект, используя инструмент статического анализа кода, и успокоиться на этом. Анализ должен выполняться регулярно.
И вот почему:
- Каждая новая версия анализатора, как правило, содержит новые диагностические правила, а значит позволят выявить больше ошибок.
- В процессе написания нового кода возникают новые ошибки. Наиболее дешёвым способом их обнаружения многих из них является использование статических анализаторов кода [http://www.viva64.com/ru/b/0105/].
Вернемся теперь к найденным в коде дефектам. Заметим, что многие из описанных ошибок относятся не к самому проекту 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()'. Таким образом, вместо штатной обработки нулевого указателя возникнет http://www.viva64.com/ru/t/0063/.
Фрагмент 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 достаточно аккуратен в диагностике и не ругается на все подряд классы, в которых отсутствует виртуальный деструктор. Понять, как анализатор диагностирует этот тип ошибки, можно познакомившись с её описанием: http://www.viva64.com/ru/d/0210/.
Фрагмент 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/. С новой моделью триала вы можете получить пользу от анализатора без его покупки
Если у вас возникли вопросы касательно этой статьи или анализатора, то предлагаю познакомиться с заметкой "Ответы на вопросы, которые часто задают после прочтения наших статей" [http://www.viva64.com/ru/b/0132/]. Также вы можете написать и задать вопросы лично мне и моим коллегам, используя http://www.viva64.com/ru/about-feedback/.
Библиографический список
- Евгений Рыжков. Сравнение статического анализа общего назначения из 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/
|
)