PVS-Studio対Chromium

今回は勝利が良かった。 むしろ、Chromiumプロジェクトのソースコード。 Chromiumは、PVS-Studioを使用してテストした最高のプロジェクトの1つです。



Chromiumは、ユーザーが高速で安全なインターネットアクセスを提供できるように設計された、Googleが開発したオープンソースのWebブラウザです。 Chromiumに基づいて、Google Chromeブラウザーが作成されます。 同時に、ChromiumはGoogle Chromeの予備バージョンであり、他の多くの代替Webブラウザーでもあります。



プログラマーの観点から見ると、Chromiumは473のプロジェクトで構成されるソリューションです。 C / C ++のソースコードの総量は約460メガバイトです。 行数を計算するのは困難です。



これらの460メガバイトには、さまざまなライブラリが多数含まれています。 それらを分離すると、約155メガバイトが残ります。 大幅に減りましたが、それでもたくさんあります。 さらに、すべては相対的です。 これらのライブラリの多くは、Chromiumを作成するタスクの一部としてChromium開発者によって作成されました。 このようなライブラリは独自に存在しますが、ブラウザ自体に簡単に帰属させることができます。



Chromiumは、 PVS-Studioのテスト中に出会った最大かつ最も高品質のプロジェクトです。 Chromiumプロジェクトで作業しているとき、実際に誰が誰をチェックしているかはあまり明確ではありませんでした。 C ++ファイルの分析とプロジェクトの特定の構造のサポートに関連するPVS-Studioのいくつかのエラーを見つけて修正しました。



ソースコードの品質Chromiumでは、多くのポイントとテクニックが使用されています。 たとえば、ほとんどのプログラマは、次の構成を使用して配列内の要素の数を決定します。

int XX[] = { 1, 2, 3, 4 }; size_t N = sizeof(XX) / sizeof(XX[0]);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

これは通常、次の形式のマクロで実行されます。

 #define count_of(arg) (sizeof(arg) / sizeof(arg[0]))
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

