PVS-Studio：ReactOSオペレーティングシステムのコードを分析します

ReactOSコードを確認した後、私は3つの願いを一度に満たすことができました。 まず、私は長い間、普通のプロジェクトに関する記事を書きたいと思っていました。 Chromiumなどのプロジェクトのコードを確認するのは面白くありません。 高品質であり、通常のプロジェクトでは利用できないリソースがこの品質の維持に費やされています。 第二に、大きな例で静的分析がどのように必要であるかを示すことができる良い例が現れました。特に異種の分散チームによって開発された場合です。 第三に、PVS-Studioがより良く、より便利になっているという確認を受けました。

PVS-Studioはどんどん良くなっています

PVS-Studioツールの利点に関して、最後の瞬間から始めましょう。 ReactOSは、PVS-Studioが正しい方向に開発されていることを間接的に確認します。 コベリティ-「コベリティの静的分析」[ 1 ]などのヘビーウェイトでReactOSをチェックすることに関するニュースがあります。 もちろん、私たちはコベリティの可能性とはほど遠いことを知っています。 しかし、それにもかかわらず、コベリティのおかげで「いくつかの新しいミスが見つかりました」と、PVS-Studioは彼らの全体の馬車と小さなカートを見つけます。 この場合、どこにもコードを送信しないでください。 プロジェクトを取得して確認するだけです。 だから我々は正しい軌道に乗っています。

ReactOSとは何ですか？

ReactOSは、Windows XP / 2003のアーキテクチャに基づいた最新の無料のオープンオペレーティングシステムです。 システムはゼロから作成され、MicrosoftのWindows-NTアーキテクチャをハードウェアからアプリケーションレベルまで繰り返すことを目的としています。 C、C ++、およびアセンブラのソースコードの量は約220メガバイトです。



さまざまなリンク：

• ReactOSプロジェクトの公式サイト 。
• ReactOSの開発に参加する方法 。
• ウィキペディア ReactOS
• Computerraの記事。 ReactOS：追いつき、追い越します。
• ラークモア。 ReactOS

ReactOSのバグ

次に、ReactOSコードで発生したエラーの処理について説明します。 もちろん、それらのすべてが記事に含まれているわけではありません。 ここに、分析中に気付いたエラーを説明するテキストファイルを投稿しました。 ファイルには、ファイル名と行番号を含む診断メッセージが含まれています。 また、短いコードでエラーを強調し、いくつかのコメントをしました。 したがって、ReactOSを編集したい人は、記事ではなく、このファイルを参考にしてください。



さらに良いことに、PVS-Studioを使用して自分でプロジェクトをダウンロードしてテストします。 結局のところ、私はプロジェクトに精通しておらず、私が理解しているエラーのみを書きました。 多くのコードフラグメントについて、エラーが含まれているかどうかはわかりません。 したがって、私の分析は非常に表面的です。 確認のためにキーを選択します。



ReactOSで見つかるエラーはさまざまです。 ただの動物園。 単一の文字にタイプミスがあります。

