Прошло более года, как мы проверили 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-проектов.
Новые найденные ошибки
Ошибка N1. Выход за границы массива
int encodings[] = {
1250,
1251,
1252,
....
};
BOOL CALLBACK DefaultNewDocDlg::run_dlgProc(
UINT Message, WPARAM wParam, LPARAM)
{
...
for (int i = 0 ; i 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 > 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);
Ошибка N7. Недописанный код
#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;
Ошибка N8. Код, который никогда не будет выполнен
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);
}
...
}
Ошибка N9. Лишний код
Есть фрагменты кода, которые не приводят к проблемам, но являются избыточными. Приведем два примера такого типа:
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 и включаете их только изредка, когда есть настроение? Нет. Аналогично нужно относиться и к диагностическим предупреждениям, выдаваемым инструментами статического анализа.
Как это сделать? Можно запускать анализ ночью на сервере. Можно использовать инкрементальный анализ, чтобы проверять только что модифицированные файлы. Если кажется, что анализ выполняется медленно и отнимает много ресурсов, то предлагаем ознакомиться с советами по повышению скорости работы.
