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

PVS-Studio VS Doom3

Компания 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 occupiedn" );

  }

  if ( !node ) {

    common->Error( "FloodThroughPortals_r: NULL noden" );

  }

  ...

}

Диагностическое сообщение 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% проектов.

Categories:
For more complete information about compiler optimizations, see our Optimization Notice.