Свіжий погляд на код Oracle VM VirtualBox

Віртуальні машини — важливий інструмент в арсеналі розробника програмного забезпечення. Мій інтерес до коду VirtualBox викликаний особистим використанням цього продукту для перевірки відкритих проектів, а також для інших завдань, пов'язаних з використанням декількох операційних систем. Перша перевірка цього проекту відбулася в 2014 році, тоді опис близько 50 помилок ледве вміщався в двох статтях. C виходом Windows 10 і VirtualBox 5.0.XX, на мій погляд, стабільність роботи програми помітно погіршилася. Тому я вирішив перевірити проект ще раз.

Введення
VirtualBox (Oracle VM VirtualBox) — програмне забезпечення для операційної системи, що дозволяє представити віртуальний набір ресурсів у певному середовищі. Підтримується наступними операційними системами: Windows, FreeBSD, Solaris/OpenSolaris, Linux, Mac OS X, DOS, ReactOS та іншими.

З першими статтями про VirtualBox можна ознайомитися за посиланнями:Вони містять більше 50 небезпечних місць, які були знайдені за допомогою PVS-Studio 5.18. У новому звіті аналізатора я не зустрів таких попереджень. Розробники не пройшли повз статей та виправили всі зазначені попередження аналізатора. Бажаючі можуть знайти ці місця в останній версії вихідного коду і подивитися, як виглядає виправлення попереджень PVS-Studio в реальному проекті. Після нової перевірки я зустрів багато інших цікавих повідомлень.

Цією статтею я хочу показати, що тільки регулярне використання статичного аналізатора (не обов'язково PVS-Studio) дозволить підтримувати високу якість коду. Досвід виправлення всіх попереджень аналізатора в коді Unreal Engine показав, що в розроблювальному проекті кількість помилок буде постійно збільшується, і після одноразових перевірок якість коду поступово стане колишнім, і почнуть накопичуватися нові помилки. У проекті VirtualBox простежується аналогічна ситуація. Зростання кількості попереджень аналізатора після разової перевірки виглядає приблизно наступним чином:



Важливо підкреслити, що під регулярним використанням аналізатора маються на увазі щоденні перевірки. Багато помилки, які виявляються на етапі тестування, можна було усунути ще на етапі написання коду.

Ще однією перевагою регулярного використання статичних аналізаторів є наявність регулярних оновлень. Так в аналізатор PVS-Studio, з моменту першої перевірки коду VirtualBox, було додано більше 50 нових діагностичних правил. Про помилки, знайдених з допомогою нових діагностик, я розповім в останньому розділі.

Вихідний код Oracle VM VirtualBox перевірявся за допомогою PVS-Studio версії 6.02.

Можливо, комусь стане в нагоді номер перевіреної ревізії:
Checked out external at revision 2796.
Checked out revision 59777.
Вперті помилки
Перед написанням статті я переглянув, що було знайдено раніше з допомогою аналізатора. І виявив дуже схожі помилки в новому коді. Можливо, що цей код писав один і той же чоловік.

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

Був схожий код в першій статті. Нагадаю, що оператор кома ',' обчислює і лівий, і правий операнд. Справа в тому, що значення лівого операнда більше не використовується, а результатом оператора є значення правого операнда. Швидше за все, тут хотіли використовувати оператор '&&', як і в інших рядках.

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

Код містить повторюваної рядка. Схоже місце з першою статті розробники виправили шляхом видалення зайвої рядки. А що в цьому фрагменті: помилка в індексі масиву або теж зайвий рядок, ми дізнаємося в наступних версіях VirtualBox.

V501 There are identical sub-expressions 'mstrFormat.equalsIgnoreCase(«text/plain»)' to the left and to the right of the '||' 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"))
{
....
}

Ну copy-paste програмування буде жити вічно. Тут і так присутні дві однакові перевірки «text/plain», так ще весь цей блок коду благополучно скопіювали в інший файл:
  • V501 There are identical sub-expressions '!RTStrICmp(pszFormat, «text/plain»)' to the left and to the right of the '||' operator. vboxdnd.cpp 834
define true false; //вдалою налагодження!
Виявляється, подібний код — не жарт, а в різних варіаціях присутня в реальних проектах.

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

На перший погляд тут не до чого прискіпатися, крім як до засобу. У документації до функції «vsnprintf» недвозначно написано, що в разі помилки повертається негативно число. В результаті я навіть передав цей фрагмент коду одного з розробників ядра аналізатора C++ коду, як приклад помилкового спрацьовування. Але ні, виявилося, що аналізатор прав.

Хто ж міг подумати, що десь в одному з тисяч заголовних файлів зустрітися рядок:
#define vsnprintf RTStrPrintfV

препроцессированном файлі вихідний фрагмент буде розкритий наступним чином:
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);
}

