Свежий взгляд на Oracle VM VirtualBox

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

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

Введение

VirtualBox (Oracle VM VirtualBox) - это универсальный полноценный виртуализатор для оборудования x86, предназначенный для использования на серверах, настольных компьютерах и встраиваемых системах. Он поддерживается следующими операционными системами: Microsoft Windows, FreeBSD, Solaris / OpenSolaris, Linux, Mac OS X, DOS, ReactOS и другими.

Вы можете найти предыдущие статьи о VirtualBox здесь:

Проверка Oracle VM VirtualBox. Часть 1.
«Проверка Oracle VM VirtualBox. Часть 2."

В этих статьях содержится более 50 опасных фрагментов, обнаруженных с помощью PVS-Studio 5.18. В новом отчете анализатора таких предупреждений не встречал. Это означает, что разработчики просмотрели статьи и исправили все ошибки, обнаруженные анализатором. Желающие могут найти эти места в последней версии исходного кода, а также могут посмотреть, как исправления предупреждений PVS-Studio появляются в реальном проекте. Но во время очередной проверки я обнаружил много других интересных сообщений.

Также хочу подчеркнуть, что только регулярное использование статического анализа (не обязательно PVS-Studio) может поддерживать высокое качество кода. Весь наш опыт исправления предупреждений анализатора в коде Unreal Engine показал, что количество ошибок в развивающемся проекте постоянно увеличивается, поэтому после разовых проверок качество кода постепенно возвращается к исходному состоянию, и появляются новые ошибки. все еще будет попадать в код. В проекте VirtualBox мы видим похожую ситуацию. Рост предупреждений анализатора после разовой проверки выглядит примерно так:

Важно подчеркнуть, что под «регулярным» использованием анализатора мы подразумеваем ежедневные проверки. Многие ошибки, обнаруженные на этапе тестирования, можно устранить еще на этапе написания кода.

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

Исходный код Oracle VM VirtualBox был протестирован с помощью PVS-Studio версии 6.02.

Возможно, кому-то понадобится номер проверенной ревизии.

Checked out external at revision 2796.
Checked out revision 59777.

Упорные ошибки

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

V521 Такие выражения с использованием оператора , опасны. Убедитесь, что выражение правильное. vboxmpwddm.cpp 1083

NTSTATUS DxgkDdiStartDevice(...)
{
  ....
  if ( ARGUMENT_PRESENT(MiniportDeviceContext) &&
        ARGUMENT_PRESENT(DxgkInterface) &&
        ARGUMENT_PRESENT(DxgkStartInfo) &&
        ARGUMENT_PRESENT(NumberOfVideoPresentSources), // <=
        ARGUMENT_PRESENT(NumberOfChildren)
        )
  {
    ....
  }
  ....
}

Подобный код был описан в первой статье. Оператор запятой ‘,’ оценивает левый и правый операнды. Дело в том, что левый операнд больше не используется, а результатом действия оператора является значение правого операнда. Скорее всего, программист хотел использовать оператор «&&», как и в других строках.

V519 Переменной pThis-› aCSR [103] два раза подряд присваиваются значения. Возможно, это ошибка. Проверьте строки: 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 Слева и справа от оператора || есть идентичные подвыражения mstrFormat.equalsIgnoreCase (« text / plain )». vboxdnddataobject.cpp 38

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

Программирование копипаста будет жить вечно. Уже есть две идентичные проверки типа «текст / простой», но, кроме того, все часы кода были скопированы в другой файл:

V501 Слева и справа от оператора ‘||’ находятся идентичные подвыражения ‘! RTStrICmp (pszFormat,« text / plain »)’. vboxdnd.cpp 834

определить истину ложь; // удачной отладки!

Без шуток - такой код в разных вариациях можно встретить в реальных проектах.

V547 Выражение всегда ложное. Значение беззнакового типа никогда не <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 );

Подозрительный код «Откуда»

V570 Переменная from- ›eval1D [i] .u1’ присваивается самой себе. 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;     // <=
  ...
}

Анализатор обнаружил подозрительные присвоения переменных самим себе. Скорее всего, справа от оператора присваивания программист должен написать объект с именем «в», а не «от».

Еще пять фрагментов в этом файле:

V570 Переменная from- ›eval1D [i] .u2’ присваивается самой себе. state_evaluators.c 1007
V570 Переменная from- ›eval2D [i] .u1’ присваивается самой себе. state_evaluators.c 1042
V570 Переменная from- ›eval2D [i] .u2’ присваивается самой себе. state_evaluators.c 1043
V570 Переменная from- ›eval2D [i] .v1’ присваивается самой себе. state_evaluators.c 1044
V570 Переменная from- ›eval2D [i] .v2’ присваивается самой себе. state_evaluators.c 1045

V625 Рассмотрите возможность проверки оператора for. Начальное и конечное значения итератора совпадают. 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();
  }
  ....
}

Описание таких ошибок я решил выделить в отдельный раздел из-за еще одного подозрительного фрагмента, содержащего имена «к» и «от».

Начальное и конечное значение счетчика цикла в этом фрагменте кода одинаковы. В результате в цикле всего одна итерация. Опять же, это, скорее всего, опечатка в названии объекта «кому».

