Source SDK - набор утилит для создания модификаций на движке Source, разработанный корпорацией Valve. Исходные коды проекта были скачаны и проверены ещё в конце 2013 года. На новогодних праздниках я планировал написать статью о результатах проверок. Но лень победила творчество, и я приступил к написанию статьи только когда вернулся на работу. Впрочем, я думаю, вряд ли за этот период что-то успело измениться в исходных кодах. Предлагаю вашему вниманию ознакомиться с подозрительными местами, которые я нашёл с помощью анализатора кода PVS-Studio.
Позаимствую описание проекта из Wikipedia:
Source SDK (англ. Software Development Kit — "набор разработчика приложений") — набор утилит для создания модификаций на движке Source, бесплатно распространяемый Valve через сеть Steam всем игрокам, купившим любую Source-игру от Valve. Данный набор позволяет редактировать карты на двух версиях движка — 15-ой и обновлённой 7-ой (старая версия движка, используемая в Half-Life 2, не используется из-за совместимости с новой версией).
Проект достаточно большой, так что не удивительно, что в нём всегда можно выявить недочёты. Анализ был выполнен с помощью инструмента PVS-Studio.
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 на следующий файл: viewpostprocess.cpp 1888
Обратите внимание, вот на это:
Это очень подозрительные выражения. Я затрудняюсь сказать, что здесь должно быть написано, но скорее всего, что-то иное.
void TextEntry::OnKeyCodePressed(KeyCode code)
{
....
if ( IsMouseCode(code) || IsNovintButtonCode(code) ||
IsJoystickCode(code) || IsJoystickButtonCode(code) ||
IsJoystickPOVCode(code) || IsJoystickPOVCode(code) ||
IsJoystickAxisCode(code) )
....
}PVS-Studio выдаёт предупреждение V501 на следующий файл: textentry.cpp 1639
Два раза вызывается функция 'IsJoystickPOVCode(code)'. Второй вызов избыточен или следовало вызвать другую функцию.
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 на следующий файл: vrad.cpp 2412.
Условие "numbounce < 0" никогда не выполнится. Беззнаковая переменная не может быть меньше нуля.
void CMultiplayRules::DeathNotice( .... )
{
....
else if ( strncmp( killer_weapon_name, "NPC_", 8 ) == 0 )
....
}PVS-Studio выдаёт предупреждение V666 на следующий файл: multiplay_gamerules.cpp 860.
Как я понимаю, хотели проверить, что название оружия начинается с символов "NPC_". Но тогда этот код содержит опечатку. Наверное, правильный вариант такой:
else if ( strncmp( killer_weapon_name, "NPC_", 4 ) == 0 )#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 на следующий файл: disp_vrad.cpp 60
Формальный аргумент "int iNeighbors[512]" это не массив. Это просто указатель. Число '512' подсказывает программисту, что, скорее всего, указатель ссылается на массив из 512 элементов. Но не более того. Выражение 'sizeof(iNeighbors)' незаконно. Оно возвращает не размер массива, а размер указателя. То есть выражение 'sizeof(iNeighbors)' будет равно 'sizeof(void *).
Эту ошибку можно было бы избежать, если использовать более безопасный макрос. Например, такой:
template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define arraysize(array) (sizeof(ArraySizeHelper(array)))При попытке вычисления размера указателя, возникнет ошибка на этапе компиляции. Такой макрос используется в проекте Chromium. Подробнее про эту магическую конструкцию можно прочитать в статье "PVS-Studio vs Chromium".
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 на следующий файл: vgui_messagechars.cpp 240
Выражение "sizeof(msg->text)" вычисляет размер указателя, а вовсе не длину строки. Скорее всего, здесь следовало написать: Q_strcpy( msg->text, data);
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 на следующий файл: baseflex.cpp 1326
Адрес временного массива помещается в переменную 'pszAct'. Так как этот массив будет разрушен, использовать адрес, содержащийся в 'pszAct' нельзя. Однако, на практике, этот код может работать, что создаёт ложную видимость корректности кода. Весьма вероятно, что область памяти, которую занимал временный массив 'szActName', далее не используется. В результате, программа ведёт себя так, как ожидает программист. Однако это везение и не более того.
#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 на следующий файл: hud_weaponselection.cpp 632, 633.
Счетчик цикла принимает значения от 0 до 6. Однако массивы xModifiers и yModifiers содержат в себе только 4 элемента. В результате, возникнет выход за границу массива.
void EmitDispLMAlphaAndNeighbors()
{
....
CCoreDispInfo *pDisp = new CCoreDispInfo;
if ( !pDisp )
{
g_CoreDispInfos.Purge();
return;
}
....
}PVS-Studio выдаёт предупреждение V668 на следующий файл: disp_vbsp.cpp 532.
Если не удаётся создать объект типа 'CCoreDispInfo', то должна быть вызвана функция g_CoreDispInfos.Purge(). Но эта функция вызвана не будет. Если произойдет ошибка выделения памяти, то возникнет исключение std::bad_alloc. Этот код является устаревшим и должен быть модифицирован с учетом изменений в поведении оператора 'new'.
Другие места, где проверяется, что вернул оператор 'new' будут приведены в конце статьи в приложении.
CNewParticleEffect::~CNewParticleEffect(void)
{
....
KeyValues *msg = new KeyValues( "ParticleSystem_Destroy" );
....
}PVS-Studio выдаёт предупреждение V509 на следующий файл: particles_new.cpp 92.
Опасно использовать в деструкторе конструкции, которые могут привести к возникновению исключения. Именно такой конструкцией и является оператор 'new'. В случае ошибки выделения памяти, он генерирует исключение.
Поясню в чем опасность. Если в программе возникает исключение, начинается свертывание стека, в ходе которого объекты разрушаются путем вызова деструкторов. Если деструктор объекта, разрушаемого при свертывании стека, бросает еще одно исключение, и это исключение покидает деструктор, библиотека C++ немедленно аварийно завершает программу, вызывая функцию terminate().
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 на следующий файл: beamdraw.cpp 592.
Обратите внимание на второй цикл:
for ( int j = 0; i < iBranches; j++ )В условии завершения вложенного цикла используется переменная 'i', относящаяся к внешнему циклу. У меня есть сильное подозрение, что мы имеем дело с опечаткой.
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 на следующий файл: networkvar.h 455.
Мне кажется, функция должна была быть написана так:
{
SetX( ix );
SetY( iy );
SetZ( iz );
SetW( iw );
}Обратите внимание на последний вызов функции.
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 на следующий файл: trace2.cpp 189.
Скорее всего, этот код писался с помощью Copy-Paste. Смотрите, в первой строке используются члены класса с именем 'x'. Во второй, с именем 'y'. А в третьей есть как 'z', так и 'y'. Скорее всего, код должен быть таким:
intens.z=OrSIMD(AndSIMD(BackgroundColor.z,no_hit_mask),
AndNotSIMD(no_hit_mask,intens.z));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 на следующий файл: vgui_fpspanel.cpp 192.
По всей видимости, должно было быть написано:
nFPSThreshold1 = 60;
nFPSThreshold2 = 50;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 на следующий файл: ai_utils.cpp 49.
Вновь опечатка, из-за которой:
Похожие беды есть в конструкторах классов CEnvTonemapController, CBasePlayerAnimState. Но описывать похожие примеры скучно, поэтому соответствующие я помещаю в приложение (см. конец статьи).
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 на следующий файл: c_baseanimating.cpp 5301, 5302.
Почему здесь возникает неопределённое поведение и почему нельзя предсказать значение переменной 'm_nResetEventsParity' хорошо описано в документации. В описании есть пример очень похожего кода.
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 на следующий файл: c_func_breakablesurf.cpp 157.
Сдвиг отрицательных чисел приводит к неопределённому поведению. В этом коде отрицательным числом является '~0x03'. Подробнее вопрос сдвига отрицательных чисел я уже рассматривал в статье "Не зная брода, не лезь в воду. Часть третья".
class CFlashlightEffect
{
....
~CFlashlightEffect();
....
};
class CHeadlightEffect : public CFlashlightEffect { .... };
CFlashlightEffect *m_pFlashlight;
C_BasePlayer::~C_BasePlayer()
{
....
delete m_pFlashlight;
}PVS-Studio выдаёт предупреждение V599 на следующий файл: c_baseplayer.cpp 454.
Существует класс CFlashlightEffect. Он имеет не виртуальный деструктор. Однако, от этого класса наследуются класс CHeadlightEffect. Вытекающие из этого последствия, думаю, понятны.
В проекте достаточно много мест, где странно сочетаются целочисленные типы и типы с плавающей точкой. Есть подозрение, что некоторые вычисления будут недостаточно точными или вообще не имеют смысла. Я продемонстрирую далее только 3 примера. Остальные подозрительные места перечислены в приложении.
void TE_BloodStream(....)
{
....
int speedCopy = amount;
....
speedCopy -= 0.00001; // so last few will drip
....
}PVS-Studio выдаёт предупреждение V674 на следующий файл: c_te_bloodstream.cpp 141.
Очень подозрительно вычитать из переменной типа 'int' значение 0.00001.
#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 на следующий файл: prediction.cpp 977.
Тип переменной 'step' выбран не очень удачно.
virtual int GetMappingWidth( ) = 0;
virtual int GetMappingHeight( ) = 0;
void CDetailObjectSystem::LevelInitPreEntity()
{
....
float flRatio = pMat->GetMappingWidth() /
pMat->GetMappingHeight();
....
}PVS-Studio выдаёт предупреждение V636 на следующий файл: detailobjectsystem.cpp 1480.
Возможно, есть смысл вычислить значение переменной 'flRatio' более точно. Причина низкой точности - целочисленное деление. Для повышения точности, можно написать так:
float flRatio = static_cast<float>(pMat->GetMappingWidth()) /
pMat->GetMappingHeight();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 на следующий файл: props.cpp 1520.
Переменная 'Reason' имеет тип PhysGunDrop_t. А 'PUNTED_BY_CANNON' относится к 'PhysGunPickup_t'.
static void Exit(const char *msg)
{
fprintf( stderr, msg );
Pause();
exit( -1 );
}PVS-Studio выдаёт предупреждение V618 на следующий файл: vice.cpp 52.
Функция 'fprintf()' может работать вполне корректно. Однако она потенциально опасна. Если случайно или сознательно в строке 'msg' будут содержаться управляющие символы, то последствия будут непредсказуемы.
Интересная заметка на эту тему: "Не зная брода, не лезь в воду. Часть вторая".
Безопасный вариант кода:
fprintf( stderr, "%s", msg );В файле будут перечислены другие предупреждения PVS-Studio, которые, на мой взгляд, заслуживают внимания. Но не следует целиком полагаться на этот список. Я смотрел проект поверхностно и мог на многое не обратить внимание. Плюс настоящая польза от статического анализа в его регулярном использовании, а не в разовой проверке проекта.
Список прочих подозрительных мест: source-sdk-addition-log.txt
Надеюсь, эта статья была интересна читателям и полезна разработчикам проекта Source SDK.