⚪️ 👩🏿‍🔧 🛀 Andrey Karpovは、ManticoreプロジェクトコードはSphinxプロジェクトコードよりも優れていると考えています 👨🏽‍🤝‍👨🏻 👉 🙍

私の読者は、コードの品質に関してManticoreとSphinxのプロジェクトを比較するように頼みました。これは、マスターした1つの方法でしか行えません。PVS-Studio静的アナライザーを使用してプロジェクトをチェックし、コードのエラー密度を計算します。したがって、これらのプロジェクトでCおよびC ++コードをテストしましたが、私の意見では、Manticoreコードの品質はSphinxコードの品質よりも高くなっています。当然、これは非常に狭い視野であり、研究の真実性を装うわけではありません。しかし、彼らは私に尋ね、私はできる限り比較をしました。

スフィンクスとマンティコア

最初に、SphinxプロジェクトとManticoreプロジェクトについて理解しましょう。

Sphinxは、Andrey Aksyonovが開発した全文検索システムであり、GNU GPLライセンスの下で配布されています。特徴的な機能は、インデックス作成と検索の高速化、および既存のDBMSおよび一般的なWebプログラミング言語のAPIとの統合です。

ここからソースコードを取りました。 CおよびC ++のコードを使用し、サードパーティライブラリを含まない場合のプロジェクトのサイズ-156 KLOC。コメントは10.2％を占めています。これは、「クリーンコード」が144 KLOCであることを意味します。

Manticoreは Sphinxのフォークです。元のSphinxチームの主要メンバーとしてスタートしたManticoreチームは、全文検索用の高速で安定した強力なフリーソフトウェアを提供するという次の目標を設定しました。

ここからソースコードを取りました。 CおよびC ++でコードを取得し、サードパーティライブラリを含まない場合のプロジェクトのサイズ-170 KLOC。コメントは10.1％を占めています。これは、「クリーンコード」が152 KLOCであることを意味します。

Manticoreプロジェクトのコードの行数はわずかに多く、見つかったエラーの密度を評価する際にこれを考慮します。

比較分析

これらのプロジェクトのコードは非常に似ており、多くの場合、1つのプロジェクトと別のプロジェクトの両方に同じエラーが存在します。今回は表面的に分析を行い、PVS-Studioアナライザーによって発行される一般的な高レベル警告のみを調査したことをすぐに言わなければなりません。

プロジェクトをより慎重に比較するのが面倒なのはなぜですか？私が言ったように、プロジェクトは非常に似ており、すでに高レベルの警告を見たとき、私は退屈していました。一般的に、状況は明確です。アナライザーは非常によく似た警告リストを生成しましたが、Sphinxプロジェクトにのみもう少し多くの警告リストがあります。他のレベルの警告でも状況はまったく同じになると思います。

この記事では、エラーのあるコードの一部のみを検討しますが、それは何らかの理由で私にとって興味深いと思われました。プロジェクトのより詳細な分析は、開発者が行うことができます。一時的なライセンスキーを提供する準備ができています。

読者はPVS-Studioのデモ版もダウンロードして、プロジェクトのコードを確認することをお勧めします。きっとあなたはそれらの中にたくさんの興味深いものを見つけるでしょう。

よくある間違い

SphinxプロジェクトとManticoreの両方で見つかったエラーから始めます。

CWE-476：NULLポインター逆参照

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が引数として渡された場合、コンストラクターのどこにもチェックがありません。 NULLポインターに対する保護はありません。 pConsts変数は大胆に逆参照されています。

ご注意 assertの形式のチェックがありますが、リリースバージョンでは役に立たないため、このようなチェックでは十分とは言えません。

次に、 Expr_StrIn_cクラスのインスタンスが作成されるCreateInNode関数のコードを見てみましょう。

 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 ); .... }

3番目の実引数はNULLです。したがって、このコードフラグメントが実行されると、nullポインターは逆参照されます。

アナライザーは、警告を発行してこのエラーを通知します。V522ヌルポインター「pConsts」の逆参照が行われる場合があります。 NULLポインターは「Expr_StrIn_c」関数に渡されます。 3番目の引数を調べます。行を確認してください：5407、5946。sphinxexpr.cpp 5407

PVS-Studioはデータフロー分析を実行し、2つの異なる関数の本体を調べるため、このエラーは興味深いものです。ただし、彼ははるかに複雑なネストされた分析を実行できます。この場合を検討してください。

まず、 SendBytes関数を検討します。実際には、nullポインターの逆参照が発生します。

 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 ); }

私は、機能全体を持ち込まなければならなかったことをおizeびします。 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つの実引数のみが関数に渡されます。残りの引数はデフォルト値を取ります。これは、5番目のパラメーターsMessage-がNULLになり、nullポインターが逆参照されることを意味します。

このエラーを示すPVS-Studioアナライザーの警告：V522 nullポインター「pBuf」の逆参照が行われる場合があります。 NULLポインターは「Ok」関数に渡されます。 3番目の引数を調べます。行を確認してください：2567、12267、12424、14979。searchd.cpp 2567

CWE-570：式は常に偽です

