Главная
>
Публикации
>
C++
>
PVS-Studio: анализируем код Doom 3

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

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

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

Часть кода 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% проектов.