Yandexの開発者に挨拶を送ります

約6か月に1回、Yandexの従業員の誰かが私たちに書いて、PVS-Studioのライセンスに興味を持ち、試用版をダウンロードして消えます。 これは普通のことで、私たちはアナライザーを大企業に販売する遅いプロセスに慣れています。 ただし、一度機会が訪れれば、Yandex開発者に挨拶を伝え、PVS-Studioツールを思い出すことは不要ではありません。



正直なところ、この記事はほとんどランダムでした。 ClickHouseをチェックするように既に申し出られましたが、どういうわけかこのアイデアは忘れられました。 先日、インターネットを歩き回って、私は再びClickHouseの言及に出会い、このプロジェクトに興味を持ちました。 今回、私はこのプロジェクトを先送りせずにチェックすることにしました。

クリックハウス

ClickHouseは、OLAP（オンライン分析クエリ処理）用のカラムナーDBMSです。 ClickHouseはYandex.Metricaのタスクを解決するためにYandexで開発されました。 ClickHouseを使用すると、更新されたデータに対してリアルタイムで分析クエリを実行できます。 このシステムは直線的にスケーラブルであり、数兆のレコードとペタバイトのデータを扱うことができます。 2016年6月、ClickHouseはApache 2.0ライセンスでオープンソースにアップロードされました。

• ウェブサイト： clickhouse.yandex
• ウィキペディアのページ： ClickHouse
• Habrahabr Webサイトの記事： YandexがClickHouseを開きます 。
• GitHub.comのリポジトリ： yandex / ClickHouse

PVS-Studioを使用したプロジェクト分析

2017年8月14日にリポジトリから取得したClickHouseソースコードを確認しました。 検証には、PVS-Studio v6.17アナライザーのベータ版を使用しました。 この記事が公開される頃には、このバージョンはすでにリリースされています。



次のディレクトリはスキャンから除外されました。

• ClickHouse / contrib
• ClickHouse / libs
• クリックハウス/ビルド
• ClickHouse / dbms / src / Common / testsなど、さまざまなテストも除外されました

残りのC ++ソースコードのサイズは213 KLOCです。 同時に、7.9％の行がコメントです。 テストされたプロプライエタリなコードはほとんどありません：約196 KLOCです。



ご覧のとおり、ClickHouseプロジェクトのサイズは小さいです。 同時に、コードの品質は非常に高く、衝撃的な記事を書くことはできません。 アナライザーは合計で130の警告（一般分析、高および中メッセージ）を発行しました。



誤検知の数について答えることは難しいと思います。 正式にfalseと呼ぶことのできない多くの警告がありますが、それらの実用的な利点はありません。 これを行う最も簡単な方法は、例を挙げることです。

int format_version; .... if (format_version < 1 || format_version > 4) throw Exception("Bad checksums format version: " + ....); if (format_version == 1) return false; if (format_version == 2) return read_v2(in); if (format_version == 3) return read_v3(in); if (format_version == 4) return read_v4(in); return false;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーは、式（format_version == 4）の評価が開始されると、常に真になるという事実に注目します。 ご覧のとおり、 format_versionの値が[ 1..4 ]を超えると、例外がスローされるという上記のチェックがあります。 また、 return falseステートメントはまったく実行されません。



正式には、アナライザーは正しく、これが誤検知であることを正当化する方法は明確ではありません。 一方、このコードが正しく、単に「安全域」で書かれていることは明らかです。



