Свежий взгляд на код Oracle VM VirtualBox10.03.2016 15:03

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

Введение

VirtualBox (Oracle VM VirtualBox) — программное обеспечение для операционной системы, позволяющее представить виртуальный набор ресурсов в определённой среде. Поддерживается следующими операционными системами: Microsoft 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 type value. 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: 0×1fbe. 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 ==», поэтому просто значение '0×1fbe' всегда будет истинным. Это хороший пример того, как статический анализ дополняет юнит-тестирование.

Корректный вариант:

if (uc == 0x131 || uc == 0x130 || uc == 0x17f || uc == 0x1fbe)
  continue;

V610 Undefined behavior. Check the shift operator '

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.1a, но это совсем не то…

Легко скачайте PVS-Studio и найдите ошибки в своём проекте. Подумайте о пользователях вашего продукта и экономии времени ваших программистов!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. A fresh eye on Oracle VM VirtualBox.

© Habrahabr.ru