Мои читатели попросили сравнить проекты Manticore и Sphinx с точки зрения качества кода. Я могу сделать это только одним освоенным мною способом — проверить проекты с помощью статического анализатора PVS-Studio и посчитать плотность ошибок в коде. Итак, я проверил C и C++ код в этих проектах и, на мой взгляд, качество кода Manticore выше, чем качество кода Sphinx. Естественно, это очень узкий взгляд, и я не претендую на достоверность своего исследования. Однако меня попросили, и я сделал сравнение так, как умею.Sphinx и Manticore
Вначале давайте познакомимся с проектами 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.
CWE-476: NULL Pointer Dereference
Expr_StrIn_c ( const CSphAttrLocator & tLoc, int iLocator, ConstList_c * pConsts, UservarIntSet_c * pUservar, ESphCollation eCollation ) : Expr_ArgVsConstSet_c<int64_t> ( NULL, pConsts ) , ExprLocatorTraits_t ( tLoc, iLocator ) , m_pStrings ( NULL ) , m_pUservar ( pUservar ) { assert ( tLoc.m_iBitOffset>=0 && tLoc.m_iBitCount>0 ); assert ( !pConsts || !pUservar ); m_fnStrCmp = GetCollationFn ( eCollation ); const char * sExpr = pConsts->m_sExpr.cstr(); // <= .... }
Я привёл достаточно большой фрагмент кода, но не пугайтесь, здесь всё просто. Обратите внимание на формальный аргумент pConsts. Этот указатель используется в конструкторе для инициализации переменной sExpr. При этом в конструкторе нигде нет проверки на тот случай, если в качестве аргумента будет передано значение NULL, т.е. нет никакой защиты от нулевого указателя. Переменная pConsts просто смело разыменовывается.
Примечание. Есть проверка в виде assert, но она не поможет в Release-версии, поэтому такая проверка не может считаться достаточной.
Теперь взглянем на код функции CreateInNode, где создается экземпляр класса Expr_StrIn_c:
ISphExpr * ExprParser_t::CreateInNode ( int iNode ) { .... case TOK_ATTR_STRING: return new Expr_StrIn_c ( tLeft.m_tLocator, tLeft.m_iLocator, NULL, // <= pUservar, m_eCollation ); .... }
Третьим фактическим аргументом является 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, в которой, собственно, и будет происходить разыменование нулевого указателя.
void ISphOutputBuffer::SendBytes ( const void * pBuf, int iLen ) { int iOff = m_dBuf.GetLength(); m_dBuf.Resize ( iOff + iLen ); memcpy ( m_dBuf.Begin() + iOff, pBuf, iLen ); }
Обратите внимание на указатель pBuf. Он нигде не проверяется и сразу передаётся как фактический аргумент в функцию memcpy. Соответственно, если указатель pBuf будет нулевым, то при вызове функции memcpy произойдёт чтение по нулевому указателю.
Почему PVS-Studio решил, что здесь будет ошибка? Чтобы ответить на этот вопрос, поднимемся по цепочке вызовов выше и рассмотрим функцию SendMysqlOkPacket.
void SendMysqlOkPacket ( ISphOutputBuffer & tOut, BYTE uPacketID, int iAffectedRows=0, int iWarns=0, const char * sMessage=NULL, bool bMoreResults=false ) { DWORD iInsert_id = 0; char sVarLen[20] = {0}; void * pBuf = sVarLen; pBuf = MysqlPack ( pBuf, iAffectedRows ); pBuf = MysqlPack ( pBuf, iInsert_id ); int iLen = (char *) pBuf - sVarLen; int iMsgLen = 0; if ( sMessage ) iMsgLen = strlen(sMessage) + 1; tOut.SendLSBDword ( (uPacketID<<24) + iLen + iMsgLen + 5); tOut.SendByte ( 0 ); tOut.SendBytes ( sVarLen, iLen ); if ( iWarns<0 ) iWarns = 0; if ( iWarns>65535 ) iWarns = 65535; DWORD uWarnStatus = iWarns<<16; if ( bMoreResults ) uWarnStatus |= ( SPH_MYSQL_FLAG_MORE_RESULTS ); tOut.SendLSBDword ( uWarnStatus ); tOut.SendBytes ( sMessage, iMsgLen ); }
Прошу прощения, что мне пришлось привести тело функции целиком. Я хотел показать, что в функции нет какой-то защиты от того, что аргумент sMessage окажется равен NULL. Указатель sMessage просто передаётся в функцию SendBytes.
Ещё хочу обратить внимание, что значение формального аргумента sMessage по умолчанию равно NULL:
const char * sMessage=NULL,
Это опасно уже само по себе. Однако то, что аргумент по умолчанию равен NULL, ещё ничего не означает. Возможно, в функцию всегда передают правильные аргументы. Поэтому пойдём дальше:
inline void Ok( int iAffectedRows=0, int iWarns=0, const char * sMessage=NULL, bool bMoreResults=false ) { SendMysqlOkPacket ( m_tOut, m_uPacketID, iAffectedRows, iWarns, sMessage, bMoreResults ); if ( bMoreResults ) m_uPacketID++; }
В функции Ok, аргумент sMessage просто передаётся в функцию SendMysqlOkPacket. Продолжим.
void HandleMysqlMultiStmt (....) { .... dRows.Ok ( 0, 0, NULL, bMoreResultsFollow ); .... }
Здесь мы заканчиваем наше путешествие. В функцию передаются только 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
CWE-570: Expression is Always False
Для начала рассмотрим перечисление ESphBinRead.
enum ESphBinRead { BIN_READ_OK, ///< bin read ok BIN_READ_EOF, ///< bin end BIN_READ_ERROR, ///< bin read error BIN_PRECACHE_OK, ///< precache ok BIN_PRECACHE_ERROR ///< precache failed };
Как видите, в нём нет именованных констант с отрицательными значениями.
На всякий случай заглянем в функцию ReadBytes и убедимся, что она честно возвращает значения без всяких хитростей.
ESphBinRead CSphBin::ReadBytes ( void * pDest, int iBytes ) { .... return BIN_READ_EOF; .... return BIN_READ_ERROR; .... return BIN_READ_OK; }
Как видите, все возвращаемые функцией значения больше или равны 0. Теперь настала очередь кода с ошибкой:
static void DictReadEntry (....) { .... if ( pBin->ReadBytes ( pKeyword, iKeywordLen )<0 ) { assert ( pBin->IsError() ); return; } .... }
Предупреждение PVS-Studio: V547 Expression is always false. sphinx.cpp 22416
Подобная проверка не имеет смысла. Условие всегда ложное, и в результате некорректные ситуации при чтении данных не обрабатываются. Скорее всего, здесь должно было быть написано:
if ( pBin->ReadBytes ( pKeyword, iKeywordLen ) != BIN_READ_OK)
Этот код демонстрирует, что автору кода только кажется, что программа будет обрабатывать некорректные ситуации. На самом деле, я очень часто встречаю дефекты в коде, который отвечает за обработку некорректных/нестандартных ситуаций. Поэтому программы нередко падают, когда что-то идёт не так. В них просто некорректно написаны обработчики ошибок.
Никакой загадки, почему так происходит, конечно нет. Тестировать такие участки программы сложно и неинтересно. Этот один из тех случаев, когда статический анализатор может оказаться хорошим помощником, так как проверяет код вне зависимости от того, как часто он получает управление.
CWE-14: Compiler Removal of Code to Clear Buffers
static bool GetFileStats (....) { .... struct_stat tStat; memset ( &tStat, 0, sizeof ( tStat ) ); if ( stat ( szFilename, &tStat ) < 0 ) { if ( pError ) *pError = strerror ( errno ); memset ( &tStat, 0, sizeof ( tStat ) ); // <= return false; } .... }
Предупреждение 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.
CWE-762: Mismatched Memory Management Routines
Для начала нам понадобится посмотреть на реализацию двух макросов:
#define SafeDelete(_x) \ { if (_x) { delete (_x); (_x) = nullptr; } } #define SafeDeleteArray(_x) \ { if (_x) { delete [] (_x); (_x) = nullptr; } }
Теперь, думаю, вам не составит труда самим обнаружить ошибку в этом коде:
int CSphIndex_VLN::DebugCheck ( FILE * fp ) { .... CSphRowitem * pInlineStorage = NULL; if ( pQword->m_iInlineAttrs ) pInlineStorage = new CSphRowitem [ pQword->m_iInlineAttrs ]; .... // cleanup SafeDelete ( pInlineStorage ); .... }
Предупреждение 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:
static bool CheckServedEntry(const ServedIndex_c * pEntry, // <= const char * sIndex, CSphString & sError );
Первый аргумент — это указатель на константный объект. Следовательно, функция не может изменить этот объект и сам указатель.
Теперь функция, содержащая ошибку:
static bool RotateIndexMT ( .... ) { .... ServedIndex_c * pServed = g_pLocalIndexes->GetWlockedEntry ( sIndex ); pServed->m_sNewPath = ""; // <= if ( !CheckServedEntry ( pServed, sIndex.cstr(), sError ) ) { if ( pServed ) // <= pServed->Unlock(); return false; } .... }
Предупреждение 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
Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