このような場合、アナライザーの警告はさまざまな方法で抑制したり、コードを書き換えたりできます。 たとえば、次のように書くことができます。

 switch(format_version) { case 1: return false; case 2: return read_v2(in); case 3: return read_v3(in); case 4: return read_v4(in); default: throw Exception("Bad checksums format version: " + ....); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

私が単に言うことができない警告がまだあります：エラーを示すかどうか。 私はこのプロジェクトに精通しておらず、一部のコードフラグメントがどのように機能するのかわかりません。 この場合を検討してください。



3つの機能を持つ特定のスコープがあります。

 namespace CurrentMemoryTracker { void alloc(Int64 size); void realloc(Int64 old_size, Int64 new_size); void free(Int64 size); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

関数の仮引数の名前は、関数が入力でいくつかのサイズを取る必要があることを示唆しています。 アナライザーは、たとえば、構造体のサイズではなく、ポインターのサイズがalloc関数に渡される場合を疑います。

 using Large = HyperLogLogCounter<K, Hash, UInt32, DenominatorType>; Large * large = nullptr; .... CurrentMemoryTracker::alloc(sizeof(large));
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーは、これが間違いであるかどうかを知りません。 私も知りませんが、私の意見では、このコードは疑わしいです。



一般的に、私はそのような場合については話しません。 ClickHouse開発者が興味を持っている場合、プロジェクトを個別に検証し、アラートのリストをより詳細に調べることができます。 この記事では、私にとって最も興味深いと思われるコードフラグメントのみを検討します。

興味深いコードスニペット

1. CWE-476：NULLポインター逆参照（3エラー）

 bool executeForNullThenElse(....) { .... const ColumnUInt8 * cond_col = typeid_cast<const ColumnUInt8 *>(arg_cond.column.get()); .... if (cond_col) { .... } else if (cond_const_col) { .... } else throw Exception( "Illegal column " + cond_col->getName() + // <= " of first argument of function " + getName() + ". Must be ColumnUInt8 or ColumnConstUInt8.", ErrorCodes::ILLEGAL_COLUMN); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio警告： V522 nullポインター 'cond_col'の逆参照が行われる場合があります。 FunctionsConditional.h 765



ここでは、エラーが発生した状況は正しく処理されません。 例外をスローする代わりに、nullポインターは逆参照されます。



エラーメッセージを作成するには、関数呼び出しcond_col-> getName（）を作成します。 cond_colポインターはヌルになるため、これは実行できません。



同様のエラーがここにあります：V522 nullポインター 'cond_col'の逆参照が行われる可能性があります。 FunctionsConditional.h 1061



NULLポインターの使用に関する別のバリ​​エーションを検討してください。

 void processHigherOrderFunction(....) { .... const DataTypeExpression * lambda_type = typeid_cast<const DataTypeExpression *>(types[i].get()); const DataTypes & lambda_argument_types = lambda_type->getArgumentTypes(); if (!lambda_type) throw Exception("Logical error: .....", ErrorCodes::LOGICAL_ERROR); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio警告： V595 nullptrに対して検証される前に、「lambda_type」ポインターが使用されました。 行を確認してください：359、361。TypeAndConstantInference.cpp 359



はじめに、 lambda_typeポインターは逆参照され、その後のみチェックされます。 コードを修正するには、ポインターチェックを少し高く移動する必要があります。

 if (!lambda_type) throw Exception("Logical error: .....", ErrorCodes::LOGICAL_ERROR); const DataTypes & lambda_argument_types = lambda_type->getArgumentTypes();
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

2. CWE-665：不適切な初期化（1エラー）

 struct TryResult { .... explicit TryResult(Entry entry_) : entry(std::move(entry)) // <= , is_usable(true) , is_up_to_date(true) { } .... Entry entry; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V546クラスのメンバーはそれ自身で初期化されます： 'entry（entry）'。 PoolWithFailoverBase.h 74



タイプミスにより、 エントリメンバはそれ自体で初期化され、その結果、実際には初期化されません。 コードを修正するには、アンダースコアを追加します。

 : entry(std::move(entry_))
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

3. CWE-672：有効期限またはリリース後のリソースの操作（1エラー）

 using Strings = std::vector<std::string>; .... int mainEntryClickhousePerformanceTest(int argc, char ** argv) { .... Strings input_files; .... for (const String filename : input_files) // <= { FS::path file(filename); if (!FS::exists(file)) throw DB::Exception(....); if (FS::is_directory(file)) { input_files.erase( // <= std::remove(input_files.begin(), // <= input_files.end(), // <= filename) , // <= input_files.end() ); // <= getFilesFromDir(file, input_files, recursive); } else { if (file.extension().string() != ".xml") throw DB::Exception(....); } } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio警告：範囲ベースのforループで使用される「input_files」コンテナのV789イテレータは、「消去」機能の呼び出し時に無効になります。 PerformanceTest.cpp 1471



input_filesコンテナは 、範囲ベースのforループで使用されます。 同時に、ループ内では、一部の要素の削除によりコンテナが変更される場合があります。 読者がこれができない理由を理解していない場合、 V789診断の説明をよく理解することをお勧めします。



4. CWE-563：使用しない変数への割り当て（「未使用変数」）（1エラー）

 struct StringRange { const char * first; const char * second; .... StringRange(TokenIterator token_begin, TokenIterator token_end) { if (token_begin == token_end) { first = token_begin->begin; // <= second = token_begin->begin; // <= } TokenIterator token_last = token_end; --token_last; first = token_begin->begin; // <= second = token_last->end; // <= } };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーは2つの警告を生成します。

• V519「最初の」変数には連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：26、33。StringRange.h 33
• V519「2番目」の変数には、連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：27、34。StringRange.h 34

特定の条件下では、最初に最初の変数と2番目の変数に値token_begin-> beginが割り当てられます 。 さらに、いずれの場合でもこれらの変数の値は再び変化します。 ほとんどの場合、このコードには何らかの論理エラーが含まれているか、何かが欠落しています。 たとえば、 returnステートメントは忘れられる場合があります。

 if (token_begin == token_end) { first = token_begin->begin; second = token_begin->begin; return; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

5. CWE-570：式は常にFalse（2エラー）

 DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override { .... if (!((.....)) || ((left_is_string || left_is_fixed_string) && (.....)) || (left_is_date && right_is_date) || (left_is_date && right_is_string) || (left_is_string && right_is_date) || (left_is_date_time && right_is_date_time) // 1 || (left_is_date_time && right_is_string) // 1 || (left_is_string && right_is_date_time) // 1 || (left_is_date_time && right_is_date_time) // 2 || (left_is_date_time && right_is_string) // 2 || (left_is_string && right_is_date_time) // 2 || (left_is_uuid && right_is_uuid) || (left_is_uuid && right_is_string) || (left_is_string && right_is_uuid) || (left_is_enum && right_is_enum && .....) || (left_is_enum && right_is_string) || (left_is_string && right_is_enum) || (left_tuple && right_tuple && .....) || (arguments[0]->equals(*arguments[1])))) throw Exception(....); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

この条件下では、3つの部分式が2回繰り返されます。 PVS-Studioの警告：

• V501インスタンス化FunctionComparison <EqualsOp、NameEquals>：「||」の左と右に同一のサブ式「（left_is_date_time && right_is_date_time）」があります 演算子。 FunctionsComparison.h 1057
• V501インスタンス化FunctionComparison <EqualsOp、NameEquals>：「||」の左と右に同一のサブ式「（left_is_date_time && right_is_string）」があります 演算子。 FunctionsComparison.h 1057
• V501インスタンス化FunctionComparison <EqualsOp、NameEquals>：「||」の左と右に同一のサブ式「（left_is_string && right_is_date_time）」があります 演算子。 FunctionsComparison.h 1057

2つのオプションが可能です。 まず、エラーはありません。条件は単純に冗長であり、単純化できます。 2番目はここでの間違いであり、一部の条件はチェックされません。 いずれにしても、著者はこのコードをチェックアウトする必要があります。



条件が常にfalseである別のケースを考えます。

 static void ipv6_scan(const char * src, unsigned char * dst) { .... uint16_t val{}; unsigned char * colonp = nullptr; while (const auto ch = *src++) { const auto num = unhex(ch); if (num != -1) { val <<= 4; val |= num; if (val > 0xffffu) // <= return clear_dst(); saw_xdigit = 1; continue; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioの警告 ： V547式 'val> 0xffffu'は常にfalseです。 符号なしshort型の値の範囲：[0、65535]。 FunctionsCoding.h 339



IPv6アドレスを含む文字列を解析すると、無効なIPv6アドレスの一部が正しいと認識されます。 値がFFFF以下の16進形式の数値を区切り文字の間に書き込むことができると予想されます。 数値が大きい場合、アドレスは間違っていると見なされます。 この状況を識別するために、コードには「 if（val> 0xffffu） 」というチェックがあります。 しかし、彼女は働きません。 変数valはuint16_t型であり、0xFFFFを超えることはできません。 その結果、関数は誤ったアドレスを「飲み込む」ことになります。 アドレスの次の部分は、区切り文字の前の最後の4つの16進数です。



6. CWE-571。 式は常に真（1エラー）

 static void formatIP(UInt32 ip, char *& out) { char * begin = out; for (auto i = 0; i < 3; ++i) *(out++) = 'x'; for (size_t offset = 8; offset <= 24; offset += 8) { if (offset > 0) // <= *(out++) = '.'; /// Get the next byte. UInt32 value = (ip >> offset) & static_cast<UInt32>(255); /// Faster than sprintf. if (value == 0) { *(out++) = '0'; } else { while (value > 0) { *(out++) = '0' + value % 10; value /= 10; } } } /// And reverse. std::reverse(begin, out); *(out++) = '\0'; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioの警告 ： V547式 'offset> 0'は常にtrueです。 FunctionsCoding.h 649



条件「 offset> 0 」は常に満たされるため、常にポイントが追加されます。 ここでは間違いはなく、検証は単に不要であるように思えます。 もちろんわかりませんが。 それでもエラーでない場合は、他のプログラマーや静的コードアナライザーを混乱させないように、チェックを削除する必要があります。

おわりに

おそらく、プロジェクトの開発者は、この記事には反映されていないアナライザーの警告を見ることで、多くのエラーを見つけることができるでしょう。 特に「こんにちは」と言うために、十分な資料があったので、話を終了します:)。



一般に、ClickHouseプロジェクトの開発者のコ​​ードの高品質に注目したいと思います。 ただし、どんなに高級な開発者であっても、彼らはエラーの影響を受けません。この記事では、これを再度示します。 PVS-Studio静的コードアナライザーは、多くのエラーの防止に役立ちます。 開発者は、新しいコードを記述するときに静的分析を最大限に活用します。 新しいコードをチェックした直後にアナライザーで検出できるエラーのデバッグに時間を費やすことは意味がありません。



PVS-Studioをダウンロードしてお試しください。





この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください：Andrey Karpov。 Yandex開発者に敬意を表します