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.
VirtualBox 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 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.
V501 There are identical sub-expressions 'fVersion' to the left and to the right of the '||' 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) // <=
{
....
}
....
}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 There are identical sub-expressions '!Module.symspace' to the left and to the right of the '||' 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 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:
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 equal two different values at a time, 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 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 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 of the sizeof() operators should be the 'uMod.Info32' variable.
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
/*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.
V568 It's odd that the argument of 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 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 It's odd that the argument of 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 code, the sizeof() operator calculates the type of the " pSub->auBitmap[0] * 8" expression. I guess one parenthesis is in the wrong place.
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;
....
}An initialization of the 'iMonitoredPrev' field is missing - and this field does exist in the structure used.
A similar issue:
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;
....
}This is quite a suspicious fragment as the "pReq->cT2ISegs" variable is first assigned its own value and then another value.
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
/* 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:
printf("usblibRemoveFilter %p\n", aID);V576 Incorrect format. A different number of actual arguments is expected while calling '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 won't be printed since the formatted string contains only one output specifier.
V576 Incorrect format. Consider checking the fifth actual 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 code should look as follows:
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:
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 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:
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 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 The 'pImage' pointer was utilized 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 pointer is only checked for being valid after it is dereferenced.
Other similar fragments:
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();
....
}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 Consider reviewing the expression of the 'A = B > C' kind. The expression is calculated as 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 '>' 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.
V599 The destructor was not declared as a virtual one, although the 'ModelItem' class contains virtual functions. uiapplianceeditorwidget.cpp 783
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:
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:
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:
V530 The return value of 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 case its value is not used and so the "papwszEnv" pointer remains null. The code under the condition should probably look like this:
_wenviron = _wgetenv(L"Path");
papwszEnv = (PCRTUTF16 * const)_wenviron;V579 The memset function receives the pointer and its size as arguments. 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 fragment, the 'memset' function cannot clear the structure completely as sizeof() returns the size of the pointer, not of the whole 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; // <=
}
}
}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.
This is the final article about checking VirtualBox by the 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.