Андрей Карпов считает, что код проекта Manticore качественнее, чем код проекта Sphinx

Вначале давайте познакомимся с проектами Sphinx и Manticore.

Sphinx — система полнотекстового поиска, разработанная Андреем Аксёновым и распространяемая по лицензии GNU GPL. Отличительной особенностью является высокая скорость индексации и поиска, а также интеграция с существующими СУБД и API для распространённых языков веб-программирования.

Исходный код я взял отсюда. Размер проекта, если брать код на языке C и С++ и не включать сторонние библиотеки — 156 KLOC. Комментарии составляют 10.2%. Это значит, что «чистого кода» — 144 KLOC.

Manticore — это форк Sphinx. Начиная в качестве ключевых членов первоначальной команды Sphinx, команда Manticore установила для себя следующую цель – поставлять быстрое, стабильное и мощное свободное обеспечение по полнотекстовому поиску.

Исходный код я взял отсюда. Размер проекта, если брать код на языке C и С++ и не включать сторонние библиотеки — 170 KLOC. Комментарии составляют 10.1%. Это значит, что «чистого кода» — 152 KLOC.

Количество строк кода в проекте Manticore чуть больше, и я учту это при оценке плотности найденных ошибок.

Сравнительный анализ

Код этих проектов очень похож, и очень часто одна и та же ошибка присутствует как в одном, так и другом проекте. Сразу скажу, что в этот раз я проводил анализ поверхностно и изучал только общие предупреждения уровня High, выданные анализатором PVS-Studio.

Почему я поленился сравнить проекты более тщательно? Как я уже сказал, проекты весьма похожи, и уже при просмотре предупреждений уровня High мне стало скучно. В целом картина и так понятна. Анализатор выдал очень похожие списки предупреждений, но только в проекте Sphinx их чуть больше. Думаю, с предупреждениями других уровней ситуация будет точно такая же.

В статье я рассмотрю только некоторые фрагменты кода с ошибками, которые по какой-то причине показались мне интересными. Более подробный анализ проектов могут выполнить их разработчики. Я готов предоставить им временные лицензионные ключи.

Предлагаю читателям также скачать демонстрационную версию PVS-Studio и проверить код своих проектов. Уверен, вы найдёте в них много интересного.

Общие ошибки

Начну я с ошибок, которые нашлись как в проекте Sphinx, так и в Manticore.

Я привёл достаточно большой фрагмент кода, но не пугайтесь, здесь всё просто. Обратите внимание на формальный аргумент pConsts. Этот указатель используется в конструкторе для инициализации переменной sExpr. При этом в конструкторе нигде нет проверки на тот случай, если в качестве аргумента будет передано значение NULL, т.е. нет никакой защиты от нулевого указателя. Переменная pConsts просто смело разыменовывается.

Примечание. Есть проверка в виде assert, но она не поможет в Release-версии, поэтому такая проверка не может считаться достаточной.

Теперь взглянем на код функции CreateInNode, где создается экземпляр класса Expr_StrIn_c:

Третьим фактическим аргументом является NULL. Соответственно, если этот фрагмент кода будет выполнен, то произойдёт разыменование нулевого указателя.

Анализатор сигнализирует об этой ошибке, выдавая предупреждение: V522 Dereferencing of the null pointer 'pConsts' might take place. The null pointer is passed into 'Expr_StrIn_c' function. Inspect the third argument. Check lines: 5407, 5946. sphinxexpr.cpp 5407

Эта ошибка интересна тем, что PVS-Studio выполняет data-flow анализ, рассматривая тела двух разных функций. Однако он способен выполнить и куда более сложный вложенный анализ. Рассмотрим такой случай.

Сначала рассмотрим функцию SendBytes, в которой, собственно, и будет происходить разыменование нулевого указателя.

Обратите внимание на указатель pBuf. Он нигде не проверяется и сразу передаётся как фактический аргумент в функцию memcpy. Соответственно, если указатель pBuf будет нулевым, то при вызове функции memcpy произойдёт чтение по нулевому указателю.

Почему PVS-Studio решил, что здесь будет ошибка? Чтобы ответить на этот вопрос, поднимемся по цепочке вызовов выше и рассмотрим функцию SendMysqlOkPacket.

Прошу прощения, что мне пришлось привести тело функции целиком. Я хотел показать, что в функции нет какой-то защиты от того, что аргумент sMessage окажется равен NULL. Указатель sMessage просто передаётся в функцию SendBytes.

Ещё хочу обратить внимание, что значение формального аргумента sMessage по умолчанию равно NULL:

Это опасно уже само по себе. Однако то, что аргумент по умолчанию равен NULL, ещё ничего не означает. Возможно, в функцию всегда передают правильные аргументы. Поэтому пойдём дальше:

В функции Ok, аргумент sMessage просто передаётся в функцию SendMysqlOkPacket. Продолжим.

Здесь мы заканчиваем наше путешествие. В функцию передаются только 4 фактических аргумента. Остальные аргументы принимают значение по умолчанию. Это означает, что пятый параметр sMessage — будет равен NULL, и произойдёт разыменование нулевого указателя.

