# Анализ проекта Source SDK Source SDK \- набор утилит для создания модификаций на движке Source, разработанный корпорацией Valve\. Исходные коды проекта были скачаны и проверены ещё в конце 2013 года\. На новогодних праздниках я планировал написать статью о результатах проверок\. Но лень победила творчество, и я приступил к написанию статьи только когда вернулся на работу\. Впрочем, я думаю, вряд ли за этот период что\-то успело измениться в исходных кодах\. Предлагаю вашему вниманию ознакомиться с подозрительными местами, которые я нашёл с помощью анализатора кода PVS\-Studio\. ![0229_Source_SDK_ru/image1.png](https://import.viva64.com/docx/blog/0229_Source_SDK_ru/image1.png) ## Что такое Source SDK Позаимствую [описание](https://ru.wikipedia.org/wiki/Source) проекта из Wikipedia: **Source SDK** \(англ\. Software Development Kit — "набор разработчика приложений"\) — набор утилит для создания модификаций на движке Source, бесплатно распространяемый [Valve](https://www.valvesoftware.com/en/) через сеть Steam всем игрокам, купившим любую Source\-игру от Valve\. Данный набор позволяет редактировать карты на двух версиях движка — 15\-ой и обновлённой 7\-ой \(старая версия движка, используемая в Half\-Life 2, не используется из\-за совместимости с новой версией\)\. Проект достаточно большой, так что не удивительно, что в нём всегда можно выявить недочёты\. Анализ был выполнен с помощью инструмента [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. ## Подозрительные выражения ### Деление переменных на самих себя ```cpp static void DrawPyroVignette(....) { .... Vector2D vMaxSize( ( float )nScreenWidth / ( float )nScreenWidth / NUM_PYRO_SEGMENTS * 2.0f, ( float )nScreenHeight / ( float )nScreenHeight / NUM_PYRO_SEGMENTS * 2.0f ); .... } ``` PVS\-Studio выдаёт предупреждение [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) на следующий файл: viewpostprocess\.cpp 1888 Обратите внимание, вот на это: * \( float \)nScreenWidth / \( float \)nScreenWidth * \( float \)nScreenHeight / \( float \)nScreenHeight Это очень подозрительные выражения\. Я затрудняюсь сказать, что здесь должно быть написано, но скорее всего, что\-то иное\. ### Двойной вызов функции IsJoystickPOVCode\(\) ```cpp void TextEntry::OnKeyCodePressed(KeyCode code) { .... if ( IsMouseCode(code) || IsNovintButtonCode(code) || IsJoystickCode(code) || IsJoystickButtonCode(code) || IsJoystickPOVCode(code) || IsJoystickPOVCode(code) || IsJoystickAxisCode(code) ) .... } ``` PVS\-Studio выдаёт предупреждение [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) на следующий файл: textentry\.cpp 1639 Два раза вызывается функция 'IsJoystickPOVCode\(code\)'\. Второй вызов избыточен или следовало вызвать другую функцию\. ### Всегда ложное условие ```cpp unsigned numbounce = 100; int ParseCommandLine( int argc, char **argv, bool *onlydetail ) { .... numbounce = atoi (argv[i]); if ( numbounce < 0 ) { Warning( "Error: expected non-negative value after '-bounce'\n"); return 1; } .... } ``` PVS\-Studio выдаёт предупреждение [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) на следующий файл: vrad\.cpp 2412\. Условие "numbounce < 0" никогда не выполнится\. Беззнаковая переменная не может быть меньше нуля\. ### Странное сравнение строк ```cpp void CMultiplayRules::DeathNotice( .... ) { .... else if ( strncmp( killer_weapon_name, "NPC_", 8 ) == 0 ) .... } ``` PVS\-Studio выдаёт предупреждение [V666](https://pvs-studio.ru/ru/docs/warnings/v666/) на следующий файл: multiplay\_gamerules\.cpp 860\. Как я понимаю, хотели проверить, что название оружия начинается с символов "NPC\_"\. Но тогда этот код содержит опечатку\. Наверное, правильный вариант такой: ```cpp else if ( strncmp( killer_weapon_name, "NPC_", 4 ) == 0 ) ``` ## Ошибки при работе с массивами ### Неправильное определение размера массива ```cpp #define RTL_NUMBER_OF_V1(A) (sizeof(A)/sizeof((A)[0])) #define _ARRAYSIZE(A) RTL_NUMBER_OF_V1(A) int GetAllNeighbors( const CCoreDispInfo *pDisp, int iNeighbors[512] ) { .... if ( nNeighbors < _ARRAYSIZE( iNeighbors ) ) iNeighbors[nNeighbors++] = pCorner->m_Neighbors[i]; .... } ``` PVS\-Studio выдаёт предупреждение [V511](https://pvs-studio.ru/ru/docs/warnings/v511/) на следующий файл: disp\_vrad\.cpp 60 Формальный аргумент "int iNeighbors\[512\]" это не массив\. Это просто указатель\. Число '512' подсказывает программисту, что, скорее всего, указатель ссылается на массив из 512 элементов\. Но не более того\. Выражение 'sizeof\(iNeighbors\)' незаконно\. Оно возвращает не размер массива, а размер указателя\. То есть выражение 'sizeof\(iNeighbors\)' будет равно 'sizeof\(void \*\)\. Эту ошибку можно было бы избежать, если использовать более безопасный макрос\. Например, такой: ```cpp template char (&ArraySizeHelper(T (&array)[N]))[N]; #define arraysize(array) (sizeof(ArraySizeHelper(array))) ``` При попытке вычисления размера указателя, возникнет ошибка на этапе компиляции\. Такой макрос используется в проекте Chromium\. Подробнее про эту магическую конструкцию можно прочитать в статье "[PVS\-Studio vs Chromium](https://pvs-studio.ru/ru/blog/posts/cpp/a0074/)"\. ### Неправильное определение длины строки ```cpp typedef struct message_s { .... char *text; .... } message_t; int CMessageCharsPanel::AddText(....) { .... msg->text = new char[ Q_strlen( data ) + 1 ]; Assert( msg->text ); Q_strncpy( msg->text, data, sizeof( msg->text ) ); .... } ``` PVS\-Studio выдаёт предупреждение [V579](https://pvs-studio.ru/ru/docs/warnings/v579/) на следующий файл: vgui\_messagechars\.cpp 240 Выражение "sizeof\(msg\-\>text\)" вычисляет размер указателя, а вовсе не длину строки\. Скорее всего, здесь следовало написать: Q\_strcpy\( msg\-\>text, data\); ### Работа с уничтоженным массивом ```cpp static Activity DetermineExpressionMoveActivity( CChoreoEvent *event, CAI_BaseNPC *pNPC ) { .... const char *pszAct = Q_strstr( sParam2, " " ); if ( pszAct ) { char szActName[256]; Q_strncpy( szActName, sParam2, sizeof(szActName) ); szActName[ (pszAct-sParam2) ] = '\0'; pszAct = szActName; } .... } ``` PVS\-Studio выдаёт предупреждение [V507](https://pvs-studio.ru/ru/docs/warnings/v507/) на следующий файл: baseflex\.cpp 1326 Адрес временного массива помещается в переменную 'pszAct'\. Так как этот массив будет разрушен, использовать адрес, содержащийся в 'pszAct' нельзя\. Однако, на практике, этот код может работать, что создаёт ложную видимость корректности кода\. Весьма вероятно, что область памяти, которую занимал временный массив 'szActName', далее не используется\. В результате, программа ведёт себя так, как ожидает программист\. Однако это везение и не более того\. ### Выход за границу массива ```cpp #define MAX_WEAPON_SLOTS 6 // hud item selection slots void CHudWeaponSelection::Paint() { .... int xModifiers[] = { 0, 1, 0, -1 }; int yModifiers[] = { -1, 0, 1, 0 }; for ( int i = 0; i < MAX_WEAPON_SLOTS; ++i ) { .... xPos += ( m_flMediumBoxWide + 5 ) * xModifiers[ i ]; yPos += ( m_flMediumBoxTall + 5 ) * yModifiers[ i ]; .... } ``` PVS\-Studio выдаёт предупреждение [V557](https://pvs-studio.ru/ru/docs/warnings/v557/) на следующий файл: hud\_weaponselection\.cpp 632, 633\. Счетчик цикла принимает значения от 0 до 6\. Однако массивы xModifiers и yModifiers содержат в себе только 4 элемента\. В результате, возникнет выход за границу массива\. ## Опасное использование оператора new ### Проверки, не имеющие смысла ```cpp void EmitDispLMAlphaAndNeighbors() { .... CCoreDispInfo *pDisp = new CCoreDispInfo; if ( !pDisp ) { g_CoreDispInfos.Purge(); return; } .... } ``` PVS\-Studio выдаёт предупреждение [V668](https://pvs-studio.ru/ru/docs/warnings/v668/) на следующий файл: disp\_vbsp\.cpp 532\. Если не удаётся создать объект типа 'CCoreDispInfo', то должна быть вызвана функция g\_CoreDispInfos\.Purge\(\)\. Но эта функция вызвана не будет\. Если произойдет ошибка выделения памяти, то возникнет исключение std::bad\_alloc\. Этот код является устаревшим и должен быть модифицирован с учетом изменений в поведении оператора 'new'\. Другие места, где проверяется, что вернул оператор 'new' будут приведены в конце статьи в приложении\. ### Оператор new в деструкторе ```cpp CNewParticleEffect::~CNewParticleEffect(void) { .... KeyValues *msg = new KeyValues( "ParticleSystem_Destroy" ); .... } ``` PVS\-Studio выдаёт предупреждение [V509](https://pvs-studio.ru/ru/docs/warnings/v509/) на следующий файл: particles\_new\.cpp 92\. Опасно использовать в деструкторе конструкции, которые могут привести к возникновению исключения\. Именно такой конструкцией и является оператор 'new'\. В случае ошибки выделения памяти, он генерирует исключение\. Поясню в чем опасность\. Если в программе возникает исключение, начинается свертывание стека, в ходе которого объекты разрушаются путем вызова деструкторов\. Если деструктор объекта, разрушаемого при свертывании стека, бросает еще одно исключение, и это исключение покидает деструктор, библиотека C\+\+ немедленно аварийно завершает программу, вызывая функцию terminate\(\)\. ## Опечатки ### Ошибка во вложенном цикле ```cpp void DrawTeslaSegs(....) { int i; .... for ( i = 0; i < segments; i++ ) { .... for ( int j = 0; i < iBranches; j++ ) { curSeg.m_flWidth *= 0.5; } .... } .... } ``` PVS\-Studio выдаёт предупреждение [V534](https://pvs-studio.ru/ru/docs/warnings/v534/) на следующий файл: beamdraw\.cpp 592\. Обратите внимание на второй цикл: ```cpp for ( int j = 0; i < iBranches; j++ ) ``` В условии завершения вложенного цикла используется переменная 'i', относящаяся к внешнему циклу\. У меня есть сильное подозрение, что мы имеем дело с опечаткой\. ### Неправильная инициализация ```cpp inline void SetX( float val ); inline void SetY( float val ); inline void SetZ( float val ); inline void SetW( float val ); inline void Init( float ix=0, float iy=0, float iz=0, float iw = 0 ) { SetX( ix ); SetY( iy ); SetZ( iz ); SetZ( iw ); } ``` PVS\-Studio выдаёт предупреждение [V525](https://pvs-studio.ru/ru/docs/warnings/v525/) на следующий файл: networkvar\.h 455\. Мне кажется, функция должна была быть написана так: ```cpp { SetX( ix ); SetY( iy ); SetZ( iz ); SetW( iw ); } ``` Обратите внимание на последний вызов функции\. ### Последствие Copy\-Paste ```cpp class ALIGN16 FourVectors { public: fltx4 x, y, z; .... }; FourVectors BackgroundColor; void RayTracingEnvironment::RenderScene(....) { .... intens.x=OrSIMD(AndSIMD(BackgroundColor.x,no_hit_mask), AndNotSIMD(no_hit_mask,intens.x)); intens.y=OrSIMD(AndSIMD(BackgroundColor.y,no_hit_mask), AndNotSIMD(no_hit_mask,intens.y)); intens.z=OrSIMD(AndSIMD(BackgroundColor.y,no_hit_mask), AndNotSIMD(no_hit_mask,intens.z)); .... } ``` PVS\-Studio выдаёт предупреждение [V537](https://pvs-studio.ru/ru/docs/warnings/v537/) на следующий файл: trace2\.cpp 189\. Скорее всего, этот код писался с помощью Copy\-Paste\. Смотрите, в первой строке используются члены класса с именем 'x'\. Во второй, с именем 'y'\. А в третьей есть как 'z', так и 'y'\. Скорее всего, код должен быть таким: ```cpp intens.z=OrSIMD(AndSIMD(BackgroundColor.z,no_hit_mask), AndNotSIMD(no_hit_mask,intens.z)); ``` ### Присваивания разных значений одной переменной ```cpp void GetFPSColor( int nFps, unsigned char ucColor[3] ) { .... int nFPSThreshold1 = 20; int nFPSThreshold2 = 15; if (IsPC() && g_pMaterialSystemHardwareConfig->GetDXSupportLevel() >= 95) { nFPSThreshold1 = 60; nFPSThreshold1 = 50; } .... } ``` PVS\-Studio выдаёт предупреждение [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) на следующий файл: vgui\_fpspanel\.cpp 192\. По всей видимости, должно было быть написано: ```cpp nFPSThreshold1 = 60; nFPSThreshold2 = 50; ``` ### Плохой конструктор ```cpp CAI_ShotRegulator::CAI_ShotRegulator() : m_nMinBurstShots(1), m_nMaxBurstShots(1) { m_flMinRestInterval = 0.0f; m_flMinRestInterval = 0.0f; m_flMinBurstInterval = 0.0f; m_flMaxBurstInterval = 0.0f; m_flNextShotTime = -1; m_nBurstShotsRemaining = 1; m_bInRestInterval = false; m_bDisabled = false; } ``` PVS\-Studio выдаёт предупреждение [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) на следующий файл: ai\_utils\.cpp 49\. Вновь опечатка, из\-за которой: 1. Переменной m\_flMinRestInterval два раза присваивается нулевое значение\. 1. Переменная m\_flMaxRestInterval остаётся неинициализированной\. Похожие беды есть в конструкторах классов CEnvTonemapController, CBasePlayerAnimState\. Но описывать похожие примеры скучно, поэтому соответствующие я помещаю в приложение \(см\. конец статьи\)\. ## Неопределённое поведение ### Сложные выражения ```cpp int m_nNewSequenceParity; int m_nResetEventsParity; void C_BaseAnimating::ResetSequenceInfo( void ) { .... m_nNewSequenceParity = ( ++m_nNewSequenceParity ) & EF_PARITY_MASK; m_nResetEventsParity = ( ++m_nResetEventsParity ) & EF_PARITY_MASK; .... } ``` PVS\-Studio выдаёт предупреждение [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) на следующий файл: c\_baseanimating\.cpp 5301, 5302\. Почему здесь возникает неопределённое поведение и почему нельзя предсказать значение переменной 'm\_nResetEventsParity' [хорошо описано](https://pvs-studio.ru/ru/docs/warnings/v567/) в документации\. В описании есть пример очень похожего кода\. ### Сдвиги ```cpp inline void SetStyleType( int w, int h, int type ) { Assert( type < NUM_EDGE_STYLES ); Assert( type >= 0 ); // Clear old value m_nPanelBits[ w ][ h ] &= ( ~0x03 << 2 ); // Insert new value m_nPanelBits[ w ][ h ] |= ( type << 2 ); } ``` PVS\-Studio выдаёт предупреждение [V610](https://pvs-studio.ru/ru/docs/warnings/v610/) на следующий файл: c\_func\_breakablesurf\.cpp 157\. Сдвиг отрицательных чисел приводит к неопределённому поведению\. В этом коде отрицательным числом является '\~0x03'\. Подробнее вопрос сдвига отрицательных чисел я уже рассматривал в статье "[Не зная брода, не лезь в воду\. Часть третья](https://pvs-studio.ru/ru/blog/posts/cpp/0142/)"\. ### Отсутствует виртуальный деструктор ```cpp class CFlashlightEffect { .... ~CFlashlightEffect(); .... }; class CHeadlightEffect : public CFlashlightEffect { .... }; CFlashlightEffect *m_pFlashlight; C_BasePlayer::~C_BasePlayer() { .... delete m_pFlashlight; } ``` PVS\-Studio выдаёт предупреждение [V599](https://pvs-studio.ru/ru/docs/warnings/v599/) на следующий файл: c\_baseplayer\.cpp 454\. Существует класс CFlashlightEffect\. Он имеет не виртуальный деструктор\. Однако, от этого класса наследуются класс CHeadlightEffect\. Вытекающие из этого последствия, думаю, понятны\. ## Подозрительная арифметика В проекте достаточно много мест, где странно сочетаются целочисленные типы и типы с плавающей точкой\. Есть подозрение, что некоторые вычисления будут недостаточно точными или вообще не имеют смысла\. Я продемонстрирую далее только 3 примера\. Остальные подозрительные места перечислены в приложении\. ### Первый подозрительный фрагмент ```cpp void TE_BloodStream(....) { .... int speedCopy = amount; .... speedCopy -= 0.00001; // so last few will drip .... } ``` PVS\-Studio выдаёт предупреждение [V674](https://pvs-studio.ru/ru/docs/warnings/v674/) на следующий файл: c\_te\_bloodstream\.cpp 141\. Очень подозрительно вычитать из переменной типа 'int' значение 0\.00001\. ### Второй подозрительный фрагмент ```cpp #define ON_EPSILON 0.1 void CPrediction::SetIdealPitch (....) { int step; .... step = floor_height[j] - floor_height[j-1]; if (step > -ON_EPSILON && step < ON_EPSILON) continue; .... } ``` PVS\-Studio выдаёт предупреждение [V674](https://pvs-studio.ru/ru/docs/warnings/v674/) на следующий файл: prediction\.cpp 977\. Тип переменной 'step' выбран не очень удачно\. ### Третий подозрительный фрагмент ```cpp virtual int GetMappingWidth( ) = 0; virtual int GetMappingHeight( ) = 0; void CDetailObjectSystem::LevelInitPreEntity() { .... float flRatio = pMat->GetMappingWidth() / pMat->GetMappingHeight(); .... } ``` PVS\-Studio выдаёт предупреждение [V636](https://pvs-studio.ru/ru/docs/warnings/v636/) на следующий файл: detailobjectsystem\.cpp 1480\. Возможно, есть смысл вычислить значение переменной 'flRatio' более точно\. Причина низкой точности \- целочисленное деление\. Для повышения точности, можно написать так: ```cpp float flRatio = static_cast(pMat->GetMappingWidth()) / pMat->GetMappingHeight(); ``` ## Разное ### Путаница с типами ```cpp enum PhysGunPickup_t { PICKED_UP_BY_CANNON, PUNTED_BY_CANNON, PICKED_UP_BY_PLAYER, }; enum PhysGunDrop_t { DROPPED_BY_PLAYER, THROWN_BY_PLAYER, DROPPED_BY_CANNON, LAUNCHED_BY_CANNON, }; void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason) { .... if( Reason == PUNTED_BY_CANNON ) { PlayPuntSound(); } .... } ``` PVS\-Studio выдаёт предупреждение [V556](https://pvs-studio.ru/ru/docs/warnings/v556/) на следующий файл: props\.cpp 1520\. Переменная 'Reason' имеет тип PhysGunDrop\_t\. А 'PUNTED\_BY\_CANNON' относится к 'PhysGunPickup\_t'\. ### Потенциально опасный fprintf ```cpp static void Exit(const char *msg) { fprintf( stderr, msg ); Pause(); exit( -1 ); } ``` PVS\-Studio выдаёт предупреждение [V618](https://pvs-studio.ru/ru/docs/warnings/v618/) на следующий файл: vice\.cpp 52\. Функция 'fprintf\(\)' может работать вполне корректно\. Однако она потенциально опасна\. Если случайно или сознательно в строке 'msg' будут содержаться управляющие символы, то последствия будут непредсказуемы\. Интересная заметка на эту тему: "[Не зная брода, не лезь в воду\. Часть вторая](https://pvs-studio.ru/ru/blog/posts/cpp/0129/)"\. Безопасный вариант кода: ```cpp fprintf( stderr, "%s", msg ); ``` ## Приложение В файле будут перечислены другие предупреждения PVS\-Studio, которые, на мой взгляд, заслуживают внимания\. Но не следует целиком полагаться на этот список\. Я смотрел проект поверхностно и мог на многое не обратить внимание\. Плюс настоящая польза от статического анализа в его регулярном использовании, а не в разовой проверке проекта\. Список прочих подозрительных мест: [source\-sdk\-addition\-log\.txt](https://pvs-studio.com/external-pictures/txt/source-sdk-addition-log.txt) ## Заключение Надеюсь, эта статья была интересна читателям и полезна разработчикам проекта Source SDK\.