Анализируем исходный код с помощью cppcheck

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

Совсем недавно cppcheck помог найти уязвимость в проекте Xorg, которая существовала там почти 23 года! Он помог уже тысячам программистов по всему миру, на официальном сайте можно найти информацию о найденных с помощью cppcheck уязвимостях в программах, и этот список постоянно растёт. Итак, если вы хотите знать, почему нужно использовать cppcheck всегда и везде — прошу под кат.

CppCat и cppcheck

Начну со сравнения этих утилит, поскольку данная просьба озвучивалась неоднократно в комментариях. Разработчики CppCat уже самостоятельно провели такое сравнение (с PVS-Studio), но с тех пор утекло много воды, а сравнение не очень объективно, так как PVS-Studio (насколько я понимаю всю замысловатость фразы «Please write us to get a price for PVS-Studio. Please specify interesting license type.») не предназначена для программистов-одиночек. CppCat же, как и cppcheck, доступен каждому (с оговоркой на привязку к VisualStudio определённых версий и лицензию на год).

Сравнить эти анализаторы будет непросто: у меня не имеется под руками ни одной версии Visual Studio под Linux. Поэтому сначала я ограничусь анализом уже проанализированного кода в недавнем обзоре CppCat: натравлю cppcheck на Notepad++, приведу статистику ошибок/предупреждений, которые можно будет сравнить с уже готовым анализом CppCat.

Параллельно пробую поставить в виртуальной машине CppCat. Надо сказать, что после того как Visual Studio 2010 был установлен, инсталлятор недолго думая выдал следующее:

Из-за чего тестирование усложнилось квестом найди-поставь-Visual-Studio-2013-переустанови-IE-11-перезагрузись-обновись, что на виртуалке, не обременённой обновлениями, заняло ровно полдня.

Что в итоге? При открытии проекта Notepad++ Visual Studio гордо завис. Попытка создать новый проект привела к тому, что в анализе CppCat посыпались ошибки о не найденных заголовочных файлах. По сему сравнивать придётся с тем, что имеем в предыдущей статье. Хотя Visual Studio я ставлю практически в первый раз, но эффект «юзабилити» налицо.

Подготовка

Так как cppcheck — проект с открытым исходным кодом, никто не мешает загрузить самую свежую версию из гита и скомпилировать её самостоятельно. Cppcheck предназначен для разработчиков, поэтому компиляция программы из исходного кода не должна вызывать каких-либо проблем:

unzip cppcheck-master.zip
cd cppcheck-master
make

Готово. Я специально взял свежую версию из гита и положил её в отдельную папку — с ней будет проще работать, так как в дальнейшем cppcheck можно сильно улучшить и настроить «под себя».

Тестовая конфигурация: RHEL 6.1, процессор i5-2400 @ 3.10GHz (для оценки времени работы анализатора).

Удобства ради действия будут производится в командной строке — при желании их можно будет повторить (в Linux:). Конечно, cppcheck имеет несколько плагинов для популярных IDE, но сегодня не об этом.

Анализ Notepad++

cppcheck устроен так, что все предупреждения отсортированы по категориям. По умолчанию включён только один вид анализа — error (ошибки). Ошибки нельзя игнорировать, так как если cppcheck выдал error — это место придётся переписывать в 99% случаев. Основной улов cppcheck — утечки памяти и переполнение буфера, а это уже чего-то стоит.

Может возникнуть резонный вопрос — как анализировать notepad++ в операционной системе Linux, если notepad++ использует исключительно WInAPI? Ответ прост — cppcheck на моей памяти единственный анализатор, который не привязан к сборочной среде или операционной системе. Он использует свой лексический анализатор, не требует обязательного присутствия всех заголовочных файлов, лояльно относится к хитросплетениям классов и т. п. Это замечательное свойство позволяет использовать cppcheck где угодно и для чего угодно, чего не скажешь о CppCat (см. квест по установке выше).

Анализ с помощью cppcheck прост до безобразия:

./cppcheck-master/cppcheck -q -j4 npp.6.5.3.src/

Пока проводится простейший анализ «из коробки». Команда определяет два параметра -q («тихий» режим — не выводить прогресс выполнения на экран) и -j4 — многопоточный анализ в 4 потока по количесву ядер процессора.

Результат выполнения предыдущей команды:

[npp.6.5.3.src/PowerEditor/src/tools/ChangeIcon/ChangeIcon.cpp:214]: (error) Mismatching allocation and deallocation: resData
[npp.6.5.3.src/PowerEditor/src/tools/ChangeIcon/ChangeIcon.cpp:216]: (error) Mismatching allocation and deallocation: resData
[npp.6.5.3.src/scintilla/lexers/LexBash.cxx] -> [npp.6.5.3.src/scintilla/lexers/LexBash.cxx:194]: (error) Internal error. Token::Match called with varid 0. Please report this to Cppcheck developers

