Мои читатели попросили сравнить проекты Manticore и Sphinx с точки зрения качества кода. Я могу сделать это только одним освоенным мною способом - проверить проекты с помощью статического анализатора PVS-Studio и посчитать плотность ошибок в коде. Итак, я проверил C и C++ код в этих проектах и, на мой взгляд, качество кода 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.
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
Для начала рассмотрим перечисление 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)Этот код демонстрирует, что автору кода только кажется, что программа будет обрабатывать некорректные ситуации. На самом деле, я очень часто встречаю дефекты в коде, который отвечает за обработку некорректных/нестандартных ситуаций. Поэтому программы нередко падают, когда что-то идёт не так. В них просто некорректно написаны обработчики ошибок.
Никакой загадки, почему так происходит, конечно нет. Тестировать такие участки программы сложно и неинтересно. Этот один из тех случаев, когда статический анализатор может оказаться хорошим помощником, так как проверяет код вне зависимости от того, как часто он получает управление.
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.
Для начала нам понадобится посмотреть на реализацию двух макросов:
#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, чтоб быть в курсе наших новых публикаций.