Прошло более года, как мы проверили Notepad++ с помощью PVS-Studio. Интересно посмотреть, насколько анализатор PVS-Studio стал лучше, и что было исправлено в Notepad++ из прежних ошибок.
Итак, мы проверили проект Notepad++ взятый из репозитория 31 января 2012. Для проверки использовался анализатор PVS-Studio версии 4.54.
Как уже было сказано, мы ранее проверяли этот проект. Ошибок нашли не много, но всё-таки что-то нашли. В новой версии проекта часть старых ошибок исправлена, а часть нет. Это странно. По всей видимости, прежняя заметка осталась незамеченной авторами Notepad++ и они не воспользовались PVS-Studio для проверки проекта. Возможно эта заметка все-таки привлечет авторов Notepad++, тем более что мы недавно изменили режим триала и все найденные с помощью PVS-Studio ошибки видны.
Исправленные ошибки, видимо были найдены другими методами тестирования или о них сообщили пользователи.
Например, исправлена ошибка, описанная в предыдущей статье, которая касалась заполнения массива _iContMap. Было так:
memset(_iContMap, -1, CONT_MAP_MAX);Исправленный вариант:
memset(_iContMap, -1, CONT_MAP_MAX * sizeof(int));Зато вот эта ошибка, продолжает жить и здравствовать:
bool isPointValid() {
return _isPointXValid && _isPointXValid;
};Диагностическое сообщение анализатора PVS-Studio:
V501 There are identical sub-expressions to the left and to the right of the '&&' operator: _isPointXValid && _isPointXValid Notepad++ parameters.h 166
Мы не будем возвращаться к тем ошибкам, которые были описаны в предыдущей статье. Рассмотрим то новое, что научился диагностировать анализатор PVS-Studio за прошедшее время:
Как всегда хотим напомнить, что это далеко не все найденные недочёты, а то только те, которые показались нам интересными для написания статьи. Не забывайте про новый режим триала, который как нельзя лучше подходит для проверки в том числе и opensource-проектов.
int encodings[] = {
1250,
1251,
1252,
....
};
BOOL CALLBACK DefaultNewDocDlg::run_dlgProc(
UINT Message, WPARAM wParam, LPARAM)
{
...
for (int i = 0 ; i <= sizeof(encodings)/sizeof(int) ; i++)
{
int cmdID = em->getIndexFromEncoding(encodings[i]);
...
}Диагностическое сообщение анализатора PVS-Studio:
V557 Array overrun is possible. The value of 'i' index could reach 46. Notepad++ preferencedlg.cpp 984
В цикле перебираются элементы массива "encodings". Ошибка заключается в неправильном условии остановки цикла. При последней итерации цикла происходит доступ к памяти за границей массива. Ошибку можно исправить, заменив условие "<=" на "<". Корректный код:
for (int i = 0 ; i < sizeof(encodings)/sizeof(int) ; i++)typedef struct tagTVITEMA {
...
LPSTR pszText;
...
} TVITEMA, *LPTVITEMA;
#define TVITEM TVITEMA
HTREEITEM TreeView::addItem(...)
{
TVITEM tvi;
...
tvi.cchTextMax =
sizeof(tvi.pszText)/sizeof(tvi.pszText[0]);
...
}Диагностическое сообщение анализатора PVS-Studio:
V514 Dividing sizeof a pointer 'sizeof (tvi.pszText)' by another value. There is a probability of logical error presence. Notepad++ treeview.cpp 88
Размер буфера вычисляется с помощью выражения "sizeof(tvi.pszText)/sizeof(tvi.pszText[0])". Это выражение не имеет смысла. В нём размер указателя делится на размер одного символа. Как исправить код сказать сложно, так как мы не знакомы с логикой работы программы.
size_t Printer::doPrint(bool justDoIt)
{
...
TCHAR headerM[headerSize] = TEXT("");
...
if (headerM != '\0')
...
}Диагностическое сообщение анализатора PVS-Studio:
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *headerM != '\0'. Notepad++ printer.cpp 380
Указатель сравнивается с нулевым значением. Ноль задан литералом вида '\0'. Это подсказывает нам, что здесь забыто разыменование указателя. Корректный код:
if (*headerM != '\0')Аналогичная ошибка допущена в другом месте:
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *headerR != '\0'. Notepad++ printer.cpp 392
DWORD WINAPI Notepad_plus::threadTextPlayer(void *params)
{
...
const char *text2display = ...;
...
if (text2display[i] == ' ' && text2display[i] == '.')
...
}Диагностическое сообщение анализатора PVS-Studio:
V547 Expression is always false. Probably the '||' operator should be used here. Notepad++ notepad_plus.cpp 4967
Условие (text2display[i] == ' ' && text2display[i] == '.') никогда не выполняется. Символ не может одновременно являться и пробелом и точкой. Видимо здесь мы имеем дело с простой опечаткой и код должен был выглядеть следующим образом:
if (text2display[i] == ' ' || text2display[i] == '.')Аналогичная ошибка допущена в другом месте:
V547 Expression is always false. Probably the '||' operator should be used here. Notepad++ notepad_plus.cpp 5032
int Notepad_plus::getHtmlXmlEncoding(....) const
{
...
if (langT != L_XML && langT != L_HTML && langT == L_PHP)
return -1;
...
}Диагностическое сообщение анализатора PVS-Studio:
V590 Consider inspecting this expression. The expression is excessive or contains a misprint. Notepad++ notepad_plus.cpp 853
Имеющуюся в коде проверку можно упростить. Тогда код будет выглядеть следующим образом:
if (langT == L_PHP)Это явно не то, что хотел программист. Видимо здесь мы опять имеем дело с опечаткой. Корректный код:
if (langT != L_XML && langT != L_HTML && langT != L_PHP)TCHAR GetASCII(WPARAM wParam, LPARAM lParam)
{
...
result=ToAscii(wParam,(lParam >> 16) && 0xff,
keys,&dwReturnedValue,0);
...
}Диагностическое сообщение анализатора PVS-Studio:
V560 A part of conditional expression is always true: 0xff. Notepad++ babygrid.cpp 694
В коде хотят извлечь третий байт из переменной 'lParam'. Из-за опечатки, код делает совсем не это. Ошибка заключается в том, что используется оператор '&&' вместо '&'. Корректный код:
result=ToAscii(wParam,(lParam >> 16) & 0xff,
keys,&dwReturnedValue,0);#define SCE_T3_BRACE 20
static inline bool IsAnOperator(const int style) {
return style == SCE_T3_OPERATOR || SCE_T3_BRACE;
}Диагностическое сообщение анализатора PVS-Studio:
V560 A part of conditional expression is always true: 20. lextads3.cxx 700
Функция IsAnOperator() всегда возвращает 'true'. Корректный код:
return style == SCE_T3_OPERATOR ||
style == SCE_T3_BRACE;static void ColouriseVHDLDoc(....)
{
...
} else if (sc.Match('-', '-')) {
sc.SetState(SCE_VHDL_COMMENT);
sc.Forward();
} else if (sc.Match('-', '-')) {
if (sc.Match("--!"))
sc.SetState(SCE_VHDL_COMMENTLINEBANG);
else
sc.SetState(SCE_VHDL_COMMENT);
}
...
}Диагностическое сообщение анализатора PVS-Studio:
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 130, 133. lexvhdl.cxx 130
Если первое условие (sc.Match('-', '-')) истинно, то вторая проверка выполнена не будет. Это приведет к тому, что случай последовательность символов "--!" никогда не будет обработана правильно. По всей видимости, здесь часть кода лишняя и должно быть написано так:
static void ColouriseVHDLDoc(....)
{
...
} else if (sc.Match('-', '-')) {
if (sc.Match("--!"))
sc.SetState(SCE_VHDL_COMMENTLINEBANG);
else
sc.SetState(SCE_VHDL_COMMENT);
}
...
}Есть фрагменты кода, которые не приводят к проблемам, но являются избыточными. Приведем два примера такого типа:
void Gripper::doTabReordering(POINT pt)
{
...
else if (_hTab == hTabOld)
{
/* delete item on switch between tabs */
::SendMessage(_hTab, TCM_DELETEITEM, iItemOld, 0);
}
else
{
if (_hTab == hTabOld)
{
/* delete item on switch between tabs */
::SendMessage(_hTab, TCM_DELETEITEM, iItemOld, 0);
}
}
...
}Диагностическое сообщение анализатора PVS-Studio:
V571 Recurring check. The 'if (_hTab == hTabOld)' condition was already verified in line 478. Notepad++ gripper.cpp 485
Вторая часть кода не имеет никакого смысла и может быть удалена.
А вот другой пример, где присутствуют лишние проверк. Указатели 'mainVerStr' и 'auxVerStr' всегда не равны нулю, так как это массивы, созданные на стеке:
LRESULT Notepad_plus::process(....)
{
...
TCHAR mainVerStr[16];
TCHAR auxVerStr[16];
...
if (mainVerStr)
mainVer = generic_atoi(mainVerStr);
if (auxVerStr)
auxVer = generic_atoi(auxVerStr);
...
}
Здесь можно написать проще:
mainVer = generic_atoi(mainVerStr);
auxVer = generic_atoi(auxVerStr);Проверки, показанные выше, встречаются в проекте Notepad++ неоднократно:
V600 Consider inspecting the condition. The 'mainVerStr' pointer is always not equal to NULL. Notepad++ nppbigswitch.cpp 938
V600 Consider inspecting the condition. The 'auxVerStr' pointer is always not equal to NULL. Notepad++ nppbigswitch.cpp 940
V600 Consider inspecting the condition. The 'intStr' pointer is always not equal to NULL. Notepad++ preferencedlg.cpp 1871
V600 Consider inspecting the condition. The 'intStr' pointer is always not equal to NULL. Notepad++ userdefinedialog.cpp 222
V600 Consider inspecting the condition. The 'intStr' pointer is always not equal to NULL. Notepad++ wordstyledlg.cpp 539
Статические анализаторы кода, это не те инструменты, которые можно использовать время от времени. Используя их регулярно можно быстро находить ошибки, тем самым в десятки раз сокращая цену их устранения.
Зачем подолгу искать странное поведение программы, из-за неочищенного массива? Код "memset(_iContMap, -1, CONT_MAP_MAX)" легко и быстро находится статическим анализатором.
Если эта ошибка найдена всё же статическим анализатором PVS-Studio, то всё равно инструмент был использован неправильно. Во-первых, другие диагностические сообщения были изучены не внимательно. Во-вторых, использовать статический анализ нужно регулярно. Это позволяет быстро устранить ошибки в только что написанном коде. Плюс в PVS-Studio постоянно появляются новые диагностические правила.
Ещё раз хочется подчеркнуть мысль, что статический анализ это инструмент для регулярного использования. Это как расширение списка warnings, выдаваемых компилятором. Вы работаете с отключенными warnings и включаете их только изредка, когда есть настроение? Нет. Аналогично нужно относиться и к диагностическим предупреждениям, выдаваемым инструментами статического анализа.
Как это сделать? Можно запускать анализ ночью на сервере. Можно использовать инкрементальный анализ, чтобы проверять только что модифицированные файлы. Если кажется, что анализ выполняется медленно и отнимает много ресурсов, то предлагаем ознакомиться с советами по повышению скорости работы.