Функція RTStrPrintfV() повертає значення беззнакового типу 'size_t', а не знакового 'int', отже, така перевірка призводить до логічної помилки, т. к. фактично ніякої перевірки не виконується.

Прототипи функцій для порівняння:
size_t RTStrPrintfV(char *, size_t, const char *, va_list args);
int vsnprintf (char *, size_t, const char *, va_list arg );
Підозрілий «From-To» код
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; // <=
...
}

Аналізатор виявив підозрілі присвоювання змінних самим собі. Швидше за все, справа від оператора присвоювання повинен використовуватися об'єкт з ім'ям 'to', а не 'from'.

Ще п'ять місць у цьому файлі:
  • V570 The 'from->eval1D[i].u2' variable is assigned to itself. state_evaluators.c 1007
  • V570 The 'from->eval2D[i].u1' variable is assigned to itself. state_evaluators.c 1042
  • V570 The 'from->eval2D[i].u2' variable is assigned to itself. state_evaluators.c 1043
  • V570 The 'from->eval2D[i].v1' variable is assigned to itself. state_evaluators.c 1044
  • V570 The 'from->eval2D[i].v2' variable is assigned to itself. state_evaluators.c 1045
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();
}
....
}

Описати такі помилки в окремому розділі я вирішив з-за того, що в коді присутня ще одна підозріле місце з використанням імен 'to' і 'from'.

У цьому фрагменті коду збігається початкове і кінцеве значення лічильника циклу. В результаті чого в циклі виконується лише одна ітерація. Швидше за все знову помилка в імені об'єкта 'to'.

Про пріоритети операцій
V564 The '&' operator is applied to bool value type. You've probably forgotten to include parentheses or intended to use 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;
....
}
}

З-за забутих дужок в умові "!map & 1" перевіряється значення змінної 'map' на рівність нулю, а не виставлений в одиницю молодший один біт. На помилку в цьому місце ще вказує і той факт, що перевірка значення 'map' на нуль вже присутній в умови припинення циклу. Таким чином, ця умова завжди помилково і оператор 'continue' ніколи не виконується.

Швидше за все, умова необхідно записати таким чином:
if ( !(map & 1) )
continue;

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. vboxdispcm.cpp 288
HRESULT vboxDispCmSessionCmdGet (....)
{
....
Assert(hr == S_OK || hr == S_FALSE);
if (hr == S_OK || hr != S_FALSE) // <=
{
return hr;
}
....
}

Аналізатор виявив підозріле умова, у якому він «hr == S_OK» ніяк не впливає на результат усього умови.

У цьому можна переконатися, поглянувши на таблицю істинності цього виразу:



До речі, поряд з цим умовою знаходиться не менш підозрілий Assert(), в якому присутня змінене умовний вираз.

Взагалі, такого типу помилки — дуже поширене явище. Наприклад, ядро FreeBSD теж не стало винятком.

Весь список підозрілих місць з VirtualBox:
  • V590 Consider inspecting the 'err == 0L || err != 1237L' expression. The expression is excessive or contains a misprint. vboxdisplay.cpp 656
  • V590 Consider inspecting the 'rc == 3209 || rc != (- 3210)' expression. The expression is excessive or contains a misprint. vd.cpp 10876
  • V590 Consider inspecting the 'rc == 3209 || rc != (- 3210)' expression. The expression is excessive or contains a misprint. vd.cpp 10947
  • V590 Consider inspecting the 'rc == 3209 || rc != (- 3210)' expression. The expression is excessive or contains a misprint. vd.cpp 11004
  • V590 Consider inspecting the 'rc == 3209 || rc != (- 3210)' expression. The expression is excessive or contains a misprint. vd.cpp 11060
Різні попередження
V511 The sizeof() operator returns size of the pointer, and not of the array, in '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));
....
}

Змінна 'plane' є лише покажчиком на масив типу 'float'. Значення «sizeof(plane)» буде 4 або 8 залежно від розрядності програми. А число '[4]' в параметрах функції лише підказує програмісту, що в функцію передають масив з 4 елементів типу 'float'. Таким чином, функція memcpy() копіює неправильна кількість байт.

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability 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;
}
....
}

З-за однакових виразів в каскаді умов частина коду, яка знаходиться у другому умови, ніколи не отримує управління.

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

Аналізатор виявив підозріле добуток двох операторів sizeof(). Якщо поглянути на макрос 'RT_BZERO', то виникає питання: «Навіщо отримувати розмір змінної типу 'uint32_t' і множити на розмір іншого типу?». В сусідніх ділянках коду розмір масиву обчислюють як «cMaxReqsInFlight * sizeof(RTFILEAIOREQ)». Швидше за все, у рядку з 'RT_BZERO' повинен використовуватися такий же розмір, але випадково допустили помилку.

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

