Chromium：6回目のプロジェクトチェックと250のバグ

このテキストは、PVS-Studio静的コードアナライザーを使用したChromiumプロジェクトの次の検証に関する記事シリーズの始まりです。 この記事では、さまざまなエラーパターンを調べ、コード内でエラーが発生する可能性を減らすための推奨事項を提案します。 ただし、開始する前に、いくつかの質問に事前に回答する一種の紹介文を作成する必要があります。また、一連の記事の終わりを待たずに修正を開始する可能性のあるすべてのバグをChromium開発者に提供する必要があります。

背景

私の名前はアンドレイ・カルポフです。私は静的解析全般、特にPVS-Studio静的解析ツールの伝道者です。 ただし、「技術的伝道者」という用語はすでに廃止されており、「開発者支持者」がそれに取って代わりつつあります。

私はコードの品質の向上とプログラムの信頼性の向上に資料を書くのに多くの時間を費やしています。 ここで、このトピックに関するいくつかの記事を書く新しい理由があります。PVS-Studioアナライザーを使用して、開いているChromiumプロジェクトをチェックします。 これは大規模なプロジェクトであり、さまざまな種類のバグが大規模なプロジェクトに存在します。 この多様性により、これらのバグの原因とその防止方法に関連するいくつかの興味深いトピックをすぐに検討できます。

Chromiumプロジェクトの最初の記事ではないことに注意してください。 私の以前の投稿：

• 2011年5月。アンドレイ・カルポフ。 PVS-Studio対Chromium
• 2011年10月。アンドレイ・カルポフ。 PVS-Studio対Chromium-続き 。
• 2013年8月。アンドレイ・カルポフ。 PVS-Studioアナライザーを使用したChromiumプロジェクトコードの3番目の検証 。

ご覧のように、興味深い記事のタイトルで私は苦労し、私の力が尽きました。 したがって、さらに私の同僚はバトンを拾いました。

• 2013年12月。エフゲニー・リジコフ。 GoogleでPVS-StudioまたはChromiumの別のバグを販売しようとしていますか？
• 2016年10月。PhilipHandelyants。 Chromiumの5番目のテストである記録にあります 。

ちなみに、私は新鮮なレポートを勉強している間、私は抵抗することができず、私が非常に気に入った1つの間違いについて短いメモを書きました。 この記事はすでに公開されているため、ここにリンクを提供します。

• 2017年1月。アンドレイ・カルポフ。 2月31日

プロジェクトをチェックするたびに、多数のエラーが含まれていました。 新しいチェックも例外ではありませんでした。 さらに、PVS-Studioアナライザーはエラーをますます検出するので、最初はすべてのエラーをどうすればよいかわかりませんでした。 レポートをすばやく確認して、約250のエラーを書き、考えました。 1つの記事で250のエラーすべてを説明しますか？ それはある種の恐怖になるでしょう：長くて退屈で面白くない。 いくつかの記事の説明を分割しますか？ どちらかといえば、退屈な1つの記事の代わりに、いくつかの記事が表示されます。

次に、タイプごとに見つかったエラーを分割し、個別に検討することにしました。 さらに、エラーを説明するだけでなく、静的コード分析に加えて、エラーに対処するいくつかの方法を提案することにしました。 結局、後で静的/動的コード分析/または何らかの方法でそれを見つけるよりも、間違いを犯さない方がはるかに良いです。 ユーザーがエラーを見つけた場合はさらに悪い。 したがって、エラーの発生がより少なくなるようにコーディングスタイルを改善できる場合は、このトピックを検討する価値があります。 これは、一連の記事で行います。

エラーパターンを見る前に、あなたが読んでいる紹介文が必要です。 たとえば、レポートを慎重に調査することができなかった理由、誤検知の割合について言えない理由、気付いたすべてのエラーをすぐに確認できる理由を説明する必要があります。

プロジェクト検証

2017年の終わりに、同僚のSvyatoslav RazmyslovがChromiumプロジェクトのソースコードをダウンロードし、何らかの方法でそれらを呼び出して、Visual Studioの生成プロジェクトとPVS-Studioレポートを提供しました。 残念ながら、Visual Studioでソリューションを操作することはできませんでした。 水曜日は5021プロジェクトを含む決定を生き延びませんでした。

すべてが非常に遅く、一般的にはしばらくすると環境が落ちます。 そのため、PVS-Studio Standaloneを使用してレポートを調査しました。 もちろん、これは通常のVisual Studio環境を使用するほど便利ではありませんが、許容範囲内です。

