Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо
to the top
Главная
>
Публикации
>
C++
>
PVS-Studio: анализируем код Doom 3
Andrey Karpov
Андрей Карпов
25 Ноя 2011
Теги:
#Cpp #GameDev

PVS-Studio: анализируем код Doom 3

25 Ноя 2011

Компания id Software имеет лицензию на PVS-Studio. Тем не менее, мы решили проверить исходные коды Doom 3, которые недавно были выложены в сеть. Результат - ошибок найдено мало, но всё-таки найдено. Я предполагаю, что это можно объяснить так.

0120-Doom3_ru/image1.png

Часть кода Doom3 используется и сейчас и, наверное, там ошибки уже исправлены. Часть кода устарела и не используется. Скорее всего, именно там и найдены подозрительные участки кода.

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

Исходный код Doom 3 опубликован на GitHub и на официальном FTP компании под лицензией GPL v3. Для анализа я использовал инструмент PVS-Studio 4.39. Кряки для программы прошу не искать. Я уже встречал трояны, замаскированные под ключи и кряки к PVS-Studio. Лучше напишите нам, и мы дадим пробный ключ на некоторое время.

Фрагмент 1. Подозрительное условие

#define BIT( num ) ( 1 << ( num ) )
const int BUTTON_ATTACK = BIT(0);
void idTarget_WaitForButton::Think( void ) {
  ...
  if ( player &&
      ( !player->oldButtons & BUTTON_ATTACK ) &&
      ( player->usercmd.buttons & BUTTON_ATTACK ) ) {
  ...
}

Диагностическое сообщение PVS-Studio: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. Game target.cpp 257

Обратите внимание на фрагмент "!player->oldButtons & BUTTON_ATTACK". Здесь хотели проверить, что самый младший бит равен 0. Но приоритет оператора '!' выше, чем оператора '&'. Это значит, что условие работает следующим образом:

(!player->oldButtons) & 1

Получается, что условие истинно, только если все биты равны нулю. Корректный вариант кода:

if ( player &&
    ( ! ( player->oldButtons & BUTTON_ATTACK ) ) &&
    ( player->usercmd.buttons & BUTTON_ATTACK ) ) {

Фрагмент 2. Подозрительный цикл

void idSurface_Polytope::FromPlanes(...)
{
  ...
  for ( j = 0; j < w.GetNumPoints(); j++ ) {
    for ( k = 0; k < verts.Num(); j++ ) {
      if ( verts[k].xyz.Compare(w[j].ToVec3(),
                                POLYTOPE_VERTEX_EPSILON ) ) {
        break;
      }
    }
    ...
  }
  ...
}

Диагностическое сообщение PVS-Studio: V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'j'. idLib surface_polytope.cpp 65

Вложенный цикл увеличивает переменную 'j', а не 'k'. Переменная 'k' вообще не увеличивается. Последствия работы такого цикла непредсказуемы. Корректный вариант кода:

for ( k = 0; k < verts.Num(); k++ ) {

Фрагмент 3. Другой подозрительный цикл

bool idMatX::IsOrthonormal( const float epsilon ) const {
  ...
  for ( int i = 0; i < numRows; i++ ) {
    ...
    for ( i = 1; i < numRows; i++ ) {
      ...
    }
    if ( idMath::Fabs( sum ) > epsilon ) {
      return false;
    }
  }
  return true;
}

Диагностическое сообщение PVS-Studio: V535 The variable 'i' is being used for this loop and for the outer loop. idLib matrix.cpp 3128

Вложенный цикл организован с помощью той же переменной, что и внешний цикл. Оба цикла имеют условие остановки: i < numRows. Получается, что внешний цикл всегда выполняет только одну итерацию. Для исправления кода, можно использовать другую переменную во внутреннем цикле.

Фрагмент 4. Неопределенное поведение

int idFileSystemLocal::ListOSFiles(...)
{
  ...
  dir_cache_index = (++dir_cache_index) % MAX_CACHED_DIRS;
  ...
}

Диагностическое сообщение PVS-Studio: V567 Undefined behavior. The 'dir_cache_index' variable is modified while being used twice between sequence points. TypeInfo filesystem.cpp 1877

Переменная "dir_cache_index" изменяется дважды в одной точке следования. То, что используется префиксный инкремент, не имеет значение. Компилятор теоретически вправе создать код следующего вида:

A = dir_cache_index;
A = A + 1;
B = A % MAX_CACHED_DIRS;
dir_cache_index = B;
dir_cache_index = A;

Конечно, скорее всего, выражение вычисляется правильно. Но уверенным в этом быть нельзя. На результат может влиять тип и версия компилятора, настройки оптимизации. Корректный вариант кода:

dir_cache_index = (dir_cache_index + 1) % MAX_CACHED_DIRS;

Фрагмент 5. Подозрительная очистка массива

void idMegaTexture::GenerateMegaMipMaps() {
  ...
  byte *newBlock = (byte *)_alloca( tileSize );
  ...
  memset( newBlock, 0, sizeof( newBlock ) );
  ...
}

Диагностическое сообщение PVS-Studio: V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. DoomDLL megatexture.cpp 542

Нулями заполняется только часть массива 'newBlock'. По всей видимости, это неправильно. Как мне кажется, раньше было написано так:

byte newBlock[ CONST_ARRAY_SIZE ];
...
memset( newBlock, 0, sizeof( newBlock ) );

Потом требования изменились и размер массива 'newBlock' стал изменяться. Но про функцию его очистки забыли. Корректный вариант кода:

memset( newBlock, 0, tileSize );

Фрагмент 6. Еще одна подозрительная очистка массива

void Sys_GetCurrentMemoryStatus( sysMemoryStats_t &stats ) {
  ...
  memset( &statex, sizeof( statex ), 0 );
  ...
}

Диагностическое сообщение PVS-Studio: V575 The 'memset' function processes '0' elements. Inspect the third argument. DoomDLL win_shared.cpp 177

Здесь при вызове функции 'memset' перепутаны аргументы. Функция очищает 0 байт. Это кстати весьма распространенная ошибка. Я встречал её многократно в разных проектах.

Корректный вызов функции:

memset( &statex, 0, sizeof( statex ) );

Фрагмент 7. Здравствуй, Copy-Paste

void idAASFileLocal::DeleteClusters( void ) {
  ...
  memset( &portal, 0, sizeof( portal ) );
  portals.Append( portal );

  memset( &cluster, 0, sizeof( portal ) );
  clusters.Append( cluster );
}

Диагностическое сообщение PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer '& cluster'. DoomDLL aasfile.cpp 1312

Обратите внимания, как похожи две верхние и две нижние строчки кода. Скорее всего, две последних строки были написаны с использованием технологии Copy-Paste. Это и привело к ошибке. В одном месте забыли заменить слово 'portal' на слово 'cluster'. В результате очищается только часть структуры. Корректный вариант кода:

memset( &cluster, 0, sizeof( cluster ) );

Я видел в коде и другие "недоочищенные" массивы, но про них писать не интересно.

Фрагмент 8. Подозрительная работа с указателем

void idBrushBSP::FloodThroughPortals_r(idBrushBSPNode *node, ...)
{
  ...
  if ( node->occupied ) {
    common->Error( "FloodThroughPortals_r: node already occupied\n" );
  }
  if ( !node ) {
    common->Error( "FloodThroughPortals_r: NULL node\n" );
  }
  ...
}

Диагностическое сообщение PVS-Studio: V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 1421, 1424. DoomDLL brushbsp.cpp 1421

Сначала указатель 'node' разименовывается: node->occupied. А затем, вдруг проверяется, не равен ли он NULL. Это очень подозрительный код. Я не знаю, как его сделать правильным, так как не знаю логику работы функции. Возможно, достаточно написать так:

if ( node && node->occupied ) {

Фрагмент 9. Подозрительный формат строки

struct gameVersion_s {
  gameVersion_s( void )
  {
    sprintf(string, "%s.%d%s %s %s",
            ENGINE_VERSION, BUILD_NUMBER, BUILD_DEBUG,
            BUILD_STRING, __DATE__, __TIME__ );
  }
  char string[256];
} gameVersion;

Диагностическое сообщение PVS-Studio: V576 Incorrect format. A different number of actual arguments is expected while calling 'sprintf' function. Expected: 7. Present: 8. Game syscvar.cpp 54

Подозрительно то, что аргумент '__TIME__' никак не используется.

Фрагмент 10. Код, который сбивает с толку

Неоднократно встречается код, который как я понимаю, работает правильно, но выглядит странно. Приведу только один пример такого кода.

static bool R_ClipLineToLight(..., const idPlane frustum[4], ...)
{
  ...
  for ( j = 0 ; j < 6 ; j++ ) {
    d1 = frustum[j].Distance( p1 );
    d2 = frustum[j].Distance( p2 );
    ...
  }
  ...
}

Для подсказки, программист написал, что массив 'frustum' состоит из 4 элементов. Но обрабатывается 6 элементов. Если посмотреть вызов 'R_ClipLineToLight', то там массив из 6 элементов. То есть всё должно работать правильно, но код заставляет задуматься.

Другие ошибки и недочеты, можно увидеть, запустив анализатор PVS-Studio. Кстати, пользуясь, случаем хочу передать привет Джону Кармаку и сообщить ему, что мы скоро исправим недочет, который не позволяет в полную силу использовать PVS-Studio в компании id Software.

Этим недочетом является низкая скорость работы анализатора. Учитывая большой объем исходного кода, с которым работает компания, это существенное ограничение. В PVS-Studio версии 4.50, которая выйдет в этом году, можно будет в качестве препроцессора использовать Clang, а не препроцессор от Visual C++. Это позволит существенно ускорить проверку проектов. Например, при использовании препроцессора от Visual C++ исходные коды Doom 3 проверяются за 26 минут. А при использовании препроцессора от Clang - за 16 минут. Пример, правда, получился не очень удачный. На большинстве проектах выигрыш по скорости анализа будет значительно сильнее.

Правда по умолчанию, пока придется использовать препроцессор от Visual C++. У Clang все еще есть некоторые несовместимости и недоделки, касающиеся Windows-платформы. Так что успешно удаётся проверить только 80% проектов.

#Cpp #GameDev
blog-subscribe blog-subscribe
blog-favorite blog-favorite

0

0

0

0

vk share
ПОДЕЛИТЬСЯ

Теги:

#Cpp #GameDev

Присылаем лучшие статьи раз в месяц

Популярные статьи по теме

Комментарии (0)

Следующие комментарии next comments
close comment form
close form

Заполните форму в два простых шага ниже:

Ваши контактные данные:

Шаг 1
Поздравляем! У вас есть промокод!

Тип желаемой лицензии:

Шаг 2
Team license
Enterprise license
* Разница между Team и Enterprise
Не получили письмо?
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности
close form
Запросите информацию о ценах
Почему мы просим корпоративную почту?
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
Не получили письмо?
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS‑Studio для специалистов Microsoft MVP
Почему мы просим корпоративную почту?
Не получили письмо?
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
Не получили письмо?
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
Почему мы просим корпоративную почту?
Не получили письмо?
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:

  • Промоакции
  • Оповещения
  • Спам