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 unexpected 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.
VirtualBox is a cross-platform virtualization product. What does that mean? First, it can run on computers with Intel or AMD processors under Windows, Mac, Linux, and other operating systems. Secondly, it extends your computer's capabilities by allowing a number of different operating systems to run simultaneously (inside virtual machines).
The project appears to be so rich in issues and defects, that I have to split the article in two parts, even if I only describe those fragments where errors are more or less evident.
In comments on our articles, readers often ask about the consequences of certain errors in runtime. Well in most cases, we ourselves do not use the projects we analyze, and moreover, we do not debug them. What prompted me to write this article, was the occurrence of certain problems during regular use of VirtualBox. I decided to keep the original - but slightly abridged - comments, or when there is no such comment, add the one from the file headline. Let everyone try to recognize their own glitch.
Virtual Box was analyzed by PVS-Studio 5.18. We used 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 'pState->fIgnoreTrailingWhite' to the left and right of the '||' operator. 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(....);
....
}Probably one of the 'pState' structure's fields to be checked should be 'fIgnoreLeadingWhite'.
V501 There are identical sub-expressions '!field("username").toString().isEmpty()' to the left and right of the '||' operator. 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);
....
}
....
}As the nearby branch of the switch() operator suggests, this fragment should also probably contain "username" and "password".
V519 The 'wcLeft' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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;
....
}It is obvious that the second assignment should be done to the 'wcRight' variable.
V519 The 'pci_conf[0xa0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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;
....
}This fragment may have appeared through the copy-paste technique. There is one redundant line at best, or a missing initialization of an item with the '0xa1' index at worst.
V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 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]; // <=
....
}No comments. It's just that it's always a leap year in VirtualBox.
V519 The 'ch' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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;
....
}It is logical to assume that the operands of the assignment operator are swapped by mistake, and it is the previous character which should be saved in this code:
chPrev = ch;
ch = ScmStreamGetCh(pStrmInput);
Assert(ch == chPrev);It should be noted that assigning a number of different values to one variable on end is not always an error - sometimes developers use magic outside of Hogwarts:
V519 The 'pixelformat' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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 Expression is always true. The '&&' operator should probably be used here. 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;
}
....
}The condition is true with any value of the "pCmd->SurfInfo.PixelFormat.c.rgbBitCount " variable: perhaps the '&&' operator should have been used, or there is a typo in one of the variables.
V547 Expression 'uCurCode < uPrevCode' is always false. Unsigned type value is never < 0. dbgmoddwarf.cpp 2887
/* 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. */
....
}
....
}The 'uPrevCode' variable is initialized to zero and is not changed anywhere, therefore the conditional expression "uCurCode < uPrevCode" will always be false, for an unsigned number can never be less than zero.
V534 It is likely that the incorrect variable is being compared inside the 'for' operator. Consider reviewing '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;
}
}
....
}The nested loop will never iterate. The error is probably in the "i < j" condition. I think the programmer actually wanted to write j < I.
V648 Priority of the '&&' operation is higher than that of the '||' operation. 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;
}
....
}This condition doesn't take a constant value, but the assumption about operation precedence looks very suspicious. Perhaps the expression with the '||' operator should have been wrapped in parentheses.
V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. 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();
....
}Text formatting in this fragment suggests that the call of the "machineLock.acquire()" function should be executed only at a certain condition, not all the time.
V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. vboxguestr3libdraganddrop.cpp 656
static int vbglR3DnDGHProcessRequestPendingMessage(....)
{
....
rc = Msg.hdr.result;
if (RT_SUCCESS(rc))
rc = Msg.uScreenId.GetUInt32(puScreenId); AssertRC(rc);
....
}This is a clearer example of formatting not meeting supposed logic.
V561 It's probably better to assign value to 'Status' variable, rather than to declare it anew. Previous declaration: vboxmpwddm.cpp, line 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;
}Declaring a new local variable 'Status', doesn't make sense. Any change of the variable within the try..except section won't change the returned value, and the external (in relation to the try {} block) variable will be changed only if an exception occurs.
V638 A terminal null is present inside a string. The '\0x01' characters were encountered. Probably meant: '\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 }
};When in a string, hexadecimal characters should be defined without zero - for example "\x01"; otherwise the '\0' character will be interpreted as an end of string.
V543 It is odd that value 'true' is assigned to the variable 'mRemoveSavedState' of HRESULT type. 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 and the bool type are completely different types in their meaning. HRESULT is a 32-bit value split into three different fields: the error severity code, device code and error code. Special constants such as S_OK, E_FAIL, E_ABORT, etc. are used to handle a HRESULT value, while macros like SUCCEEDED and FAILED are used to check such values.
Other fragments where HRESULT variables are used:
V567 Undefined behavior. The 'curg' variable is modified while being used twice between sequence points. 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;
}
....
};The 'curg' variable is used twice in one sequence point. As a result, you can't predict what such an expression will evaluate to. For details, see the V567 diagnostic's description.
Other similar fragments:
V614 Potentially uninitialized variable 'rc' used. 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;
}If the 'hSMgr' variable has an incorrect status, the function will return the uninitialized variable 'rc'.
Another similar fragment:
V611 The memory was allocated using 'malloc/realloc' function, but was released using the 'delete' operator. Consider inspecting operation logics behind the 'pBuffer' variable. 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;
....
}Incompatible methods are used to allocate and free memory for the buffer.
V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. 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)
{....}
....
}The programmer must have got lost in his thoughts, and went on absently typing commas between the loop arguments. The comma operator calculates both arguments, but returns the second one; therefore, one of the conditions here does not affect the loop.
V529 Odd semicolon ';' after 'for' operator. 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]);
....
}The semicolon after the loop has completely changed the planned logic. The supposed loop body won't iterate, and will only execute once after the loop is terminated.
V654 The condition of the loop is always true. 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;
....
}What is dangerous about this loop, is that the counter value doesn't change, so if the very first array item doesn't coincide with 'pszName', we'll get an infinite loop.
V606 Ownerless token '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;
}
....
}A 'return' is missing.
V626 Consider checking for misprints. It's possible that ',' should be replaced by ';'. 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;
}The comma placed like that takes the next operator under 'else'. I don't think it was meant that way.
I hope this article about the analysis of VirtualBox will get rich feedback, and that it will help make the product so important for developers, testers and other active users, even better.
Using static analysis regularly will help you save plenty of time, so you can work on tasks which are more serious.