Chromiumプロジェクトは大きなプロジェクトであることを理解する必要があります。 それさえ言わなかった。 これは大きなプロジェクトです。

Chromiumプロジェクトとそこで使用されるライブラリは、114 201 CおよびC ++ファイルで構成されています。 コードの行数は30,263,757です。このうち、コメントは16％です。

PVS-Studioは、このサイズのプロジェクトがすでに成果であることを確認できます。

私が見つけたもの

年末年始が続いている間に、3晩にわたってレポートを調べ、約250のコードフラグメントを書きました。 レポートを注意深く確認する時間とエネルギーが見つからなかったことは認めます。 私は多くの警告を非常に迅速に調べ、何らかの種類のエラーにうんざりしているとき、いくつかをまったく無視しました。 これについては、次の章で詳しく説明します。

いくつかの記事を書くのに十分な多くの間違いを見つけたことは重要です。 最後の行を公開し終える頃には、プロジェクトで見つかったエラーに関する情報は少し古くなっているかもしれません。 しかし、それは問題ではありません。 私の目標は、 静的コード分析手法の機能を実証し、コーディングスタイルリーダーといくつかの推奨事項を共有することです。

Chromiumとライブラリの開発者が気づいたエラーを修正できるように、一連の記事の終わりを待たずに、別のファイルに書き込みました。 これは、おそらくすべてのエラーメッセージが記事に含まれるわけではないという理由で行われるべきです。

指摘された欠陥の説明があるファイルへのリンク： chroma.txt 。

なぜレポートを注意深く見なかったのですか？

誤検知の数を減らすためにアナライザーを調整しませんでした。 そのため、誤った警告のためにレポートを見ることができず、多くの場合、同じタイプのメッセージを見落とすことなく見落としていました。

さらに、エラーがあるかどうかがすぐにはわからないコードフラグメントはスキップしました。 メッセージはたくさんありますが、私は一人です。 コードを注意深く覗き始めたら、数か月後に記事を書くことになります。

特に不慣れなコードの場合、いくつかの警告を理解するのが難しい理由を例を挙げて説明します。 また、Chromiumでは、全体のコードがわかりません。

そのため、PVS-StudioアナライザーはV8プロジェクトファイルの1つに警告を発行しました。

V547 CWE-570「切り捨てられた」式は常に偽です。 objects.cc 2867

エラーが見つかりましたか、それとも誤検知ですか？ 自分で問題が何であるかを把握してみてください。 アナライザーが指す場所にコメント「// <=」を追加しました。

