Oracle VM VirtualBoxの確認。 パート1

仮想マシンは、さまざまなニーズに使用されます。 私自身も数年間VirtualBoxを使用してソフトウェアをテストし、さまざまなLinuxディストリビューションを簡単に研究しています。 実際、長期間使用した後、未定義の動作が定期的に発生したため、オープンソースプロジェクトのチェックに経験を活用し、Oracle VM Virtual Boxのソースコードを分析することにしました。



VirtualBoxは、クロスプラットフォームの仮想化アプリケーションです。 これはどういう意味ですか？ まず、Windows、Mac、Linuxなどを実行するIntelまたはAMDプロセッサーを搭載したコンピューターで動作します。 次に、コンピューターの機能が拡張され、複数のオペレーティングシステムで（仮想マシン内で）同時に作業できるようになります。



このプロジェクトは問題領域が非常に豊富であることが判明したため、エラーが多かれ少なかれ明らかな場所のみを説明するため、資料を2つの記事に分割する必要があります。



記事へのコメントの中で彼らはよく尋ねます：実行時のエラーは何につながりますか？ 私たちの大部分は、検証済みのプロジェクトを使用せず、さらにそれらをデバッグしません。 この記事を書いているとき、私はVirutalBoxの通常の使用に関する問題に拍車がかかった。 元のコメントを残すことを決めましたが、コメントを少し短くしました。コメントがない場合は、ファイルヘッダーからコメントを追加します。 みんなにグリッチを見つけさせてください。



Virtual Boxは、 PVS-Studio 5.19を使用してテストされました。 WindowsでのアセンブリにはkBuildアセンブリシステムが使用されるため、検証には特別なユーティリティPVS-Studio Standaloneを使用しました。これについては、 PVS-StudioがWindows上のビルドシステムとコンパイラをサポートするようになりました。 簡単ですぐに使用できます 。

変数と文字列のタイプミス

V501 「||」の左右に同一のサブ式「pState-> fIgnoreTrailingWhite」があります。 演算子。 scmdiff.cpp 238

