Анализ проекта Source SDK

• Что такое Source SDK
• Подозрительные выражения
  • Деление переменных на самих себя
  • Двойной вызов функции IsJoystickPOVCode()
  • Всегда ложное условие
  • Странное сравнение строк
• Ошибки при работе с массивами
  • Неправильное определение размера массива
  • Неправильное определение длины строки
  • Работа с уничтоженным массивом
  • Выход за границу массива
• Опасное использование оператора new
  • Проверки, не имеющие смысла
  • Оператор new в деструкторе
• Опечатки
  • Ошибка во вложенном цикле
  • Неправильная инициализация
  • Последствие Copy-Paste
  • Присваивания разных значений одной переменной
  • Плохой конструктор
• Неопределённое поведение
  • Сложные выражения
  • Сдвиги
  • Отсутствует виртуальный деструктор
• Подозрительная арифметика
  • Первый подозрительный фрагмент
  • Второй подозрительный фрагмент
  • Третий подозрительный фрагмент
• Разное
  • Путаница с типами
  • Потенциально опасный fprintf
• Приложение
• Заключение

Source SDK - набор утилит для создания модификаций на движке Source, разработанный корпорацией Valve. Исходные коды проекта были скачаны и проверены ещё в конце 2013 года. На новогодних праздниках я планировал написать статью о результатах проверок. Но лень победила творчество, и я приступил к написанию статьи только когда вернулся на работу. Впрочем, я думаю, вряд ли за этот период что-то успело измениться в исходных кодах. Предлагаю вашему вниманию ознакомиться с подозрительными местами, которые я нашёл с помощью анализатора кода PVS-Studio.

Что такое Source SDK

Позаимствую описание проекта из 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

Обратите внимание, вот на это:

• ( float )nScreenWidth / ( float )nScreenWidth
• ( float )nScreenHeight / ( float )nScreenHeight

Это очень подозрительные выражения. Я затрудняюсь сказать, что здесь должно быть написано, но скорее всего, что-то иное.

Двойной вызов функции IsJoystickPOVCode()

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 элемента. В результате, возникнет выход за границу массива.

Опасное использование оператора new

Проверки, не имеющие смысла

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' будут приведены в конце статьи в приложении.

Оператор 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 );
}

Обратите внимание на последний вызов функции.

Последствие Copy-Paste

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.

Вновь опечатка, из-за которой:

• Переменной m_flMinRestInterval два раза присваивается нулевое значение.
• Переменная m_flMaxRestInterval остаётся неинициализированной.

Похожие беды есть в конструкторах классов 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'.

Потенциально опасный fprintf

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.