Предупреждение анализатора PVS-Studio, которое указывает на эту ошибку: V522 Dereferencing of the null pointer 'pBuf' might take place. The null pointer is passed into 'Ok' function. Inspect the third argument. Check lines: 2567, 12267, 12424, 14979. searchd.cpp 2567

Для начала рассмотрим перечисление ESphBinRead.

Как видите, в нём нет именованных констант с отрицательными значениями.

На всякий случай заглянем в функцию ReadBytes и убедимся, что она честно возвращает значения без всяких хитростей.

Как видите, все возвращаемые функцией значения больше или равны 0. Теперь настала очередь кода с ошибкой:

Предупреждение PVS-Studio: V547 Expression is always false. sphinx.cpp 22416

Подобная проверка не имеет смысла. Условие всегда ложное, и в результате некорректные ситуации при чтении данных не обрабатываются. Скорее всего, здесь должно было быть написано:

Этот код демонстрирует, что автору кода только кажется, что программа будет обрабатывать некорректные ситуации. На самом деле, я очень часто встречаю дефекты в коде, который отвечает за обработку некорректных/нестандартных ситуаций. Поэтому программы нередко падают, когда что-то идёт не так. В них просто некорректно написаны обработчики ошибок.

Никакой загадки, почему так происходит, конечно нет. Тестировать такие участки программы сложно и неинтересно. Этот один из тех случаев, когда статический анализатор может оказаться хорошим помощником, так как проверяет код вне зависимости от того, как часто он получает управление.

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'tStat' object. The memset_s() function should be used to erase the private data. sphinx.cpp 19987

Компилятор вправе удалить вызов функции memset, которая, в случае возникновения ошибки в программе, должна очистить приватные данные в tStat.

Почему компилятор так поступает, я писал много раз, поэтому не буду повторяться. Для тех, кто ещё не сталкивался с такими ситуациями, предлагаю прочитать описание диагностики V597 или посмотреть описание CWE-14.

Для начала нам понадобится посмотреть на реализацию двух макросов:

Теперь, думаю, вам не составит труда самим обнаружить ошибку в этом коде:

Предупреждение PVS-Studio: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] pInlineStorage;'. sphinx.cpp 19178

Как видите, память выделяется для массива, а освобождается, как если бы создавался только один элемент. Вместо макроса SafeDelete здесь следовало использовать макрос SafeDeleteArray.

Уникальные ошибки

Выше я рассмотрел несколько ошибок, которые обнаруживают себя как в коде Sphinx, так и Manticore. При этом, конечно, есть ошибки, свойственные только одному проекту. Рассмотрим для примера один такой случай.

В обоих проектах есть функция RotateIndexMT. Но реализована она по-разному. В реализации проекта Sphinx эта функция содержит дефект CWE-690 (Unchecked Return Value to NULL Pointer Dereference).

Вначале посмотрим на объявление функции CheckServedEntry:

Первый аргумент — это указатель на константный объект. Следовательно, функция не может изменить этот объект и сам указатель.

Теперь функция, содержащая ошибку:

Предупреждение PVS-Studio: V595 The 'pServed' pointer was utilized before it was verified against nullptr. Check lines: 17334, 17337. searchd.cpp 17334

Сначала указатель pServed разыменовывается. Далее вызывается функция CheckServedEntry, которая, как мы уже выяснили, не может изменить указатель pServed, передаваемый в качестве первого фактического аргумента.

Далее указатель pServed проверяется на равенство NULL. Ага! Значит указатель мог быть нулевым. Следовательно, выше перед первым разыменованием указателя нужно добавить проверку.

Другой вариант: проверка if ( pServed ) лишняя, если указатель никогда не равен NULL. В любом случае код надо исправить.

Подведём итоги

Проект Sphinx меньше по размеру проекта Manticore. При этом в проекте Sphinx я заметил больше ошибок и «кода с запахом», чем в проекте Manticore.

Учитывая размер проектов и количество замеченных дефектов, я получил следующий результат. Возьмём плотность ошибок в Manticore за 1. Тогда в проекте Sphinx плотность ошибок по моим прикидкам составляет 1,5.

Мои выводы. Плотность ошибок в проекте Sphinx в полтора раза выше, по сравнению с проектом Manticore. Следовательно, качество кода Manticore лучше, чем у проекта Sphinx. Форк получился качественнее оригинала.

Вновь повторю, что это моё субъективное мнение, основанное на очень малом количестве информации. Плотность ошибок в коде некоторых компонент ещё не говорит о качестве и надёжности проекта в целом.

Скачайте и попробуйте анализатор PVS-Studio. Это просто. В конце концов, даже если вы пишете идеальный код, то всегда можно поискать ошибки в коде своих коллег :).

Спасибо за внимание. Подписывайтесь на Twitter или RSS, чтоб быть в курсе наших новых публикаций.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov considers that code of the Manticore project is better than code of the Sphinx project