PVS-StudioコードアナライザーでTortoiseSVNを再確認する

TortoiseSVN開発者にPVS-Studioアナライザーの無料のキーをしばらく送信しました。 彼らがそれを使用する時間があるまで、私はTortoiseSVNのソースコードをすばやくダウンロードし、自分で分析を実行することにしました。 目標は明確です。 PVS-Studioを宣伝するためのもう1つの小さな記事。



TortoiseSVNプロジェクトは既にテスト済みです。 それはずっと前のことです。 プロジェクトの検証は、一般的な診断ルールが最初に登場したPVS-Studio 4.00のリリースと同時に行われました。



定期的にチェックを繰り返し、ツールを定期的に使用することの利点を実証します。 プロジェクトを1回または2回チェックすることは意味がありません。 可変コードでは、新しいエラーが常に表示されます。 そして、ゆっくりと悲しいことに修正しました。 したがって、PVS-Studioを毎日使用することで最大のメリットが得られます。 さらに良いのは、 増分分析を使用することです。



それでは、 PVS-Studioバージョン5.05を使用したプロジェクトで興味深いことがわかったものを見てみましょう。 TortoiseSVNのソースコードは、2013年6月19日にhttp://tortoisesvn.googlecode.com/svn/trunkから取得されました。 ところで、TortoiseSVNプロジェクトは非常に高品質で、ユーザープログラマーの巨大なデータベースを持っています。 したがって、少なくとも何かを見つけた場合、これはすでに大きな成果です。

奇妙な条件

