Geekly Articles each Day
📜 ⬆️ ⬇️

A fresh look at the Oracle VM VirtualBox code

Virtual machines are an important tool in the software developer’s arsenal. My interest in VirtualBox code is caused by the personal use of this product for checking open projects, as well as for other various tasks associated with using several operating systems. The first test of this project took place in 2014, then the description of about 50 errors barely fit into two articles. With the release of Windows 10 and VirtualBox 5.0.XX, in my opinion, the stability of the program has noticeably deteriorated. So I decided to check the project again.

Introduction


VirtualBox (Oracle VM VirtualBox) - software for the operating system that allows you to present a virtual set of resources in a particular environment. Supported by the following operating systems: Microsoft Windows, FreeBSD, Solaris / OpenSolaris, Linux, Mac OS X, DOS, ReactOS and others.

The first articles about VirtualBox can be found at the links:
They contain more than 50 dangerous places that were found with PVS-Studio 5.18. In the new analyzer report, I have not met such warnings. The developers did not pass by the articles and corrected all the warnings indicated by the analyzer. Those who are interested can find these places in the latest version of the source code and see how the correction of PVS-Studio warnings looks like in a real project. After the new check, I met many other interesting messages.

With this article I want to show that only regular use of a static analyzer (not necessarily PVS-Studio) will allow maintaining high quality code. The experience of fixing all the analyzer warnings in the Unreal Engine code showed that the number of errors in the developed project will constantly increase, and after one-time checks, the quality of the code will gradually become the same, and new errors will begin to accumulate. The VirtualBox project has a similar situation. The increase in the number of analyzer warnings after a one-time check is approximately as follows:
')


It is important to emphasize that regular use of the analyzer means daily checks. Many of the errors that are detected at the testing stage could be eliminated at the stage of writing the code.

Another advantage of regular use of static analyzers is the availability of regular updates. Since PVS-Studio analyzer, since the first verification of the VirtualBox code, more than 50 new diagnostic rules have been added. About errors found with the help of new diagnostics, I will tell in the last section.

The source code of Oracle VM VirtualBox was checked with PVS-Studio version 6.02.

Probably, the number of the audited revision is useful to someone:
Checked out external at revision 2796. Checked out revision 59777.

Stubborn mistakes


Before writing the article, I looked at what was found earlier using the analyzer. And I found very similar errors in the new code. It is possible that this code was written by the same person.

