Святослав Размыслов

22 Сен 2014

Теги:

#Cpp

Проверяем Oracle VM VirtualBox. Часть 2

22 Сен 2014

Автор: Святослав Размыслов

Опечатки в условиях
Аргументы sizeof()
Опечатки при инициализации
Некорректный ввод/вывод
Про указатели
Приоритеты операций
Утечка памяти
Ещё проблемные места
Заключение

Виртуальные машины используются для самых разных нужд. Сам я уже не один год использую VirtualBox для тестирования ПО и просто изучения различных дистрибутивов Linux, собственно, после длительного использования, периодически сталкиваясь с неопределённым поведением, я решил воспользоваться своим опытом в проверке open-source проектов и проанализировать исходный код Oracle VM Virtual Box. В этой статье я продолжу описывать многочисленные подозрительные места, найденные в проекте.

Ссылка на предыдущую статью: Проверяем Oracle VM VirtualBox. Часть 1.

VirtualBox является кроссплатформенным приложением виртуализации. Что это значит? Во-первых, он работает на компьютерах с процессорами Intel или AMD под управлением операционных систем Windows, Mac, Linux и других. Во-вторых, он расширяет возможности вашего компьютера тем, что позволяет работать множеству операционных систем одновременно (внутри виртуальных машин).

Virtual Box проверялся с помощью PVS-Studio 5.19. Для сборки в Windows используется сборочная система kBuild, поэтому для проверки я воспользовался специальной утилитой PVS-Studio Standalone, которая описана в статье: PVS-Studio теперь поддерживает любую сборочную систему на Windows и любой компилятор. Легко и "из коробки".

Опечатки в условиях

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

Мы видим объявление пяти логических переменных, состояние которых проверяется в одном условном выражении, но после copy-paste одну переменную забыли переименовать в '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;
  ....
}

Все переменные в условии имеют тип 'uint32_t' и в случае нулевого значения одной из них - выполняется выход из функции. Скорее всего, имя одной из повторяющихся переменных должно быть 'Module.symsize'.

Аналогичное место в этом файле:

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] == '\\')
  {
    ....
  }
  ....
}

Одна переменная не может быть одновременно равна двух разным значениям, следовательно, условие всегда ложно.

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

Условием остановки цикла является конец строки или один из слешей. Обратный слеш (\) следует специально экранировать. А к прямому слешу (/), вероятно, случайно добавили ещё один слеш, получив неправильное значение 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));
  ....
}

Вероятно, аргументом одного оператора sizeof() должна быть переменная 'uMod.Info32'.

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

Ветвь 'else' никогда не получит управления и статус 'VERR_VD_GEN_INVALID_HEADER' не будет установлен в случае ошибки.

Аргументы 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';         // <=
  ....
}

Оператор sizeof() вычисляет тип выражения и возвращает размер этого типа. В данном случае терминальный ноль будет записан не в конец строки, а в позицию "sizeof(size_t)".

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

В этом фрагменте оператор sizeof() вычисляет тип выражения " pSub->auBitmap[0] * 8". Скорее всего скобка не на своём месте.

Опечатки при инициализации

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

Пропущена инициализация поля 'iMonitoredPrev'. Такое поле в используемой структуре имеется.

Похожее место:

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

Подозрительное место, потому что переменной "pReq->cT2ISegs" сначала присваивается собственное значение, потом этой же переменной присваивается другое значение.

Некорректный ввод/вывод

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

Этот код ошибочен, поскольку будет работать только в 32-х системах. А, например, в Win64 этот код уже распечатает только младшую часть указателя 'aID'. Корректный вариант кода:

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

Второй аргумент не будет напечатан, так как в форматированной строке всего один спецификатор вывода.

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

Правильный вариант:

  swprintf_s(wszDest, RT_ELEMENTS(wszDest),
    L"%s%c", pwszDestDir, L'\0');
  swprintf_s(wszSource, RT_ELEMENTS(wszSource),
    L"%s%c", pwszSourceDir, L'\0');

Ещё такие места:

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

Про указатели

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

Если приглядеться к этому фрагменту, то можно увидеть, что если указатель 'pParent' корректный, то выполняется выход из функции, а дальше этот указатель используется. Похоже пропущен оператор отрицания. Корректный вариант кода:

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

Не имеет смысла проверять указатель на массив, объявленный на стеке. Ведь в случае нехватки выделяемой памяти, генерируется исключение.

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

Корректность указателя проверяется уже после разыменования.

Похожие места:

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

Приоритеты операций

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

Здесь операция приведение к типу имеет более высокий приоритет, чем оператор '?:'. Но, благодаря везению, код в итоге отрабатывает как задумано.

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

Приоритет оператора '>' выше, чем у '=', следовательно, здесь переменная 'cEthernetAdapters' принимает только два значения: 0 и 1. И далее используется некорректное значение.

Утечка памяти

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

Анализатор обнаружил потенциальную ошибку, связанную с отсутствием в базовом классе виртуального деструктора. Найдя описание класса "ModelItem", мы увидим, как объявлен деструктор:

class ModelItem
{
public:
  ....
  ~ModelItem();                           // <=
  ....
  virtual Qt::ItemFlags itemFlags(int) const { return 0; }
  virtual bool setData(int, const QVariant &, int) {....}
  ....
};

Действительно, деструктор не помечен как виртуальный и вот пример класса, который из-за этого может быть не полностью удалён из памяти:

class VirtualSystemItem: public ModelItem // <=
{
public:
  VirtualSystemItem(....);
  virtual QVariant data(int column, int role) const;
  virtual void putBack(....);
private:
  CVirtualSystemDescription m_desc;       // <=
};

Ещё один подозрительный класс:

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

Ещё проблемные места

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

Функция "_wgetenv" возвращает значение переменной среды, но в данном фрагменте её значение не используется и указатель "papwszEnv" остаётся нулевым. Вероятно, код под условием должен быть таким:

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

В данном фрагменте, функция 'memset' не полностью обнуляет структуру, так как sizeof() возвращает размер указателя, а не всей структуры.

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

Потенциальное деление на ноль из-за отсутствия проверки. Нет гарантий, что функция "vboxWddmCalcBitsPerPixel" не возвращает ноль сейчас или не будет возвращать ноль после чьей-нибудь правки.

Заключение

Это заключительная статья о проверке VirtualBox анализатором PVS-Studio. Я более, чем уверен, что авторами проекта здесь может быть найдены ещё множество опасных мест, как с помощью этого анализатора, так и другими.

Надеюсь статья о проверке VirtualBox получит максимальную отдачу и столь важный продукт для разработчиков, тестировщиков и других активных пользователей станет немного качественнее.

Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач. Также контроль за качеством кода можно переложить на других, например, воспользовавшись новой услугой: регулярный аудит Си/Си++ кода.

#Cpp