Перевіряємо Oracle VM VirtualBox. Частина 1


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

VirtualBox є кросплатформним додатком віртуалізації. Що це означає? По-перше, він працює на комп'ютерах з процесорами Intel або AMD під управлінням операційних систем Windows, Mac, Linux та інших. По-друге, він розширює можливості вашого комп'ютера тим, що дозволяє працювати безлічі операційних систем одночасно (всередині віртуальних машин).

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

У коментарях до статей часто запитують: до чого призводить помилка в runtime'е? У переважній більшості ми не використовуємо перевіряються проекти і, тим більше, не налагоджує їх. На написання цієї статті мене спонукали проблеми при регулярному використанні VirutalBox. Я вирішив, що буду залишати оригінальний, але трохи скорочений коментар, а якщо його не було, то додам коментар з шапки файлу. Нехай кожен спробує дізнатися свій глюк.

Virtual Box перевірявся за допомогою PVS-Studio 5.19. Для складання 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 value type 'mRemoveSavedState'. The SUCCEEDED FAILED or macro should be used instead. machineimpl.cpp 14312
  • V545 Such conditional expression of 'if' operator is incorrect for the HRESULT value type 'procCaller.rc()'. The SUCCEEDED FAILED or macro should be used instead. guestsessionimpl.cpp 1981
  • V545 Such conditional expression of 'if' operator is incorrect for the HRESULT value type 'machCaller.rc()'. The SUCCEEDED FAILED or 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 отримає максимальну віддачу і такий важливий продукт для розробників, тестувальників та інших активних користувачів стане трохи якісніше.

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

Ця стаття англійською
Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Svyatoslav Razmyslov. Checking Oracle VM VirtualBox. Part 1.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio і CppCat, версія 2014. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.