# PVS\-Studio: анализируем код Doom 3 Компания id Software имеет лицензию на PVS\-Studio\. Тем не менее, мы решили проверить исходные коды Doom 3, которые недавно были выложены в сеть\. Результат \- ошибок найдено мало, но всё\-таки найдено\. Я предполагаю, что это можно объяснить так\. ![0120-Doom3_ru/image1.png](https://import.viva64.com/docx/blog/0120-Doom3_ru/image1.png) Часть кода Doom3 используется и сейчас и, наверное, там ошибки уже исправлены\. Часть кода устарела и не используется\. Скорее всего, именно там и найдены подозрительные участки кода\. Для тех, кто интересуется данной тематикой, предлагаю вниманию фрагменты кода, на которые указал анализатор PVS\-Studio\. Как всегда напоминаю, что рассматриваю только некоторые предупреждения\. Другие участки проекта требуют знания структуры программы, и я их не изучал\. Исходный код Doom 3 опубликован на [GitHub](https://github.com/TTimo/doom3.gpl) и на официальном [FTP](ftp://ftp.idsoftware.com/idstuff/source/idtech4-doom3-source-GPL.zip) компании под лицензией GPL v3\. Для анализа я использовал инструмент [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) 4\.39\. Кряки для программы прошу не искать\. Я уже встречал трояны, замаскированные под ключи и кряки к PVS\-Studio\. Лучше [напишите](https://pvs-studio.ru/ru/about-feedback/) нам, и мы дадим пробный ключ на некоторое время\. ## Фрагмент 1\. Подозрительное условие ```cpp #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\. Но [приоритет](https://pvs-studio.ru/ru/blog/terms/0064/) оператора '\!' выше, чем оператора '&'\. Это значит, что условие работает следующим образом: ```cpp (!player->oldButtons) & 1 ``` Получается, что условие истинно, только если все биты равны нулю\. Корректный вариант кода: ```cpp if ( player && ( ! ( player->oldButtons & BUTTON_ATTACK ) ) && ( player->usercmd.buttons & BUTTON_ATTACK ) ) { ``` ## Фрагмент 2\. Подозрительный цикл ```cpp 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' вообще не увеличивается\. Последствия работы такого цикла непредсказуемы\. Корректный вариант кода: ```cpp for ( k = 0; k < verts.Num(); k++ ) { ``` ## Фрагмент 3\. Другой подозрительный цикл ```cpp 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\. Неопределенное поведение ```cpp 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" изменяется дважды в одной [точке следования](https://pvs-studio.ru/ru/blog/terms/0065/)\. То, что используется префиксный инкремент, не имеет значение\. Компилятор теоретически вправе создать код следующего вида: ```cpp A = dir_cache_index; A = A + 1; B = A % MAX_CACHED_DIRS; dir_cache_index = B; dir_cache_index = A; ``` Конечно, скорее всего, выражение вычисляется правильно\. Но уверенным в этом быть нельзя\. На результат может влиять тип и версия компилятора, настройки оптимизации\. Корректный вариант кода: ```cpp dir_cache_index = (dir_cache_index + 1) % MAX_CACHED_DIRS; ``` ## Фрагмент 5\. Подозрительная очистка массива ```cpp 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'\. По всей видимости, это неправильно\. Как мне кажется, раньше было написано так: ```cpp byte newBlock[ CONST_ARRAY_SIZE ]; ... memset( newBlock, 0, sizeof( newBlock ) ); ``` Потом требования изменились и размер массива 'newBlock' стал изменяться\. Но про функцию его очистки забыли\. Корректный вариант кода: ```cpp memset( newBlock, 0, tileSize ); ``` ## Фрагмент 6\. Еще одна подозрительная очистка массива ```cpp 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 байт\. Это кстати весьма распространенная ошибка\. Я встречал её многократно в разных проектах\. Корректный вызов функции: ```cpp memset( &statex, 0, sizeof( statex ) ); ``` ## Фрагмент 7\. Здравствуй, Copy\-Paste ```cpp 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'\. В результате очищается только часть структуры\. Корректный вариант кода: ```cpp memset( &cluster, 0, sizeof( cluster ) ); ``` Я видел в коде и другие "недоочищенные" массивы, но про них писать не интересно\. ## Фрагмент 8\. Подозрительная работа с указателем ```cpp 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\. Это очень подозрительный код\. Я не знаю, как его сделать правильным, так как не знаю логику работы функции\. Возможно, достаточно написать так: ```cpp if ( node && node->occupied ) { ``` ## Фрагмент 9\. Подозрительный формат строки ```cpp 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\. Код, который сбивает с толку Неоднократно встречается код, который как я понимаю, работает правильно, но выглядит странно\. Приведу только один пример такого кода\. ```cpp 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% проектов\.