Компания 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% проектов.