static void ColouriseA68kDoc (....) { if (((sc.state == SCE_A68K_NUMBER_DEC) && isdigit(sc.ch)) .... || ((sc.state == SCE_A68K_MACRO_ARG) && isdigit(sc.ch)) || ((sc.state == SCE_A68K_MACRO_ARG) && isdigit(sc.ch)) .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V501「||」の左と右に同一の副次式「（（sc.state == 11）&& isdigit（sc.ch））」があります 演算子。 lexa68k.cxx 160



2つの同一の比較があります。 タイプミスがあります。



タイプミスは、おそらく次のコードに存在します。 変数「rv」の値は2回チェックされます。

 struct hentry * AffixMgr::compound_check( .... if (rv && forceucase && (rv) && ....) .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V501 '&&'演算子の左右に同じ副次式があります：rv && forceucase &&（rv）：

• affixmgr.cxx 1784
• affixmgr.cxx 1879

次のコードスニペット：

 bool IsAllASCII7 (const CString& s) { for (int i = 0, count = s.GetLength(); i < count; ++i) if (s[i] >= 0x80) return false; return true; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V547 Expression 's [i]> = 0x80'は常にfalseです。 char型の値の範囲：[-128、127]。 logdlgfilter.cpp 34



IsAllASCII7（）関数は常に「true」を返します。 条件の[i]> = 0x80 'は常にfalseです。 タイプ 'char'の変数の値は0x80以上にはできません。



誤った比較を含む次のコードスニペット：

 int main(int argc, char **argv) { .... DWORD ticks; .... if (run_timers(now, &next)) { ticks = next - GETTICKCOUNT(); if (ticks < 0) ticks = 0; } else { ticks = INFINITE; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V547式 'ticks <0'は常にfalseです。 符号なしの型の値が<0になることはありませんwinplink.c 635



変数 'ticks'は符号なしです。 つまり、「if（ticks <0）」をチェックしても意味がありません。 オーバーフロー状況は処理されません。



strncmp関数が文字列を完全に比較できないエラーを考慮してください。

 int AffixMgr::parse_convtable(...., const char * keyword) { char * piece; .... if (strncmp(piece, keyword, sizeof(keyword)) != 0) { .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V579 strncmp関数は、ポインターとそのサイズを引数として受け取ります。 間違いかもしれません。 3番目の引数を調べます。 affixmgr.cxx 3654



'sizeof'演算子は、ポインターのサイズを計算します。 この値は、文字列の長さとは関係ありません。

疑わしいストリング形成

可変数の引数を持つ関数は常にどこにでもあり、常に危険です。

 class CTSVNPath { .... private: mutable CString m_sBackslashPath; mutable CString m_sLongBackslashPath; mutable CString m_sFwdslashPath; .... }; const FileStatusCacheEntry * SVNFolderStatus::BuildCache( const CTSVNPath& filepath, ....) { .... CTraceToOutputDebugString::Instance() (_T(__FUNCTION__) _T(": building cache for %s\n"), filepath); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V510 'operator（）'関数は、2番目の実引数としてクラス型変数を受け取ることを期待されていません。

• svnfolderstatus.cpp 150
• svnfolderstatus.cpp 355
• svnfolderstatus.cpp 360

指定子 "％s"は、関数が文字列を実際の引数として期待することを示します。 ただし、変数 'filepath'はまったく文字列ではなく、多くの行で構成される複雑なオブジェクトです。 何が印刷されるのか、このコードがまったく落ちるのかどうかを言うのは途方に暮れています。



「printf（）」などの関数を次のように使用するのは危険です：「printf（myStr）;」。 'myStr'内にコントロール修飾子が存在する場合、プログラムは想定されていないものやクラッシュする可能性のあるものを出力する場合があります。



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

 BOOL CPOFile::ParseFile(....) { .... printf(File.getloc().name().c_str()); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V618このような方法で 'printf'関数を呼び出すのは危険です。渡される行には形式の仕様が含まれている可能性があるためです。 安全なコードの例：printf（ "％s"、str）; pofile.cpp 158



ファイル名が「myfile％s％i％s.txt」の場合、結果は悲惨なものになります。



ご注意 printf（）関数を使用することの危険性について興味深いメモがあります 。

配列の誤ったゼロ化

TortoiseSVNがバッファーの内容をリセットせずにそのままにしておくことの危険性はわかりません。 おそらく一般的に安全です。 ただし、バッファをリセットするコードがあります。 また、機能しないため、言及する価値があります。 エラーは次のようになります。

 static void sha_mpint(SHA_State * s, Bignum b) { unsigned char lenbuf[4]; .... memset(lenbuf, 0, sizeof(lenbuf)); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V597コンパイラーは、「lenbuf」バッファーのフラッシュに使用される「memset」関数呼び出しを削除できました。 RtlSecureZeroMemory（）関数を使用して、プライベートデータを消去する必要があります。 sshdss.c 23



関数を終了する前に、配列「lenbuf」をクリアする必要があります。 その後、配列は使用されなくなるため、オプティマイザは「memset」関数の呼び出しを削除します。 これを防ぐには、特別な機能を使用する必要があります。



コンパイラが「memset（）」の呼び出しを削除する他の場所：

• sshdss.c 37
• sshdss.c 587
• sshdes.c 861
• sshdes.c 874
• sshdes.c 890
• sshdes.c 906
• sshmd5.c 252
• sshrsa.c 113
• sshpubk.c 153
• sshpubk.c 361
• sshpubk.c 1121
• sshsha.c 256

奇妙な

 BOOL InitInstance(HINSTANCE hResource, int nCmdShow) { .... app.hwndTT; // handle to the ToolTip control .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V607所有者なしの表現「app.hwndTT」。 tortoiseblame.cpp 1782



ほとんどの場合、関数「InitInstance（）」では、メンバー「hwndTT」は何かで初期化される必要があります。 しかし、タイプミスにより、コードは不完全であることが判明しました。

64ビットエラー

私はエラーの非常に表面的な検索を行います。 丁寧で、記事を書くのに十分な例があります。 いいえ、私は別れません。 プロジェクトの作成者が、私ができるよりも優れた分析を実行するだけです。



64ビットエラーをさらに表面的に見ます。 プロジェクトの構造を知らずに、このエラーまたはそのエラーが発生するかどうかを判断することは非常に困難です。



危険な場所をいくつかだけ紹介します。

 void LoginDialog::CreateModule(void) { .... DialogBoxParam(g_hmodThisDll, MAKEINTRESOURCE(IDD_LOGIN), g_hwndMain, (DLGPROC)(LoginDialogProc), (long)this); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V220型キャストの不審なシーケンス：memsize-> 32ビット整数-> memsize。 キャストされる値： 'this'。 logindialog.cpp 105



これへのポインターは、long型に明示的にキャストされます。 次に、LPARAM型（LONG_PTR）に暗黙的に展開されます。 重要なことは、ポインターがしばらく「長い」に変わることです。 プログラムが64ビットの場合、これは悪いことです。 ポインターは64ビットを占有します。 Win64の「long」型は、まだ32ビット型です。 その結果、64ビット変数の最上位ビットが失われます。



オブジェクトが若い4 GBのRAMの外側に作成された場合、プログラムは予測不能になります。 そのようなイベントの可能性は確かに大きくありませんが、そのようなエラーを再現することは非常に困難です。



正しいコード：DialogBoxParam（....、（LPARAM）this）;



別の危険なキャストを検討してください：

 static int cmpforsearch(void *av, void *bv) { Actual_Socket b = (Actual_Socket) bv; unsigned long as = (unsigned long) av, bs = (unsigned long) b->s; if (as < bs) return -1; if (as > bs) return +1; return 0; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V205ポインター型から32ビット整数型への明示的な変換：（符号なしlong）av：

• winnet.c 139
• winhandl.c 359
• winhandl.c 348

ポインタは明示的に「unsigned long」型にキャストされ、変数「as」および「bs」に配置されます。 この場合、アドレスの上位ビットが失われる可能性があるため、比較が正しく機能しない場合があります。 一般に、ここでポインタが整数型にキャストされる理由は明確ではありません。 ポインタを単純に比較できます。

非推奨のヌルポインターチェック

「new」演算子は、メモリを割り当てることができない場合、ずっと前にNULLを返しませんでした。 例外std :: bad_allocをスローします。 もちろん、演算子 'new'が0を返すようにすることもできますが、これは今では無関係です。



それでも、次のコードはプログラム内に存在し続けます。

 int _tmain(....) { .... pBuf = new char[maxlength]; if (pBuf == NULL) { _tprintf(_T("Could not allocate enough memory!\n")); delete [] wc; delete [] dst; delete [] src; return ERR_ALLOC; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

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

• subwcrev.cpp 912
• repositorybrowser.cpp 2565
• repositorybrowser.cpp 4225
• svnstatuslistctct.cpp 5254
• svnprogressdlg.cpp 2357
• bugtraqassociations.cpp 116
• xmessagebox.cpp 792
• xmessagebox.cpp 797
• hyperlink_base.cpp 166
• affixmgr.cxx 272
• hashmgr.cxx 363
• hashmgr.cxx 611

そしてそれは行く

コードの学習中に発生する多くのエラーについては、記事には書きません。 事実は、彼らがプログラムに干渉しないということです。 今回は、そのようなケースをいくつか書くことにしました。 プログラムが正しく書かれているが、運のために機能しない状況を見るのはとても面白いです。

 void CBaseView::OnContextMenu(CPoint point, DiffStates state) { .... popup.AppendMenu(MF_STRING | oWhites.HasTrailWhiteChars ? MF_ENABLED : (MF_DISABLED|MF_GRAYED), POPUPCOMMAND_REMOVETRAILWHITES, temp); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

診断メッセージ：V502おそらく '？：'オペレーターは予想とは異なる方法で動作します。 「？：」演算子の優先順位は「|」よりも低い 演算子。 baseview.cpp 2246



変数 'oWhites.HasTrailWhiteChars'の値に応じて、定数の組み合わせの1つが必要です。

• MF_STRING | MF_ENABLED
• MF_STRING | MF_DISABLED | MF_GRAYED

しかし、コードはそのようには機能しません。 操作の優先度「|」 操作の優先度「？：」よりも高い。 わかりやすくするために括弧を付けています。



（MF_STRING | oWhites.HasTrailWhiteChars）？ MF_ENABLED：MF_DISABLED | MF_GRAYED



このコードは、定数「MF_STRING」が0であるためにのみ正しく機能します。結果には影響しません。 その結果、間違った式が正しく機能します。



運の別の例を考えてみましょう。 TortoiseSVNでは、HWNDタイプは「符号なし」タイプとしてよく使用されます。 このためには、明示的な型変換を実行する必要があります。 たとえば、次の関数に示すように：

 HWND m_hWnd; UINT_PTR uId; INT_PTR CBaseView::OnToolHitTest(....) const { .... pTI->uId = (UINT)m_hWnd; .... } UINT_PTR idFrom; HWND m_hWnd; BOOL CBaseView::OnToolTipNotify( UINT, NMHDR *pNMHDR, LRESULT *pResult) { if (pNMHDR->idFrom != (UINT)m_hWnd) return FALSE; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

または、たとえば、HWND型の変数の値は、 'long'型であるかのように出力されます。

 bool CCommonAppUtils::RunTortoiseProc(....) { .... CString sCmdLine; sCmdLine.Format(L"%s /hwnd:%ld", (LPCTSTR)sCommandLine, AfxGetMainWnd()->GetSafeHwnd()); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

正式には、このコードは正しくありません。 実際には、タイプ 'HWND'はポインターです。 これは、32ビット整数型に変換できないことを意味します。 そして、PVS-Studioアナライザーはこれを心配しており、警告を発します。



しかし、興味深いのは、このコードが完全に正しく機能することです！



HWND型は、さまざまなシステムオブジェクトを操作するためにWindowsで使用される記述子を格納するために使用されます。 同じタイプは、ハンドル、HMENU、HPALETTE、HBITMAPなどです。



記述子は64ビットポインターですが、互換性を高めるため（たとえば、32ビットプロセスと64ビットプロセス間の相互運用性のため）、下位32ビットのみを使用します。 詳細については、「 Microsoftインターフェイス定義言語（MIDL）：64ビットポーティングガイド 」（USERおよびGDIハンドルは符号拡張32b値）を参照してください。



HWND型を32ビット型に配置するとき、開発者はこれらの仮定にほとんど基づいていませんでした。 ほとんどの場合、これはWindows API開発者の幸運と努力のために、正しく動作するあまりきれいなコードではありません。

おわりに

開発中は静的解析を定期的に使用してください。初期段階で多くのエラーが見つかります。 当然、最初にPVS-Studioコードアナライザーを理解することをお勧めします。 ただし、他にも多くの優れたコードアナライザーがあります。 静的コード分析ツールです。

参照資料

記事に記載されている微妙な点のいくつかを明確にする追加のリンク。

1. 知識ベース。 メモリの上書き-なぜですか？
2. ドキュメンテーション V668 「new」演算子を使用してメモリが割り当てられたため、nullに対してポインターをテストしても意味がありません。
3. 知識ベース。 64ビットプログラムでintにポインタを正しくキャストする方法は？
4. カルポフ・アンドレイ、エフゲニー・リズコフ。 64ビットC / C ++アプリケーションの開発のためのレッスン 。