記録を狙う：Chromiumの5番目のテスト

Chromiumは繰り返し確認されているようです。 注意深い読者は論理的な質問をします。「なぜ別のチェックが必要なのですか？ それで十分ではなかったのですか？」 間違いなく、Chromiumコードはその清潔さで注目に値します。検証中に毎回確信していましたが、エラーは必然的に検出され続けます。 繰り返しチェックすることで、静的解析を使用するほど、より良い結果が得られます。 プロジェクトが毎日チェックされるとよいでしょう。 さらに良いのは、プログラマーが作業中にアナライザーを直接使用する場合（変更されたコードの自動分析）。

少しの背景

ChromiumはPVS-Studioですでに4回テストされています。

• 最初のチェック （2011年5月23日）
• 2番目のチェック （10/13/2011）
• 3回目のチェック （2013年8月12日）
• 4番目のチェック （12/02/2013）

以前は、すべてのチェックはWindowsバージョンのPVS-Studioアナライザーによって実行されていました。 最近、PVS-StudioもLinuxで動作するため、このバージョンが分析に使用されました。

この間、プロジェクトのサイズは大きくなりました。3回目のチェックで、プロジェクトの数は1169に達しました。執筆時点では、4420でした。

4つのチェックについて、このような大規模プロジェクトの最高品質のコードが注目されました。 2年半後に状況は変わりましたか？ いや 品質は依然としてトップです。 しかし、大量のコードとその絶え間ない開発により、再び多くのエラーが見つかります。

確認方法

Chromiumの確認方法について説明します。 今回はLinuxで実行します。 depot_toolsを使用してソースをダウンロードして準備した後（詳細については、 「構築」セクションを参照）、プロジェクトを組み立てます。

pvs-studio-analyzer trace -- ninja -C out/Default chrome 

次に、コマンドを実行します（これは1行です）。

 pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic -o /path/to/save/chromium.log -j<N> 

-jフラグはマルチスレッド分析を開始します。 推奨されるスレッド数は、物理CPUコアの数に1を加えたものです（たとえば、クアッドコアプロセッサでは、フラグは「-j5」のようになります）。

その結果、PVS-Studioアナライザーレポートが受信されます。 PVS-Studio配布キットの一部であるPlogConverterユーティリティを使用して、xml、errorfile、tasklistの3つの読み取り可能な形式のいずれかに変換できます。 タスクリスト形式を使用してメッセージを表示します。 現在のスキャンでは、すべてのレベル（高、中、低）の一般的な分析警告のみが表示されます。 変換コマンドは次のようになります（1行で）。

 plog-converter -t tasklist -o /path/to/save/chromium.tasks -a GA:1,2,3 /path/to/saved/chromium.log 

PlogConverterのすべてのパラメーターの詳細については、 こちらをご覧ください 。 次のコマンドを使用して、タスクリスト「chromium.tasks」をQtCreatorにダウンロードします（プリインストールする必要があります）。

 qtcreator path/to/saved/chromium.tasks 

高レベルおよび中レベルの警告とともにレポートの表示を開始することを強くお勧めします-コードに誤った指示がある可能性が非常に高くなります。 低レベルの警告も潜在的なエラーを示す可能性がありますが、誤検知の可能性が高いため、記事を書くとき、通常は調査されません。

QtCreatorでレポート自体を表示すると、次のようになります。


図1-QtCreatorでのアナライザーの結果の操作（画像をクリックして拡大）

アナライザーは何を伝えましたか？

Chromiumプロジェクトを確認した後、2,312件の警告が受信されました。 次の図は、重大度レベルごとのアラートの分布を示しています。

図2-重大度レベルによるアラートの分布

下の図について簡単にコメントしましょう。171高レベル警告、290中レベル警告、1851低レベル警告が受信されました。

かなりの数の警告にもかかわらず、このような巨大なプロジェクトではこれでは十分ではありません。 ライブラリなしのソースコード（SLOC）の合計行数は6468751です。高レベルと中レベルの警告のみを考慮に入れると、そのうち220の本当のエラーを指摘できます。 数字は数字ですが、実際には、コード1000行あたり0.034エラーのエラー密度が得られます。 もちろん、これはすべてのエラーの密度ではなく、PVS-Studioが検出したエラーの密度のみです。 むしろ、レポートを見ているときに気づいた間違い。

