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

19 Сен 2014

Теги:

#Cpp

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

19 Сен 2014

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

Опечатки в переменных и строках
Константные условия
Сбивающие с толку конструкции
Неопределённое поведение
Просто очень досадные места
Заключение

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

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

Проект оказался настолько богат проблемными местами, что описывая только те места, в которых ошибка более-менее очевидна, мне придётся разбить материал на две статьи.

В комментариях к статьям часто спрашивают: к чему приводит ошибка в runtime'е? В подавляющем большинстве мы не используем проверяемые проекты и, тем более, не отлаживаем их. На написание этой статьи меня сподвигли проблемы при регулярном использовании VirutalBox. Я решил, что буду оставлять оригинальный, но немного сокращённый комментарий, а если его не было, то добавлю комментарий из шапки файла. Пусть каждый попробует узнать свой глюк.

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

Опечатки в переменных и строках

V501 There are identical sub-expressions 'pState->fIgnoreTrailingWhite' to the left and to the 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(....);
  ....
}

Вероятно, одним из проверяемых полей структуры 'pState' должно быть 'fIgnoreLeadingWhite'.

V501 There are identical sub-expressions '!field("username").toString().isEmpty()' to the left and to the 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);
    ....
  }
  ....
}

Судя по соседней ветви оператора switch(), скорее всего тут тоже должно быть "username" и "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;
  ....
}

Очевидно, что второе присваивание должно быть переменной '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;
  ....
}

Данный фрагмент может быть следствием copy-paste. В лучшем случае здесь лишняя строка, но в худшем - пропущена инициализация элемента с индексом '0xa1'.

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

Без комментариев. Просто в 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;
  ....
}

Логично предположить, что операнды оператора присваивания перепутаны местами и в этом месте должен сохраняться предыдущий символ:

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

Стоит отметить, что присваивание одной переменной подряд нескольких разных значений не всегда является ошибкой - иногда авторы используют магию вне Хогвартса:

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

Условие истинно при любом значении переменной "pCmd->SurfInfo.PixelFormat.c.rgbBitCount ": возможно, должен использоваться оператор '&&' или в одной из переменных присутствует опечатка.

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. */
    ....
  }
  .... 
}

Переменная 'uPrevCode' инициализируется нулём и больше нигде не изменяется, следовательно, условное выражение "uCurCode < uPrevCode" всегда будет ложным, т.к. беззнаковое число не будет меньше нуля.

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

Вложенный цикл никогда не выполнит ни одной итерации. Возможно, ошибка в условии "i < j". Скорее всего, хотели написать: 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;
  }
  ....
}

Данное условие не принимает постоянное значение, но приоритеты операций выглядят подозрительно. Возможно, следовало обернуть в скобки выражение с оператором '||'.

Сбивающие с толку конструкции

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

Форматирование текста в данном фрагменте заставляет предположить, что вызов функции "machineLock.acquire()" должен осуществляться только при определённом условии, а не всегда.

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

Более наглядный пример форматирования, которое не отражает задуманную логику.

V561 It's probably better to assign value to 'Status' variable 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;
}

Зря объявлена новая локальная переменная 'Status'. Любое изменение переменной в секции try..except не изменит возвращаемое значение, а внешняя (по отношению к блоку try {}) переменная будет изменена только в случае возникновения исключительной ситуации.

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

Задавать шестнадцатеричные символы в строке необходимо без нуля, например, "\x01", иначе символ '\0' интерпретируется как терминальный ноль.

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 и тип bool - это совершенно разные по смыслу типы. HRESULT - это 32-разрядное значение, разделенное на три различных поля: код серьезности ошибки, код устройства и код ошибки. Для работы со значением HRESULT служат специальные константы, такие как S_OK, E_FAIL, E_ABORT и так далее. А для проверки значений тип HRESULT предназначены такие макросы, как SUCCEEDED, FAILED.

Аналогичные места использования переменных HRESULT:

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

Неопределённое поведение

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

Переменная 'curg' дважды используется в одной точке следования. В результате невозможно предсказать результат работы такого выражения. Подробнее в описании диагностики V567.

Аналогичные места:

V567 Undefined behavior. The 'curp' variable is modified while being used twice between sequence points. consoleevents.h 95
V567 Undefined behavior. The 'curp' variable is modified while being used twice between sequence points. 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;
}

В случае некорректного статуса переменной 'hSMgr' функция вернёт неинициализированную переменную 'rc'.

Аналогичное место:

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

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

Память для буфера выделяется и освобождаться несовместимыми между собой способами.

Просто очень досадные места

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

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

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

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

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

Опасность данного цикла в том, что значение счётчика не изменяется, следовательно, если самый первый элемент массива не совпадёт с 'pszName', то цикл будет вечным.

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

Пропущен '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;
}

Поставленная таким образом запятая берёт следующий оператор под 'else'. Скорее всего, так не планировалось.

Заключение

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

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

#Cpp