О приоритетах операций

V564 Оператор & применяется к значению типа bool. Вероятно, вы забыли заключить скобки или намеревались использовать оператор &&. 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 Рассмотрите возможность изучения этого выражения. Выражение является чрезмерным или содержит опечатку. 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 Рассмотрите возможность проверки ‘err == 0L || err! = 1237L ’выражение. Выражение является чрезмерным или содержит опечатку. vboxdisplay.cpp 656
V590 Рассмотрите возможность проверки ‘rc == 3209 || rc! = (- 3210) ’выражение. Выражение является чрезмерным или содержит опечатку. vd.cpp 10876
V590 Рассмотрите возможность проверки ‘rc == 3209 || rc! = (- 3210) ’выражение. Выражение является чрезмерным или содержит опечатку. vd.cpp 10947
V590 Рассмотрите возможность проверки ‘rc == 3209 || rc! = (- 3210) ’выражение. Выражение является чрезмерным или содержит опечатку. vd.cpp 11004
V590 Рассмотрите возможность проверки ‘rc == 3209 || rc! = (- 3210) ’выражение. Выражение является чрезмерным или содержит опечатку. vd.cpp 11060

Различные предупреждения

V511 Оператор sizeof () возвращает размер указателя, а не массива, в выражении sizeof (plane). 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]» в параметрах функции подсказывает программисту, что в функцию будет передан массив типа «float», содержащий 4 элемента. Таким образом, функция memcpy () копирует неправильное количество байтов.

V517 Использование шаблона if (A) {…} else if (A) {…}. Есть вероятность наличия логической ошибки. Проверить строки: 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 Странно, что оператор sizeof () умножается на 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 Выражение sd› = 0 всегда верно. Значение беззнакового типа всегда ›= 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 Часть условного выражения всегда истинна: 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);
    }
  }
  ....
}

Обычно мы не пишем в статьях о предупреждениях, выдаваемых для тестовых файлов. Кстати, очень легко исключить полученные сообщения для всех файлов в указанном каталоге. Тем не менее, я решил написать об одном из них здесь. Это довольно странно из-за того, что тест на самом деле ничего не проверяет из-за опечатки. Оператор continue выполняется на каждой итерации цикла for (). Значение 0x1fbe всегда будет истинным, поскольку в условии отсутствует выражение uc ==. Это хороший пример того, как статический анализ дополняет модульное тестирование.

Правильная версия:

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

V610 Неопределенное поведение. Проверьте оператор смены ‘‹(- 2) отрицательный. 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 ++, смещение отрицательного числа приводит к неопределенному поведению.

Еще два похожих фрагмента:

V610 Неопределенное поведение. Проверьте оператор смены ‘‹
V610 Неопределенное поведение. Проверьте оператор смены ‘‹

TODOs

V523 Оператор then эквивалентен оператору else. 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 Необходимо использовать возвращаемое значение функции e1kHandleRxPacket. 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 Строка типа wchar_t * неправильно преобразована в строку типа BSTR. Рассмотрите возможность использования функции SysAllocString. vboxcredentialprovider.cpp 231

static HRESULT VBoxCredentialProviderRegisterSENS(void)
{
  ....
  hr = pIEventSubscription->put_EventClassID(
                      L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}");
  ....
}

Анализатор увидел, что строка типа «wchar_t *» обрабатывается как строка типа BSTR.

BSTR (базовая строка или двоичная строка) - это строковый тип данных, который используется в функциях COM, автоматизации и взаимодействия. Строка этого типа состоит из префикса длиной 4 байта, строки данных и разделителя из двух нулевых символов. Префикс длины указывается перед первым символом строки и не учитывает символ-разделитель. В этом случае префикс длины перед началом строки будет отсутствовать.

Исправленная версия с использованием функции SysAllocString ():

static HRESULT VBoxCredentialProviderRegisterSENS(void)
{
  ....
  hr = pIEventSubscription->put_EventClassID(SysAllocString(
                     L"{d5978630-5b9f-11d1-8dd2-00aa004abd5e}"));
  ....
}

Более подозрительные фрагменты:

V745 Строка типа «wchar_t *» неправильно преобразована в строку типа «BSTR». Рассмотрите возможность использования функции «SysAllocString». vboxcredentialprovider.cpp 277
V745 Строка типа «wchar_t *» неправильно преобразована в строку типа «BSTR». Рассмотрите возможность использования функции «SysAllocString». vboxcredentialprovider.cpp 344
V745 Строка типа «wchar_t *» неправильно преобразована в строку типа «BSTR». Рассмотрите возможность использования функции «SysAllocString». string.cpp 31

V746 Нарезка шрифтов. Исключение следует перехватывать по ссылке, а не по значению. 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 Нарезка типов. Исключение следует перехватывать по ссылке, а не по значению. extpackutil.cpp 330

Вывод:

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

Я бы с удовольствием посмотрел и MS Word, который несколько раз зависал на 7–10 минут, полностью загружая процессор, когда я писал статью. Но такой возможности нет. Мы провели некоторое археологическое исследование MS Word 1.1a, но это уже другая история.

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