BOOL WINAPI GetMenuItemInfoA(...) { ... mii->cch = mii->cch; ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

実際、「mii-> cch = miiW-> cch;」のように記述する必要があります。 手紙「W」を失いました。 その結果、プログラムはGetMenuItemInfoA関数を信頼できなくなりました。



そして、ここに1文字の別のタイプミスがあります。 今回は、2つの名前の比較が正しく機能しません。

 static void _Stl_loc_combine_names(_Locale_impl* L, const char* name1, const char* name2, locale::category c) { if ((c & locale::all) == 0 || strcmp(name1, name1) == 0) ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

&&と＆演算子の間には混乱があります。 非常によくある間違い。 私は彼らがビットまたはファイル属性を扱うほとんどすべてのプロジェクトで会います。

 static LRESULT APIENTRY ACEditSubclassProc() { ... if ((This->options && ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

正しいコードは「（This-> options＆ACO_AUTOSUGGEST）」のようになります。 以下の例には、関連するエラーが含まれています。そのため、条件全体が常にfalseになっています。

 void adns__querysend_tcp(adns_query qu, struct timeval now) { ... if (!(errno == EAGAIN || EWOULDBLOCK || errno == EINTR || errno == ENOSPC || errno == ENOBUFS || errno == ENOMEM)) { ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

よく見ると、「|| EWOULDBLOCK ||」という陰湿な断片に気付くことができます。



ところで、ReactOSには、常にtrueまたはfalseである多くの条件がありました。 たとえば、assert（）マクロ内にあるため、恐れないものもあります。 しかし、私の意見では、重要なものがあります。

 INT WSAAPI connect(IN SOCKET s, IN CONST struct sockaddr *name, IN INT namelen) { ... /* Check if error code was due to the host not being found */ if ((Status == SOCKET_ERROR) && (ErrorCode == WSAEHOSTUNREACH) && (ErrorCode == WSAENETUNREACH)) { ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

「接続」などの機能の実装は、可能な限り完全にテストする必要があることに同意します。 そして、ここには常に偽の条件があります。 欠陥をすばやく見つけるのはそれほど簡単ではないので、エラーの本質を強調します。

 (ErrorCode == 10065) && (ErrorCode == 10051)
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ちなみに、ソケットに関連する部分は一般に生のように見えます。 おそらくこれは、Linuxの世界ではSOCKETが通常は符号タイプとして宣言されているという事実によるものです。 Windowsでは、署名されていません。

 typedef UINT_PTR SOCKET;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

その結果、比較演算にさまざまなエラーがあります。

 void adns_finish(adns_state ads) { ... if (ads->tcpsocket >= 0) adns_socket_close(ads->tcpsocket); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

「ads-> tcpsocket> = 0」という表現は、常に正しいので意味がありません。



ただ奇妙な断片があります。 ほとんどの場合、これらは未完成で忘れられているコードのセクションです。

 if (ERROR_SUCCESS == hres) { Names[count] = HeapAlloc(GetProcessHeap(), 0, strlenW(szValue) + 1); if (Names[count]) strcmpW(Names[count], szValue); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

結果が何らかの方法で使用されない場合、なぜ「strcmpW」を呼び出すのですか？



操作の優先度に関連するエラーがあります。

 VOID NTAPI AtapiDmaInit(...) { ... ULONG treg = 0x54 + (dev < 3) ? (dev << 1) : 7; ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

この式が実際にどのように機能するかを明確にするために括弧を付けます：

 ULONG treg = (0x54 + (dev < 3)) ? (dev << 1) : 7;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

大規模プロジェクトでは、次のエラーが発生する必要があります。 ReactOSにはカップルがあります。 これは追加のセミコロン-';'です。

 BOOLEAN CTEScheduleEvent(PCTE_DELAYED_EVENT Event, PVOID Context) { ... if (!Event->Queued); { Event->Queued = TRUE; Event->Context = Context; ExQueueWorkItem(&Event->WorkItem, CriticalWorkQueue); } ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

また、配列要素の初期化でエラーが発生します。 理由はわかりません。 彼らは感動しています。 おそらく、Basic言語の配列を使った最初の実験を思い出します。

 HPALETTE CardWindow::CreateCardPalette() { ... //include button text colours cols[0] = RGB(0, 0, 0); cols[1] = RGB(255, 255, 255); //include the base background colour cols[1] = crBackgnd; //include the standard button colours... cols[3] = CardButton::GetHighlight(crBackgnd); cols[4] = CardButton::GetShadow(crBackgnd); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

コードのさまざまな興味深いセクションを引き続き引用できます。 残念ながら、記事は長くなりすぎて、停止する必要があります。 このファイルの ReactOSで見つけた他のエラーを見ることができることを思い出させてください。 デザートについては、次のコードを提供します。

 #define SWAP(a,b,c) c = a;\ a = b;\ a = c
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

使用例：

 BOOL FASTCALL IntEngGradientFillTriangle(...) { ... SWAP(v2,v3,t); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

これは傑作です。

静的コード分析

ReactOSは、定期的な静的コード分析が不可欠なプロジェクトの非常に良い例だと思います。 そして、ここでのポイントは、開発者の資格ではありません。 プロジェクトは大きく、さまざまなサブシステムが含まれています。 これは、多くの人々が常にそのようなプロジェクトに取り組んでいることを意味します。 そして、大規模なチームでは、だれかが常により悪いプログラムを作り、誰かがより良いプログラムを作ります。 誰かが別のスタイルを使用します。 しかし、誰も間違いから安全ではありません。 ここを見て。



1人がReactOSで次のように書いて書いた：

 if ((res = setsockopt(....) == -1))
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

コードはあなたが望むことをしません。 正しいオプション：if（（res = setsockopt（....））== -1）。 最初に定数を書く習慣に固執している場合、偶然にifステートメント内で誤った割り当てを行うことはありません。 ここには別のタイプのエラーがあります。 ただし、上記のルールに従ってコードを記述する場合、問題の式「if（-1 == res = setsockopt（....））」を間違えることはありません。



この方法を知っているだけでは、他の人が別の方法でエラーを犯すことを防ぐことはできません。

 static DWORD CALLBACK RegistrationProc(LPVOID Parameter) { ... if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR, UnknownError, sizeof(UnknownError) / sizeof(UnknownError[0] - 20))) ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここでは、最初は定数0が美しく書かれていますが、これは必要のない場所で括弧が閉じているだけです。 通常のタイプミス。



なぜこれらすべての例なのですか？ そして、誰もプログラマーではないという事実は完璧です。 また、コーディング標準もプログラミング技術も自己制御も、コードにエラーがないことを保証するものではありません。



大規模なプロジェクトでは、動的分析や静的分析などの支援技術が必要になります。 私は強調します：



静的コード分析は、ReactOSやその他の大規模プロジェクトの開発サイクルに不可欠な要素であると考えています。



私の声明を説明します。 このようなシステムでは、単体テストまたは回帰テストでテストするときに、100％に近いコードカバレッジを実現することは不可能です。 少し明確にします。 もちろん可能ですが、そのようなテストの作成と保守のコストは容認できないほど高くなります。



その理由は、考えられるシステム状態の数とコード分岐を実行する考えられる方法が非常に大きいためです。 一部のブランチはほとんど制御を受け取りませんが、これは不要になりません。 ここで、静的解析の利点を確認できます。 プログラム中にどのくらいの頻度で制御を取得しても、すべてのコードをチェックします。



ほとんど制御されないコードをチェックする例：

 static HRESULT STDMETHODCALLTYPE CBindStatusCallback_OnProgress(...) { ... if (This->szMimeType[0] != _T('\0')) _tprintf(_T("Length: %I64u [%s]\n"), This->Size, This->szMimeType); else _tprintf(_T("Length: %ull\n"), This->Size); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

最も可能性が高いのは、コードが最初は誤って記述されていたことです。 次に、メッセージが正しく形成されていないことに気づき、「％I64u」と書いて修正しました。 しかし、彼は近所のコードに注意を払いませんでした。 そして、まだ間違ったフォーマット「％ull」があります。 どうやらブランチはめったに呼び出されません。 静的解析ではこれを見逃しません。 実際、彼は見逃しませんでした。この例を実証できるからです。



別の良い例は、ReactOSで見た多数のメモリクリーンアップエラーです。 なぜそんなにたくさんあるのか、わかりました。 メモリがいっぱいかどうかをテストしている人はいません。 まず、これらの単純な場所で間違いを犯す可能性があることを理解するのは困難です。 次に、関数内で一時バッファがクリアされたかどうかを確認するのはそれほど簡単ではありません。 ここで、静的解析は再び標準に達しました。 いくつかの例を挙げます。 実際、一定の値で配列を埋めるエラーを少なくとも13個カウントしました。

 #define MEMSET_BZERO(p,l) memset((p), 0, (l)) char *SHA384_End(SHA384_CTX* context, char buffer[]) { ... MEMSET_BZERO(context, sizeof(context)); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

sizeof（コンテキスト）は構造体ではなくポインターのサイズを返すため、配列の最初のバイトのみをクリアします。

 #define RtlFillMemory(Destination, Length, Fill) \ memset(Destination, Fill, Length) #define IOPM_FULL_SIZE 8196 HalpRestoreIopm(VOID) { ... RtlFillMemory(HalpSavedIoMap, 0xFF, IOPM_FULL_SIZE); ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

RtlFillMemoryマクロを使用するときに引数が混同されます。 呼び出しは次のようになります。

 RtlFillMemory(HalpSavedIoMap, IOPM_FULL_SIZE, 0xFF);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

再びタブとスペース

コメントでこのトピックに関する新しい白熱した議論を開始しないように事前にお願いしたいと思います。 意見を述べます。 あなたは彼に同意しても同意しなくてもかまいません。 しかし、議論する価値はありません。



和解できないキャンプが2つあります。 コードで表を使用する人もいます。これにより、コードの表示を自分で調整できるようになります[ 2 ]。 他の人は、これはまだ機能せず、タブを使用する合理的な理由はないと言います。 集計からは、書式設定が損なわれたり分散されたりするだけです。 私はその中にいます[ 3 ]。



好きなだけタブを正しく使用する必要があると言えば、すべてがうまくいくでしょう。 残念ながら、これは、1つのプロジェクトで単独で作業し、外の世界に直面していない人々によって言われています。 開いているプロジェクトや単純に大規模なプロジェクトでは、どのフォームでもタブを使用できる場合、通常のコード設計を実現することはできません。



私は抽象的な推論を扱いません。 今回は、相手に良い例を示します。 これで、ReactOSコードがそのような例になります。



ReactOSコーディング標準[ 4 ]では、理論的な観点から適切なルールが記述されています。

TABの使用に関する一般的な注意：フォーマットにはTABを使用しないでください。 インデントのみにタブを使用し、フォーマットにはスペースのみを使用します。

例： 
 NTSTATUS
 SomeApi（INタイプParam1、
 [スペース] INタイプParam2）
 {
 [TAB] ULONG MyVar;
 [TAB] MyVar = 0;
 [TAB] if（（MyVar == 3）&&
 [TAB] [sp]（Param1 == TRUE））
 [TAB] {
 [TAB] [TAB] CallSomeFunc（）;
 ... 

タブファンは幸せです。 ただし、ReactOSのソースコードを開くと、多くの場所で破損した書式が表示されます。 なんで？











答えはもちろん明らかです。 TABを押す場所といくつかのスペースを配置する場所を覚えるのは難しいため、これが作業する唯一のプロジェクトではない場合。 したがって、人々は常に間違いを犯します。 もしそうなら、理論家はいませんが、実務家でなければなりません。 一般にタブの使用を禁止する必要があります。 そして、すべてのものがすべて同じになり、集計表を使い始めた犯人は見つけやすくなり、提案することができます。



これは、コードの設計の後退ではありません！ これは一歩前進です！ これが次の認識レベルです。 カスタムインデントの理論的な美しさは、実践と両立しません。 まず、コードを明確に提示し、大規模なチームで開発を容易にすることが重要です。 Googleはこれを理解しています。 そして、それらの標準は、フォーマットにスペースのみを使用します[ 5 ]。 タブの使用を提唱する人々は、Chromiumを開発する一流の専門家の分散チームがギャップを正確に選択した理由についてもう一度考えることをお勧めします。



そしてもう一度。 カスタムインデントの理論的な美しさは、実践と両立しません。 理論がうまくいかなくても、理論がどれほど美しいかは関係ありません。 そして、これはまさにReactOSの場合です。



ReactOS開発チームが標準を変更し、タブの使用を停止することをお勧めします。 どのタブもエラーと見なされ、コードから削除される必要があります。



ところで、この方法を使用すると、ReactOSコードでこのような不名誉を見つけることができます。

 BOOLEAN KdInitSystem(IN ULONG BootPhase, IN PLOADER_PARAMETER_BLOCK LoaderBlock) { ... /* Check if this is a comma, a space or a tab */ if ((*DebugOptionEnd == ',') || (*DebugOptionEnd == ' ') || (*DebugOptionEnd == ' ')) ... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

最後の比較は、タブ文字との比較であり、スペースではないようです。 通常のコードは、「（* DebugOptionEnd == '\ t'）」のようになります。



タブに関する注意。 タブを正しく使用する方法を再度教えていただく必要はありません。 そして、これは私のコードではありません。 見てください、非常に具体的なReactOSプロジェクトがあります。 コードの形式が不適切です。 新しいプログラマーがプロジェクトコードを開くことを可能にし、エディター設定で設定する必要のあるタブ文字のサイズを推測しないアクションを考えてください。 「すぐに正しく書かなければならなかった」という精神の思想には、実用的な価値はありません。

書誌リスト

1. ReactOS N79ニュースリリース。 Coverityの静的分析。 http://www.viva64.com/go.php?url=722
2. コードでタブの代わりにスペースを使用します。 http://www.viva64.com/go.php?url=725
3. コードでタブを使用するために提携する時が来ました。 http://www.viva64.com/go.php?url=726
4. ReactOS コーディングスタイル。 http://www.viva64.com/go.php?url=724
5. Google C ++スタイルガイド。 http://www.viva64.com/go.php?url=679