PVS-Studioアナライザーを使用した3番目のChromiumプロジェクトコードの検証

Chromiumブラウザは非常に高速に開発されています。 たとえば、2011年にこのプロジェクト（ソリューション）を初めてテストしたとき、473プロジェクトで構成されていました。 現在、すでに1169個のプロジェクトで構成されています。 Googleの開発者がChromiumの開発のスピードで最高品質のコードを維持できるかどうか疑問に思っていました。 はい、できます。

クロム

Chromiumは、Googleが開発したオープンソースのウェブブラウザです。 Chromiumに基づいて、Google Chromeブラウザーが作成されます。 Get the Codeページで、このプロジェクトのソースコードをダウンロードする方法を学ぶことができます。

一般的な情報

以前はChromiumプロジェクトを確認していましたが、これについては2つの記事があります。 最初の チェック （05/23/2011）、 2番目のチェック （13/10/2011）です。 そして、常にエラーを見つけました。 これは、コードアナライザーの利点に関する微妙なヒントです。



現在（プロジェクトのソースコードは2013年7月にダウンロードされています）Chromiumは1169プロジェクトで構成されています 。 C / C ++のソースコードの総量は260メガバイトです。 これに加えて、使用済みの外部ライブラリをさらに450メガバイト追加できます。



2011年にChromiumプロジェクトの最初のテストを行った場合、外部ライブラリ全体のボリュームは変わらなかったことがわかります。 しかし、プロジェクト自体のコードは155メガバイトから260メガバイトに大きく成長しました。

楽しみのために、循環的複雑度を計算しましょう

PVS-Studioアナライザーには、 サイクロマティックの複雑さが非常に大きい関数を検索する機能があります 。 通常、このような関数はリファクタリングの候補です。 1,160件のプロジェクトをチェックした後、私は当然、ノミネートの「最も複雑な機能」のチャンピオンとなるプロジェクトに興味を持ちました。



2782に等しい最高の循環的複雑度は、ChromiumプロジェクトのValidateChunkAMD64（）関数に属します。 しかし、彼女は競技会から失格にならなければなりませんでした。 この関数は、自動生成されるvalidator_x86_64.cファイルにあります。 残念だ。 そして、それは壮大な記録保持者になるでしょう。 このような循環的な複雑さに出会ったことはありません。



したがって、最初の3つの場所は次の機能を受け取ります。

1. WebKitライブラリ。 htmltokenizer.cppファイルのHTMLTokenizer :: nextToken（）関数。 循環困難1106 。
2. メサ図書館。 glsl_lexer.ccファイルの_mesa_glsl_lex（）関数。 サイクロマティック難易度1088 。
3. Usrsctplibライブラリ（未知のアスリート）。 htmltokenizer.cppファイルのsctp_setopt（）関数。 サイクロマティック難易度1026 。

循環的複雑度が1000であることが誰かにわからない場合は、知らないでください。 精神的健康は良くなります:)。 一般的に、これの多く。

コード品質

Chromiumプロジェクトコードの品質については何が言えますか？ 品質はまだ優れています。 はい、大規模なプロジェクトと同様に、常にエラーを見つけることができます。 しかし、その数をコードの量で割ると、その密度は無視できます。 これは、エラーの少ない非常に優れたコードです。 きれいなコードにメダルを与えます。 前のメダルは、MicrosoftのCasablancaプロジェクト（C ++ REST SDK）に送られました。



図1. Chromiumの作成者へのメダル。



会社では、Chromiumとともに、それに含まれるサードパーティライブラリがチェックされました。 しかし、それらで見つかったエラーを説明することは興味深いことではありません。 さらに、レポートを非常に表面的に見ました。 いいえ、私は悪い人ではありません。 1169プロジェクトの検証に関するレポートを完全に研究しようとした場合、私はあなたを見ます。 クイックルックで気づいたことを、エラーのデータベース例に入れました。 この記事では、Chromium自体のコード（プラグインなど）で気づいたエラーのみに触れたいと思います。



Chromiumプロジェクトは非常に優れているため、見つかったエラーの例を挙げるのはなぜですか？ すべてが非常に簡単です。 PVS-Studioアナライザーの能力を実証したいと思います。 彼がChromiumでエラーを見つけることができた場合、このツールは注目に値します。



