# Андрей Карпов считает, что код проекта Manticore качественнее, чем код проекта Sphinx Мои читатели попросили сравнить проекты Manticore и Sphinx с точки зрения качества кода\. Я могу сделать это только одним освоенным мною способом \- проверить проекты с помощью статического анализатора PVS\-Studio и посчитать плотность ошибок в коде\. Итак, я проверил C и C\+\+ код в этих проектах и, на мой взгляд, качество кода Manticore выше, чем качество кода Sphinx\. Естественно, это очень узкий взгляд, и я не претендую на достоверность своего исследования\. Однако меня попросили, и я сделал сравнение так, как умею\. ![0538_Manticore_vs_Sphinx_ru/image1.png](https://import.viva64.com/docx/blog/0538_Manticore_vs_Sphinx_ru/image1.png) ## Sphinx и Manticore Вначале давайте познакомимся с проектами Sphinx и Manticore\. **Sphinx** — система полнотекстового поиска, разработанная Андреем Аксёновым и распространяемая по лицензии GNU GPL\. Отличительной особенностью является высокая скорость индексации и поиска, а также интеграция с существующими СУБД и API для распространённых языков веб\-программирования\. Исходный код я взял [отсюда](https://github.com/sphinxsearch/sphinx)\. Размер проекта, если брать код на языке C и С\+\+ и не включать сторонние библиотеки \- 156 KLOC\. Комментарии составляют 10\.2%\. Это значит, что "чистого кода" \- 144 KLOC\. **Manticore** **\-** это форк Sphinx\. Начиная в качестве ключевых членов первоначальной команды Sphinx, команда Manticore установила для себя следующую цель – поставлять быстрое, стабильное и мощное свободное обеспечение по полнотекстовому поиску\. Исходный код я взял [отсюда](https://github.com/manticoresoftware/manticoresearch)\. Размер проекта, если брать код на языке C и С\+\+ и не включать сторонние библиотеки \- 170 KLOC\. Комментарии составляют 10\.1%\. Это значит, что "чистого кода" \- 152 KLOC\. Количество строк кода в проекте Manticore чуть больше, и я учту это при оценке плотности найденных ошибок\. ## Сравнительный анализ Код этих проектов очень похож, и очень часто одна и та же ошибка присутствует как в одном, так и другом проекте\. Сразу скажу, что в этот раз я проводил анализ поверхностно и изучал только общие предупреждения уровня High, выданные анализатором PVS\-Studio\. Почему я поленился сравнить проекты более тщательно? Как я уже сказал, проекты весьма похожи, и уже при просмотре предупреждений уровня High мне стало скучно\. В целом картина и так понятна\. Анализатор выдал очень похожие списки предупреждений, но только в проекте Sphinx их чуть больше\. Думаю, с предупреждениями других уровней ситуация будет точно такая же\. В статье я рассмотрю только некоторые фрагменты кода с ошибками, которые по какой\-то причине показались мне интересными\. Более подробный анализ проектов могут выполнить их разработчики\. Я готов предоставить им временные лицензионные ключи\. Предлагаю читателям также скачать демонстрационную версию [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) и проверить код своих проектов\. Уверен, вы найдёте в них много интересного\. ## Общие ошибки Начну я с ошибок, которые нашлись как в проекте Sphinx, так и в Manticore\. ### CWE\-476: NULL Pointer Dereference ```cpp Expr_StrIn_c ( const CSphAttrLocator & tLoc, int iLocator, ConstList_c * pConsts, UservarIntSet_c * pUservar, ESphCollation eCollation ) : Expr_ArgVsConstSet_c ( 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_: ```cpp 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_, в которой, собственно, и будет происходить разыменование нулевого указателя\. ```cpp 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_\. ```cpp 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_: ```cpp const char * sMessage=NULL, ``` Это опасно уже само по себе\. Однако то, что аргумент по умолчанию равен _NULL_, ещё ничего не означает\. Возможно, в функцию всегда передают правильные аргументы\. Поэтому пойдём дальше: ```cpp 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_\. Продолжим\. ```cpp 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_\. ```cpp 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_ и убедимся, что она честно возвращает значения без всяких хитростей\. ```cpp ESphBinRead CSphBin::ReadBytes ( void * pDest, int iBytes ) { .... return BIN_READ_EOF; .... return BIN_READ_ERROR; .... return BIN_READ_OK; } ``` Как видите, все возвращаемые функцией значения больше или равны 0\. Теперь настала очередь кода с ошибкой: ```cpp static void DictReadEntry (....) { .... if ( pBin->ReadBytes ( pKeyword, iKeywordLen )<0 ) { assert ( pBin->IsError() ); return; } .... } ``` Предупреждение PVS\-Studio: V547 Expression is always false\. sphinx\.cpp 22416 Подобная проверка не имеет смысла\. Условие всегда ложное, и в результате некорректные ситуации при чтении данных не обрабатываются\. Скорее всего, здесь должно было быть написано: ```cpp if ( pBin->ReadBytes ( pKeyword, iKeywordLen ) != BIN_READ_OK) ``` Этот код демонстрирует, что автору кода только кажется, что программа будет обрабатывать некорректные ситуации\. На самом деле, я очень часто встречаю дефекты в коде, который отвечает за обработку некорректных/нестандартных ситуаций\. Поэтому программы нередко падают, когда что\-то идёт не так\. В них просто некорректно написаны обработчики ошибок\. Никакой загадки, почему так происходит, конечно нет\. Тестировать такие участки программы сложно и неинтересно\. Этот один из тех случаев, когда статический анализатор может оказаться хорошим помощником, так как проверяет код вне зависимости от того, как часто он получает управление\. ### CWE\-14: Compiler Removal of Code to Clear Buffers ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v597/) или посмотреть описание [CWE\-14](https://cwe.mitre.org/data/definitions/14.html)\. ### CWE\-762: Mismatched Memory Management Routines Для начала нам понадобится посмотреть на реализацию двух макросов: ```cpp #define SafeDelete(_x) \ { if (_x) { delete (_x); (_x) = nullptr; } } #define SafeDeleteArray(_x) \ { if (_x) { delete [] (_x); (_x) = nullptr; } } ``` Теперь, думаю, вам не составит труда самим обнаружить ошибку в этом коде: ```cpp 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_: ```cpp static bool CheckServedEntry(const ServedIndex_c * pEntry, // <= const char * sIndex, CSphString & sError ); ``` Первый аргумент \- это указатель на константный объект\. Следовательно, функция не может изменить этот объект и сам указатель\. Теперь функция, содержащая ошибку: ```cpp 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\. Это просто\. В конце концов, даже если вы пишете идеальный код, то всегда можно [поискать](https://pvs-studio.ru/ru/blog/posts/0525/) ошибки в коде своих коллег :\)\. Спасибо за внимание\. Подписывайтесь на [Twitter](https://twitter.com/Code_Analysis) или [RSS](http://feeds.feedburner.com/viva64-blog-ru), чтоб быть в курсе наших новых публикаций\.