まず、 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式は常にfalseです。 sphinx.cpp 22416

このようなチェックは意味がありません。条件は常にfalseであり、その結果、データの読み取り時の誤った状況は処理されません。ほとんどの場合、それは次のように書かれているはずです。

 if ( pBin->ReadBytes ( pKeyword, iKeywordLen ) != BIN_READ_OK)

このコードは、コードの作成者が、プログラムが誤った状況を処理するだけだと考えていることを示しています。実際、私は非常に頻繁に、不正確/非標準の状況を処理するコードの欠陥に遭遇します。したがって、何かがうまくいかないと、プログラムはしばしばクラッシュします。エラーハンドラは、単に間違って記述されています。

もちろん、これが起こる理由は不明です。プログラムのこのようなセクションのテストは難しく、面白くない。これは、静的アナライザーがコードをチェックする頻度に関係なくチェックするため、静的アナライザーが優れたヘルパーになる可能性があるケースの1つです。

CWE-14：コンパイラーがコードを削除してバッファーをクリアする

 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コンパイラは、 'tStat'オブジェクトのフラッシュに使用される 'memset'関数呼び出しを削除できました。プライベートデータを消去するには、memset_s（）関数を使用する必要があります。 sphinx.cpp 19987

コンパイラーは、 memset関数の呼び出しを削除する権利を持っています。memset関数は、プログラムでエラーが発生した場合、 tStatのプライベートデータをクリアする必要があります。

コンパイラがこれを行う理由は、何度も書いたので、繰り返しません。まだそのような状況に遭遇していない人のために、 V597診断の説明を読むか、 CWE-14の説明を参照することを提案します。

CWE-762：メモリ管理ルーチンの不一致

まず、2つのマクロの実装を確認する必要があります。

 #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メモリは「new T []」演算子を使用して割り当てられましたが、「delete」演算子を使用して解放されました。このコードを調べることを検討してください。「delete [] pInlineStorage;」を使用することをお勧めします。 sphinx.cpp 19178

ご覧のとおり、1つの要素のみが作成されたかのように、メモリが配列に割り当てられ、解放されます。 SafeDeleteマクロの代わりに、 ここでSafeDeleteArrayマクロを使用する必要があります。

ユニークなバグ

上記では、SphinxとManticoreの両方のコードに存在するいくつかのバグを見ました。この場合、もちろん、1つのプロジェクトのみに固有のエラーがあります。たとえば、そのような場合を考えてみましょう。

どちらのプロジェクトにもRotateIndexMT関数があります。ただし、さまざまな方法で実装されます。 Sphinxプロジェクトの実装では、この関数には欠陥CWE-690（NULLポインター逆参照への未チェックの戻り値）が含まれています。

まず、 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 nullptrに対して検証される前に、「pServed」ポインターが使用されました。行を確認してください：17334、17337。searchd.cpp 17334

pServedポインターは最初に逆参照されます。次に、 CheckServedEntry関数が呼び出されます 。これは、既にわかっているように、最初の実際の引数として渡されたpServedポインターを変更できません。

次に、 pServedポインターのNULL がチェックされます。うん！そのため、ポインターはヌルになる可能性があります。したがって、上記の最初のポインターの間接参照の前に、チェックを追加する必要があります。

別のオプション：ポインターがNULLにならない場合、 （pServed）が不要かどうかをチェックします。いずれにしても、コードを修正する必要があります。

まとめると

Sphinxプロジェクトは、Manticoreプロジェクトよりもサイズが小さくなっています。同時に、Sphinxプロジェクトでは、Manticoreプロジェクトよりも多くのエラーと「匂いのあるコード」に気付きました。

プロジェクトのサイズと気づいた欠陥の数を考えると、次の結果が得られました。 Manticoreのエラー密度を1としてみましょう。その後、Sphinxプロジェクトでは、私の推定によるエラー密度は1.5です。

私の調査結果。 Sphinxプロジェクトのエラー密度は、Manticoreプロジェクトと比較して1.5倍高いです。したがって、Manticoreコードの品質は、Sphinxプロジェクトの品質よりも優れています。フォークは元のものよりも良くなった。

繰り返しますが、これは非常に少量の情報に基づいた私の主観的な意見です。一部のコンポーネントのコードのエラー密度は、プロジェクト全体の品質と信頼性を意味しません。

PVS-Studioアナライザーをダウンロードして試してください。簡単です。最終的には、完璧なコードを書いたとしても、同僚のコードのエラーをいつでも探すことができます:)。

ご清聴ありがとうございました。 TwitterまたはRSSを購読して、新しい出版物に遅れないようにしてください。

この記事を英語圏の聴衆と共有したい場合は、翻訳リンクを使用してください： Andrey Karpovは、ManticoreプロジェクトのコードはSphinxプロジェクトのコードよりも優れていると考えています

記事を読んで質問がありますか？

多くの場合、記事には同じ質問が寄せられます。ここで回答を収集しました： PVS-Studioバージョン2015に関する記事の読者からの質問への回答。リストをご覧ください。

Andrey Karpovは、ManticoreプロジェクトコードはSphinxプロジェクトコードよりも優れていると考えています