これは完全に機能する便利なマクロです。 正直なところ、私自身は常にこのようなマクロを使用しています。 ただし、単純なポインターを誤って滑ってしまう可能性があり、気にしないので、エラーの原因になる可能性があります。 例で説明します。

 void Test(int C[3]) { int A[3]; int *B = Foo(); size_t x = count_of( A ); // Ok x = count_of( B ); // Error x = count_of( C ); // Error }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

count_of（A）コンストラクトは正しく機能し、配列Aの要素数3を返します。



しかし、誤ってcount_of（）をポインターに適用すると、結果は無意味な値になります。 問題は、マクロがcount_of（B）の形式の奇妙な構成についてプログラマに警告しないことです。 ポインターのサイズは、配列要素のサイズで除算されます。 この状況は、とてつもなく人為的であるように見えますが、実際のアプリケーションでそれを満たしました。 Miranda IMプロジェクトの例を挙げます。

 #define SIZEOF(X) (sizeof(X)/sizeof(X[0])) int Cache_GetLineText(..., LPTSTR text, int text_size, ...) { ... tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, SIZEOF(text), 0); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

そのため、そのようなエラーにはかなりの場所があり、それらを防御できると便利です。 引数として渡された配列のサイズを計算しようとすると、間違いを犯しやすくなります。

 void Test(int C[3]) { x = count_of( C ); // Error }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

C ++言語標準によれば、変数「C」は通常のポインターであり、配列ではありません。 その結果、プログラムでは、転送された配列の一部のみを処理することがよくあります。

このようなエラーについて話しているので、送信された配列のサイズを調べる方法を説明します。 これを行うには、リンクで渡します：

 void Test(int (&C)[3]) { x = count_of( C ); // Ok }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

これで、式count_of（C）の結果は3です。



Chromiumに戻ります。 上記のエラーを回避するマクロを使用します。 その実装は次のとおりです。

 template <typename T, size_t N> char (&ArraySizeHelper(T (&array)[N]))[N]; #define arraysize(array) (sizeof(ArraySizeHelper(array)))
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

この魔法の呪文のアイデアは次のとおりです。 ArraySizeHelperテンプレート関数は、長さNの任意のタイプの配列の入力を受け入れます。この関数は、長さNの「char」要素の配列を返します。必要がないため、関数の実装はありません。 sizeof（）演算子の場合、ArraySizeHelper関数を宣言するだけで十分です。 'arraysize'マクロは、ArraySizeHelper関数によって返されるバイト配列のサイズを計算します。 このサイズは、長さを計算する配列内の要素の数です。



あなたの頭が腫れている場合、それが機能するという私の言葉を取ることができます。 また、以前に考慮されたマクロ「count_of（）」よりもはるかに優れた動作をします。 ArraySizeHelper関数は参照によって配列を取得するため、単純なポインターを渡すことはできません。 テストコードを書きましょう：

 template <typename T, size_t N> char (&ArraySizeHelper(T (&array)[N]))[N]; #define arraysize(array) (sizeof(ArraySizeHelper(array))) void Test(int C[3]) { int A[3]; int *B = Foo(); size_t x = arraysize( A ); // Ok x = arraysize( B ); //   x = arraysize( C ); //   }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

間違ったコードは単にコンパイルされません。 私の意見では、コンパイル段階で潜在的なエラーを防ぐことができれば、これは素晴らしいことです。 これは、プログラミングアプローチの品質を反映した素晴らしい例です。 Googleの開発者に対する私の敬意。



別の例を挙げましょう。完全に異なる計画ですが、コードの品質についても説明します。

 if (!file_util::Delete(db_name, false) && !file_util::Delete(db_name, false)) { // Try to delete twice. If we can't, fail. LOG(ERROR) << "unable to delete old TopSites file"; return false; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このコードは多くのプログラマーにとって奇妙に見えるかもしれません。 ファイルを2回削除しようとする意味は何ですか？ しかし、感覚があります。 それを書いた人は、プログラムであることの啓発と本質を達成しました。 ファイルは間違いなく削除されるか、教科書と抽象的な世界でのみ削除されません。 実際のシステムでは、ファイルを削除できず、しばらくしてから-可能です。 これの理由は、ウイルス対策、ウイルス、バージョン管理システム、そして神は他に何を知っているかもしれません。 プログラマーはしばしばそのような状況について考えません。 彼らはこのように考えます。ファイルを削除することは遠隔ではないため、機能しないことを意味します。 ただし、カタログを整理するのではなく、うまくやりたい場合は、これらの無関係な影響を考慮する必要があります。 ファイルが1000起動ごとに1回削除されない場合、まったく同じ状況に遭遇しました。 そして、決定はまったく同じでした。 まあ、念のためにSleep（0）が中央に挿入されている場合を除きます。



しかし、PVS-Studioで確認するのはどうですか？ Chromiumコードは、おそらく私が見た中で最高の品質のコードです。 これは、発見できる非常に低いエラー密度によって確認されます。 定量的に考えると、もちろん多くの間違いがあります。 しかし、このエラー数をコードの量で割ると、実際には何もないことがわかります。 これらの間違いは何ですか？ 最も普通。 いくつかの例：

 V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. platform time_win.cc 116
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 void NaCl::Time::Explode(bool is_local, Exploded* exploded) const { ... ZeroMemory(exploded, sizeof(exploded)); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

タイプミスはそれをすべて行います。 彼らはただ星を忘れました。 次のようになっているはずです：sizeof（*展開）。

 V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 static const int kClientEdgeThickness; int height() const; bool ShouldShowClientEdge() const; void CustomFrameView::PaintMaximizedFrameBorder(gfx::Canvas* canvas) { ... int edge_height = titlebar_bottom->height() - ShouldShowClientEdge() ? kClientEdgeThickness : 0; ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

陰湿な演算子「？：」は、減算よりも優先順位が低くなります。 ここには追加のブラケットが必要です。

 int edge_height = titlebar_bottom->height() - (ShouldShowClientEdge() ? kClientEdgeThickness : 0);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

意味をなさないチェック。

 V547 Expression 'count < 0' is always false. Unsigned type value is never < 0. ncdecode_tablegen ncdecode_tablegen.c 197
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 static void CharAdvance(char** buffer, size_t* buffer_size, size_t count) { if (count < 0) { NaClFatal("Unable to advance buffer by count!"); } else { ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

条件「count <0」は常にfalseです。 保護は機能せず、一部のバッファーがいっぱいになる可能性があります。 ところで、これは、静的アナライザーを使用して脆弱性を検索する方法の例です。 攻撃者は、さらに注意深い分析のために、エラーを含むコードのセクションを自分ですばやく特定できます。 セキュリティの観点から興味深いと思われる別のコードを次に示します。

 V511 The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof (salt)' expression. common visitedlink_common.cc 84
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 void MD5Update(MD5Context* context, const void* buf, size_t len); VisitedLinkCommon::Fingerprint VisitedLinkCommon::ComputeURLFingerprint( ... const uint8 salt[LINK_SALT_LENGTH]) { ... MD5Update(&ctx, salt, sizeof(salt)); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

MD5Update（）関数は、ポインターが占有するバイト数を処理します。 データ暗号化に関して潜在的な穴はありますか？ このすべてに危険があるかどうかはわかりません。 しかし、攻撃者の観点から見ると、これは間違いなくより詳細な調査のための興味深い場所です。



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

 MD5Update(&ctx, salt, sizeof(salt[0]) * LINK_SALT_LENGTH);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

またはこのように：

 VisitedLinkCommon::Fingerprint VisitedLinkCommon::ComputeURLFingerprint( ... const uint8 (&salt)[LINK_SALT_LENGTH]) { ... MD5Update(&ctx, salt, sizeof(salt)); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

別のタイプミスの例：

 V501 There are identical sub-expressions 'host != buzz::XmlConstants::str_empty ()' to the left and to the right of the '&&' operator. chromoting_jingle_glue iq_request.cc 248
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 void JingleInfoRequest::OnResponse(const buzz::XmlElement* stanza) { ... std::string host = server->Attr(buzz::QN_JINGLE_INFO_HOST); std::string port_str = server->Attr(buzz::QN_JINGLE_INFO_UDP); if (host != buzz::STR_EMPTY && host != buzz::STR_EMPTY) { ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

実際、port_str変数を確認する必要があります。

 if (host != buzz::STR_EMPTY && port_str != buzz::STR_EMPTY) {
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

古典から少し：

 V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 bool NpProxyService::GetProxyValueJSONString(std::string* output) { DCHECK(output); output->empty(); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

次のようになります：output-> clear（）;



ただし、nullポインターを使用することもできます。

 V522 Dereferencing of the null pointer 'plugin_instance' might take place. Check the logical condition. chrome_frame_npapi chrome_frame_npapi.cc 517
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 bool ChromeFrameNPAPI::Invoke(...) { ChromeFrameNPAPI* plugin_instance = ChromeFrameInstanceFromNPObject(header); if (!plugin_instance && (plugin_instance->automation_client_.get())) return false; ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

動作しない別のテスト例：

 V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 IdleState CalculateIdleState(unsigned int idle_threshold) { ... DWORD current_idle_time = 0; ... // Will go -ve if we have been idle for a long time (2gb seconds). if (current_idle_time < 0) current_idle_time = INT_MAX; ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

おそらく停止する必要があります。 私は続けることができますが、それは退屈になります。 そして、これはChromium自体に関連するものです。 ただし、次のようなエラーを含むテストはまだあります。

 V554 Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. interactive_ui_tests accessibility_win_browsertest.cc 306
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 void AccessibleChecker::CheckAccessibleChildren(IAccessible* parent) { ... auto_ptr<VARIANT> child_array(new VARIANT[child_count]); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

また、Chromiumが構築されている膨大な数のライブラリ。 さらに、ライブラリのサイズはChromium自体のサイズよりもはるかに大きくなります。 また、コードには多くの興味深い場所があります。 おそらく、エラーコードはどこでも使用されていないことは明らかですが、これによるエラーはエラーではなくなりません。 最初に出くわす例（ICUライブラリー）：

 V547 Expression '* string != 0 || * string != '_'' is always true. Probably the '&&' operator should be used here. icui18n ucol_sit.cpp 242
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

 U_CDECL_BEGIN static const char* U_CALLCONV _processVariableTop(...) { ... if(i == locElementCapacity && (*string != 0 || *string != '_')) { *status = U_BUFFER_OVERFLOW_ERROR; } ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

式「（* string！= 0 || * string！= '_'）」は常に真です。 どうやら：（* string == 0 || * string == '_'）。

おわりに

PVS-Studioは敗北しました。 Chromiumコードは、分析した最高のコードの1つです。 Chromiumにはほとんど何も見つかりませんでした。 むしろ、多くの間違いを発見しましたが、この記事ではそれらのほんの一部を示しました。 しかし、これらのエラーがすべて460メガバイトのソースコードに分散していることを考慮すると、実際には何もないことがわかります。



PS



質問に答えます。見つかったバグをChromium開発者に通知しますか？ いいえ、お知らせしません。 これは非常に大量の作業であり、無料で行うことはできません。 Chromiumチェックは、Miranda IM チェックでもUltimate Toolboxチェックでもありません。 これは大きな仕事です。すべてのメッセージを注意深く調べて、これが本当に間違いかどうかを判断する必要があります。 これを行うには、プロジェクトをナビゲートする必要があります。 この記事の翻訳をChromium開発者に転送します。興味がある場合は、プロジェクト自体を分析し、すべての診断メッセージを分析できます。 はい。このため、PVS-Studioを購入する必要があります。 ただし、Googleのどの部門でも簡単に購入できます。



PPS



いいえ、私たちは貪欲ではありません。 FlylinkDC ++のようなオープンソースプロジェクトを支援する準備ができています 。 しかし、これらは2つの異なるものです。