We check Oracle VM VirtualBox. Part 2

Virtual machines are used for various needs. I myself have been using VirtualBox for more than one year to test software and just study various Linux distributions, in fact, after prolonged use, periodically encountering uncertain behavior, I decided to use my experience in checking open-source projects and analyze the source code of Oracle VM Virtual Box.

In this article I will continue to describe the numerous suspicious sites found in the project.

Link to previous article: Checking Oracle VM VirtualBox. Part 1.

VirtualBox is a cross-platform virtualization application. What does it mean? First, it works on computers with Intel or AMD processors running Windows, Mac, Linux, and other operating systems. Secondly, it expands the capabilities of your computer by allowing multiple operating systems to work simultaneously (inside virtual machines).
')
Virtual Box was tested with PVS-Studio 5.19. The build system in Windows uses the kBuild build system, so for testing I used the special utility PVS-Studio Standalone, which is described in the article: PVS-Studio now supports any build system on Windows and any compiler. Easy and out of the box .

Typos in terms of

V501 There are identical sub-expressions "fVersion" operator. applianceimplexport.cpp 1071

/* 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) //<== { .... } .... } 

We see the declaration of five logical variables, the state of which is checked in one conditional expression, but after copy-paste one variable was forgot to be renamed to 'fVendor'.

V501 There are identical sub-expressions'! Module.symspace '||' operator. dbgpluginsolaris.cpp 519

 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 variables in the condition are of the type 'uint32_t' and in the case of zero value of one of them, the function is exited. Most likely, the name of one of the duplicate variables should be 'Module.symsize'.

Similar place in this file:

• V501 There are identical sub-expressions'! Module.symspace '||' operator. dbgpluginsolaris.cpp 665

V547 Expression is always false. Probably the '||' operator should be used here. vboxmanageguestctrl.cpp 2365

 /* 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 be simultaneously equal to two different values, therefore, the condition is always false.

V682 Suspicious literal is present: '//'. It is possible that a backslash should be used here instead: '\\'. supr3hardenedmain-win.cpp 936

 /* 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 condition for stopping the loop is the end of the line or one of the slashes. Backslash (\) should be specially screened. And to the forward slash (/), probably, accidentally added one more slash, getting the wrong value of 0x2F2F.

V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: sizeof (uMod.Info64). dbgplugindarwin.cpp 557

 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 sizeof () operator should be the variable 'uMod.Info32'.

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 541, 674. vcicache.cpp 541

 /*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 'VERR_VD_GEN_INVALID_HEADER' status will not be set in case of an error.

Sizeof () arguments

V568 It's odd that the sizeof () operator is the sizeof (pSh-> Name) 'expression. ldrpe.cpp 1598

 /* @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 evaluates the type of expression and returns the size of this type. In this case, the terminal zero will be written not at the end of the line, but in the position “sizeof (size_t)”.

V568 It's odd that the sizeof () operator is the 'pSub-> auBitmap [0] * 8' expression. mmpagepool.cpp 234

 /* 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 fragment, the sizeof () operator evaluates the type of the expression "pSub-> auBitmap [0] * 8". Most likely the bracket is out of place.

Typos at initialization

V519 The 'pPool-> aPages [0] .iMonitoredNext' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 300, 301. pgmpool.cpp 301

 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; .... } 

Initialization of the 'iMonitoredPrev' field is missing. There is such a field in the structure used.

Similar place:

• V519 The 'pPage-> iMonitoredNext' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 482, 483. pgmpool.cpp 483

V570 The 'pReq-> cT2ISegs' variable is assigned to itself. iscsi.cpp 4743

 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; .... } 

Suspicious place, because the variable “pReq-> cT2ISegs” is first assigned its own value, then another value is assigned to the same variable.

Invalid I / O

V576 Incorrect format. Consider the second printf function. To print the value of the% p 'should be used. usbtest.cpp 191

 /* Remove USB device filter */ int usbMonRemoveFilter (void *aID) { .... printf("usblibRemoveFilter %x\n", aID); .... } 

This code is erroneous, since it will work only in 32 systems. And, for example, in Win64 this code already prints only the lower part of the 'aID' pointer. The correct code is:

 printf("usblibRemoveFilter %p\n", aID); 

V576 Incorrect format. A different number of actual arguments is expected while calling the 'swprintf_s' function. Expected: 4. Present: 5. vboxinstallhelper.cpp 311

 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 will not be printed, since there is only one output specifier in the formatted string.

V576 Incorrect format. Consider checking the fifth argument of the swprintf_s function. The wchar_t type argument is expected. vboxinstallhelper.cpp 340

 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 option is:

 swprintf_s(wszDest, RT_ELEMENTS(wszDest), L"%s%c", pwszDestDir, L'\0'); swprintf_s(wszSource, RT_ELEMENTS(wszSource), L"%s%c", pwszSourceDir, L'\0'); 

More places like this:

• V576 Incorrect format. Consider checking the fifth argument of the swprintf_s function. The wchar_t type argument is expected. vboxinstallhelper.cpp 341
• V576 Incorrect format. Consider checking the fifth argument of the swprintf_s function. The wchar_t type argument is expected. vboxinstallhelper.cpp 370
• V576 Incorrect format. Consider checking the fifth argument of the swprintf_s function. The wchar_t type argument is expected. vboxinstallhelper.cpp 399
• V576 Incorrect format. Consider checking the fifth argument of the swprintf_s function. The wchar_t type argument is expected. vboxinstallhelper.cpp 400

About pointers