void String::StringShortPrint(StringStream* accumulator, bool show_details) { int len = length(); if (len > kMaxShortPrintLength) { accumulator->Add("<Very long string[%u]>", len); return; } if (!LooksValid()) { accumulator->Add("<Invalid String>"); return; } StringCharacterStream stream(this); bool truncated = false; if (len > kMaxShortPrintLength) { len = kMaxShortPrintLength; truncated = true; } bool one_byte = true; for (int i = 0; i < len; i++) { uint16_t c = stream.GetNext(); if (c < 32 || c >= 127) { one_byte = false; } } stream.Reset(this); if (one_byte) { if (show_details) accumulator->Add("<String[%u]: ", length()); for (int i = 0; i < len; i++) { accumulator->Put(static_cast<char>(stream.GetNext())); } if (show_details) accumulator->Put('>'); } else { // Backslash indicates that the string contains control // characters and that backslashes are therefore escaped. if (show_details) accumulator->Add("<String[%u]\\: ", length()); for (int i = 0; i < len; i++) { uint16_t c = stream.GetNext(); if (c == '\n') { accumulator->Add("\\n"); } else if (c == '\r') { accumulator->Add("\\r"); } else if (c == '\\') { accumulator->Add("\\\\"); } else if (c < 32 || c > 126) { accumulator->Add("\\x%02x", c); } else { accumulator->Put(static_cast<char>(c)); } } if (truncated) { // <= accumulator->Put('.'); accumulator->Put('.'); accumulator->Put('.'); } if (show_details) accumulator->Put('>'); } return; } 

わかった？ 難しいですか？

難しい そして、それがまさにアナライザーのすべての警告を研究できない理由です。

理解するのが面倒な人のために、私はポイントが何であるかを説明します。

そのため、アナライザーはif（truncated）が常にfalseであると主張します。 一番下の行を残して、関数を短くしましょう：

 void F() { int len = length(); if (len > kMaxShortPrintLength) return; bool truncated = false; if (len > kMaxShortPrintLength) truncated = true; if (truncated) { // <= accumulator->Put('.'); accumulator->Put('.'); accumulator->Put('.'); } } 

テキストが長すぎる場合、つまりif条件が満たされている場合（len> kMaxShortPrintLength） 、 切り捨てられたフラグはtrueでなければなりません。 ただし、テキストが長すぎる場合、上記の関数は終了します。

切り捨てが常にfalseであり、最後に3つのポイントが追加されないのはこのためです。

そして今でも、アナライザーが警告を出す理由を見つけたので、コードの書き方がわかりません。 または、実際には、すぐに関数を終了する必要があります。その場合、ポイントを追加するコードは不要です。 またはポイントが必要です。最初のチェックを削除する必要があります。これにより、スケジュールより早く機能が終了します。 サードパーティのコードのエラーを調べることは非常に困難です。

PVS-Studioアナライザーは、V547によって多くの警告を生成しました。 私は彼らの10番目の部分のどこかを見ました。 したがって、あなたが慎重に勉強することを約束した場合、私が書いたよりもはるかに多くのエラーが見つかります。

これらの警告にうんざりした理由のもう1つの例を次に示します。

 void ResourcePrefetcher::OnReadCompleted(net::URLRequest* request, int bytes_read) { DCHECK_NE(net::ERR_IO_PENDING, bytes_read); if (bytes_read <= 0) { FinishRequest(request); return; } if (bytes_read > 0) ReadFullResponse(request); } 

PVS-Studio警告：V547 CWE-571式 'bytes_read> 0'は常にtrueです。 resource_prefetcher.cc 308

前のケースとは異なり、すべてがシンプルです。 アナライザーは、2番目の条件が常に真であることを正確に述べています。

ただし、これはエラーではなく、冗長なコードです。 そのようなコードを編集する必要がありますか？ これは難しい質問です。 ちなみに、これがアナライザーの監視下ですぐにコードを書く方がはるかに良い理由であり、一度限りの起動で警告を勇敢に歩き回るのではありません。

アナライザーが定期的に使用された場合、そのような冗長なコードはバージョン管理システムに入り込まない可能性が高いでしょう。 プログラマは警告を見て、よりエレガントに書くでしょう。 たとえば、次のように：

 void ResourcePrefetcher::OnReadCompleted(net::URLRequest* request, int bytes_read) { DCHECK_NE(net::ERR_IO_PENDING, bytes_read); if (bytes_read <= 0) FinishRequest(request); else ReadFullResponse(request); } 

アナライザーはここでは沈黙しています。 同時に、コードは短くなり、シンプルになり、理解しやすくなりました。

V547に加えて、アナライザーは大量のV560メッセージを生成しました。 この警告は、すべての条件ではなく、その一部が常に真または偽であることを示しています。

私もこれらのメッセージを勉強するのに飽きていました。 これは、V560の警告が悪いことを意味するものではありません。 しかし、真に重大な間違いはまれです。 基本的に、これらの警告は低品質の冗長コードを示しています。

退屈な冗長チェックの例：

 template <typename ConditionT, typename ActionT> std::unique_ptr<DeclarativeRule<ConditionT, ActionT>> DeclarativeRule<ConditionT, ActionT>::Create(....) { .... bool bad_message = false; // <= std::unique_ptr<ActionSet> actions = ActionSet::Create( browser_context, extension, rule->actions, error, &bad_message); // <= if (bad_message) { // <= *error = "An action of a rule set had an invalid " "structure that should have been caught " "by the JSON validator."; return std::move(error_result); } if (!error->empty() || bad_message) // <= return std::move(error_result); .... } 

PVS-Studio警告：V560 CWE-570条件式の一部は常にfalseです：bad_message。 declarative_rule.h 472

条件：

 if (!error->empty() || bad_message) 

次のように簡略化できます。

 if (!error->empty()) 

次のようなコードを書き換えるオプションもあります。

  if (bad_message) { *error = "An action of a rule set had an invalid " "structure that should have been caught " "by the JSON validator."; } if (!error->empty() || bad_message) return std::move(error_result); 

なぜこのレポートを注意深く研究しなかったのかを説明できるといいのですが。 これは多くの時間のかかる作業です。

偽陽性率

誤検知の割合を言うことはできません。 まず、ログを最後まで見ることができず、PVS-Studioで検出されたエラーの正確な数がわかりません。 第二に、最初にアナライザーをセットアップせずに誤検知の割合について話すことは意味がありません。

PVS-Studioアナライザーを構成すると、誤検知の10〜15％が予想されます。 このような構成の例は、記事「 EFLコアライブラリの例を使用したPVS-Studioアナライザー仕様、誤検知の10〜15％ 」で説明されています。

もちろん、Chromiumに対してこのような設定を行うこともできますが、記事でいくつかの数値を示すためだけにこれを行うのは非合理的です。 これは私たちが準備ができている多くの仕事ですが、有料です。 Googleは、アナライザーを構成すると同時に、見つかったすべてのエラーを修正するようにチームを引き付ける可能性があります。 はい、これをヒントと考えてください。

カスタマイズは間違いなく可能であり、良い結果が得られます。 たとえば、すべての誤検知の約半分は、コードでのDCHECKマクロの使用に関連付けられています。

このマクロは次のようになります。

 #define LAZY_STREAM(stream, condition) \ !(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream) #define DCHECK(condition) \ LAZY_STREAM(LOG_STREAM(DCHECK), !ANALYZER_ASSUME_TRUE(condition))\ << "Check failed: " #condition ". " 

アナライザーの観点から見ると、PVS-Studioは、ある種の条件と一連のアクションをチェックするだけであり、その後、残りのコードは引き続き実行されます。

その結果、アナライザーは、たとえば次のようなコードに対して誤った警告を生成します。

 bool Value::Equals(const Value* other) const { DCHECK(other); return *this == *other; } 

PVS-Studioのレポート： V1004 CWE-476 nullptrに対して検証された後、「その他」のポインターが安全に使用されませんでした。 行を確認：621、622。values.cc 622

アナライザーの観点から、 他のポインターはnullptrの同等性についてチェックされます。 ただし、 otherが NULLポインターであるかどうかに関係なく、逆参照が発生します。 アナライザーは、そのようなアクションは疑わしいと見なします。

DCHECKマクロは、 アサートマクロの一種です。 しかし、アナライザーがassertを知っている場合、 DCHECKとは何か-理解していません。 何が起こっているかをより良く説明するために、擬似コードを書きます。

 bool Equals(T* ptr) const { if (!ptr) LogMessage(); return *this == *ptr; } 

これは、PVS-Studioアナライザーがコードを認識する方法です。 最初に、 nullptrの同等性についてポインターがチェックされます。 ポインターがヌルの場合、 LogMessage関数が呼び出されます。 ただし、関数は制御を返さないとマークされていません。 つまり、 ptrが NULLポインターであるかどうかに関係なく、関数は実行を継続します。

次に、ポインターが逆参照されます。 しかし、ゼロに等しいかどうかをチェックするチェックがありました！ したがって、ポインターがヌルである可能性があり、アナライザーはコード内の問題を通知します。 これにより、アナライザーは完全に正しいが役に立たないメッセージを大量に生成します。

ところで、このようなマクロの実装はPVS-Studioだけでなく混乱を招きます。 そのため、Visual Studioに組み込まれたアナライザー用に特別な「バックアップ」が作成されました。

 #if defined(_PREFAST_) && defined(OS_WIN) // See comments on the previous use of __analysis_assume. #define DCHECK(condition) \ __analysis_assume(!!(condition)), \ LAZY_STREAM(LOG_STREAM(DCHECK), false) \ << "Check failed: " #condition ". " #define DPCHECK(condition) \ __analysis_assume(!!(condition)), \ LAZY_STREAM(PLOG_STREAM(DCHECK), false) \ << "Check failed: " #condition ". " #else // !(defined(_PREFAST_) && defined(OS_WIN)) 

PVS-Studioアナライザーに対して同様のバックアップを作成すると、誤検知の状況が劇的に変わります。 私の推定によると、誤検知の半分はすぐに消えます。 はい、ちょうど半分。 問題は、 DCHECKマクロが膨大な回数使用されることです。

その他の出版物

ここで紹介記事が終了します。ここでは、他の記事へのリンクを徐々に配置します。 ご清聴ありがとうございました。

1. 美しいChromiumと不器用なmemset 。
2. ブレークおよびフォールスルーステートメント 。
3. Chromium：メモリリーク 。
4. クロム：タイプミス 。
5. Chromium：不正確なデータの使用 。
6. malloc関数が何を返したかを確認することが重要なのはなぜですか 。
7. Chromium：その他のエラー 。

この記事を英語圏の聴衆と共有したい場合は、翻訳へのリンクを使用してください：Andrey Karpov。 Chromium：プロジェクトの6番目のチェックについて 。