Проверка проекта Dolphin-emu

24 Фев 2012

Автор: Андрей Карпов

Введение
Опечатки
Ошибки низкоуровневой работы с памяти
Работа с float
Насилие над строками
Заключение

Нам регулярно предлагают проверить различные открытые проекты с помощью анализатора PVS-Studio. Если вы тоже хотите предложить что-то для проверки, то прошу перейти по этой ссылке. Очередным проектом стал проект Dolphin-emu.

Введение

Dolphin-emu это эмулятор Gamecube и Wii. Так как это проект с открытыми исходными кодами, любой может вносить улучшения. Код размещен на GitHub.

Мы нашли в этом проекте совсем мало ошибок. В первую очередь это связанно с тем, что это небольшой по объему проект. Размер проекта составляет около 260 000 строк кода. Оставшуюся часть проекта (1340 000 строк кода) составляют сторонние библиотеки, которые тестировать не так интересно.

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

Опечатки

Опечатки коварны и их можно встретить в любом приложении. Программисты уверены, что допускают только сложные ошибки и анализаторы в первую очередь должны искать утечки памяти и ошибки синхронизации. К сожалению, реальность намного банальнее и самыми распространенными ошибками являются опечатки и неправильно скопированные участки кода. Например, в Dolphin-emu есть такая функция:

bool IRBuilder::maskedValueIsZero(
  InstLoc Op1, InstLoc Op2) const
{
  return (~ComputeKnownZeroBits(Op1) &
          ~ComputeKnownZeroBits(Op1)) == 0;
}

Сообщение анализатора PVS-Studio:

V501 There are identical sub-expressions '~ComputeKnownZeroBits(Op1)' to the left and to the right of the '&' operator. Core ir.cpp 1215

Из-за опечатки два раза используется переменная 'Op1' и ни разу не используется 'Op2'. А вот другой пример, где не там поставлена закрывающаяся круглая скобка ')'.

static const char iplverPAL[0x100] = "(C) 1999-2001 ....";
static const char iplverNTSC[0x100]= "(C) 1999-2001 ....";
CEXIIPL::CEXIIPL() : ....
{
  ...
  memcpy(m_pIPL, m_bNTSC ? iplverNTSC : iplverPAL,
         sizeof(m_bNTSC ? iplverNTSC : iplverPAL));
  ...
}

Предупреждение PVS-Studio:

V568 It's odd that the argument of sizeof() operator is the 'm_bNTSC ? iplverNTSC : iplverPAL' expression. Core exi_deviceipl.cpp 112

Выражение внутри оператора sizeof() не вычисляется. Этот код работает только благодаря тому, что типы массивов 'iplverNTSC' и 'iplverPAL' совпадают. Получается, что sizeof() всегда возвращает 0x100. Это интересный момент. Если бы размер массива 'iplverNTSC' и 'iplverPAL ' отличался, то всё бы работало совсем по-другому. Рассмотрим тестовый пример для наглядности:

const char A[10] = "123";
const char B[10] = "123";
const char C[20] = "123";
cout << sizeof(true ? A : B) << ", "
     << sizeof(true ? A : C) << endl;

Результат работы программы: 10, 4.

В первом случае печатается размер массива, а во втором - размер указателя.

Ошибки низкоуровневой работы с памяти

Помимо опечаток очень часто встречаются ошибки работы с такими функциями как memset() и memcpy().

u32 Flatten(..., BlockStats *st, ...)
{
  ...
  memset(st, 0, sizeof(st));
  ...
}

Предупреждение PVS-Studio:

V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. Core ppcanalyst.cpp 302

Случайно вычисляется размер указателя на объект, а не размер самого объекта BlockStats. Правильный вариант: sizeof(*st).

А вот другая аналогичная ситуация:

void drawShadedTexSubQuad(...,
  const MathUtil::Rectangle<float>* rDest, ...)
{
  ...
  if (stsq_observer ||
      memcmp(rDest,
        &tex_sub_quad_data.rdest, sizeof(rDest)) != 0 ||
      tex_sub_quad_data.u1 != u1 ||
      tex_sub_quad_data.v1 != v1 ||
      tex_sub_quad_data.u2 != u2 ||
      tex_sub_quad_data.v2 != v2 ||
      tex_sub_quad_data.G != G)
  ...
}

Сравнивается только часть структуры. Правильный вариант: sizeof(*rDest).

Работа с float

В некоторых местах проекта Dolphin-emu, работа с переменными типа float, выглядит излишне оптимистично. Переменные типа float сравниваются с помощью операторов == и !=. Подобные сравнения допустимы только в определенных случаях. Более подробно о сравнении переменных типа float можно познакомиться в документации к диагностическому правилу V550. Рассмотрим пример:

float Slope::GetValue(float dx, float dy)
{
  return f0 + (dfdx * dx) + (dfdy * dy);
}

void BuildBlock(s32 blockX, s32 blockY)
{
  ...
  float invW = 1.0f / WSlope.GetValue(dx, dy);
  ...
  float q = TexSlopes[i][2].GetValue(dx, dy) * invW;
  if (q != 0.0f)
    projection = invW / q;
  ...
}

Предупреждение PVS-Studio

V550 An odd precise comparison: q != 0.0f. It's probably better to use a comparison with defined precision: fabs(A - B) > Epsilon. VideoSoftware rasterizer.cpp 264

Обратите внимание на сравнение "if (q != 0.0f)". Как видно из кода, переменная 'q' вычисляется достаточно сложным образом. И как следствие, практически невероятно, что она ТОЧНО будет равна нулю. Скорее всего, переменная может получить, например значение 0.00000002. Это не 0, но деление на такое маленькое число может привести к переполнению. Необходима специальная проверка на подобные случаи.

Насилие над строками

void CMemoryWindow::onSearch(wxCommandEvent& event)
{
  ...
  //sprintf(tmpstr, "%s%s", tmpstr, rawData.c_str());
  //strcpy(&tmpstr[1], rawData.ToAscii());
  //memcpy(&tmpstr[1], &rawData.c_str()[0], rawData.size());
  sprintf(tmpstr, "%s%s", tmpstr, (const char *)rawData.mb_str());
  ...
}

Из закомментированного кода видно, что это больное место. Это уже четвертая попытка сформировать строку. К сожалению, она тоже далека от идеала. Анализатор PVS-Studio предупреждает:

V541 It is dangerous to print the string 'tmpstr' into itself. Dolphin memorywindow.cpp 344

Опасность заключается в том, что строка "tmpstr" печатается в саму себя. Этот код может корректно работать, но так лучше не делать. В зависимости от реализации функции sprintf() можно неожиданно получить некорректный результат. Возможно, здесь более уместно было бы использовать функцию strcat().

Есть и другие участки кода, которые хотя и работают, но весьма потенциально опасны:

V541 It is dangerous to print the string 'pathData_bin' into itself. Dolphin wiisavecrypted.cpp 513

V541 It is dangerous to print the string 'regs' into itself. Core interpreter.cpp 84

V541 It is dangerous to print the string 'fregs' into itself. Core interpreter.cpp 89