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

    Sphinx vs ManticoreМои читатели попросили сравнить проекты 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. Пожалуйста, ознакомьтесь со списком.
    • +13
    • 6,1k
    • 7
    PVS-Studio 194,26
    Ищем ошибки в C, C++ и C# на Windows и Linux
    Поделиться публикацией
    Похожие публикации
    Комментарии 7
    • +1
      У меня сложилось впечатление, что в Sphinx отсутствие проверок на ошибочные данные было/стало скорее фичёй. Это следует из оборачивания проверок в assert() — в случае реально возникающей проблемы (а не гипотетической), запускается дебажная версия, и баг фиксится, а в продакшене ресурсы процессора на бесполезные проверки не расходуются. Я, правда, столкнулся с ситуацией, когда дебажная версия всегда падает на определенном запросе к определенному набору данных, а обычная — работает. Да и не выставишь дебажную версию в продакшен — она не справляется с нагрузкой (к вопросу об оверхеде), а некоторые проблемы вылезают только там (видмо есть race condition). Еще можно отметить трудноуловимые глюки с тредами под FreeBSD, из-за чего пришлось перенести его на линукс, где эта скульптура из подпорок находится в более сбалансированном состоянии ;) Но в целом альтернатив Sphinx по производительности нет.
      • +4
        отсутствие проверок на ошибочные данные было/стало скорее фичёй

        Нет, это не совсем так.


        Концепция всегда была (и есть) следующая:


        • Если можно "почти бесплатно" не падать на кривом входе, следует не падать.
        • Если это дорого, делаем утилиты для проверок (это и assert, и indextool, и т.д.), но сохраняем производительность.

        Причём про "вход" тут речь вообще-то в первую очередь скорее о данных индекса, чем пользовательском вводе. Пользовательскому как раз веры почти (почти) никакой, проверяем его что есть сил, где не забываем. Одно хорошо: что не вебсервер, и редко голым поротом в интернет смотрим, типично хотя бы в VPN.


        Заодно замечу, что единственный опубликованный тут "уникальный" пример "ошибки" вообще не про это. Там честное кажущееся разыменование NULL, никак не связано ни с какими оптимизациями. При этом, насколько я помню, фактически словить именно в этой точке NULL как бы невозможно, тк. а) наличие элемента в хеше проверяется ранее в функции, и б) по уставу с хешем в этот момент должен работать только текущий тред, вот ровно эта функция, внезапно стереть оттуда элемент некому. Но все эти предположения могли сломаться за годы мутации кода, конечно, и в идеале лучше подложить соломки, хотя бы и ассертом. Подложу, если этот код вообще останется в живых по итогам :-)

      • 0

        Интересно, а вот такой фрагмент:


        int sphCollateLibcCS ( const BYTE...


                    ...
                    // big strings on heap
                    char * pBuf1 = new char [ iLen ];
                    char * pBuf2 = new char [ iLen ];
        
                    memcpy ( pBuf1, pStr1, iLen );
                    memcpy ( pBuf2, pStr2, iLen );
                    pBuf1[iLen] = pBuf2[iLen] = '\0';
                    ...

        нашёлся? Тут то явный buffer overflow, правда в реальной жизни на него достаточно сложно "наступить" (ну, всего-то "байтик лишний" в конце записали...)

        • 0
          Красивая ошибка. Нет, пока такую взаимосвязь видеть ещё не научили. Но работаем и в этом направлении.
          • +1
            У нас явно разные понятия «красоты» — это не красивая в моём личном понимании — это скучный и тупой off-by-one классический.

            Последнюю именно «красивую» ошибку даже и не вспомню навскидку. Первое и последнее, что моментально приходит в голову — в итоге оказалось вообще не в C++ коде совсем :)
            • +2
              Хм, очень странно и неожиданно что PVS еще не ловит такое.

              В целом ощущение что Manticore один раз (когда-то давно) прогнали через Coverity, но пофиксили только часть, а Аксенову все еще лень(?) прикрутить такой CI через travis.org
          • 0
            Почему не передавать ссылку?

            Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

            Самое читаемое