Время работы — 5 минут. В глаза сразу бросается ошибка "(error) Internal error. Token::Match called with varid 0. Please report this to Cppcheck developers". Это означает, что вместо бага в анализируемой программе нашёлся баг в самом анализаторе:) Сделаем скидку на то, что проект заточен под Win, и cppcheck не подозревает, что означают DWORD, LPTR и т. п. Возможно, в Win он покажет себя иначе.

Реально нашлась всего одна ошибка (с разницей в 2 строчки). Неплохо, возможно, что автор notepad++ сам пользуется cppcheck. Участок кода, который вызвал у cppcheck подозрение:

	BYTE* resData = new BYTE[cbRes];
	LPBYTE writePtr = resData;
...
	if(!UpdateResource(hUpdate, RT_GROUP_ICON, lpResName, resLangId, resData, cbRes)) { _tprintf(_T("Unable to update icon groupn")); delete resData; return false; }
	IFDEBUG( _tprintf(_T("Updated group %d (lang %d)n"), lpResName, resLangId); )
	delete resData;
	}

Это выглядит как ложное срабатывание, хотя исходники, честно говоря, шокируют.

Таким образом, cppcheck не нашёл ни одной ошибки, но это ещё не всё. Дело в том, что девизом cppcheck является отсутствие ложных срабатываний, то есть по умолчанию сканер ищет только очень критические ошибки — переполнение буфера, утечки памяти. После того как все ошибки найдены, можно просканировать повторно, включив флаги предупреждений:

./cppcheck-master/cppcheck -q -j4 --enable=performance,portability,warning,style npp.6.5.3.src/ 2> npp.out

Используется параметр --enable, который включает категории проверок:

— performance — проблемы производительности;
— portability — проблемы совместимости;
— warning — предупреждения — подозрительные места программы;
— style — ошибки стиля программирования.

В таком режиме вылавливается львиная доля стилистических/логических ошибок и потенциальных багов (т. е. ошибки, в которых cppcheck «не уверен»). Время сканирования — 5 минут. Результат я сразу отправил в файл, чтобы собрать статистику.

Всего сообщений:

wc -l < npp.out
379

Небольшая статистика по типу найденных ошибок:

tr '()' '*' < npp.out | cut -d* -f2 | sort | uniq -c
      3 error
     39 performance
     14 portability
    211 style
    112 warning

sort -t] -k2 npp.out | grep -v '(error)' | cut -d) -f2- | sed "s/'[^']*'/%{VAR}/g" | sort | uniq -c | sort -n
      1  Function parameter %{VAR} should be passed by reference.
      1  memset() called to fill 0 bytes of %{VAR}.
      1  scanf without field width limits can crash with huge input data.
      1  The class %{VAR} does not have a constructor.
      1  Unused variable: ent
      1  Unused variable: loc
      2  Array index %{VAR} is used before limits check.
      2  Checking if unsigned variable %{VAR} is less than zero.
      2  Found duplicate branches for %{VAR} and %{VAR}.
      2  The class %{VAR} defines member variable with name %{VAR} also defined in its parent class %{VAR}.
      2  Unsigned variable %{VAR} can't be negative so it is unnecessary to test it.
      2  %{VAR} should return %{VAR}.
      3  scanf without field width limits can crash with huge input data on some versions of libc.
      4  Same expression on both sides of %{VAR}.
      4  %{VAR} does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.
      5  Assignment of function parameter has no effect outside the function.
      5  Ineffective call of function %{VAR}. Did you intend to call %{VAR} instead?
      7  Consecutive return, break, continue, goto or throw statements are unnecessary.
     11  Exception should be caught by reference.
     11  The extra qualification %{VAR} is unnecessary and is considered an error by many compilers.
     12  Variable %{VAR} is reassigned a value before the old one has been used.
     22  Variable %{VAR} is assigned a value that is never used.
     26  Variable %{VAR} is assigned in constructor body. Consider performing initialization in initialization list.
     42  C-style pointer casting
     98  Member variable %{VAR} is not initialized in the constructor.
    108  The scope of the variable %{VAR} can be reduced.

Всего 28 уникальных сообщений.

Сообщения «The scope of the variable %{VAR} can be reduced», «C-style pointer casting», «Variable %{VAR} is assigned in constructor body.» можно не рассматривать — это стилистические рекомендации, очень характерные для многих проектов, написанных на старых плюсах.

