🌘 🔽 😋 Oracle VM VirtualBoxコードの見た目 🍃 🤛🏽 ⤵️

仮想マシンは、ソフトウェア開発者にとって重要なツールです。 VirtualBoxコードに対する私の関心は、開いているプロジェクトをチェックするためのこの製品の個人的な使用、およびいくつかのオペレーティングシステムの使用に関連する他のさまざまなタスクに起因しています。このプロジェクトの最初のチェックは2014年に行われ、その後、約50のエラーの説明は2つの記事にほとんど当てはまりません。私の意見では、Windows 10とVirtualBox 5.0.XXのリリースにより、プログラムの安定性は著しく悪化しました。それで、私は再びプロジェクトをチェックすることにしました。

はじめに

VirtualBox （Oracle VM VirtualBox）-特定の環境でリソースの仮想セットを表示できるオペレーティングシステム用のソフトウェア。次のオペレーティングシステムでサポートされています：Microsoft Windows、FreeBSD、Solaris / OpenSolaris、Linux、Mac OS X、DOS、ReactOSなど。

VirtualBoxの最初の記事は、次のリンクにあります。

PVS-Studio 5.18を使用して見つかった50以上の危険な場所が含まれています。新しい分析レポートでは、このような警告は表示されませんでした。開発者は記事を通り越さず、アナライザーの示されたすべての警告を修正しました。興味のある方は、ソースコードの最新バージョンでこれらの場所を見つけ、実際のプロジェクトでのPVS-Studio警告の修正がどのように見えるかを確認できます。新しいチェックの後、私は他の多くの興味深いメッセージに会いました。

この記事では、静的アナライザー（PVS-Studioとは限りません）を定期的に使用するだけで高品質のコードが維持されることを示したいと思います。アンリアルエンジンコードのすべてのアナライザー警告を修正した経験から、開発中のプロジェクトのエラー数は絶えず増加し、1回限りのチェックの後、コードの品質は徐々に同じになり、新しいエラーが蓄積されることがわかりました。 VirtualBoxプロジェクトにも同様の状況があります。単一のチェック後のアナライザー警告の数の増加は、次のようになります。

アナライザーの定期的な使用は毎日のチェックを意味することを強調することが重要です。テスト段階で検出される多くのエラーは、コードを記述する段階で排除できます。

静的アナライザーを定期的に使用する別の利点は、定期的な更新が利用できることです。そのため、VirtualBoxコードの最初のチェック以降、50を超える新しい診断ルールがPVS-Studioアナライザーに追加されました。最後のセクションで、新しい診断を使用して検出されたエラーについて説明します。

Oracle VM VirtualBoxのソースコードは、 PVS-Studioバージョン6.02を使用して検証されました。

おそらく誰かが検証済みのリビジョン番号を必要とするでしょう：

Checked out external at revision 2796. Checked out revision 59777.

頑固な間違い

記事を書く前に、アナライザーを使用して以前に見つけたものを調べました。そして、新しいコードで非常によく似たエラーを見つけました。同じ人がこのコードを書いた可能性があります。