原則として、他のプロジェクトでは、エラーの密度が高くなります。 Chromium開発者は素晴らしいです！ ただし、リラックスしてはいけません。間違いがあり、無害ではありません。

最も興味深いエラーについてさらに詳しく考えてみましょう。

新たに見つかったエラー

コピーペースト

アナライザーの警告 ： V501 「&&」演算子の左側と右側には、同一のサブ式「request_body_send_buf_ == nullptr」があります。 http_stream_parser.cc 1222

 bool HttpStreamParser::SendRequestBuffersEmpty() { return request_headers_ == nullptr && request_body_send_buf_ == nullptr && request_body_send_buf_ == nullptr; // <= } 

ジャンルの古典。 プログラマーはrequest_body_send_buf_ポインターをnullptrと2回比較しました。 これはおそらくタイプミスであり、 nullptrではもう1つのクラスメンバーを比較する必要があります。

アナライザーの警告 ： V766同じキー '"colorSectionBorder"'を持つアイテムが既に追加されています。 ntp_resource_cache.cc 581

 void NTPResourceCache::CreateNewTabCSS() { .... substitutions["colorSectionBorder"] = // <= SkColorToRGBAString(color_section_border); .... substitutions["colorSectionBorder"] = // <= SkColorToRGBComponents(color_section_border); .... } 

アナライザーは、 「colorSectionBorder」キーを使用して、オブジェクトの疑わしい二重初期化を報告します。 このコンテキストの置換変数は連想配列です。 最初の初期化では、タイプSkColor （ uint32_tとして定義）のcolor_section_border変数がRGBAストリング表現に変換され（ SkColorToRGBAStringメソッドの名前によって判断）、キー「colorSectionBorder」によって保存されます。 再割り当てされると、 color_section_borderは別の文字列形式（ SkColorToRGBComponentsメソッド）に変換され、同じキーを使用して書き込まれます。 これは、キー「colorSectionBorder」の以前の値が破棄されることを意味します。 おそらくこれは意図したものですが、その後、初期化の1つを削除する必要があります。 それ以外の場合は、異なるキーで色成分を保存する必要があります。

ご注意 ちなみに、これは実際のプロジェクトでV766診断を使用して検出された最初のエラーです。 エラーのタイプは非常に特定的ですが、Chromiumプロジェクトは非常に大きいため、異国の欠陥に対応できます。

ポインターの無効な作業

私は読者に少しでもストレッチをして、自分で間違いを見つけてみてください。

 // Returns the item associated with the component |id| or nullptr // in case of errors. CrxUpdateItem* FindUpdateItemById(const std::string& id) const; void ActionWait::Run(UpdateContext* update_context, Callback callback) { .... while (!update_context->queue.empty()) { auto* item = FindUpdateItemById(update_context->queue.front()); if (!item) { item->error_category = static_cast<int>(ErrorCategory::kServiceError); item->error_code = static_cast<int>(ServiceError::ERROR_WAIT); ChangeItemState(item, CrxUpdateItem::State::kNoUpdate); } else { NOTREACHED(); } update_context->queue.pop(); } .... } 

アナライザーの警告 ： V522ヌルポインター「アイテム」の逆参照が行われる場合があります。 action_wait.cc 41

ここで、プログラマーは明示的に自分の足を撃つことに決めました。 コードでは、文字列表現の識別子を含むキューqueueは順番にバイパスされます。 キューから識別子が抽出され、 FindUpdateItemByIdメソッドは識別子によりタイプCrxUpdateItemのオブジェクトへのポインタを返す必要があります。 FindUpdateItemByIdメソッドでエラーが発生した場合、 nullptrが返され、 ifステートメントの thenブランチで逆参照されます。

正しいコードは次のようになります。

 .... while (!update_context->queue.empty()) { auto* item = FindUpdateItemById(update_context->queue.front()); if (item != nullptr) { .... } .... } .... 

アナライザーの警告 ： V620 sizeof（T）* N kindの式がT型へのポインターと合計されることはまれです。 string_conversion.cc 62

 int UTF8ToUTF16Char(const char *in, int in_length, uint16_t out[2]) { const UTF8 *source_ptr = reinterpret_cast<const UTF8 *>(in); const UTF8 *source_end_ptr = source_ptr + sizeof(char); uint16_t *target_ptr = out; uint16_t *target_end_ptr = target_ptr + 2 * sizeof(uint16_t); // <= out[0] = out[1] = 0; .... } 

アナライザーは、疑わしいアドレス演算を伴うコードを検出しました。 名前が示すように、関数は文字をエンコーディングUTF-8からUTF-16に変換します。 現在のUnicode 6.x標準では、UTF-8文字が4バイトに拡張されています。 この点で、UTF-8文字はUTF-16の2文字としてデコードされます（UTF-16文字は2バイトでハードエンコードされます）。 デコードには、4つのポインターが使用されます-inおよびout配列の開始と終了へのポインター。 コード内の配列の末尾へのポインタは、STLイテレータのように機能します。これらは、配列の最後の要素の後のメモリ領域を参照します。 source_end_ptrの場合、ポインターが正しく受信されていれば、 target_end_ptrではすべてが「バラ色」ではありません。 out配列の2番目の要素の後のメモリ領域を参照する必要があることが理解されました（つまり、 outポインタに対して4バイトシフトします）が、代わりにポインタは4番目の要素の後のメモリ領域を参照します（8バイトのオフセット）。

話すべき言葉を説明します。



実際に起こった方法：



正しいコードは次のようになります。

 int UTF8ToUTF16Char(const char *in, int in_length, uint16_t out[2]) { const UTF8 *source_ptr = reinterpret_cast<const UTF8 *>(in); const UTF8 *source_end_ptr = source_ptr + 1; uint16_t *target_ptr = out; uint16_t *target_end_ptr = target_ptr + 2; out[0] = out[1] = 0; .... } 

アナライザーは、別の疑わしい場所も見つけました。

• V620 sizeof（T）* N kindの式がT型へのポインターと合計されることは異常です。 string_conversion.cc 106

さまざまなエラー

再度ウォームアップして、コード内のエラーを自分で見つけようとすることを提案します。

 CheckReturnValue& operator=(const CheckReturnValue& other) { if (this != &other) { DCHECK(checked_); value_ = other.value_; checked_ = other.checked_; other.checked_ = true; } } 

アナライザーの警告 ： V591非void関数は値を返す必要があります。 memory_allocator.h 39

上記のコードの動作は未定義です。C++標準では、void以外のメソッドは値を返す必要があるとされています。 上記のコードには何が含まれていますか？ 割り当て演算子は、それ自体への割り当てをチェックし（オブジェクトをポインターで比較）、フィールドをコピーします（ポインターが異なる場合）。 ただし、メソッドはそれ自体への参照を返しませんでした（ return * this ）。

プロジェクトには、void以外のメソッドが値を返さない場所がさらに2つありました。

• V591非void関数は値を返す必要があります。 sandbox_bpf.cc 115
• V591非void関数は値を返す必要があります。 events_x.cc 73

アナライザーの警告 ： V583 「？：」演算子は、その条件式に関係なく、常に1つの同じ値を返します。1. configurator_impl.cc 133

 int ConfiguratorImpl::StepDelay() const { return fast_update_ ? 1 : 1; } 

コードは常に1に等しい遅延を返します。 おそらくこれは将来のために痛いですが、今のところそのような三項演算子の使用はありません。

アナライザーの警告 ： V590 'rv == OKの検査を検討してください|| rv！= ERR_ADDRESS_IN_USE '式。 表現が過剰であるか、誤植が含まれています。 udp_socket_posix.cc 735

 int UDPSocketPosix::RandomBind(const IPAddress& address) { DCHECK(bind_type_ == DatagramSocket::RANDOM_BIND && !rand_int_cb_.is_null()); for (int i = 0; i < kBindRetries; ++i) { int rv = DoBind(IPEndPoint(address, rand_int_cb_ .Run(kPortStart, kPortEnd))); if (rv == OK || rv != ERR_ADDRESS_IN_USE) // <= return rv; } return DoBind(IPEndPoint(address, 0)); } 

アナライザーは、過剰な比較の可能性を警告します。 コードでは、ランダムポートがIPアドレスにバインドされています。 バインドが成功した場合、ループは停止します（これは、ポートをアドレスにバインドする試行回数を意味します）。 ロジックに基づいて、比較の1つを残すことができます（バインドが成功した場合はサイクルが停止するか、別のアドレスでポートを使用するエラーが返されません）。

アナライザーの警告 ： V523 「then」ステートメントは「else」ステートメントと同等です。

 bool ResourcePrefetcher::ShouldContinueReadingRequest( net::URLRequest* request, int bytes_read ) { if (bytes_read == 0) { // When bytes_read == 0, no more data. if (request->was_cached()) FinishRequest(request); // <= else FinishRequest(request); // <= return false; } return true; } 

アナライザーは、 ifステートメントの thenブランチとelseブランチで同じステートメントを報告しました。 これは何につながりますか？ コードに基づいて、キャッシュされていないURLリクエスト（ net :: URLRequest * request ）とキャッシュされたURLリクエストが完了します。 そうである場合は、分岐演算子を削除できます。

 .... if (bytes_read == 0) { // When bytes_read == 0, no more data. FinishRequest(request); // <= return false; } .... 

別のロジックが暗示されている場合、間違ったメソッドが呼び出され、「眠れない夜」と「コーヒーの海」につながる可能性があります。

アナライザーの警告 ： V609ゼロ除算。 分母範囲[0..4096]。 addr.h 159

 static int BlockSizeForFileType(FileType file_type) { switch (file_type) { .... default: return 0; // <= } } static int RequiredBlocks(int size, FileType file_type) { int block_size = BlockSizeForFileType(file_type); return (size + block_size - 1) / block_size; // <= } 

ここで何が見えますか？ このコードは微妙なエラーにつながる可能性があります。RequiredBlocksメソッドでは、 block_size変数が値（ BlockSizeForFileTypeメソッドを使用して計算）で除算されます。 BlockSizeForFileTypeメソッドでは、 switchステートメントの FileType列挙の転送された値に応じて、特定の値が返されますが、デフォルト値も0に設定されました。 。 これにより、未定義の動作が発生します。C++標準に従って、ゼロによる除算はプログラム例外を引き起こしません。 代わりに、標準のtry / catchブロックではキャッチできないハードウェア例外が発生します （代わりに、シグナルハンドラが使用されます。詳細については、 こちらとこちらをご覧ください ）。

アナライザーの警告 ： V519 「*リスト」変数には、値が連続して2回割り当てられます。 おそらくこれは間違いです。 チェック行：136、138。util.cc 138

 bool GetListName(ListType list_id, std::string* list) { switch (list_id) { .... case IPBLACKLIST: *list = kIPBlacklist; break; case UNWANTEDURL: *list = kUnwantedUrlList; break; case MODULEWHITELIST: *list = kModuleWhitelist; // <= case RESOURCEBLACKLIST: *list = kResourceBlacklist; break; default: return false; } .... } 

switchステートメントを記述するときの典型的な間違い。 変数list_idがListType列挙からMODULEWHITELIST値を取得すると、 リストポインターの行が値kModuleWhitelistで初期化され、 switchステートメントが中断されることが予想されます。 ただし、 breakステートメントが欠落しているため、次のブランチRESOURCEBLACKLISTへの移行が発生し、文字列kResourceBlacklistが実際に* listに保存されます 。

結論

クロムは堅いままです。 それでも、PVS-Studioアナライザーは何度もエラーを見つけることができます。 静的解析の方法を使用すると、テスト段階の前のコードを書く段階でもエラーを見つけることができます。

静的コード分析に使用できるツールは何ですか？ 実際、 多くのツールがあります 。 当然、PVS-Studioを試すことをお勧めします。この製品はVisual Studio環境に非常に便利に統合されているか、任意のビルドシステムで使用できます。 そして最近では、Linuxバージョンが利用可能になりました。 WindowsおよびLinuxバージョンの詳細については、 こちらとこちらをご覧ください 。


この記事を英語圏の聴衆と共有したい場合は、翻訳リンクを使用してください：フィリップ・カンデリアンツ。 記録の見出し：Chromium、5番目のチェック 。