Море переменных не инициализировано в конструкторе: (warning) Member variable %{VAR} is not initialized in the constructor. Это ошибку cppcheck считает предупреждением. Возможно, поведение такого кода зависит от компилятора, потому что npp каким-то чудом работает.

//[npp.6.5.3.src/PowerEditor/src/WinControls/AnsiCharPanel/ansiCharPanel.h:46]: (warning) Member variable 'AnsiCharPanel::_ppEditView' is not initialized in the constructor.
class AnsiCharPanel : public DockingDlgInterface {
public:
	AnsiCharPanel(): DockingDlgInterface(IDD_ANSIASCII_PANEL) {};

	void init(HINSTANCE hInst, HWND hPere, ScintillaEditView **ppEditView) {
		DockingDlgInterface::init(hInst, hPere);
		_ppEditView = ppEditView;
	};

    virtual void display(bool toShow = true) const {
        DockingDlgInterface::display(toShow);
    };

    void setParent(HWND parent2set){
        _hParent = parent2set;
    };

	void switchEncoding();
	void insertChar(unsigned char char2insert) const;

protected:
	virtual BOOL CALLBACK AnsiCharPanel::run_dlgProc(UINT message, WPARAM wParam, LPARAM lParam);

private:
	ScintillaEditView **_ppEditView;
	ListView _listView;
};

(style) Same expression on both sides of '||'. Проверка одного и того же условия. Эту же ошибку выдавал CppCat, однако то ли в той статье версия npp старая, то ли ошибку уже пофиксили, но сейчас тот самый код выглядит так:

while (closeFound.success && (styleAt == SCE_H_DOUBLESTRING || styleAt == SCE_H_SINGLESTRING) && searchStartPoint <= caret);

А это — новая добыча cppcheck:

if (!(!commentLineSybol || !commentLineSybol[0] || commentLineSybol == NULL))

(performance) Variable 'lineIndent' is reassigned a value before the old one has been used. По сути — двойное присваивание. Обычно это следствие копипасты, но cppckeck характеризует такую ошибку как ошибку производительности. Этот код стоит проверить, так как неизвестно, что подразумевал автор программы. Таких двойных присваиваний, а также неиспользуемых значений переменных по коду очень много:

        int lineIndent = lineStart;
...
        lineIndent = _pEditView->execute(SCI_GETLINEINDENTPOSITION, i);
		_pEditView->getGenericText(linebuf, linebufferSize, lineIndent, lineEnd);

Это предупреждение обычно бесполезно — редко, когда в данном участке кода есть ошибка, просто при рефакторинге забыли удалить старое значение.

(portability) The extra qualification 'FunctionListPanel::' is unnecessary and is considered an error by many compilers. Полезное предупреждение, которого физически нет и не планируется в CppCat: ошибки переносимости между разными платформами (portability). Данный кусок кода будет работать не во всех компиляторах:

virtual BOOL CALLBACK FunctionListPanel::run_dlgProc(UINT message, WPARAM wParam, LPARAM lParam);

Это «политкорректное» сообщение на самом деле означает, что код не соберётся без костылей в компиляторе gcc. Если вы планируете запускать приложение больше чем на одной платформе — cppcheck станет хорошим подспорьем.

(style) Exception should be caught by reference. Интересное предупреждение — ловить исключение следует по ссылке, а не по значению:

        catch(std::exception e) {
		::MessageBoxA(NULL, e.what(), "Exception", MB_OK);
		return -1;
	}

(style) Consecutive return, break, continue, goto or throw statements are unnecessary. Мёртвый код: break после return:

		switch(lpnm->wID) {
			case REBAR_BAR_TOOLBAR: {
...
				return TRUE;
				break; }
		}

(warning) Assignment of function parameter has no effect outside the function. Обычно это полезное предупреждение, сигнализирующее об опечатке — значение, присвоенное внутри функции, никуда не передаётся. Однако здесь это очевидно ложное срабатывание, так как value — переменная класса:

void SetValue( const TCHAR* _value )	{ value = _value; }

(portability) scanf without field width limits can crash with huge input data on some versions of libc. Подобная привычка использования scanf может привести к опасному переполнению буфера. В случае числовых переменных это банальный undefined behaviour:

if ( sscanf( value.c_str(), "%d", ival ) == 1 )

Для преобразования чисел лучше использовать безопасный strtol.

Ещё один представитель:

sscanf( wordBuffer, "%[^.<>|&=\/]", sKeywordBuffer );

Здесь нет переполнения буфера только потому, что wordBuffer и sKeywordBuffer одинакового размера.

(style) 'TiXmlStringA::operator=' should return 'TiXmlStringA &'. Оператор = возвращает void:

void operator = (const TiXmlStringA & copy);

С таким оператором нельзя использовать стандартную для C++ цепочку:

