We check Oracle VM VirtualBox. Part 1

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. Actually, after prolonged use, periodically faced with uncertain behavior, I decided to use my experience in checking open-source projects and analyze the source code of the Oracle VM Virtual Box.

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).

The project turned out to be so rich in problem areas that by describing only those places where the error is more or less obvious, I will have to break the material into two articles.
')
In the comments to the articles they often ask: what does an error in runtime lead to? Overwhelmingly, we do not use checked projects and, moreover, do not debug them. Writing this article made me a problem with regular use of VirutalBox. I decided that I would leave the original, but slightly abbreviated comment, and if it was not there, I would add a comment from the file header. Let everyone try to find out their glitch.

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 variables and strings

V501 There are identical sub-expressions "pState-> fIgnoreTrailingWhite" 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 checked fields in the 'pState' structure should be 'fIgnoreLeadingWhite'.

V501 There are identical sub-expressions'! Field ("username"). ToString (). IsEmpty () " 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); .... } .... } 

Judging by the neighboring branch of the switch () operator, most likely there should also be “username” and “password” here.

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

Obviously, the second assignment should be the variable 'wcRight'.

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 be due to copy-paste. At best, there is an extra line, but at worst, the initialization of the element with the index '0xa1' is missing.

V583 The '?:' Operator, regardless of its conditional expression, always returns 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. Just in VirtualBox is always a leap year.

V519 The 'ch' variable is assigned 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 interchanged and the previous character should be kept in this place:

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

It should be noted that the assignment of several different values ​​to one variable in a row is not always an error - sometimes the authors 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 ); 

Constant conditions

V547 Expression is always true. Probably the '&&' operator should 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 for any value of the variable “pCmd-> SurfInfo.PixelFormat.c.rgbBitCount”: perhaps the '&&' operator should be 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 variable 'uPrevCode' is initialized to zero and does not change anywhere else, therefore, the conditional expression “uCurCode <uPrevCode” will always be false, since The unsigned number will not be less than zero.

V534 It is likely that a 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; } } .... } 

A nested loop will never perform any iteration. Perhaps an error in the condition "i <j". Most likely, they 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 does not take a constant value, but the priorities of operations look suspicious. Perhaps you should wrap an expression with the operator '||'.

Confusing designs

V640 The code's operational logic does not correspond with its formatting. The statement is indented. 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(); .... } 

The formatting of the text in this fragment suggests that the call to the “machineLock.acquire ()” function should be carried out only under a certain condition, and not always.

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

A clearer example of formatting that does not reflect the intended logic.

V561 It's not possible to declare it anew variable. 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; } 

In vain, the new local variable 'Status' was declared. Any change to a variable in the try..except section will not change the return value, and the external (with respect to the try {} block) variable will be changed only in the case of an exceptional situation.

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

It is necessary to specify hexadecimal characters in the string without a zero, for example, "\ x01", otherwise the '\ 0' character is interpreted as a terminal zero.

V543 It is the odd that it 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 bool type are completely different types. HRESULT is a 32-bit value divided into three different fields: the error severity code, the device code, and the error code. To work with the HRESULT value, use special constants, such as S_OK, E_FAIL, E_ABORT, and so on. And for checking the values ​​of type HRESULT, macros such as SUCCEEDED, FAILED are intended.

Similar locations for using HRESULT variables:

• V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'mRemoveSavedState'. The SUCCEEDED or FAILED macro should be used instead. machineimpl.cpp 14312
• V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'procCaller.rc ()'. The SUCCEEDED or FAILED macro should be used instead. guestsessionimpl.cpp 1981
• V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'machCaller.rc ()'. The SUCCEEDED or FAILED macro should be used instead. virtualboximpl.cpp 3079

Undefined behavior

V567 Undefined behavior. The variable is variable. 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 variable 'curg' is used twice in one sequence point. As a result, it is impossible to predict the result of the operation of such an expression. See details in the V567 diagnostic description .

Similar places:

• V567 Undefined behavior. The variable is variable. consoleevents.h 95
• V567 Undefined behavior. The variable is variable. consoleevents.h 122

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

In the case of incorrect status of the 'hSMgr' variable, the function will return the uninitialized variable 'rc'.

Similar place:

• V614 Potentially uninitialized variable 'rc' used. suplib-win.cpp 416

V611 The memory was allocated using the 'malloc / realloc' function. 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; .... } 

Memory for the buffer is allocated and released in incompatible ways.

Just very annoying places

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

Apparently, the programmer thought and continued to put commas between the arguments of the cycle. The comma operator evaluates both arguments, and returns the second, therefore, here one condition does not affect the operation of the cycle.

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

Semicolon after the cycle completely changed the conceived logic. The assumed loop body will not participate in the iterations, but will be executed once after the loop operation.

V654 The condition of 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; .... } 

The danger of this cycle is that the value of the counter does not change, therefore, if the very first element of the array does not coincide with 'pszName', then the cycle will be eternal.

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

Missing 'return'.

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

A comma supplied in this way takes the next operator under the 'else'. Most likely, it was not planned.

Conclusion

I hope that 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 become 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 1 .