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

Мои читатели попросили сравнить проекты 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, чтоб быть в курсе наших новых публикаций.