V522 Dereferencing of the null pointer 'pParent' might take place. stam.cpp 1135

 /* 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 take a closer look at this fragment, you can see that if the 'pParent' pointer is correct, then the function is exited, and then this pointer is used. Looks like the negation operator is missing. The correct code is:

 if (!pParent) return; if (pParent->cDescsInTree == 0 && pParent->pParent) { .... } 

V547 Expression 'gCtx.au64LastCpuLoad_Kernel == 0' is always false. Pointer 'gCtx.au64LastCpuLoad_Kernel'! = NULL. vboxservicestats.cpp 220

 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 does not make sense to check the pointer to the array declared on the stack. Indeed, in the case of a shortage of allocated memory, an exception is generated.

V595 The 'pImage' pointer was used before it was verified against nullptr. Check lines: 6299, 6305. vmdk.cpp 6299

 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 correctness of the pointer is checked after dereference.

Similar places:

• V595 The 'pExtent-> pszBasename' pointer has been verified against nullptr. Check lines: 2900, 2902. vmdk.cpp 2900
• V595 The 'pszSymbol' pointer was used before it was verified against nullptr. Check lines: 202, 213. suplibldr.cpp 202
• V595 The 'papLunOld' pointer was used before it was verified against nullptr. Check lines: 214, 216. vscsidevice.cpp 214
• V595 The 'conn' pointer was used before it was verified nullptr. Check lines: 323, 327. api_msg.c 323
• V595 The m_pRefreshAct 'pointer was used before it was verified against nullptr. Check lines: 2906, 2914. vboxdbgstatsqt4.cpp 2906
• V595 The m_pRefreshAct 'pointer was used before it was verified against nullptr. Check lines: 2929, 2937. vboxdbgstatsqt4.cpp 2929

Priorities for operations

V542 Consider inspecting an odd type cast: 'bool' to 'wchar_t *'. qifiledialog.cpp 483

 /* Reimplementation of QFileDialog::getSaveFileName() * that removes some oddities and limitations. */ QString QIFileDialog::getSaveFileName (....) { .... ofn.lpstrFilter = (TCHAR *) winFilters.isNull() ? 0 : winFilters.utf16(); .... } 

Here the cast operation has a higher priority than the '?:' Operator. But, thanks to luck, the code eventually works as intended.

V593 Consider reviewing the A = B> C 'kind. The expression is calculated as the following: 'A = (B> C)'. applianceimplimport.cpp 384

 HRESULT Appliance::interpret() { .... if (vsysThis.pelmVBoxMachine) { .... } else if (size_t cEthernetAdapters = vsysThis.llEthernetAdapters.size() > 0) { if (cEthernetAdapters > maxNetworkAdapters) .... } .... } 

The priority of the operator '>' is higher than that of the '=', therefore, here the variable 'cEthernetAdapters' takes only two values: 0 and 1. And then the incorrect value is used.

Memory leak

It’s a virtual one, although it contains a class with virtual functions. uiapplianceeditorwidget.cpp 783

 class VirtualSystemModel: public QAbstractItemModel { .... private: ModelItem *m_pRootItem; }; VirtualSystemModel::~VirtualSystemModel() { if (m_pRootItem) delete m_pRootItem; //<== } 

The analyzer detected a potential error related to the absence of a virtual destructor in the base class. Having found the description of the “ModelItem” class, we will see how the destructor is declared:

 class ModelItem { public: .... ~ModelItem(); //<== .... virtual Qt::ItemFlags itemFlags(int) const { return 0; } virtual bool setData(int, const QVariant &, int) {....} .... }; 

Indeed, the destructor is not marked as virtual and here is an example of a class that, because of this, may not be completely removed from memory:

 class VirtualSystemItem: public ModelItem //<== { public: VirtualSystemItem(....); virtual QVariant data(int column, int role) const; virtual void putBack(....); private: CVirtualSystemDescription m_desc; //<== }; 

Another suspicious class:

• V599 The virtual class contains virtual functions. performance.h 855

More problem areas

V530 The return value of the function '_wgetenv' is required to be utilized. env-generic.cpp 239

 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 fragment its value is not used and the "papwszEnv" pointer remains zero. Probably, the code under the condition should be:

 _wenviron = _wgetenv(L"Path"); papwszEnv = (PCRTUTF16 * const)_wenviron; 

V579 It is possibly a mistake. Inspect the third argument. vboxdispd3dif.cpp 980

 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 snippet, the 'memset' function does not completely nullify the structure, since sizeof () returns the size of the pointer, rather than the entire structure.

V609 Divide by zero. Denominator range [0..64]. vboxmpif.h 746

 DECLINLINE(UINT) vboxWddmCalcWidthForPitch(....) { switch (enmFormat) { .... default: { /* the default is just to calculate it from bpp */ UINT bpp = vboxWddmCalcBitsPerPixel(enmFormat); return (Pitch << 3) / bpp; //<== } } } 

Potential division by zero due to lack of verification. There is no guarantee that the “vboxWddmCalcBitsPerPixel” function will not return zero now or will not return zero after someone else edits.

Conclusion

This is the final article about checking VirtualBox with PVS-Studio analyzer. I am more than sure that the authors of the project can find many more dangerous places here, both with the help of this analyzer and others.

I hope the article on checking VirtualBox will get the most out of it and such an important product for developers, testers and other active users will be a little better.

Using static analysis regularly, you can save a lot of time on solving more useful tasks. Also, control over the quality of the code can be shifted to others, for example, using the new service: regular auditing of C / C ++ code .

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Checking Oracle VM VirtualBox. Part 2 .