Тип SOCKET (в Visual C++) є беззнаковим, тому перевірка «sd >=0» безглузда. Причина такого коду зрозуміла: проект збирається для різних операційних систем, а в Unix системах значення сокета зберігаються у знаковій змінної 'int'. В цілому, код для роботи із сокетами написаний правильно: для перевірки станів скрізь використовуються константи з системних заголовних файлів. Але багатоплатформовий код містить дуже багато умовних директив препроцесора, тому в одному місці не помітили перевірку, яка для Windows завжди істинна.

V560 A part of 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);
}
}
....
}

Зазвичай в статті не потрапляють попередження, видані на файли для тестування. Зі звіту PVS-Studio легко вилучити повідомлення, видані на всі файли у вказаному каталозі. Але один приклад я все ж вирішив виписати. Він цікавий тим, що із-за помилки тест нічого не перевіряє. На кожній ітерації циклу for() виконується оператор 'continue'. В умові пропустили вираз «uc ==», тому просто значення '0x1fbe' завжди буде істинним. Це хороший приклад того, як статичний аналіз доповнює юніт-тестування.

Коректний варіант:
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);
....
}

Згідно сучасним стандартам мови C і C++, зсув від'ємного числа призводить до невизначеної поведінки.

Ось ще два схожих місця:
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand is negative ('i64' = [-1..0]). tarvfs.cpp 234
  • V610 Undefined behavior. Check the shift operator '<<'. The left operand '-16' is negative. translate.c 2761
TODO'шки
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;
}
....
}

Позначки «FIXME» і «TODO» можуть жити у вихідному коді дуже довго. Але статичний аналізатор не дасть забути про недописанном коді.

V530 The return value of function 'e1kHandleRxPacket' 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);
....
}

В інших частинах вихідного коду результат функції e1kHandleRxPacket() зазвичай зберігають в змінну 'rc'. Але поки що код не дописаний, тут результат функції не використовується, а в статус завжди зберігають «VINF_SUCCESS».

Нові діагностики
У цьому розділі я опишу попередження аналізатора, які з'явилися в PVS-Studio після попередньої перевірки проекту VirtualBox.

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

Аналізатор виявив, що з рядком типу «wchar_t *» починають працювати як з рядком типу BSTR.

BSTR (basic string або binary string ) — це строковий тип даних, який використовується в COM, Automation і Interop функціях. Рядок цього типу складається з префікса довжини розміром 4 байти, рядки даних і обмежувача з двох нульових символів. Префікс довжини вказується безпосередньо перед першим символом рядка і не враховує символ-обмежувач. При такому використанні, префікс довжини перед початком рядка даних буде відсутній.

Виправлений варіант з допомогою функції SysAllocString():
static HRESULT VBoxCredentialProviderRegisterSEns(void)
{
....
hr = pIEventSubscription->put_EventClassID(SysAllocString(
L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}"));
....
}

Ще кілька підозрілих місць:
  • V745 A 'wchar_t *' type string is incorrectly converted to 'BSTR' type string. Consider using 'SysAllocString' function. vboxcredentialprovider.cpp 277
  • V745 A 'wchar_t *' type string is incorrectly converted to 'BSTR' type string. Consider using 'SysAllocString' function. vboxcredentialprovider.cpp 344
  • V745 A 'wchar_t *' type string is incorrectly converted to 'BSTR' type string. Consider using 'SysAllocString' function. string.cpp 31
V746 Type slicing. An exception should be caught 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());
}
....
}

Аналізатор виявив потенційну помилку, пов'язану з перехопленням виключення за значенням. Це означає, що з допомогою конструктора копіювання буде сконструйований новий об'єкт 'Err' типу xml::XmlError. При цьому буде втрачена частина інформації про виключення, яка зберігалася в класах, успадкованих від xml::XmlError.

Ще підозріле місце:
  • V746 Type slicing. An exception should be caught by reference rather than by value. extpackutil.cpp 330
Висновок


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

Ще я з радістю перевірив код MS Word, який під час написання цієї статті кілька разів підвисав на 7-10 хвилин, повністю завантажуючи процесор. Але такої можливості поки що немає. Ми, звичайно, проводили археологічні дослідження, взявши MS Word 1.1 a, але це зовсім не те…

Легко завантажити PVS-Studio і знайдіть помилки у своєму проекті. Подумайте про користувачів вашого продукту і економії часу ваших програмістів!


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Svyatoslav Razmyslov. A fresh eye on Oracle VM VirtualBox.

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


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

0 коментарів

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