アナライザーは、合計で710メガバイトの何万ものファイルを噛み砕くことができましたが、これにとらわれませんでした。 プロジェクトは高度な資格を持つ開発者によって開発され、さまざまなツールによってチェックされているという事実にもかかわらず、PVS-Studioは依然として欠陥を特定することができました。 これは素晴らしい成果です！ そして最後に-並列検証（AMD FX-8320 / 3.50 GHz / 8コアプロセッサ、16.0 GB RAM）により、妥当な時間（約5時間）でそれを行いました。

見つかったバグの一部

レポートを表示するときに私の目が落ち着いたコード例を検討することを提案します。 詳細な調査により、もっと興味深いものを見つけることができると確信しています。

見られるN1-タイプミス

Vector3dF Matrix3F::SolveEigenproblem(Matrix3F* eigenvectors) const { // The matrix must be symmetric. const float epsilon = std::numeric_limits<float>::epsilon(); if (std::abs(data_[M01] - data_[M10]) > epsilon || std::abs(data_[M02] - data_[M02]) > epsilon || std::abs(data_[M12] - data_[M21]) > epsilon) { NOTREACHED(); return Vector3dF(); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V501 '-'演算子の左右に同じ副次式があります：data_ [M02]-data_ [M02] matrix3_f.cc 128



3x3行列が対称であることを確認する必要があります。



図2. 3x3マトリックス。



これを行うには、次の要素を比較します。

• M01およびM10
• M02およびM20
• M12およびM21

ほとんどの場合、コードはCopy-Pasteテクノロジーを使用して記述されています 。 その結果、セルM02はそれ自体と比較されます。 これがそのような楽しい行列クラスです。



別の簡単なタイプミス：

 bool IsTextField(const FormFieldData& field) { return field.form_control_type == "text" || field.form_control_type == "search" || field.form_control_type == "tel" || field.form_control_type == "url" || field.form_control_type == "email" || field.form_control_type == "text"; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V501「||」の左側と右側に同じサブ式「field.form_control_type == "text"」があります 演算子。 autocomplete_history_manager.cc 35



2回、文字列 "text"との比較があります。 これは疑わしいです。 たぶん、1行だけでは不要です。 または、他の比較は必要ありません。

見られるN2-反対の条件

 static void ParseRequestCookieLine( const std::string& header_value, ParsedRequestCookies* parsed_cookies) { std::string::const_iterator i = header_value.begin(); .... if (*i == '"') { while (i != header_value.end() && *i != '"') ++i; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V637反対の2つの条件が発生しました。 2番目の条件は常にfalseです。 行を確認してください：500、501。web_request_api_helpers.cc 500



このコードは、二重引用符で囲まれたテキストを渡す必要があるように思えます。 しかし実際には、このコードは何もしません。 条件はすぐに偽です。 わかりやすくするために、エラーの本質を強調する擬似コードを作成します。

 if ( A == 'X' ) { while ( .... && A != 'X' ) ....;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ほとんどの場合、彼らはここでポインタを1文字に移動するのを忘れており、コードは次のようになっているはずです。

 if (*i == '"') { ++i; while (i != header_value.end() && *i != '"') ++i;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

見られるN3-削除要素の失敗

 void ShortcutsProvider::DeleteMatchesWithURLs( const std::set<GURL>& urls) { std::remove_if(matches_.begin(), matches_.end(), RemoveMatchPredicate(urls)); listener_->OnProviderUpdate(true); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V530関数 'remove_if'の戻り値を使用する必要があります。 shortcuts_provider.cc 136



コンテナから要素を削除するには、std :: remove_if（）関数を使用します。 しかし、それは誤って使用されます。 実際、remove_if（）は何も削除しません。 要素を先頭に移動し、反復子をゴミ箱に戻します。 コンテナでerase（）関数を呼び出して、ごみを自分で削除する必要があります。 ウィキペディアの記事「 イレーズ削除イディオム 」も参照してください。



正しいコードは次のとおりです。

 matches_.erase(std::remove_if(.....), matches_.end());
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Seen N4-SOCKETとの永遠の混乱

Linuxの世界では、SOCKETは整数のSIGNATURE DATAタイプです。



Windowsの世界のSOCKETは、整数のUNSIGNABLEデータ型です。



Visual C ++ヘッダーファイルでは、SOCKET型は次のように宣言されます。

 typedef UINT_PTR SOCKET;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

しかし、彼らは常にそれを忘れて、次の形式のコードを書きます。

 class NET_EXPORT_PRIVATE TCPServerSocketWin { .... SOCKET socket_; .... }; int TCPServerSocketWin::Listen(....) { .... socket_ = socket(address.GetSockAddrFamily(), SOCK_STREAM, IPPROTO_TCP); if (socket_ < 0) { PLOG(ERROR) << "socket() returned an error"; return MapSystemError(WSAGetLastError()); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V547式 'socket_ <0'は常にfalseです。 符号なしの型の値が0未満になることはありませんtcp_server_socket_win.cc 48



符号なし変数は常にゼロ以上です。 これは、「socket_ <0」のチェックが意味をなさないことを意味します。 プログラムの動作中にソケットを開くことができない場合、この状況は正しく処理されません。

N5を参照-操作との混乱〜および！

 enum FontStyle { NORMAL = 0, BOLD = 1, ITALIC = 2, UNDERLINE = 4, }; void LabelButton::SetIsDefault(bool is_default) { .... style = is_default ? style | gfx::Font::BOLD : style & !gfx::Font::BOLD; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V564「＆」演算子はブール型の値に適用されます。 括弧を含めるのを忘れているか、「&&」演算子を使用することを意図している可能性があります。 label_button.cc 131



コードは次のように機能するはずだったようです。

• 変数「is_default」がtrueの場合、BOLDスタイルを担当するビットを常に設定する必要があります。
• 変数 'is_default'がfalseの場合、BOLDスタイルの原因であるビットを常にクリアする必要があります。

ただし、「style＆！Gfx :: Font :: BOLD」という表現は、プログラマが期待するとおりには機能しません。 操作の結果！Gfx :: Font :: BOLDはfalseになります。 または、言い換えると0です。上記のコードはこれと同等です。

 style = is_default ? style | gfx::Font::BOLD : 0;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

コードが正しく機能するためには、操作「〜」を使用する必要があります。

 style = is_default ? style | gfx::Font::BOLD : style & ~gfx::Font::BOLD;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

見られるN6-一時オブジェクトの疑わしい作成

 base::win::ScopedComPtr<IDirect3DSurface9> scaler_scratch_surfaces_[2]; bool AcceleratedSurfaceTransformer::ResizeBilinear( IDirect3DSurface9* src_surface, ....) { .... IDirect3DSurface9* read_buffer = (i == 0) ? src_surface : scaler_scratch_surfaces_[read_buffer_index]; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V623「？：」演算子の検査を検討してください。 「ScopedComPtr」タイプの一時オブジェクトが作成され、その後破棄されます。 第2オペランドを確認してください。 accelerator_surface_transformer_win.cc 391



このコードでエラーが発生することはほとんどありませんが、説明する価値があります。 一部のプログラマは、C ++言語の新しい興味深いトラップについて学習するようです。



一見、すべてがシンプルです。 条件に応じて、ポインター「src_surface」または配列「scaler_scratch_surfaces_」の要素のいずれかを選択します。 配列は、base :: win :: ScopedComPtr <IDirect3DSurface9>型のオブジェクトで構成され、IDirect3DSurface9へのポインターに自動的にキャストできます。



悪魔は詳細にあります。



三項演算子 '？：'条件に応じて異なる型を返すことはできません。 簡単な例で説明します。

 int A = 1; auto X = v ? A : 2.0;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The ?:演算子は「double」のタイプを返します。 その結果、変数 'X'もdouble型になります。 しかし、それは問題ではありません。 変数「A」を暗黙的に展開して「double」を入力することが重要です！



次のような記述を行うと、問題が発生します。

 CString s1(L"1"); wchar_t s2[] = L"2"; bool a = false; const wchar_t *s = a ? s1 : s2;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このコードの実行の結果、変数 's'は、CString型の一時オブジェクト内のデータを示します。 問題は、このオブジェクトがすぐに破棄されることです。



Chromiumのソースコードに戻ります。

 IDirect3DSurface9* read_buffer = (i == 0) ? src_surface : scaler_scratch_surfaces_[read_buffer_index];
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここで、条件 'i == 0'が満たされると、次のことが起こります。

• base型の一時オブジェクト:: win :: ScopedComPtr <IDirect3DSurface9>は、ポインター 'src_surface'から構築されます。
• 一時オブジェクトは暗黙的にIDirect3DSurface9ポインターにキャストされ、read_buffer変数に配置されます。
• 一時オブジェクトは破棄されます。

私はプログラムのロジックとScopedComPtrクラスを知らないので、否定的な結果が生じるかどうかを言うのは難しいと思います。 ほとんどの場合、コンストラクターではリンク数のカウンターが増加し、デストラクタでは減少します。 そして、すべてがうまくいきます。



そうでない場合、誤って無効なポインタを取得したり、参照カウントを壊したりする可能性があります。



一言で言えば、たとえ間違いがなくても、読者が何か新しいことを学べたら嬉しいです。 三項演算子は、思ったよりもはるかに危険です。



ここにそのような疑わしい場所があります：

 typedef GenericScopedHandle<HandleTraits, VerifierTraits> ScopedHandle; DWORD HandlePolicy::DuplicateHandleProxyAction(....) { .... base::win::ScopedHandle remote_target_process; .... HANDLE target_process = remote_target_process.IsValid() ? remote_target_process : ::GetCurrentProcess(); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V623「？：」演算子の検査を検討してください。 「GenericScopedHandle」タイプの一時オブジェクトが作成され、その後破棄されます。 第3オペランドを確認してください。 handle_policy.cc 81

見られるN7-重複チェック

 string16 GetAccessString(HandleType handle_type, ACCESS_MASK access) { .... if (access & FILE_WRITE_ATTRIBUTES) output.append(ASCIIToUTF16("\tFILE_WRITE_ATTRIBUTES\n")); if (access & FILE_WRITE_DATA) output.append(ASCIIToUTF16("\tFILE_WRITE_DATA\n")); if (access & FILE_WRITE_EA) output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n")); if (access & FILE_WRITE_EA) output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n")); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V581互いに並んでいる「if」演算子の条件式は同一です。 行を確認してください：176、178。handle_enumerator_win.cc 178



FILE_WRITE_EAフラグが設定されている場合、ドレイン「\ tFILE_WRITE_EA \ n」が2回追加されます。 非常に疑わしいコード。



同様の奇妙な画像がここで観察できます：

 static bool PasswordFormComparator(const PasswordForm& pf1, const PasswordForm& pf2) { if (pf1.submit_element < pf2.submit_element) return true; if (pf1.username_element < pf2.username_element) return true; if (pf1.username_value < pf2.username_value) return true; if (pf1.username_value < pf2.username_value) return true; if (pf1.password_element < pf2.password_element) return true; if (pf1.password_value < pf2.password_value) return true; return false; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V581互いに並んでいる「if」演算子の条件式は同一です。 行を確認してください：259、261。profile_sync_service_password_unittest.cc 261



「pf1.username_value <pf2.username_value」チェックは2回繰り返されます。 たぶん、1行だけでは不要です。 あるいは、他の何かをチェックするのを忘れて、まったく異なる条件を記述する必要があります。

見られるN8-「ワンタイム」サイクル

 ResourceProvider::ResourceId PictureLayerImpl::ContentsResourceId() const { .... for (PictureLayerTilingSet::CoverageIterator iter(....); iter; ++iter) { if (!*iter) return 0; const ManagedTileState::TileVersion& tile_version = ....; if (....) return 0; if (iter.geometry_rect() != content_rect) return 0; return tile_version.get_resource_id(); } return 0; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V612ループ内の無条件の「戻り」。 picture_layer_impl.cc 638



このサイクルには何か問題があります。 ループは1回だけ反復を実行します。 ループの最後には、無条件のreturnステートメントがあります。 考えられる理由：

• だから、それは構想されています。 しかし、それは非常に疑わしいです。 ループを作成したり、反復子を作成したりするのはなぜですか。
• 「リターン」の1つではなく、「継続」を記述する必要があります。 しかしまた疑わしい。
• 最も可能性が高いのは、最後の「リターン」の前に何らかの条件が欠落していることです。

1回だけ実行される他の奇妙なループがあります。

 scoped_ptr<ActionInfo> ActionInfo::Load(....) { .... for (base::ListValue::const_iterator iter = icons->begin(); iter != icons->end(); ++iter) { std::string path; if (....); return scoped_ptr<ActionInfo>(); } result->default_icon.Add(....); break; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V612ループ内の無条件の「ブレーク」。 action_info.cc 76

 const BluetoothServiceRecord* BluetoothDeviceWin::GetServiceRecord( const std::string& uuid) const { for (ServiceRecordList::const_iterator iter = service_record_list_.begin(); iter != service_record_list_.end(); ++iter) { return *iter; } return NULL; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V612ループ内の無条件の「戻り」。 bluetooth_device_win.cc 224

N9で表示-初期化されていない変数

 HRESULT IEEventSink::Attach(IWebBrowser2* browser) { DCHECK(browser); HRESULT result; if (browser) { web_browser2_ = browser; FindIEProcessId(); result = DispEventAdvise(web_browser2_, &DIID_DWebBrowserEvents2); } return result; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V614潜在的に初期化されていない変数 'result'が使用されました。 ie_event_sink.cc 240



「ブラウザ」ポインタがゼロの場合、関数は初期化されていない変数を返します。



別のコード：

 void SavePackage::GetSaveInfo() { .... bool skip_dir_check; .... if (....) { ....->GetSaveDir(...., &skip_dir_check); } .... BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(..., skip_dir_check, ...)); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V614潜在的に初期化されていない変数 'skip_dir_check'が使用されました。 「バインド」関数の5番目の実引数を確認することを検討してください。 save_package.cc 1326



変数 'skip_dir_check'は初期化されないままになる場合があります。

N10に注意-コードのアライメントがその操作のロジックと一致しない

 void OnTraceNotification(int notification) { if (notification & TraceLog::EVENT_WATCH_NOTIFICATION) ++event_watch_notification_; notifications_received_ |= notification; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V640コードの操作ロジックがそのフォーマットに対応していません。 ステートメントは右側にインデントされますが、常に実行されます。 中括弧が欠落している可能性があります。 trace_event_unittest.cc 57



このようなコードを考えると、中括弧がここで忘れられているかどうかは明らかではありません。 コードが正しい場合でも、他のプログラマーが思慮深い状態にならないように修正する必要があります。



ここに、非常に疑わしいコード配置がある場所をいくつか示します。

• nss_memio.c 152
• nss_memio.c 184

N11に注目-新しい後のポインターチェック

多くのプログラムには、「new」演算子が例外をスローしなかった時代に書き戻された古いレガシーコードがあります。 以前は、メモリが不十分な場合、nullポインタを返していました。



この点でChromiumも例外ではなく、そのようなチェックが含まれています。 問題は、意味のないチェックが実行されることではありません。 NULLポインターでは、いくつかのアクションを前に実行する必要があったり、関数が特定の値を返すことは危険です。 現在、例外の生成により、作業のロジックが変更されました。 メモリ割り当てエラーで制御が与えられるべきだったコードは、現在は非アクティブです。



例を考えてみましょう：

 static base::DictionaryValue* GetDictValueStats( const webrtc::StatsReport& report) { .... DictionaryValue* dict = new base::DictionaryValue(); if (!dict) return NULL; dict->SetDouble("timestamp", report.timestamp); base::ListValue* values = new base::ListValue(); if (!values) { delete dict; return NULL; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V668「new」演算子を使用してメモリが割り当てられたため、「dict」ポインターをnullに対してテストしても意味がありません。 メモリ割り当てエラーの場合、例外が生成されます。 peer_connection_tracker.cc 164



V668メモリーが「新規」演算子を使用して割り振られたため、「値」ポインターをヌルに対してテストしても意味がありません。 メモリ割り当てエラーの場合、例外が生成されます。 peer_connection_tracker.cc 169



最初のチェック「if（！Dict）return NULL;」は害を及ぼさない可能性があります。 しかし、2番目のチェックはさらに悪いです。 「new base :: ListValue（）」を使用してオブジェクトを作成するときにメモリを割り当てることができない場合、例外 'std :: bad_alloc'がスローされます。 これで、GetDictValueStats（）関数の操作が完了しました。



結果として、このコードは次のとおりです。

 if (!values) { delete dict; return NULL; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アドレスが「dict」変数に格納されているオブジェクトを決して破壊しません。



ここでの正しい解決策は、コードをリファクタリングし、スマートポインターを使用することです。



別のコードを検討してください。

 bool Target::Init() { { .... ctx_ = new uint8_t[abi_->GetContextSize()]; if (NULL == ctx_) { Destroy(); return false; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V668メモリは「new」演算子を使用して割り当てられたため、「ctx_」ポインターをnullに対してテストする意味はありません。 メモリ割り当てエラーの場合、例外が生成されます。 target.cc 73



メモリ割り当てエラーが発生した場合、Destroy（）関数は呼び出されません。



これについてさらに書くのは面白くない。 コード内で気付いた他の潜在的に危険な場所のリストを簡単に示します。

• 「データ」ポインター。 target.cc 109
• 「page_data」ポインター。 mock_printer.cc 229
• 「モジュール」ポインター。 pepper_entrypoints.cc 39
• 「c_protocols」ポインター。 websocket.cc 44
• 「type_enum」ポインター。 pin_base_win.cc 96
• 「pin_enum」ポインター。 filter_base_win.cc 75
• 「port_data」。 port_monitor.cc 388
• 「xcv_data」ポインター。 port_monitor.cc 552
• 「monitor_data」。 port_monitor.cc 625
• 「sender_」ポインター。 crash_service.cc 221
• 「キャッシュ」ポインター。 crash_cache.cc 269
• 「current_browser」ポインター。 print_preview_dialog_controller.cc 403
• 「udp_socket」ポインター。 network_stats.cc 212
• 「popup_」ポインター。 try_chrome_dialog_view.cc 90

見られるN12-テストが不十分なテスト

単体テストは、プログラムの品質を向上させる素晴らしい技術です。 ただし、テスト自体には多くの場合エラーが含まれており、その結果、テストは機能を果たしません。 もちろん、テスト用のテストを書くのは多すぎます。 静的コード分析が役立ちます。 このアイデアについては、記事「 静的分析がTDDを補足する方法 」で詳しく検討しました。



Chromiumのテストで発生したエラーの例を次に示します。

 std::string TestAudioConfig::TestValidConfigs() { .... static const uint32_t kRequestFrameCounts[] = { PP_AUDIOMINSAMPLEFRAMECOUNT, PP_AUDIOMAXSAMPLEFRAMECOUNT, 1024, 2048, 4096 }; .... for (size_t j = 0; j < sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts); j++) { .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V501「/」演算子の左側と右側には、同一のサブ式「sizeof（kRequestFrameCounts）」があります。 test_audio_config.cc 56



ループで実行されるテストは1つだけです。 エラーは、「sizeof（kRequestFrameCounts）/ sizeof（kRequestFrameCounts）」が1つであることです。 正しい式は「sizeof（kRequestFrameCounts）/ sizeof（kRequestFrameCounts [0]）」です。



別のエラーテスト：

 void DiskCacheEntryTest::ExternalSyncIOBackground(....) { .... scoped_refptr<net::IOBuffer> buffer1(new net::IOBuffer(kSize1)); scoped_refptr<net::IOBuffer> buffer2(new net::IOBuffer(kSize2)); .... EXPECT_EQ(0, memcmp(buffer2->data(), buffer2->data(), 10000)); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V549 'memcmp'関数の最初の引数は2番目の引数と同じです。 entry_unittest.cc 393



memcmp（）関数は、バッファとそれ自身を比較します。 その結果、テストは必要なテストを実行しません。 どうやら、ここにあるはずです：

 EXPECT_EQ(0, memcmp(buffer1->data(), buffer2->data(), 10000));
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

そして、他のテストを突然中断する可能性のあるテストを次に示します。

 static const int kNumPainters = 3; static const struct { const char* name; GPUPainter* painter; } painters[] = { { "CPU CSC + GPU Render", new CPUColorPainter() }, { "GPU CSC/Render", new GPUColorWithLuminancePainter() }, }; int main(int argc, char** argv) { .... // Run GPU painter tests. for (int i = 0; i < kNumPainters; i++) { scoped_ptr<GPUPainter> painter(painters[i].painter); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V557配列のオーバーランが可能です。 「i」インデックスの値は2に達する可能性があります。shader_bench.cc 152



おそらく以前の「画家」の配列は3つの要素で構成されていました。 現在、そのうちの2つのみです。 定数 'kNumPainters'の値は3のままです。



私が注目に値すると思うテストの他の場所：



V579文字列関数は、引数としてポインターとそのサイズを受け取ります。 間違いかもしれません。 2番目の引数を調べます。 syncable_unittest.cc 1790



V579文字列関数は、引数としてポインターとそのサイズを受け取ります。 間違いかもしれません。 2番目の引数を調べます。 syncable_unittest.cc 1800



V579文字列関数は、引数としてポインターとそのサイズを受け取ります。 間違いかもしれません。 2番目の引数を調べます。 syncable_unittest.cc 1810



V595 nullptrに対して検証される前に、「ブラウザー」ポインターが使用されました。 行を確認してください：5489、5493。testing_automation_provider.cc 5489



V595 nullptrに対して検証される前に、 'waiting_for_.get（）'ポインターが使用されました。 行を確認してください：205、222。downloads_api_unittest.cc 205



V595 nullptrに対して検証される前に、「pNPWindow」ポインターが使用されました。 チェック行：34、35。plugin_windowed_test.cc 34



V595 nullptrに対して検証される前に、「pNPWindow」ポインターが使用されました。 16、20行を確認してください。plugin_window_size_test.cc 16



V595 nulltextrに対して検証される前に、「textfield_view_」ポインターが使用されました。 行を確認してください：182、191。native_textfield_views_unittest.cc 182



V595 nullptrに対して検証される前に、 'message_loop_'ポインターが使用されました。 行を確認してください：53、55。test_flash_message_loop.cc 53

See N13-可変数の引数を持つ関数

すべてのプログラムで、エラーを処理し、不正な入力データに応答するように設計されたコードに多くの欠陥が見つかります。 これは、そのような場所をテストするのが難しいためです。 そして、原則として、それらはテストされていません。 その結果、エラーが発生すると、プログラムは計画よりもはるかに奇妙な動作を開始します。



例：

 DWORD GetLastError(VOID); void TryOpenFile(wchar_t *path, FILE *output) { wchar_t path_expanded[MAX_PATH] = {0}; DWORD size = ::ExpandEnvironmentStrings( path, path_expanded, MAX_PATH - 1); if (!size) { fprintf(output, "[ERROR] Cannot expand \"%S\". Error %S.\r\n", path, ::GetLastError()); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V576形式が正しくありません。 'fprintf'関数の4番目の実引数を確認することを検討してください。 wchar_t型シンボルの文字列へのポインターが必要です。 fs.cc 17



変数 'size'がゼロの場合、プログラムはテキストメッセージをファイルに書き込もうとします。 しかし、このメッセージには最後にビリヤードが含まれる可能性があります。 さらに、このコードはアクセス違反につながる可能性があります 。



書き込みにはfprintf（）関数を使用します。 この関数は、引数のタイプを制御しません。 彼女は、最後の引数が文字列へのポインタであることを期待しています。 しかし実際には、実際の引数は数字（エラーコード）です。 この番号はアドレスに変換され、プログラムがさらにどのように動作するかは不明です。

気づかれない

もう一度、私はメッセージのリストを表面的に見ていた。 この記事では、注意を引いたものだけを引用しました。 さらに、記事で書いた以上のことに気付きました。 すべてを説明すると、記事が長すぎます。 そして彼女はすでに大きすぎます。



読者にとってあまり面白くないと思うほど多くのコードを捨てました。 明確にするために、いくつかの例を挙げます。

 bool ManagedUserService::UserMayLoad( const extensions::Extension* extension, string16* error) const { if (extension_service && extension_service->GetInstalledExtension(extension->id())) return true; if (extension) { bool was_installed_by_default = extension->was_installed_by_default(); ..... } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V595 nullptrに対して検証される前に、「拡張」ポインターが使用されました。 行を確認してください：277、280。managed_user_service.cc 277



最初は、式「extension-> id（）」で「extension」ポインターが逆参照されます。 次に、このポインターがゼロに等しいかどうかがチェックされます。



多くの場合、このようなコードにはエラーが含まれていません。 ポインタを単純にゼロにすることはできず、チェックは冗長です。 したがって、このような場所をリストするのは意味がありません。間違いを犯して、エラーに対して完全に機能するコードを提供できるからです。



気付かないことを選択した別の診断例を次に示します。

 bool WebMClusterParser::ParseBlock(....) { int timecode = buf[1] << 8 | buf[2]; .... if (timecode & 0x8000) timecode |= (-1 << 16); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V610未定義の動作。 シフト演算子 '<<を確認してください。 左のオペランド '-1'は負です。 webm_cluster_parser.cc 217



正式には、負の値のシフトは未定義の動作につながります 。 ただし、多くのコンパイラは、プログラマが期待するとおりに安定して正確に動作します。 その結果、このコードは必須ではありませんが、長期間正常に機能します。 私は今これと戦う気はありません、そしてそのようなメッセージを見逃します。 この問題をより詳細に理解したい人には、「 フォードを知らなくても、水に入らないでください-パート3 」という記事をお勧めします。

誤検知について

よく質問されます：



記事では、発見されたエラーの例を非常に巧みに引用しています。 ただし、発行されたメッセージの総数は何と言うかはわかりません。 多くの場合、静的アナライザーは多くの誤検知を生成し、その中で実際のエラーを見つけることはほとんど不可能です。 PVS-Studioアナライザーの状況はどうですか？



そして、私はいつもこの質問にすぐに答えるべきか分からない。 私は2つの正反対の答えを持っています：1つ目はたくさんあり、2つ目は十分ではありません。 それはすべて、発行されたメッセージリストの検討にどのようにアプローチするかに依存します。 次に、Chromiumの例を使用して、このような二重性の本質を説明します。



PVS-Studioアナライザーは、 3582の第1レベルのアラート（GA一般ルールセット）を生成しました。 これはたくさんあります。 そして、これらのメッセージのほとんどは偽です。 「真正面から」進んで、リスト全体をすぐに閲覧し始めると、すぐに疲れてしまいます。 そして、印象はひどいものになります。 同じタイプのいくつかの固体誤検知。興味深いものは何もありません。悪いツール。



このようなユーザーの間違いは、最小限のツールのセットアップでさえ完了していないことです。はい、PVS-Studioツールを作成して、インストール後すぐに動作するようにします。何も設定する必要がないようにします。人は単に自分のプロジェクトをチェックし、発行された警告のリストを調べる必要があります。



ただし、これは常にうまくいくとは限りません。これはChromiumでは機能しませんでした。たとえば、マクロ「DVLOG」が原因で膨大な数の誤ったメッセージが発生しました。このマクロは何かを記録しています。それは巧妙に書かれており、PVS-Studioは誤っている可能性があると誤って考えていました。マクロは非常に積極的に使用されているため、多くの誤ったメッセージが判明しました。 DVLOGマクロが何回使用されているかがわかるため、レポートには多くの誤った警告が入ります。つまり、マクロについて約2300の偽メッセージ「V501 There is same sub-expressions .....」が発行されました。



マクロ宣言の横にあるヘッダーファイルに書き込むことで、これらの警告を抑制することができます。コメントは次の



とおりです。//-V：DVLOG：501



見てください。メッセージの65％をすぐに削除しました。そして今、それらを無駄に見る必要はありません。



多くの労力をかけずに、これらの改良と設定をさらにいくつか行うことができます。その結果、ほとんどの誤検知は消えます。場合によっては、チューニング後に分析を再開する必要がありますが、そうでない場合があります。これについては、ドキュメントセクション「誤警告の抑制」で詳しく説明しています。特に、マクロでエラーが発生した場合は、分析を再開する必要があります。



はっきりしていることを願っています。なぜ最初の答えなのか-多くの誤検知があります。そして、なぜ2番目の答え-誤検知はほとんどありません。それはすべて、人が少なくとも少し時間をかけて製品を研究する準備ができているかどうかと、不必要なメッセージから身​​を守る方法にかかっています。

読者への別れの言葉

私はこの機会に両親に挨拶します。ああ、それは何かおかしい。この機会を利用して、私はプログラマーに挨拶を伝え、それを思い出したい：

• 「プロジェクトで見つかったバグを開発者に報告しましたか？」という質問に対する回答は、「記事を読んだ後によく聞かれる質問への回答」というメモにあります。
• ウェブサイトのフィードバックフォームを使用して、お問い合わせください。このためにツイッターを使用したり、サードパーティのサイトのどこかに記事にコメントを付けたりしないでください。
• twitter：@Code_Analysisにご参加ください。私は、プログラミングのトピックとC ++言語に関する興味深いリンクを常に収集して公開しています。