V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. vboxmpwddm.cpp 1083

 NTSTATUS DxgkDdiStartDevice(...) { .... if ( ARGUMENT_PRESENT(MiniportDeviceContext) && ARGUMENT_PRESENT(DxgkInterface) && ARGUMENT_PRESENT(DxgkStartInfo) && ARGUMENT_PRESENT(NumberOfVideoPresentSources), // <= ARGUMENT_PRESENT(NumberOfChildren) ) { .... } .... }

There was a similar code in the first article. Let me remind you that the comma operator ',' calculates both the left and right operand. The fact is that the value of the left operand is no longer used, and the result of the operator is the value of the right operand. Most likely, they wanted to use the '&&' operator, as in the other lines.

V519 The 'pThis-> aCSR [103]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1230, 1231. devpcnet.cpp 1231

 static void pcnetSoftReset(PPCNETSTATE pThis) { .... pThis->aCSR[94] = 0x0000; pThis->aCSR[100] = 0x0200; pThis->aCSR[103] = 0x0105; // <= pThis->aCSR[103] = 0x0105; // <= .... }

The code contains duplicate lines. A similar place from the first article, the developers have corrected by removing the extra line. And what about this snippet: an error in the array index or also an extra string, we will find out in the next versions of VirtualBox.

V501 There are identical sub-expressions 'mstrFormat.equalsIgnoreCase ("text / plain")' operator. vboxdnddataobject.cpp 382

 STDMETHODIMP VBoxDnDDataObject::GetData(....) { .... else if( mstrFormat.equalsIgnoreCase("text/plain") // <= || mstrFormat.equalsIgnoreCase("text/html") || mstrFormat.equalsIgnoreCase("text/plain;charset=utf-8") || mstrFormat.equalsIgnoreCase("text/plain;charset=utf-16") || mstrFormat.equalsIgnoreCase("text/plain") // <= || mstrFormat.equalsIgnoreCase("text/richtext") || mstrFormat.equalsIgnoreCase("UTF8_STRING") || mstrFormat.equalsIgnoreCase("TEXT") || mstrFormat.equalsIgnoreCase("STRING")) { .... }

Well copy-paste programming will live forever. Here and so there are two identical text / plain checks, so this whole block of code was safely copied to another file:

define true false; // successful debugging!


It turns out that such a code is not a joke, but is present in various projects in real projects.

V547 Expression is always false. Unsigned type value is never <0. dt_subr.c 715

 int dt_printf(dtrace_hdl_t *dtp, FILE *fp, const char *format, ...) { .... if (vsnprintf(&dtp->dt_buffered_buf[dtp->dt_buffered_offs], //<= avail, format, ap) < 0) { rval = dt_set_errno(dtp, errno); va_end(ap); return (rval); } .... }

At first glance, there is nothing to complain about except the analyzer. The documentation for the vsnprintf function explicitly states that a negative number is returned in the event of an error. As a result, I even passed this code fragment to one of the developers of the C ++ code analyzer core, as an example of a false positive. But no, it turned out that the analyzer is right.

Who would have thought that somewhere in one of the thousands of header files to meet the line:
 #define vsnprintf RTStrPrintfV

In the preprocessed file, the source fragment will be disclosed as follows:
 if (RTStrPrintfV(&dtp->dt_buffered_buf[dtp->dt_buffered_offs], avail, format, ap) < 0) { rval = dt_set_errno(dtp, (*_errno())); ( ap = (va_list)0 ); return (rval); }

The RTStrPrintfV () function returns the value of the unsigned type 'size_t', rather than the signed 'int', therefore, this check leads to a logical error, since virtually no verification is performed.

Function prototypes for comparison:
 size_t RTStrPrintfV(char *, size_t, const char *, va_list args); int vsnprintf (char *, size_t, const char *, va_list arg );

Suspicious "From-To" code


V570 The 'from-> eval1D [i] .u1' variable is assigned to itself. state_evaluators.c 1006

 void crStateEvaluatorDiff(CREvaluatorBits *e, CRbitvalue *bitID, CRContext *fromCtx, CRContext *toCtx) { .... from->eval1D[i].order = to->eval1D[i].order; from->eval1D[i].u1 = from->eval1D[i].u1; // <= from->eval1D[i].u2 = from->eval1D[i].u2; // <= ... }

The analyzer detected suspicious variable assignments to itself. Most likely, an object with the name 'to', rather than 'from', should be used to the right of the assignment operator.

Five more places in this file:
V625 Consider inspecting the 'for' operator. Initial and final values ​​of the iterator are the same. state_transform.c 1365

 void crStateTransformDiff(...., CRContext *fromCtx, CRContext *toCtx ) { .... for (i = to->colorStack.depth; i <= to->colorStack.depth; i++) { LOADMATRIX(to->colorStack.stack + i); from->colorStack.stack[i] = to->colorStack.stack[i]; /* Don't want to push on the current matrix */ if (i != to->colorStack.depth) diff_api.PushMatrix(); } .... }

I decided to describe such errors in a separate section due to the fact that there is another suspicious place in the code using the names 'to' and 'from'.

In this code snippet, the starting and ending values ​​of the loop counter match. As a result, only one iteration is performed in the loop. Most likely again a typo in the name of the object 'to'.

About the priorities of operations


V564 The '&' operator is applied to bool type value. You've probably forgotten to include the operator. glsl_shader.c 4102

 static void generate_texcoord_assignment(....) { DWORD map; unsigned int i; char reg_mask[6]; if (!ps) return; for (i = 0, map = ps->baseShader.reg_maps.texcoord; map && i < min(8, MAX_REG_TEXCRD); map >>= 1, ++i) { if (!map & 1) // <= continue; .... } }

Because of the forgotten brackets in the condition "! Map & 1" the value of the variable 'map' is checked for equality to zero, and the least significant one bit is not set to one. The error in this place is also indicated by the fact that checking the value of 'map' for zero is already present in the condition of stopping the cycle. Thus, this condition is always false and the 'continue' operator is never executed.

Most likely, the condition must be written as follows:
 if ( !(map & 1) ) continue;

V590 Consider inspecting this expression. The expression is misprint. vboxdispcm.cpp 288

 HRESULT vboxDispCmSessionCmdGet(....) { .... Assert(hr == S_OK || hr == S_FALSE); if (hr == S_OK || hr != S_FALSE) // <= { return hr; } .... }

The analyzer detected a suspicious condition in which the subroutine "hr == S_OK" does not affect the result of the whole condition.

This can be seen by looking at the truth table of this conditional expression:



By the way, next to this condition is no less suspicious Assert (), in which there is a modified conditional expression.

In general, this type of error is a very common phenomenon. For example, the FreeBSD kernel is no exception.

The entire list of suspicious places from VirtualBox:

Miscellaneous Warnings


V511 The sizeof () operator, the sizeof (plane) expression. devvga-svga3d-win.cpp 4650

 int vmsvga3dSetClipPlane(...., float plane[4]) // <= { .... /* Store for vm state save/restore. */ pContext->state.aClipPlane[index].fValid = true; memcpy(pContext->state.aClipPlane[....], plane, sizeof(plane)); .... }

The variable 'plane' is only a pointer to an array of type 'float'. The value of “sizeof (plane)” will be 4 or 8, depending on the bitness of the program. And the number '[4]' in the parameters of the function only prompts the programmer that an array of 4 elements of the type 'float' is passed to the function. Thus, the memcpy () function copies the wrong number of bytes.

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 411, 418. mp-r0drv-nt.cpp 411

 static int rtMpCallUsingDpcs(....) { .... if (enmCpuid == RT_NT_CPUID_SPECIFIC) // <= { KeInitializeDpc(&paExecCpuDpcs[0], rtmpNtDPCWrapper, pArgs); KeSetImportanceDpc(&paExecCpuDpcs[0], HighImportance); KeSetTargetProcessorDpc(&paExecCpuDpcs[0], (int)idCpu); pArgs->idCpu = idCpu; } else if (enmCpuid == RT_NT_CPUID_SPECIFIC) // <= { KeInitializeDpc(&paExecCpuDpcs[0], rtmpNtDPCWrapper, pArgs); KeSetImportanceDpc(&paExecCpuDpcs[0], HighImportance); KeSetTargetProcessorDpc(&paExecCpuDpcs[0], (int)idCpu); pArgs->idCpu = idCpu; KeInitializeDpc(&paExecCpuDpcs[1], rtmpNtDPCWrapper, pArgs); KeSetImportanceDpc(&paExecCpuDpcs[1], HighImportance); KeSetTargetProcessorDpc(&paExecCpuDpcs[1], (int)idCpu2); pArgs->idCpu2 = idCpu2; } .... }

Because of the same expressions in the conditions cascade, the part of the code that is in the second condition never gets control.

V531 It is odd that a sizeof () operator is multiplied by sizeof (). tstrtfileaio.cpp 61

 void tstFileAioTestReadWriteBasic(...., uint32_t cMaxReqsInFlight) { /* Allocate request array. */ RTFILEAIOREQ *paReqs; paReqs = (...., cMaxReqsInFlight * sizeof(RTFILEAIOREQ)); RTTESTI_CHECK_RETV(paReqs); RT_BZERO(..., sizeof(cMaxReqsInFlight) * sizeof(RTFILEAIOREQ)); /* Allocate array holding pointer to data buffers. */ void **papvBuf = (...., cMaxReqsInFlight * sizeof(void *)); .... }

The analyzer detected a suspicious product of two sizeof () operators. If you look at the macro 'RT_BZERO', then the question arises: "Why get the size of a variable of the type 'uint32_t' and multiply by the size of another type?". In the adjacent sections of the code, the size of the array is calculated as "cMaxReqsInFlight * sizeof (RTFILEAIOREQ)". Most likely, the same size should be used in the line with 'RT_BZERO', but accidentally made a mistake.

V547 Expression 'sd> = 0' is always true. Unsigned type value is always> = 0. vboxservicevminfo.cpp 1086

 static int vgsvcVMInfoWriteNetwork(void) { .... SOCKET sd = WSASocket(AF_INET, SOCK_DGRAM, 0, 0, 0, 0); .... if (pAdpInfo) RTMemFree(pAdpInfo); if (sd >= 0) // <= closesocket(sd); .... }

The SOCKET type (in Visual C ++) is unsigned, so the “sd> = 0” check is meaningless. The reason for this code is clear: the project is built for different operating systems, and on Unix systems, socket values ​​are stored in the signed variable 'int'. In general, the code for working with sockets is written correctly: constants from system header files are used everywhere to check states. But the cross-platform code contains a lot of conditional preprocessor directives, so in one place did not notice the check, which for Windows is always true.

V560 A part of the conditional expression is always true: 0x1fbe. tstiprtministring.cpp 442

 static void test2(RTTEST hTest) { .... for (RTUNICP uc = 1; uc <= 0x10fffd; uc++) { if (uc == 0x131 || uc == 0x130 || uc == 0x17f || 0x1fbe)// <= continue; //^^^^^^ if (RTUniCpIsLower(uc)) { RTTESTI_CHECK_MSG(....), ("%#x\n", uc)); strLower.appendCodePoint(uc); } if (RTUniCpIsUpper(uc)) { RTTESTI_CHECK_MSG(....), ("%#x\n", uc)); strUpper.appendCodePoint(uc); } } .... }

Typically, the article does not get warnings issued on files for testing. It is easy to exclude messages issued for all files in the specified directory from the PVS-Studio report. But I decided to write one example. It is interesting because the test does not check anything because of a typo. At each iteration of the for () loop, the 'continue' statement is executed. The condition omitted the expression "uc ==", so just the value '0x1fbe' will always be true. This is a good example of how static analysis complements unit testing .

Correct option:
 if (uc == 0x131 || uc == 0x130 || uc == 0x17f || uc == 0x1fbe) continue;

V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 2)' is negative. translate.c 2708

 static void gen_push_T1(DisasContext *s) { .... if (s->ss32 && !s->addseg) gen_op_mov_reg_A0(1, R_ESP); else gen_stack_update(s, (-2) << s->dflag); .... }

According to modern C and C ++ standards, a negative number shift leads to undefined behavior.

Here are two more similar places:

Todo'shki


V523 The 'then' statement is equivalent to the 'else' statement. state_evaluators.c 479

 static void map2(G....) { .... if (g->extensions.NV_vertex_program) { /* XXX FIXME */ i = target - GL_MAP2_COLOR_4; } else { i = target - GL_MAP2_COLOR_4; } .... }

The marks "FIXME" and "TODO" can live in the source code for a very long time. But the static analyzer will not let you forget about the unfinished code.

V530 e1kHandleRxPacket 'return value of function is required to be utilized. deve1000.cpp 3913

 static void e1kTransmitFrame(PE1KSTATE pThis, bool fOnWorkerThread) { .... /** @todo do we actually need to check that we're in loopback mode here? */ if (GET_BITS(RCTL, LBM) == RCTL_LBM_TCVR) { E1KRXDST status; RT_ZERO(status); status.fPIF = true; e1kHandleRxPacket(pThis, pSg->aSegs[0].pvSeg, ....); // <= rc = VINF_SUCCESS; // <= } e1kXmitFreeBuf(pThis); .... }

In other parts of the source code, the result of the e1kHandleRxPacket () function is usually stored in the 'rc' variable. But until the code is added, the result of the function is not used here, and “VINF_SUCCESS” is always saved in the status.

New diagnostics


In this section, I will describe the warnings of the analyzer that appeared in PVS-Studio after the previous verification of the VirtualBox project.

V745 A 'wchar_t *' type string is incorrectly converted to 'BSTR' type string. Consider using 'SysAllocString' function. vboxcredentialprovider.cpp 231

 static HRESULT VBoxCredentialProviderRegisterSENS(void) { .... hr = pIEventSubscription->put_EventClassID( L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}"); .... }

The analyzer found that with a string like “wchar_t *” they start working as with a string of type BSTR.

BSTR (basic string or binary string) is a string data type used in COM, Automation, and Interop functions. A string of this type consists of a prefix of 4 bytes in length, a data string, and a delimiter of two null characters. The length prefix is ​​indicated immediately before the first character of the string and does not take into account the delimiter character. With this use, the length prefix before the beginning of the data line will be absent.

Corrected using the SysAllocString () function:
 static HRESULT VBoxCredentialProviderRegisterSENS(void) { .... hr = pIEventSubscription->put_EventClassID(SysAllocString( L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}")); .... }

A few more suspicious places:
V746 Type slicing. By reference rather than by value. extpackutil.cpp 257

 RTCString *VBoxExtPackLoadDesc(....) { .... xml::XmlFileParser Parser; try { Parser.read(szFilePath, Doc); } catch (xml::XmlError Err) // <= { return new RTCString(Err.what()); } .... }

The analyzer has detected a potential error related to catching an exception by value. This means that a new 'Err' object of type xml :: XmlError will be constructed using the copy constructor. This will lose some of the exception information that was stored in classes inherited from xml :: XmlError.

Another suspicious place:

Conclusion




The VirtualBox project is a good example of the importance of regularly applying static analysis in a developing project. This scenario of using the analyzer prevents the growth of potential errors in the development process and allows you to get the latest updates used by the analysis tool.

I would also be happy to check the MS Word code, which at the time of writing this article, hung several times for 7-10 minutes, completely loading the processor. But there is no such possibility yet. We, of course, conducted an archaeological study, taking MS Word 1.1a , but this is not at all that ...

Easily download PVS-Studio and find errors in your project. Think about the users of your product and save the time of your programmers!


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. A fresh eye on Oracle VM VirtualBox .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/278959/

More articles:


All Articles