V521 「、」演算子を使用したこのような式は危険です。式が正しいことを確認してください。 vboxmpwddm.cpp 1083

 NTSTATUS DxgkDdiStartDevice(...) { .... if ( ARGUMENT_PRESENT(MiniportDeviceContext) && ARGUMENT_PRESENT(DxgkInterface) && ARGUMENT_PRESENT(DxgkStartInfo) && ARGUMENT_PRESENT(NumberOfVideoPresentSources), // <= ARGUMENT_PRESENT(NumberOfChildren) ) { .... } .... }

最初の記事にも同様のコードがありました。コンマ演算子 '、'が左オペランドと右オペランドの両方を計算することを思い出させてください。実際には、左側のオペランドの値は使用されなくなり、演算子の結果は右側のオペランドの値になります。ほとんどの場合、他の行と同様に、ここで「&&」演算子を使用したいと考えていました。

V519 「pThis-> aCSR [103]」変数には、連続して2回値が割り当てられます。おそらくこれは間違いです。行をチェック：1230、1231。devpcnet.cpp 1231

 static void pcnetSoftReset(PPCNETSTATE pThis) { .... pThis->aCSR[94] = 0x0000; pThis->aCSR[100] = 0x0200; pThis->aCSR[103] = 0x0105; // <= pThis->aCSR[103] = 0x0105; // <= .... }

コードに重複した行が含まれています。開発者は、余分な行を削除することにより、最初の記事の同様の場所を修正しました。そして、このフラグメントについてはどうでしょうか：配列のインデックスのエラーまたは余分な文字列は、VirtualBoxの将来のバージョンで発見されるでしょう。

V501 「||」の左と右に同一のサブ式「mstrFormat.equalsIgnoreCase（ "text / plain"）」があります。演算子。 vboxdnddataobject.cpp 382

 STDMETHODIMP VBoxDnDDataObject::GetData(....) { .... else if( mstrFormat.equalsIgnoreCase("text/plain") // <= || mstrFormat.equalsIgnoreCase("text/html") || mstrFormat.equalsIgnoreCase("text/plain;charset=utf-8") || mstrFormat.equalsIgnoreCase("text/plain;charset=utf-16") || mstrFormat.equalsIgnoreCase("text/plain") // <= || mstrFormat.equalsIgnoreCase("text/richtext") || mstrFormat.equalsIgnoreCase("UTF8_STRING") || mstrFormat.equalsIgnoreCase("TEXT") || mstrFormat.equalsIgnoreCase("STRING")) { .... }

コピーペーストプログラミングは永遠に生き続けます。ここには2つの同一の「テキスト/プレーン」チェックがあるため、このコードブロック全体が別のファイルに安全にコピーされています。

V501「||」の左と右に同一のサブ式「！RTStrICmp（pszFormat、「text / plain」）」があります。演算子。 vboxdnd.cpp 834

trueまたはfalseを定義します。 //成功したデバッグ！

そのようなコードは冗談ではなく、実際のプロジェクトにはさまざまなバリエーションが存在することがわかります。

V547式は常に偽です。符号なしの型の値が<0になることはありません。dt_subr.c715

 int dt_printf(dtrace_hdl_t *dtp, FILE *fp, const char *format, ...) { .... if (vsnprintf(&dtp->dt_buffered_buf[dtp->dt_buffered_offs], //<= avail, format, ap) < 0) { rval = dt_set_errno(dtp, errno); va_end(ap); return (rval); } .... }

一見すると、アナライザーを除いて、文句を言う必要はありません。 vsnprintf関数のドキュメントには、エラーが発生した場合に負の数が返されることが明示的に記載されています。その結果、誤検知の例として、このコードをコアC ++コードアナライザーの開発者の1人に渡しました。しかし、いや、アナライザーが正しいことが判明しました。

何千ものヘッダーファイルのどこかに行があると誰が考えたでしょうか。

 #define vsnprintf RTStrPrintfV

前処理されたファイルでは、ソースフラグメントは次のように展開されます。

 if (RTStrPrintfV(&dtp->dt_buffered_buf[dtp->dt_buffered_offs], avail, format, ap) < 0) { rval = dt_set_errno(dtp, (*_errno())); ( ap = (va_list)0 ); return (rval); }

RTStrPrintfV（）関数は、符号付きの 'int'ではなく、符号なしの型 'size_t'の値を返します。したがって、このようなチェックは論理エラーにつながります。実質的に検証は行われません。

比較のための関数プロトタイプ：

 size_t RTStrPrintfV(char *, size_t, const char *, va_list args); int vsnprintf (char *, size_t, const char *, va_list arg );

疑わしい「From-To」コード

V570 「 from- > eval1D [i] .u1」変数がそれ自体に割り当てられます。 state_evaluators.c 1006

 void crStateEvaluatorDiff(CREvaluatorBits *e, CRbitvalue *bitID, CRContext *fromCtx, CRContext *toCtx) { .... from->eval1D[i].order = to->eval1D[i].order; from->eval1D[i].u1 = from->eval1D[i].u1; // <= from->eval1D[i].u2 = from->eval1D[i].u2; // <= ... }

アナライザーは、疑わしい変数の割り当てを検出しました。多くの場合、「from」ではなく、「to」という名前のオブジェクトを割り当て演算子の右側で使用する必要があります。

このファイルにはさらに5つの場所があります。

V570「from-> eval1D [i] .u2」変数がそれ自体に割り当てられます。 state_evaluators.c 1007
V570「from-> eval2D [i] .u1」変数がそれ自体に割り当てられます。 state_evaluators.c 1042
V570「from-> eval2D [i] .u2」変数がそれ自体に割り当てられます。 state_evaluators.c 1043
V570「from-> eval2D [i] .v1」変数がそれ自体に割り当てられます。 state_evaluators.c 1044
V570 'from-> eval2D [i] .v2'変数はそれ自体に割り当てられます。 state_evaluators.c 1045

V625 「for」演算子の検査を検討してください。イテレータの初期値と最終値は同じです。 state_transform.c 1365

 void crStateTransformDiff(...., CRContext *fromCtx, CRContext *toCtx ) { .... for (i = to->colorStack.depth; i <= to->colorStack.depth; i++) { LOADMATRIX(to->colorStack.stack + i); from->colorStack.stack[i] = to->colorStack.stack[i]; /* Don't want to push on the current matrix */ if (i != to->colorStack.depth) diff_api.PushMatrix(); } .... }

「to」と「from」という名前を使用してコード内に別の疑わしい場所があるという事実のため、私はそのようなエラーを別のセクションで説明することにしました。

このコードでは、ループカウンターの初期値と最終値が一致しています。その結果、ループでは1回の反復のみが実行されます。「to」オブジェクトの名前の間違いの可能性があります。

操作の優先順位について

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

 static void generate_texcoord_assignment(....) { DWORD map; unsigned int i; char reg_mask[6]; if (!ps) return; for (i = 0, map = ps->baseShader.reg_maps.texcoord; map && i < min(8, MAX_REG_TEXCRD); map >>= 1, ++i) { if (!map & 1) // <= continue; .... } }

条件 "！Map＆1"の角かっこが忘れられているため、変数 'map'の値はゼロに等しく、最下位ビットが1に設定されているかどうかがチェックされます。この場所のエラーは、「map」の値のゼロのチェックが、ループが停止する状態ですでに存在しているという事実によっても示されます。したがって、この条件は常にfalseであり、 'continue'ステートメントは実行されません。

ほとんどの場合、条件は次のように記述する必要があります。

 if ( !(map & 1) ) continue;

V590この表現を調べることを検討してください。表現が過剰であるか、誤植が含まれています。 vboxdispcm.cpp 288

 HRESULT vboxDispCmSessionCmdGet(....) { .... Assert(hr == S_OK || hr == S_FALSE); if (hr == S_OK || hr != S_FALSE) // <= { return hr; } .... }

アナライザーは、部分式「hr == S_OK」が条件全体の結果に影響しない疑わしい条件を検出しました。

この条件式の真理値表を見ると、これを確認できます。

ちなみに、この条件の次に疑わしいAssert（）があり、そこには修正された条件式があります。

一般に、このタイプのエラーは非常に一般的な発生です。たとえば、FreeBSDカーネルも例外ではありませんでした。

VirtualBoxの不審な場所の全リスト：

V590 'err == 0Lの検査を検討する|| err！= 1237L '式。表現が過剰であるか、誤植が含まれています。 vboxdisplay.cpp 656
V590「rc == 3209 || rc！=（-3210） '式。表現が過剰であるか、誤植が含まれています。 vd.cpp 10876
V590「rc == 3209 || rc！=（-3210） '式。表現が過剰であるか、誤植が含まれています。 vd.cpp 10947
V590「rc == 3209 || rc！=（-3210） '式。表現が過剰であるか、誤植が含まれています。 vd.cpp 11004
V590「rc == 3209 || rc！=（-3210） '式。表現が過剰であるか、誤植が含まれています。 vd.cpp 11060

その他の警告

V511 sizeof（）演算子は、配列のサイズではなく、ポインターのサイズを「sizeof（plane）」式で返します。 devvga-svga3d-win.cpp 4650

 int vmsvga3dSetClipPlane(...., float plane[4]) // <= { .... /* Store for vm state save/restore. */ pContext->state.aClipPlane[index].fValid = true; memcpy(pContext->state.aClipPlane[....], plane, sizeof(plane)); .... }

変数 'plane'は、 'float'型の配列への単なるポインターです。「sizeof（plane）」の値は、プログラムの容量に応じて4または8になります。また、関数パラメーターの数字「[4]」は、「float」型の4つの要素の配列が関数に渡されることをプログラマーに伝えるだけです。したがって、memcpy（）関数は間違ったバイト数をコピーします。

V517 「if（A）{...} else if（A）{...}」パターンの使用が検出されました。論理エラーが存在する可能性があります。行を確認してください：411、418。mp-r0drv-nt.cpp 411

 static int rtMpCallUsingDpcs(....) { .... if (enmCpuid == RT_NT_CPUID_SPECIFIC) // <= { KeInitializeDpc(&paExecCpuDpcs[0], rtmpNtDPCWrapper, pArgs); KeSetImportanceDpc(&paExecCpuDpcs[0], HighImportance); KeSetTargetProcessorDpc(&paExecCpuDpcs[0], (int)idCpu); pArgs->idCpu = idCpu; } else if (enmCpuid == RT_NT_CPUID_SPECIFIC) // <= { KeInitializeDpc(&paExecCpuDpcs[0], rtmpNtDPCWrapper, pArgs); KeSetImportanceDpc(&paExecCpuDpcs[0], HighImportance); KeSetTargetProcessorDpc(&paExecCpuDpcs[0], (int)idCpu); pArgs->idCpu = idCpu; KeInitializeDpc(&paExecCpuDpcs[1], rtmpNtDPCWrapper, pArgs); KeSetImportanceDpc(&paExecCpuDpcs[1], HighImportance); KeSetTargetProcessorDpc(&paExecCpuDpcs[1], (int)idCpu2); pArgs->idCpu2 = idCpu2; } .... }

条件のカスケード内の式は同一であるため、2番目の条件にあるコードの部分は制御されません。

V531 sizeof（）演算子にsizeof（）を掛けることは奇妙です。 tstrtfileaio.cpp 61

 void tstFileAioTestReadWriteBasic(...., uint32_t cMaxReqsInFlight) { /* Allocate request array. */ RTFILEAIOREQ *paReqs; paReqs = (...., cMaxReqsInFlight * sizeof(RTFILEAIOREQ)); RTTESTI_CHECK_RETV(paReqs); RT_BZERO(..., sizeof(cMaxReqsInFlight) * sizeof(RTFILEAIOREQ)); /* Allocate array holding pointer to data buffers. */ void **papvBuf = (...., cMaxReqsInFlight * sizeof(void *)); .... }

アナライザーは、2つのsizeof（）ステートメントの疑わしい製品を検出しました。マクロ「RT_BZERO」を見ると、「なぜ「uint32_t」型の変数のサイズを取得し、別の型のサイズを掛けるのか」という疑問が生じます。コードの隣接するセクションでは、配列のサイズは「cMaxReqsInFlight * sizeof（RTFILEAIOREQ）」として計算されます。ほとんどの場合、「RT_BZERO」の行は同じサイズを使用する必要がありますが、誤って間違えました。

V547式 'sd> = 0'は常に真です。符号なしの型の値は常に0以上です。vboxservicevminfo.cpp1086

 static int vgsvcVMInfoWriteNetwork(void) { .... SOCKET sd = WSASocket(AF_INET, SOCK_DGRAM, 0, 0, 0, 0); .... if (pAdpInfo) RTMemFree(pAdpInfo); if (sd >= 0) // <= closesocket(sd); .... }

SOCKET型（Visual C ++）は符号なしですので、「sd> = 0」のチェックは無意味です。このコードの理由は理解できます。プロジェクトはさまざまなオペレーティングシステム用に構築されており、Unixシステムではソケット値は符号付き変数「int」に格納されます。一般に、ソケットを操作するためのコードは正しく記述されています。どこでも状態をチェックするために、システムヘッダーファイルの定数が使用されます。しかし、クロスプラットフォームコードには多くの条件付きプリプロセッサディレクティブが含まれているため、ある場所ではチェックに気付かず、これは常にWindowsに当てはまります。

V560条件式の一部は常に真です：0x1fbe。 tstiprtministring.cpp 442

 static void test2(RTTEST hTest) { .... for (RTUNICP uc = 1; uc <= 0x10fffd; uc++) { if (uc == 0x131 || uc == 0x130 || uc == 0x17f || 0x1fbe)// <= continue; //^^^^^^ if (RTUniCpIsLower(uc)) { RTTESTI_CHECK_MSG(....), ("%#x\n", uc)); strLower.appendCodePoint(uc); } if (RTUniCpIsUpper(uc)) { RTTESTI_CHECK_MSG(....), ("%#x\n", uc)); strUpper.appendCodePoint(uc); } } .... }

通常、記事はテスト用のファイルに対して発行された警告を受け取りません。 PVS-Studioレポートから、指定されたディレクトリ内のすべてのファイルに発行されたメッセージを簡単に除外できます。しかし、私は1つの例を書くことに決めました。タイプミスのため、テストは何もチェックしません。 for（）ループの各反復で、 'continue'ステートメントが実行されます。条件では式「uc ==」が省略されているため、値「0x1fbe」のみが常にtrueになります。これは、静的分析が単体テストを補完する方法の良い例です。

正しいオプション：

 if (uc == 0x131 || uc == 0x130 || uc == 0x17f || uc == 0x1fbe) continue;

V610未定義の動作。シフト演算子「<<」を確認してください。左のオペランド '（-2）'は負です。 translate.c 2708

 static void gen_push_T1(DisasContext *s) { .... if (s->ss32 && !s->addseg) gen_op_mov_reg_A0(1, R_ESP); else gen_stack_update(s, (-2) << s->dflag); .... }

最新のCおよびC ++言語標準によれば、負の数をシフトすると未定義の動作が発生します。

以下に2つの類似した場所を示します。

V610未定義の動作。シフト演算子「<<」を確認してください。左のオペランドは負です（ 'i64' = [-1..0]）。 tarvfs.cpp 234
V610未定義の動作。シフト演算子「<<」を確認してください。左のオペランド '-16'は負です。 translate.c 2761

トドシキ

V523 「then」ステートメントは「else」ステートメントと同等です。 state_evaluators.c 479

 static void map2(G....) { .... if (g->extensions.NV_vertex_program) { /* XXX FIXME */ i = target - GL_MAP2_COLOR_4; } else { i = target - GL_MAP2_COLOR_4; } .... }

「FIXME」および「TODO」タグは、ソースコード内で非常に長い期間使用できます。しかし、静的アナライザーは、不完全なコードを忘れさせません。

V530関数 'e1kHandleRxPacket'の戻り値を利用する必要があります。 deve1000.cpp 3913

 static void e1kTransmitFrame(PE1KSTATE pThis, bool fOnWorkerThread) { .... /** @todo do we actually need to check that we're in loopback mode here? */ if (GET_BITS(RCTL, LBM) == RCTL_LBM_TCVR) { E1KRXDST status; RT_ZERO(status); status.fPIF = true; e1kHandleRxPacket(pThis, pSg->aSegs[0].pvSeg, ....); // <= rc = VINF_SUCCESS; // <= } e1kXmitFreeBuf(pThis); .... }

ソースコードの他の部分では、e1kHandleRxPacket（）関数の結果は通常、変数「rc」に格納されます。ただし、コードが追加されるまで、関数の結果はここでは使用されず、「VINF_SUCCESS」は常にステータスに保存されます。

新しい診断

このセクションでは、VirtualBoxプロジェクトの前回のチェック後にPVS-Studioに表示されたアナライザーの警告について説明します。

V745 「wchar_t *」タイプの文字列が「BSTR」タイプの文字列に誤って変換されます。「SysAllocString」関数の使用を検討してください。 vboxcredentialprovider.cpp 231

 static HRESULT VBoxCredentialProviderRegisterSENS(void) { .... hr = pIEventSubscription->put_EventClassID( L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}"); .... }

アナライザーは、タイプ「wchar_t *」のストリングで、タイプBSTRのストリングと同様に機能し始めることを発見しました。

BSTR（基本文字列またはバイナリ文字列）は、COM、オートメーション、および相互運用機能で使用される文字列データ型です。このタイプの文字列は、4バイトの長さのプレフィックス、データ文字列、および2つのヌル文字の区切り文字で構成されます。長さの接頭辞は、文字列の最初の文字の直前に示され、制限文字は考慮されません。これを使用すると、データ行の先頭の前に長さの接頭辞がなくなります。

SysAllocString（）関数を使用して修正されたオプション：

 static HRESULT VBoxCredentialProviderRegisterSENS(void) { .... hr = pIEventSubscription->put_EventClassID(SysAllocString( L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}")); .... }

さらにいくつかの疑わしい場所：

V745「wchar_t *」タイプの文字列が「BSTR」タイプの文字列に誤って変換されます。「SysAllocString」関数の使用を検討してください。 vboxcredentialprovider.cpp 277
V745「wchar_t *」タイプの文字列が「BSTR」タイプの文字列に誤って変換されます。「SysAllocString」関数の使用を検討してください。 vboxcredentialprovider.cpp 344
V745「wchar_t *」タイプの文字列が「BSTR」タイプの文字列に誤って変換されます。「SysAllocString」関数の使用を検討してください。 string.cpp 31

V746タイプのスライス。例外は、値ではなく参照によってキャッチする必要があります。 extpackutil.cpp 257

 RTCString *VBoxExtPackLoadDesc(....) { .... xml::XmlFileParser Parser; try { Parser.read(szFilePath, Doc); } catch (xml::XmlError Err) // <= { return new RTCString(Err.what()); } .... }

アナライザーは、値による例外のキャッチに関連する潜在的なエラーを検出しました。これは、コピーコンストラクターを使用して、タイプxml :: XmlErrorの新しい「Err」オブジェクトが作成されることを意味します。これにより、xml :: XmlErrorから継承したクラスに保存された例外情報の一部が失われます。

別の不審な場所：

V746タイプのスライス。例外は、値ではなく参照によってキャッチする必要があります。 extpackutil.cpp 330

おわりに

VirtualBoxプロジェクトは、開発プロジェクトで静的解析を定期的に適用することがいかに重要であるかの良い例です。アナライザーを使用するこのシナリオは、開発プロセス中の潜在的なエラーの増大を防ぎ、使用する分析ツールの最新の更新を受け取ることができます。

また、この記事の執筆中に7〜10分間数回ハングし、プロセッサを完全にロードするMS Wordコードを確認していただければ幸いです。しかし、そのような可能性はまだありません。もちろん、 MS Word 1.1aを使用して考古学調査を実施しましたが、これはまったくありません...

PVS-Studioを簡単にダウンロードして、プロジェクトのエラーを見つけてください。製品のユーザーについて考え、プログラマーの時間を節約してください！

英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Svyatoslav Razmyslov。 Oracle VM VirtualBoxに対する新しい目。

記事を読んで質問がありますか？

多くの場合、記事には同じ質問が寄せられます。ここで回答を集めました： PVS-Studioバージョン2015に関する記事の読者からの質問への回答。リストをご覧ください。

Oracle VM VirtualBoxコードの見た目