# Checking Oracle VM VirtualBox\. Part 2 Virtual machines are used for very different tasks\. Personally I have been using VirtualBox for many years to test software and simply study various Linux distributions\. And now, after years of using the tool and encountering undefined behavior every now and then, I've decided to make use of my experience in analysis of open\-source projects and check the source code of Oracle VM Virtual Box\.In this article, I will continue describing the numerous suspicious fragments found in the project\. The first part of the article: [Checking Oracle VM VirtualBox\. Part 1](https://pvs-studio.com/en/blog/posts/cpp/0281/)\. ![0282_N2_VirtualBox/image1.png](https://import.viva64.com/docx/blog/0282_N2_VirtualBox/image1.png) [VirtualBox](https://www.virtualbox.org/) is a cross\-platform virtualization product\. What does it mean? First, it can run on computers with Intel or AMD processors under Windows, Mac, Linux and other operating systems\. Second, it extends your computer's capabilities by allowing a number of different operating systems to run simultaneously \(inside virtual machines\)\. Virtual Box was analyzed by [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) 5\.19\. We use the kBuild build system to build it under Windows, so I had to use a special utility PVS\-Studio Standalone described in the article [PVS\-Studio Now Supports Any Build System under Windows and Any Compiler\. Easy and Right Out of the Box](https://pvs-studio.com/en/blog/posts/0243/)\. ## Typos in conditions [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'fVersion' to the left and to the right of the '\|\|' operator\. applianceimplexport\.cpp 1071 ```cpp /* Called from Appliance::i_buildXML() for each virtual * system (machine) that needs XML written out.*/ void Appliance::i_buildXMLForOneVirtualSystem(....) { .... bool fProduct = .... ; bool fProductUrl = .... ; bool fVendor = .... ; bool fVendorUrl = .... ; bool fVersion = .... ; if (fProduct || fProductUrl || fVersion || // <= fVendorUrl || fVersion) // <= { .... } .... } ``` Here we see five logic variables declared whose states are checked in one conditional expression, but using the copy\-paste technique resulted in the programmer having forgotten to rename one of the variables into 'fVendor'\. [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions '\!Module\.symspace' to the left and to the right of the '\|\|' operator\. dbgpluginsolaris\.cpp 519 ```cpp static void dbgDiggerSolarisProcessModCtl32(....) { .... /* Ignore modules without symbols. */ if (!Module.symtbl || !Module.strings || !Module.symspace || !Module.symspace) // <= return; //Check that the symtbl and strings points inside the symspace. if (Module.strings - Module.symspace >= Module.symsize) return; if (Module.symtbl - Module.symspace >= Module.symsize) return; .... } ``` All the variables in the condition are of the 'uint32\_t' type and if one of them equals zero, the program leaves the function\. Most likely, one of the two repeating variables should be named 'Module\.symsize'\. Another similar defect in the same file: * V501 There are identical sub\-expressions '\!Module\.symspace' to the left and to the right of the '\|\|' operator\. dbgpluginsolaris\.cpp 665 [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression is always false\. Probably the '\|\|' operator should be used here\. vboxmanageguestctrl\.cpp 2365 ```cpp /* Creates a source root by stripping file names or filters * of the specified source.*/ static int ctrlCopyCreateSourceRoot(....) { .... size_t lenRoot = strlen(pszNewRoot); if ( lenRoot && pszNewRoot[lenRoot - 1] == '/' && pszNewRoot[lenRoot - 1] == '\\' && lenRoot > 1 && pszNewRoot[lenRoot - 2] == '/' && pszNewRoot[lenRoot - 2] == '\\') { .... } .... } ``` One variable cannot equal two different values at a time, therefore the condition is always false\. [V682](https://pvs-studio.com/en/docs/warnings/v682/) Suspicious literal is present: '//'\. It is possible that a backslash should be used here instead: '\\\\'\. supr3hardenedmain\-win\.cpp 936 ```cpp /* Fixes up a path possibly containing one or more alternative * 8-dot-3 style components. */ static void supR3HardenedWinFix8dot3Path(....) { .... while ((wc = *pwszFixEnd) != '\0' && wc != '\\' && wc != '//') pwszFixEnd++; .... } ``` The loop termination condition is represented by an end of string character or one of the slashes\. The backward slash \(\\\) should be shielded in a special way, while the programmer must have accidentally added one more slash to the forward slash \(/\), thus getting an incorrect value 0x2F2F\. [V583](https://pvs-studio.com/en/docs/warnings/v583/) The '?:' operator, regardless of its conditional expression, always returns one and the same value: sizeof \(uMod\.Info64\)\. dbgplugindarwin\.cpp 557 ```cpp static DECLCALLBACK(int) dbgDiggerDarwinInit(PUVM pUVM, void *pvData) { .... union { OSX64_kmod_info_t Info64; OSX32_kmod_info_t Info32; } uMod; RT_ZERO(uMod); rc = DBGFR3MemRead(pUVM, 0 /*idCpu*/, &AddrModInfo, &uMod, f64Bit ? sizeof(uMod.Info64) : sizeof(uMod.Info64)); .... } ``` Probably the argument of one of the sizeof\(\) operators should be the 'uMod\.Info32' variable\. [V517](https://pvs-studio.com/en/docs/warnings/v517/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 541, 674\. vcicache\.cpp 541 ```cpp /*Loads the block map from the specified medium and creates all necessary in memory structures to manage used and free blocks.*/ static int vciBlkMapLoad(....) { .... rc = vdIfIoIntFileReadSync(....) if (RT_SUCCESS(rc)) // <= { .... } else if (RT_SECCESS(rc)) // <= rc = VERR_VD_GEN_INVALID_HEADER; .... LogFlowFunc(("returns rc=%Rrc\n", rc)); return rc; } ``` The 'else' branch will never get control, and the status 'VERR\_VD\_GEN\_INVALID\_HEADER' won't be set in case of an error\. ## Arguments of sizeof\(\) [V568](https://pvs-studio.com/en/docs/warnings/v568/) It's odd that the argument of sizeof\(\) operator is the 'sizeof \(pSh\-\>Name\)' expression\. ldrpe\.cpp 1598 ```cpp /* @file * IPRT - Binary Image Loader, Portable Executable (PE). */ typedef struct _IMAGE_SECTION_HEADER { uint8_t Name[IMAGE_SIZEOF_SHORT_NAME]; .... } IMAGE_SECTION_HEADER; static DECLCALLBACK(int) rtldrPE_EnumSegments(....) { PCIMAGE_SECTION_HEADER pSh = pModPe->paSections; .... szName[sizeof(sizeof(pSh->Name))] = '\0'; // <= .... } ``` The sizeof\(\) operator calculates the expression's type and returns the size of this type\. In this code fragment, the terminal null character will be written into the "sizeof\(size\_t\)" position instead of the end of the string\. [V568](https://pvs-studio.com/en/docs/warnings/v568/) It's odd that the argument of sizeof\(\) operator is the 'pSub\-\>auBitmap\[0\] \* 8' expression\. mmpagepool\.cpp 234 ```cpp /* Allocates a page from the page pool. */ DECLINLINE(void *) mmR3PagePoolAlloc(PMMPAGEPOOL pPool) { .... int rc = MMHyperAlloc(pPool->pVM, RT_OFFSETOF(MMPAGESUBPOOL, auBitmap[cPages / (sizeof(pSub->auBitmap[0] * 8))]) + ....); .... } ``` In this code, the sizeof\(\) operator calculates the type of the " pSub\-\>auBitmap\[0\] \* 8" expression\. I guess one parenthesis is in the wrong place\. ## Typos in initialization [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'pPool\-\>aPages\[0\]\.iMonitoredNext' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 300, 301\. pgmpool\.cpp 301 ```cpp typedef struct PGMPOOLPAGE { .... uint16_t iMonitoredNext; uint16_t iMonitoredPrev; .... } PGMPOOLPAGE; /* Initializes the pool */ int pgmR3PoolInit(PVM pVM) { .... pPool->aPages[NIL_PGMPOOL_IDX].iModifiedNext = NIL_PGMPOOL_IDX; pPool->aPages[NIL_PGMPOOL_IDX].iModifiedPrev = NIL_PGMPOOL_IDX; pPool->aPages[NIL_PGMPOOL_IDX].iMonitoredNext= NIL_PGMPOOL_IDX; pPool->aPages[NIL_PGMPOOL_IDX].iMonitoredNext= NIL_PGMPOOL_IDX; pPool->aPages[NIL_PGMPOOL_IDX].iAgeNext = NIL_PGMPOOL_IDX; pPool->aPages[NIL_PGMPOOL_IDX].iAgePrev = NIL_PGMPOOL_IDX; .... } ``` An initialization of the 'iMonitoredPrev' field is missing \- and this field does exist in the structure used\. A similar issue: * V519 The 'pPage\-\>iMonitoredNext' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 482, 483\. pgmpool\.cpp 483 [V570](https://pvs-studio.com/en/docs/warnings/v570/) The 'pReq\-\>cT2ISegs' variable is assigned to itself\. iscsi\.cpp 4743 ```cpp static int iscsiRead(....) { .... pReq->enmXfer = SCSIXFER_FROM_TARGET; pReq->cbCDB = cbCDB; pReq->cbI2TData = 0; pReq->paI2TSegs = NULL; pReq->cI2TSegs = 0; pReq->cbT2IData = cbToRead; pReq->paT2ISegs = &pReq->aSegs[pReq->cI2TSegs]; pReq->cT2ISegs = pReq->cT2ISegs; // <= pReq->cbSense = sizeof(pReq->abSense); pReq->cT2ISegs = cT2ISegs; // <= pReq->pIoCtx = pIoCtx; pReq->cSenseRetries = 10; pReq->rcSense = VERR_READ_ERROR; .... } ``` This is quite a suspicious fragment as the "pReq\-\>cT2ISegs" variable is first assigned its own value and then another value\. ## Incorrect input/output [V576](https://pvs-studio.com/en/docs/warnings/v576/) Incorrect format\. Consider checking the second actual argument of the 'printf' function\. To print the value of pointer the '%p' should be used\. usbtest\.cpp 191 ```cpp /* Remove USB device filter */ int usbMonRemoveFilter (void *aID) { .... printf("usblibRemoveFilter %x\n", aID); .... } ``` This code is incorrect because it will work only in 32\-bit systems, while in Win64 it will print only the low part of the 'aID' pointer\. The correct code should look as follows: ```cpp printf("usblibRemoveFilter %p\n", aID); ``` [V576](https://pvs-studio.com/en/docs/warnings/v576/) Incorrect format\. A different number of actual arguments is expected while calling 'swprintf\_s' function\. Expected: 4\. Present: 5\. vboxinstallhelper\.cpp 311 ```cpp static LONG installBrandingValue(....) { .... if (wcsicmp(L"General", pwszSection) != 0) swprintf_s(wszKey, RT_ELEMENTS(wszKey), L"SOFTWARE\\%s\\VirtualBox\\Branding\\", VBOX_VENDOR_SHORT, pwszSection); // <= .... } ``` The second argument won't be printed since the formatted string contains only one output specifier\. [V576](https://pvs-studio.com/en/docs/warnings/v576/) Incorrect format\. Consider checking the fifth actual argument of the 'swprintf\_s' function\. The wchar\_t type argument is expected\. vboxinstallhelper\.cpp 340 ```cpp UINT CopyDir(MSIHANDLE hModule, const WCHAR *pwszDestDir, const WCHAR *pwszSourceDir) { .... swprintf_s(wszDest, RT_ELEMENTS(wszDest), L"%s%c", pwszDestDir, '\0'); // <= swprintf_s(wszSource, RT_ELEMENTS(wszSource), L"%s%c", pwszSourceDir, '\0'); // <= .... } ``` The correct code should look as follows: ```cpp swprintf_s(wszDest, RT_ELEMENTS(wszDest), L"%s%c", pwszDestDir, L'\0'); swprintf_s(wszSource, RT_ELEMENTS(wszSource), L"%s%c", pwszSourceDir, L'\0'); ``` Other similar fragments: * V576 Incorrect format\. Consider checking the fifth actual argument of the 'swprintf\_s' function\. The wchar\_t type argument is expected\. vboxinstallhelper\.cpp 341 * V576 Incorrect format\. Consider checking the fifth actual argument of the 'swprintf\_s' function\. The wchar\_t type argument is expected\. vboxinstallhelper\.cpp 370 * V576 Incorrect format\. Consider checking the fifth actual argument of the 'swprintf\_s' function\. The wchar\_t type argument is expected\. vboxinstallhelper\.cpp 399 * V576 Incorrect format\. Consider checking the fifth actual argument of the 'swprintf\_s' function\. The wchar\_t type argument is expected\. vboxinstallhelper\.cpp 400 ## About pointers [V522](https://pvs-studio.com/en/docs/warnings/v522/) Dereferencing of the null pointer 'pParent' might take place\. stam\.cpp 1135 ```cpp /* Frees empty lookup nodes if it's worth it. */ static void stamR3LookupMaybeFree(PSTAMLOOKUP pLookup) { .... PSTAMLOOKUP pCur = pLookup->pParent; if (!pCur) return; if (pCur->cDescsInTree > 0) return; PSTAMLOOKUP pParent = pCur->pParent; if (pParent) // <= return; if (pParent->cDescsInTree == 0 && pParent->pParent) // <= { pCur = pParent; pParent = pCur->pParent; } .... } ``` If you look close at this code fragment, you'll notice that the program leaves the function if the 'pParent' pointer is correct, and then this pointer is used a bit later\. It seems like there is a complementary operator missing\. The correct code should look as follows: ```cpp if (!pParent) return; if (pParent->cDescsInTree == 0 && pParent->pParent) { .... } ``` [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'gCtx\.au64LastCpuLoad\_Kernel \=\= 0' is always false\. Pointer 'gCtx\.au64LastCpuLoad\_Kernel' \!\= NULL\. vboxservicestats\.cpp 220 ```cpp uint64_t au64LastCpuLoad_Kernel[VMM_MAX_CPU_COUNT]; static void VBoxServiceVMStatsReport(void) { .... if (gCtx.au64LastCpuLoad_Kernel == 0) { /* first time */ gCtx.au64LastCpuLoad_Idle[0] =pProcInfo->IdleTime.QuadPart; gCtx.au64LastCpuLoad_Kernel[0]=pProcInfo->KernelTime.QuadPart; gCtx.au64LastCpuLoad_User[0] =pProcInfo->UserTime.QuadPart; Sleep(250); rc = gCtx.pfnNtQuerySystemInformation(....); Assert(!rc); } .... } ``` It doesn't make sense to check a pointer to an array declared on the stack, for if the allocated memory amount is not enough, an exception will be thrown\. [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'pImage' pointer was utilized before it was verified against nullptr\. Check lines: 6299, 6305\. vmdk\.cpp 6299 ```cpp static int vmdkSetComment(....) { .... if (pImage->uOpenFlags & VD_VMDK_IMAGE_FLAGS_STREAM_OPTIMIZED) { rc = VERR_NOT_SUPPORTED; goto out; } if (pImage) rc = vmdkSetImageComment(pImage, pszComment); else rc = VERR_VD_NOT_OPENED; .... } ``` The pointer is only checked for being valid after it is dereferenced\. Other similar fragments: * V595 The 'pExtent\-\>pszBasename' pointer was utilized before it was verified against nullptr\. Check lines: 2900, 2902\. vmdk\.cpp 2900 * V595 The 'pszSymbol' pointer was utilized before it was verified against nullptr\. Check lines: 202, 213\. suplibldr\.cpp 202 * V595 The 'papLunOld' pointer was utilized before it was verified against nullptr\. Check lines: 214, 216\. vscsidevice\.cpp 214 * V595 The 'conn' pointer was utilized before it was verified against nullptr\. Check lines: 323, 327\. api\_msg\.c 323 * V595 The 'm\_pRefreshAct' pointer was utilized before it was verified against nullptr\. Check lines: 2906, 2914\. vboxdbgstatsqt4\.cpp 2906 * V595 The 'm\_pRefreshAct' pointer was utilized before it was verified against nullptr\. Check lines: 2929, 2937\. vboxdbgstatsqt4\.cpp 2929 ## Operation precedence [V542](https://pvs-studio.com/en/docs/warnings/v542/) Consider inspecting an odd type cast: 'bool' to 'wchar\_t \*'\. qifiledialog\.cpp 483 ```cpp /* Reimplementation of QFileDialog::getSaveFileName() * that removes some oddities and limitations. */ QString QIFileDialog::getSaveFileName (....) { .... ofn.lpstrFilter = (TCHAR *) winFilters.isNull() ? 0 : winFilters.utf16(); .... } ``` The type conversion operator here has a higher priority than the '?:' operator\. But the programmer is lucky enough for the code to execute as expected\. [V593](https://pvs-studio.com/en/docs/warnings/v593/) Consider reviewing the expression of the 'A \= B \> C' kind\. The expression is calculated as following: 'A \= \(B \> C\)'\. applianceimplimport\.cpp 384 ```cpp HRESULT Appliance::interpret() { .... if (vsysThis.pelmVBoxMachine) { .... } else if (size_t cEthernetAdapters = vsysThis.llEthernetAdapters.size() > 0) { if (cEthernetAdapters > maxNetworkAdapters) .... } .... } ``` The '\>' operator's priority is higher than that of the '\=' operator, therefore the 'cEthernetAdapters' variable here takes only two values \- 0 and 1, and then the program goes on using an incorrect value\. ## Memory leak [V599](https://pvs-studio.com/en/docs/warnings/v599/) The destructor was not declared as a virtual one, although the 'ModelItem' class contains virtual functions\. uiapplianceeditorwidget\.cpp 783 ```cpp class VirtualSystemModel: public QAbstractItemModel { .... private: ModelItem *m_pRootItem; }; VirtualSystemModel::~VirtualSystemModel() { if (m_pRootItem) delete m_pRootItem; // <= } ``` The analyzer has detected a potential error caused by the absence of a virtual destructor in the base class\. From the description of the "ModelItem" class, we can know how the destructor is declared: ```cpp class ModelItem { public: .... ~ModelItem(); // <= .... virtual Qt::ItemFlags itemFlags(int) const { return 0; } virtual bool setData(int, const QVariant &, int) {....} .... }; ``` Indeed, the destructor is not declared as virtual and below is an example of a class that cannot be completely cleared because of that: ```cpp class VirtualSystemItem: public ModelItem // <= { public: VirtualSystemItem(....); virtual QVariant data(int column, int role) const; virtual void putBack(....); private: CVirtualSystemDescription m_desc; // <= }; ``` Here is one more suspicious class: * V599 The virtual destructor is not present, although the 'Aggregate' class contains virtual functions\. performance\.h 855 ## A few more problems [V530](https://pvs-studio.com/en/docs/warnings/v530/) The return value of function '\_wgetenv' is required to be utilized\. env\-generic\.cpp 239 ```cpp RTDECL(int) RTEnvClone(PRTENV pEnv, RTENV EnvToClone) { .... papwszEnv = (PCRTUTF16 * const)_wenviron; if (!papwszEnv) { _wgetenv(L"Path"); /* Force the CRT to initalize it. */ papwszEnv = (PCRTUTF16 * const)_wenviron; } .... } ``` The "\_wgetenv" function returns the value of the environment variable but in this case its value is not used and so the "papwszEnv" pointer remains null\. The code under the condition should probably look like this: ```cpp _wenviron = _wgetenv(L"Path"); papwszEnv = (PCRTUTF16 * const)_wenviron; ``` [V579](https://pvs-studio.com/en/docs/warnings/v579/) The memset function receives the pointer and its size as arguments\. It is possibly a mistake\. Inspect the third argument\. vboxdispd3dif\.cpp 980 ```cpp typedef struct VBOXWDDMDISP_FORMATS { uint32_t cFormstOps; const struct _FORMATOP* paFormstOps; uint32_t cSurfDescs; struct _DDSURFACEDESC *paSurfDescs; } VBOXWDDMDISP_FORMATS, *PVBOXWDDMDISP_FORMATS; // <= static void vboxDispD3DGlobalD3DFormatsInit(PVBOXWDDMDISP_FORMATS pFormats) { memset(pFormats, 0, sizeof (pFormats)); // <= pFormats->paFormstOps = gVBoxFormatOps3D; pFormats->cFormstOps = RT_ELEMENTS(gVBoxFormatOps3D); } ``` In this fragment, the 'memset' function cannot clear the structure completely as sizeof\(\) returns the size of the pointer, not of the whole structure\. [V609](https://pvs-studio.com/en/docs/warnings/v609/) Divide by zero\. Denominator range \[0\.\.64\]\. vboxmpif\.h 746 ```cpp DECLINLINE(UINT) vboxWddmCalcWidthForPitch(....) { switch (enmFormat) { .... default: { /* the default is just to calculate it from bpp */ UINT bpp = vboxWddmCalcBitsPerPixel(enmFormat); return (Pitch << 3) / bpp; // <= } } } ``` Division by zero is possible because of an absent check\. You cannot know for sure that the "vboxWddmCalcBitsPerPixel" function doesn't return zero currently or won't start doing that after somebody editing it\. ## Conclusion This is the final article about checking VirtualBox by the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) analyzer\. I bet the project authors could find plenty of other dangerous fragments with the help of our analyzer or any other\. I hope this article about the analysis of VirtualBox will get rich feedback and it will help make the product so important for developers, testers and other active users a bit better\. Using static analysis regularly will help you save plenty of time to solve more serious tasks\.