typedef struct SCMDIFFSTATE { .... bool fIgnoreTrailingWhite; bool fIgnoreLeadingWhite; .... } SCMDIFFSTATE; /* Pointer to a diff state. */ typedef SCMDIFFSTATE *PSCMDIFFSTATE; /* Compare two lines */ DECLINLINE(bool) scmDiffCompare(PSCMDIFFSTATE pState, ....) { .... if (pState->fIgnoreTrailingWhite //<== || pState->fIgnoreTrailingWhite) //<== return scmDiffCompareSlow(....); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

おそらく、「pState」構造のチェック済みフィールドの1つは「fIgnoreLeadingWhite」である必要があります。



V501同一のサブ式があります '！フィールド（ "ユーザー名"）。ToString（）。IsEmpty（）'の左と右の '||' 演算子。 uiwizardexportapp.cpp 177

 /* @file * VBox frontends: Qt4 GUI ("VirtualBox") */ QString UIWizardExportApp::uri(bool fWithFile) const { .... case SunCloud: { ... QString uri("SunCloud://"); .... if (!field("username").toString().isEmpty() || //<== !field("username").toString().isEmpty()) //<== uri = QString("%1@").arg(uri); .... } case S3: { QString uri("S3://"); .... if (!field("username").toString().isEmpty() || !field("password").toString().isEmpty()) uri = QString("%1@").arg(uri); .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

switch（）演算子の隣接ブランチから判断すると、おそらく「ユーザー名」と「パスワード」もあるはずです。



V519 「wcLeft」変数には、連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：472、473。supr3hardenedmain-win.cpp 473

 /* Verify string cache compare function. */ static bool supR3HardenedWinVerifyCacheIsMatch(....) { .... wcLeft = wcLeft != '/' ? RT_C_TO_LOWER(wcLeft) : '\\'; wcLeft = wcRight != '/' ? RT_C_TO_LOWER(wcRight) : '\\'; //<== if (wcLeft != wcRight) return false; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

明らかに、2番目の割り当ては変数 'wcRight'でなければなりません。



V519 「pci_conf [0xa0]」変数には、連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：806、807。devpci.cpp 807

 /* @file * DevPCI - PCI BUS Device. */ static void pciR3Piix3Reset(PIIX3State *d) { .... pci_conf[0x82] = 0x02; pci_conf[0xa0] = 0x08; //<== pci_conf[0xa0] = 0x08; //<== pci_conf[0xa2] = 0x00; pci_conf[0xa3] = 0x00; pci_conf[0xa4] = 0x00; pci_conf[0xa5] = 0x00; pci_conf[0xa6] = 0x00; pci_conf[0xa7] = 0x00; pci_conf[0xa8] = 0x0f; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このフラグメントはコピーアンドペーストが原因である可能性があります。 最良の場合、余分な行がありますが、最悪の場合、インデックス「0xa1」による要素の初期化はスキップされます。



V583 「？：」演算子は、条件式に関係なく、常に1つの同じ値を返します：g_acDaysInMonthsLeap [pTime-> u8Month-1]。 time.cpp 453

 static const uint8_t g_acDaysInMonths[12] = { /*Jan Feb Mar Arp May Jun Jul Aug Sep Oct Nov Dec */ 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; static const uint8_t g_acDaysInMonthsLeap[12] = { /*Jan Feb Mar Arp May Jun Jul Aug Sep Oct Nov Dec */ 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; static PRTTIME rtTimeNormalizeInternal(PRTTIME pTime) { .... unsigned cDaysInMonth = fLeapYear ? g_acDaysInMonthsLeap[pTime->u8Month - 1] //<== : g_acDaysInMonthsLeap[pTime->u8Month - 1]; //<== .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

コメントはありません。 VirtualBoxは常にうるう年だというだけです。



V519 「ch」変数には、連続して2回値が割り当てられます。 おそらくこれは間違いです。 行を確認してください：1135、1136。vboxcpp.cpp 1136

 /* Skips white spaces, including escaped new-lines. */ static void vbcppProcessSkipWhiteAndEscapedEol(PSCMSTREAM pStrmInput) { .... if (ch == '\r' || ch == '\n') { .... } else if (RT_C_IS_SPACE(ch)) { ch = chPrev; //<== ch = ScmStreamGetCh(pStrmInput); //<== Assert(ch == chPrev); } else break; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

代入演算子のオペランドが逆であり、前の文字がこの場所に格納されると仮定することは論理的です：

 chPrev = ch; ch = ScmStreamGetCh(pStrmInput); Assert(ch == chPrev);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

行の1つの変数をいくつかの異なる値に割り当てることは必ずしも間違いではないことに注意してください-作成者はホグワーツ以外で魔法を使うことがあります：



V519 「pixelformat」変数には、値が連続して2回割り当てられます。 おそらくこれは間違いです。 行をチェック：686、688。renderspu_wgl.c 688

 /* Okay, we were loaded manually. Call the GDI functions. */ pixelformat = ChoosePixelFormat( hdc, ppfd ); /* doing this twice is normal Win32 magic */ pixelformat = ChoosePixelFormat( hdc, ppfd );
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

一定の条件

V547式は常に真です。 ここでは、おそらく「&&」演算子を使用する必要があります。 vboxfboverlay.cpp 2259

 /* @file * VBoxFBOverlay implementation int */ VBoxVHWAImage::reset(VHWACommandList * pCmdList) { .... if (pCmd->SurfInfo.PixelFormat.c.rgbBitCount != 32 || pCmd->SurfInfo.PixelFormat.c.rgbBitCount != 24) { AssertFailed(); pCmd->u.out.ErrInfo = -1; return VINF_SUCCESS; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

条件は、変数「pCmd-> SurfInfo.PixelFormat.c.rgbBitCount」のすべての値に当てはまります。おそらく、演算子「&&」を使用するか、変数のいずれかにタイプミスが存在する必要があります。



V547式「uCurCode <uPrevCode」は常にfalseです。 符号なしの型の値が<0になることはありません。dbgmoddwarf.cpp2887

 /* Deals with a cache miss in rtDwarfAbbrev_Lookup. */ static PCRTDWARFABBREV rtDwarfAbbrev_LookupMiss(....) { .... uint32_t uPrevCode = 0; for (;;) { /* Read the 'header'. Skipping zero code bytes. */ uint32_t const uCurCode =rtDwarfCursor_GetULeb128AsU32(....); if (pRet && (uCurCode == 0 || uCurCode < uPrevCode)) //<== break; /* probably end of unit. */ .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

変数「uPrevCode」はゼロに初期化され、他のどこでも変更されません。したがって、条件式「uCurCode <uPrevCode」は常にfalseになります。 符号なしの数値はゼロより小さくなりません。



V534 「for」演算子内で間違った変数が比較されている可能性があります。 「i」の検討を検討してください。 vboxdispd3d.cpp 4470

 /* @file * VBoxVideo Display D3D User mode dll */ static HRESULT APIENTRY vboxWddmDDevCreateResource(....) { .... for (UINT i = 0; i < pResource->SurfCount; ++i) { .... if (SUCCEEDED(hr)) { .... } else { for (UINT j = 0; i < j; ++j) { .... } break; } } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ネストされたループは、単一の反復を実行しません。 おそらく、エラーの状態は「i <j」です。 おそらく、彼らは次のように書きたかったのです。j <i。



V648 「&&」操作の優先度は「||」の優先度よりも高い 操作。 drvacpi.cpp 132

 /*Get the current power source of the host system. */ static DECLCALLBACK(int) drvACPIQueryPowerSource(....) { .... /* running on battery? */ if (powerStatus.ACLineStatus == 0 /* Offline */ || powerStatus.ACLineStatus == 255 /* Unknown */ && (powerStatus.BatteryFlag & 15)) { *pPowerSource = PDM_ACPI_POWER_SOURCE_BATTERY; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

この条件は定数値を取りませんが、操作の優先順位は疑わしいように見えます。 おそらく、「||」演算子で式を括弧で囲む必要があります。

紛らわしいデザイン

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

 /* Called by the Console when it's done saving the VM state into *the snapshot (if online) and reconfiguring the hard disks. */ STDMETHODIMP SessionMachine::EndTakingSnapshot(BOOL aSuccess) { .... if (fOnline) //no need to test for whether the saved state file is shared: //an online snapshot means that a new saved state file was //created, which we must clean up now RTFileDelete(mConsoleTaskData.mSnapshot->....); machineLock.acquire(); //<== mConsoleTaskData.mSnapshot->uninit(); machineLock.release(); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このフラグメント内のテキストの書式設定は、「machineLock.acquire（）」関数の呼び出しを特定の条件下でのみ実行する必要があり、常にではないことを示唆しています。



V640コードの操作ロジックがそのフォーマットに対応していません。 2番目のステートメントは常に実行されます。 中括弧が欠落している可能性があります。 vboxguestr3libdraganddrop.cpp 656

 static int vbglR3DnDGHProcessRequestPendingMessage(....) { .... rc = Msg.hdr.result; if (RT_SUCCESS(rc)) rc = Msg.uScreenId.GetUInt32(puScreenId); AssertRC(rc); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

意図したロジックを反映しないフォーマットのより視覚的な例。



V561新たに宣言するよりも、 'Status'変数に値を割り当てる方がおそらく良いでしょう。 前の宣言：vboxmpwddm.cpp、行5723。vboxmpwddm.cpp 5728

 /* @file * VBox WDDM Miniport driver */ static NTSTATUS APIENTRY DxgkDdiRenderNew(CONST HANDLE hContext, DXGKARG_RENDER *pRender) { .... NTSTATUS Status = STATUS_SUCCESS; //<== __try { .... NTSTATUS Status = STATUS_SUCCESS; //<== .... } __except (EXCEPTION_EXECUTE_HANDLER) { Status = STATUS_INVALID_PARAMETER; WARN(("invalid parameter")); } return Status; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

無駄な新しいローカル変数 'Status'が宣言されています。 try..exceptセクションで変数を変更しても戻り値は変更されず、外部（try {}ブロックに関して）変数は例外の場合にのみ変更されます。



V638文字列内に端末ヌルが存在します。 「\ 0x01」文字が見つかりました。 おそらく意味： '\ x01'。 devsmc.cpp 129

 /* @file * DevSMC - SMC device emulation. */ static struct AppleSMCData data[] = { {6, "REV ", "\0x01\0x13\0x0f\0x00\0x00\0x03"}, //<== {32,"OSK0", osk }, {32,"OSK1", osk+32 }, {1, "NATJ", "\0" }, {1, "MSSP", "\0" }, {1, "MSSD", "\0x3" }, //<== {1, "NTOK", "\0"}, {0, NULL, NULL } };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

「\ x01」のように、ゼロなしの文字列に16進文字を指定する必要があります。指定しない場合、文字「\ 0」は終端ゼロとして解釈されます。



V543値 'true'がHRESULT型の変数 'mRemoveSavedState'に割り当てられているのは奇妙です。 machineimpl.cpp 12247

 class ATL_NO_VTABLE SessionMachine : public Machine { .... HRESULT mRemoveSavedState; .... } HRESULT SessionMachine::init(Machine *aMachine) { .... /* default is to delete saved state on * Saved -> PoweredOff transition */ mRemoveSavedState = true; .... } HRESULT SessionMachine::i_setMachineState(....) { .... if (mRemoveSavedState) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

HRESULTとtype boolはまったく異なる種類の意味です。 HRESULTは、エラーの重大度コード、デバイスコード、エラーコードの3つの異なるフィールドに分割された32ビット値です。 HRESULT値を操作するには、S_OK、E_FAIL、E_ABORTなどの特別な定数が使用されます。 また、HRESULT型の値をチェックするには、SUCCEEDED、FAILEDなどのマクロが対象です。



HRESULT変数を使用する同様の場所：

• V545このような「if」演算子の条件式は、HRESULT型の値「mRemoveSavedState」に対して正しくありません。 代わりにSUCCEEDEDまたはFAILEDマクロを使用する必要があります。 machineimpl.cpp 14312
• V545このような「if」演算子の条件式は、HRESULT型の値「procCaller.rc（）」に対して正しくありません。 代わりにSUCCEEDEDまたはFAILEDマクロを使用する必要があります。 guestsessionimpl.cpp 1981
• V545このような「if」演算子の条件式は、HRESULT型の値「machCaller.rc（）」に対して正しくありません。 代わりにSUCCEEDEDまたはFAILEDマクロを使用する必要があります。 virtualboximpl.cpp 3079

未定義の動作

V567未定義の動作。 「curg」変数は、シーケンスポイント間で2回使用されている間に変更されます。 consoleevents.h 75

 template<class C> class ConsoleEventBuffer { public: .... C get() { C c; if (full || curg != curp) { c = buf[curg]; ++curg %= sz; //<== full = false; } return c; } .... };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

変数 'c​​urg'は、同じ連続点で2回使用されます。 その結果、そのような式の結果を予測することは不可能です。 詳細については、診断V567の説明を参照してください。



同様の場所：

• V567未定義の動作。 'curp'変数は、シーケンスポイント間で2回使用されている間に変更されます。 consoleevents.h 95
• V567未定義の動作。 'curp'変数は、シーケンスポイント間で2回使用されている間に変更されます。 consoleevents.h 122

V614潜在的に初期化されていない変数「rc」が使用されました。 suplib-win.cpp 367

 /* Stops a possibly running service. */ static int suplibOsStopService(void) { /* Assume it didn't exist, so we'll create the service. */ int rc; SC_HANDLE hSMgr = OpenSCManager(....); .... if (hSMgr) { .... rc = VINF_SUCCESS; .... } return rc; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

変数「hSMgr」のステータスが正しくない場合、関数は初期化されていない変数「rc」を返します。



同様の場所：

• V614潜在的に初期化されていない変数「rc」が使用されました。 suplib-win.cpp 416

V611メモリは「malloc / realloc」機能を使用して割り当てられましたが、「delete」演算子を使用して解放されました。 「pBuffer」変数の背後にある操作ロジックを調べることを検討してください。 tsmfhook.cpp 1261

 /* @file * VBoxMMR - Multimedia Redirection */ void ReadTSMF(uint32_t u32ChannelHandle, uint32_t u32HGCMClientId, uint32_t u32SizeAvailable) { .... PBYTE pBuffer = (PBYTE)malloc(u32SizeAvailable + sizeof(....)); .... delete [] pBuffer; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

バッファのメモリは、互換性のない方法で割り当てられ、解放されます。

とても迷惑な場所

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

 void Appliance::i_importMachines(....) { .... /* Iterate through all virtual systems of that appliance */ size_t i = 0; for (it = reader.m_llVirtualSystems.begin(), it1 = m->virtualSystemDescriptions.begin(); it != reader.m_llVirtualSystems.end(), //<== it1 != m->virtualSystemDescriptions.end(); ++it, ++it1, ++i) {....} .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

どうやら、プログラマはそれについて考え、ループの引数の間にコンマを入れ続けました。 コンマ演算子は両方の引数を計算し、2番目の引数を返します。したがって、ここでは、1つの条件がループの動作に影響を与えません。



V529奇数セミコロン ';' 「for」演算子の後。 server_getshaders.c 92

 /* @file * VBox OpenGL GLSL related get functions */ void SERVER_DISPATCH_APIENTRY crServerDispatchGetAttachedShaders(....) { .... for (i=0; i<*pLocal; ++i); //<== ids[i] = crStateGLSLShaderHWIDtoID(ids[i]); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

サイクル後のセミコロンは、構想されたロジックを完全に変更しました。 想定されるループの本体は反復に関与しませんが、ループの実行後に1回実行されます。



V654ループの条件は常に真です。 suphardenedverifyprocess-win.cpp 1732

 /* Opens a loader cache entry. */ DECLHIDDEN(int) supHardNtLdrCacheOpen(const char *pszName, ....) { .... uint32_t i = 0; while (i < RT_ELEMENTS(g_apszSupNtVpAllowedDlls)) if (!strcmp(pszName, g_apszSupNtVpAllowedDlls[i])) break; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このループの危険性は、カウンターの値が変わらないことです。したがって、配列の最初の要素が「pszName」と一致しない場合、ループは永遠になります。



V606所有者なしトークン '0'。 vboxmpvbva.cpp 997

 /** @file * VBox WDDM Miniport driver */ VBOXCMDVBVA_HDR* VBoxCmdVbvaSubmitLock(....) { if (VBoxVBVAExGetSize(&pVbva->Vbva) < cbCmd) { WARN(("....")); NULL; //<== } if (!VBoxVBVAExBufferBeginUpdate(....) { WARN(("VBoxVBVAExBufferBeginUpdate failed!")); return NULL; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

「戻り」を逃しました。



V626ミスプリントのチェックを検討してください。 「、」を「;」に置き換える必要がある場合があります。 ldrmemory.cpp 317

 /*@file *IPRT-Binary Image Loader, The Memory/Debugger Oriented Parts.*/ RTDECL(int) RTLdrOpenInMemory(....) { if (RT_SUCCESS(rc)) { .... } else pfnDtor(pvUser), //<== *phLdrMod = NIL_RTLDRMOD; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

この方法で設定されたコンマは、「else」の下の次のステートメントを取ります。 おそらく、これは計画されていなかった。

おわりに

VirtualBoxの検証記事が最大限に活用され、開発者、テスター、その他のアクティブユーザーにとってこのような重要な製品が少し良くなることを願っています。



静的分析を定期的に使用することで、より便利なタスクを解決するために多くの時間を節約できます。 また、コードの品質管理は、たとえば、新しいサービスを使用して、C / C ++コードの定期的な監査など、他のサービスに移行できます 。

この記事は英語です。

英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Svyatoslav Razmyslov。 Oracle VM VirtualBoxの確認。 パート1 。