Checking Oracle VM VirtualBox. Part 2




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.

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

Typos in conditions

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:

  • V501 There are identical sub-expressions '!Module.symspace' to the left and to the right of the '||' 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 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.

Arguments of sizeof()

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.

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

An initialization of the 'iMonitoredPrev' field is missing - and this field does exist in the structure used.

A similar issue:

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

This is quite a suspicious fragment as the "pReq->cT2ISegs" variable is first assigned its own value and then another value.

Incorrect input/output

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:

  • V576 Incorrect format. Consider checking the fifth actual argument of the 'swprintf_s' function. The wchar_t type argument is expected. vboxinstallhelper.cpp 341
  • V576 Incorrect format. Consider checking the fifth actual argument of the 'swprintf_s' function. The wchar_t type argument is expected. vboxinstallhelper.cpp 370
  • V576 Incorrect format. Consider checking the fifth actual argument of the 'swprintf_s' function. The wchar_t type argument is expected. vboxinstallhelper.cpp 399
  • V576 Incorrect format. Consider checking the fifth actual 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 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:

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

Operation precedence

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.

Memory leak

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:

  • V599 The virtual destructor is not present, although the 'Aggregate' class contains virtual functions. performance.h 855

A few more problems

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.

Conclusion

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.



Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;
On our website we use a cookie to collect information of a technical nature.
If you do not agree, please leave the site. Learn More →
Do not show again