a = b = c;

(warning) The class 'ControlsTab' defines member variable with name '_isVertical' also defined in its parent class 'TabBar'. Ошибка двойного определения переменной в классе:

class ControlsTab : public TabBar
{
public :
...
private :
...
    bool _isVertical;
};

которая уже определена в родительском классе:

class TabBar : public Window
{
...
protected:
...
	bool _isVertical;
};

Не являясь экспертом в плюсах, не могу сразу ответить, можно ли так делать (protected/private).

(style) Found duplicate branches for 'if' and 'else'. Аналогичные ошибки нашёл и CppCat. Лишнее условие:

		if(eol_mode == SC_EOL_CRLF)
			extraEOLLength = 2;
		else if(eol_mode == SC_EOL_LF)
			extraEOLLength = 1;
		else // SC_EOL_CR
			extraEOLLength = 1;

(style) Checking if unsigned variable 'lenFile' is less than zero. Аналогичное сообщение выдавал CppCat за исключением того, что cppcheck, не обнаружив файла windows.h не стал строить гипотезы относительно типов вроде WPARAM. Недочёт неориентированности исключительно на Windows всё же есть.

		size_t lenFile = 0;
...
			if (lenFile <= 0) break;

Думаю, если бы у меня были заголовочные файлы винды, можно указать путь к ним через параметр -I, тогда ошибок будет значительно больше.

(style) Array index 'j' is used before limits check. Несмотря на то что предупреждение низкоприоритетное, найденная ошибка представляет опасность выхода за границы массива:

     int j;
     int ReturnValue;
     j=startcol;
     if(direction == 1){j++;}
     if(direction != 1){j--;}

     while((BGHS[SI].columnwidths[j] == 0)&&(j<=BGHS[SI].cols)&&(j>0))

Учитывая, что параметр startcol внешний, можно вылететь за границу массива, не говоря об индексе -1.

Мелкие недочёты

(style) Unused variable: ent. Это предупреждение умеют выдавать и компиляторы, ничего интересного.

Класс без конструктора:

	class CachedValue
	{
		generic_string fullname;
		int index;
	};

(performance) Function parameter 'range' should be passed by reference. cppcheck рекомендует передавать параметр по ссылке, чтобы избежать копирования аргумента:

XYScrollPosition XYScrollToMakeVisible(const SelectionRange range, const XYScrollOptions options);

(warning) Ineffective call of function 'empty()'. Did you intend to call 'clear()' instead? Метод empty имеет смысл только внутри условия и не очищает строку. Это ложное срабатывание, cppcheck не подозревал, что автор сделает свой класс String:) Следует проверить логику именования методов.

(style) 'class ByteArray' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory. cppcheck просто рекомендует создать в классе отсутствующий конструктор копирования на случай, если программист забыл его реализовать.

(style) Unsigned variable 'i' can't be negative so it is unnecessary to test it. Довольно мелкая придирка:

for(unsigned int i = position_of_click; i >= 0; --i)

Вывод

Оба анализатора наковыряли приличное количество ошибок, причём многие из них уникальны для каждого анализатора. В целом, было бы неплохо иметь cppcheck под рукой всегда, так как он открыт, кроссплатформенный, реально находит ошибки и помогает улучшать стиль программирования. Использование cppcheck обычно не представляет проблем.

Из этого анализа можно сделать вывод, что инструменты хорошо друг друга дополняют. Для кого-то глаынм минусом cppcheck является отсутствие плагина под Visual Studio, поэтому авторы cppcheck любезно предлагают попробовать PVS. Несмотря на интерфейс командной строки, пользоваться cppcheck очень удобно, кроме того, я специально поставил в виртуальной машине сборку cppcheck для Windows — она имеет приятный графический интерфейс, устанавливается быстро и без проблем выполняет анализ:

Результат анализа можно экспортировать в XML и смотреть в браузере.

Обоим проектам стоит пожелать успехов в развитии — это действительно очень нужные программы. А в развитии cppcheck вы можете принять участие самостоятельно прямо сейчас: проверяйте свои проекты, пишите разработчику cppcheck о найденных или не найденных ошибках, сообщайте о багах, присылайте полезные патчи на гитхаб. Если ещё недавно cppcheck не мог найти ошибку if(malloc()), то сейчас просто сыпет сообщениями об утечках памяти — результат конкуренции налицо.

Данный анализ можно было бы сильно улучшить, если явно указать cppcheck, где искать заголовочные файлы, какие функции выделяют и освобождают память. Так как статья получилась большой, про то, как настроить cppcheck под конкретный проект, улучшить качество анализа и писать свои правила для cppcheck — в следующий раз.

Автор: RPG

Источник