Шукаємо аномалії в X-Ray Engine

X-Ray Engine — ігровий движок, який використовується в іграх серії S. T. A. L. K. E. R. 16 вересня 2014 року його вихідний код був викладений у відкритий доступ, і з тих пір його розвитком займаються фанати. Великий розмір проекту, величезна кількість багів в іграх — все це розташовує до відмінної демонстрації можливостей статичного аналізатора коду PVS-Studio.




Вступ
X-Ray був створений українською компанією GSC GameWorld для гри S. T. A. L. K. E. R.: Тінь Чорнобиля. Движок включає рендер з підтримкою DirectX 8.1/9.0 c/10/10.1/11, фізичний і звуковий движки, мультиплеер і систему штучного інтелекту A-Life. Згодом компанія створювала движок версії 2.0 для своєї нової гри, але розробка була припинена і вихідні коди витекли в мережу.

Проект разом з усіма його залежностями легко збирається в Visual Studio 2015. Для перевірки використовувався вихідний код системи версії 1.6 репозиторію на GitHub і статичний аналізатор коду PVS-Studio 6.04, який можна завантажити за посилання.

Copy-paste
Для початку розглянемо помилки, пов'язані з копіюванням коду. Сценарій їх виникнення в різних випадках зазвичай схожий: скопіювали код, поміняли частину змінних, а кілька — забули. Такі помилки можуть швидко поширюватися по кодової базі, і без статичного аналізатора їх дуже легко пропустити.



MxMatrix& MxQuadric::homogeneous(MxMatrix& H) const
{
....
unsigned int i, j;

for(i=0; i<A. dim(); i++) for(j=0; j<A. dim(); i++)
H(i,j) = A(i,j);
....
}

Попередження PVS-Studio: V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. mxqmetric.cpp 76

Аналізатор виявив, що у вкладеному циклі for инкрементируется мінлива i, а перевіряється мінлива j, що призводить до нескінченного циклу. Швидше за все, при копіюванні її просто забули поміняти.
void CBaseMonster::settings_read(CInifile const * ini,
LPCSTR section, 
SMonsterSettings &data)
{
....
if (ini->line_exist(ppi_section,"color_base"))
sscanf(ini->r_string(ppi_section,"color_base"), "%f%f%f", 
&data.m_attack_effector.ppi.color_base.r, 
&data.m_attack_effector.ppi.color_base.g, 
&data.m_attack_effector.ppi.color_base.b); 
if (ini->line_exist(ppi_section,"color_base"))
sscanf(ini->r_string(ppi_section,"color_gray"), "%f%f%f", 
&data.m_attack_effector.ppi.color_gray.r, 
&data.m_attack_effector.ppi.color_gray.g, 
&data.m_attack_effector.ppi.color_gray.b);
if (ini->line_exist(ppi_section,"color_base"))
sscanf(ini->r_string(ppi_section,"color_add"), "%f%f%f", 
&data.m_attack_effector.ppi.color_add.r, 
&data.m_attack_effector.ppi.color_add.g, 
&data.m_attack_effector.ppi.color_add.b);
....
}

Попередження PVS-Studio:
  • V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 445, 447. base_monster_startup.cpp 447
  • V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 447, 449. base_monster_startup.cpp 449
У даному фрагменті використовуються поспіль кілька однакових умовних виразів. Очевидно, що необхідно замінити color_base color_gray color_add у відповідності з кодом в тілі if гілки.
/* process a single statement */
static void ProcessStatement(char *buff, int len)
{
....
if (strncmp(buff,"\\pauthr\\",8) == 0)
{
ProcessPlayerAuth(buff, len);
} else if (strncmp(buff,"\\getpidr\\",9) == 0)
{
ProcessGetPid(buff, len);
} else if (strncmp(buff,"\\getpidr\\",9) == 0)
{
ProcessGetPid(buff, len);
} else if (strncmp(buff,"\\getpdr\\",8) == 0)
{
ProcessGetData(buff, len);
} else if (strncmp(buff,"\\setpdr\\",8) == 0)
{
ProcessSetData(buff, len);
} 
}

Попередження PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1502, 1505. gstats.c 1502

Як і в попередньому прикладі, тут використовуються два однакових умови (strncmp(buff,"\\getpidr\\",9) == 0). Складно сказати напевно, чи є це помилкою чи просто недосяжним кодом, але на це варто звернути увагу. Можливо, що тут повинні бути блоки з getpidr/setpidr за аналогією з getpdr/setpdr.

class RGBAMipMappedCubeMap
{
....
size_t height() const
{
return cubeFaces[0].height();
}

size_t width() const
{
return cubeFaces[0].height();
}
....
};

Попередження PVS-Studio: V524 It is odd that the body of 'width' function is fully equivalent to the body of 'height' function. tpixel.h 1090

Методи height() width() мають однакове тіло. Враховуючи, що обчислюються розміри граней куба, можливо, помилки тут немає. Але краще переписати метод width() наступним чином:
size_t width() const
{
return cubeFaces[0].width();
}

Неправильне використання C++
C++ — чудовий мову, який надає програмісту багато можливостей… вполювати собі ногу особливо жорстоким чином. Невизначений поведінка, витоку пам'яті і, звичайно ж, помилки — про помилки такого роду піде мова в поточному розділі.



template < class T>
struct _matrix33
{
public:
typedef _matrix33<T>Self;
typedef Self& SelfRef;
....
IC SelfRef sMTxV(Tvector& R, float s1, const Tvector& V1) const
{
R. x = s1*(m[0][0] * V1.x + m[1][0] * V1.y + m[2][0] * V1.z);
R. y = s1*(m[0][1] * V1.x + m[1][1] * V1.y + m[2][1] * V1.z);
R. z = s1*(m[0][2] * V1.x + m[1][2] * V1.y + m[2][2] * V1.z);
}
....
}

Попередження PVS-Studio: V591 Non-void function should return a value. _matrix33.h 435

В кінці методу пропущено return *this. За стандартом подібний код призведе до невизначеного поведінки. Так як повернуте значення є посиланням, це, швидше за все, призведе до падіння програми при спробі звернутися до повертається значенням.
ETOOLS_API int __stdcall ogg_enc (....)
{
....
FILE *in, *out = NULL;
....
input_format *format;
....
in = fopen(in_fn, "rb");

if(in == NULL) return 0;

format = open_audio_file(in, &enc_opts);
if(!format){
fclose(in);
return 0;
};

out = fopen(out_fn, "wb");
if(out == NULL){
fclose(out);
return 0;
} 
....
}

Попередження PVS-Studio: V575 The null pointer is passed into 'fclose' function. Inspect the first argument. ogg_enc.cpp 47

Досить цікавий приклад. Аналізатор виявив, що аргумент у виклику fclose дорівнює nullptr, що робить виклик функції безглуздим. Можна припустити, що повинні були закрити потік in.
void NVI_Image::ABGR8_To_ARGB8()
{
// swaps RGB for all pixels
assert(IsDataValid());
assert(GetBytesPerPixel() == 4);
UINT hxw = GetNumPixels();
for (UINT i = 0; i < hxw; i++)
{
DWORD col;
GetPixel_ARGB8(&col, i);
DWORD a = (col >> 24) && 0x000000FF;
DWORD b = (col >> 16) && 0x000000FF;
DWORD g = (col >> 8) && 0x000000FF;
DWORD r = (col >> 0) && 0x000000FF;
col = (a << 24) | (r << 16) | (g << 8) | b;
SetPixel_ARGB8(i, col);
}
}

Попередження PVS-Studio:
  • V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 170
  • V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 171
  • V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 172
  • V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 173
В даному ділянці коду переплутані логічні і бітові операції. Результат буде не таким, якого очікував програміст: col буде завжди дорівнює 0x01010101 незалежно від вхідних даних.

Правильний варіант:
DWORD a = (col >> 24) & 0x000000FF;
DWORD b = (col >> 16) & 0x000000FF;
DWORD g = (col >> 8) & 0x000000FF;
DWORD r = (col >> 0) & 0x000000FF;

Ще один приклад дивного коду:
VertexCache::VertexCache()
{
VertexCache(16);
}

Попередження PVS-Studio: V603 The object was created but it is not being used. If you wish to call constructor, 'this->VertexCache::VertexCache (....)' should be used. vertexcache.cpp 6

Замість одного виклику конструктора з іншого для ініціалізації примірника буде створений і тут же знищений новий об'єкт типу VertexCache. В результаті члени створюваного об'єкта залишаться непроинициализированными.
BOOL CActor::net_Spawn(CSE_Abstract* DC)
{
....
m_States.empty();
....
}

Попередження PVS-Studio: V530 The return value of function 'empty' is required to be utilized. actor_network.cpp 657

Аналізатор попереджає, що повертається функцією значення не використовується. Схоже, що програміст переплутав методи empty() clear(): empty() не очищає масив, а перевіряє, чи є він порожнім.

Такі помилки нерідко зустрічаються в різних проектах. Проблема в тому, що ім'я empty() не очевидно: деякі сприймають його як дію — видалення. Для того, щоб такої неоднозначності не виникало краще додавати дієслова has, is до початку методу: дійсно, isEmpty() clear() складно переплутати.

Схоже попередження:

V530 The return value of function 'unique' is required to be utilized. uidragdroplistex.cpp 780
size_t xrDebug::BuildStackTrace(EXCEPTION_POINTERS* exPtrs,
char *buffer,
size_t capacity,
size_t lineCapacity)
{
memset(buffer, capacity*lineCapacity, 0);
....
}

Попередження PVS-Studio: V575 The 'memset' function processes '0' elements. Inspect the third argument. xrdebug.cpp 104

При виклику memset аргументи переплутали місцями і в підсумку буфер не обнуляється, як спочатку замислювалося. Подібна помилка може жити в проекті дуже довго, так як її дуже важко виявити. У таких місцях на виручку програмісту приходить статичний аналізатор.

Коректне використання memset:
memset(buffer, 0, capacity*lineCapacity);

Наступна помилка пов'язана з неправильно складених логічним виразом.
void configs_dumper::dumper_thread(void* my_ptr)
{
....
DWORD wait_result = WaitForSingleObject(
this_ptr->m_make_start_event, INFINITE);
while ( wait_result != WAIT_ABANDONED) ||
(wait_result != WAIT_FAILED))
....
}

Попередження PVS-Studio: V547 Expression is always true. Probably the '&&' operator should be used here. configs_dumper.cpp 262

Вирази виду 'x != a || x != b' завжди є істинним. Найімовірніше замість оператора || мався на увазі оператор &&.

Детальніше про помилки в логічних виразах можна прочитати у статті "Логічні вирази в C/C++. Як помиляються професіонали".
void SBoneProtections::reload(const shared_str& bone_sect, 
IKinematics* kinematics)
{
....
CInifile::Секта &protections = pSettings->r_section(bone_sect);
for (CInifile::SectCIt i=protections.Data.begin();
protections.Data.end() != i; i++) 
{
string256 buffer;
BoneProtection BP;
....
BP.BonePassBullet = (BOOL) (
atoi( _GetItem(i->second.c_str(), 2, buffer) )>0.5 f);
....
}
}

Попередження PVS-Studio: V674 The '0.5 f' literal of the 'float' type is compared to a value of the 'int' type. boneprotections.cpp 54

Аналізатор виявив порівняння цілочисельного значення з речовою константою. Можливо, що тут за аналогією повинна була використовуватися функція atof, а не atoi, в іншому випадку варто переписати це порівняння, щоб воно не виглядало підозріло. Однак сказати напевно, чи є цей приклад помилковим чи ні, може тільки розробник, писав його.
class IGameObject :
public virtual IFactoryObject,
public virtual ISpatial,
public virtual ISheduled,
public virtual IRenderable,
public virtual ICollidable
{
public:
....
virtual u16 ID() const = 0;
....
}

BOOL CBulletManager::test_callback(
const collide::ray_defs& rd,
IGameObject* object,
LPVOID params)
{
bullet_test_callback_data* pData = 
(bullet_test_callback_data*)params;
SBullet* bullet = pData->pBullet;

if( (object->ID() == bullet->parent_id) && 
(bullet->fly_dist<parent_ignore_distance) &&
(!bullet->flags.ricochet_was)) return FALSE;

BOOL bRes = TRUE;
if (object){
....
}

return bRes;
}

Попередження PVS-Studio: V595 The 'object' pointer was utilized before it was verified against nullptr. Check lines: 42, 47. level_bullet_manager_firetrace.cpp 42

Перевірка покажчика object на рівність nullptr йде після того, як разыменовали object->ID(). У випадку, коли object дорівнює nullptr, це призведе до падіння програми.
#ifdef _EDITOR
BOOL WINAPI DllEntryPoint (....)
#else
BOOL WINAPI DllMain (....)
#endif
{
switch (ul_reason_for_call)
{
....
case DLL_THREAD_ATTACH:
if (!strstr(GetCommandLine(), "-editor"))
CoInitializeEx NULL, COINIT_MULTITHREADED);
timeBeginPeriod(1);
break;
....
}
return TRUE;
}

Попередження PVS-Studio: V718 The 'CoInitializeEx' function should not be called from 'DllMain' function. xrcore.cpp 205

У тілі DllMain не використовувати частину функцій WinAPI, включаючи CoInitializeEx. Переконатися в цьому можна, прочитавши документацію на MSDN. Не можна дати якусь однозначну пораду, як варто переписати цю функцію, але варто розуміти, що така ситуація небезпечна, так як вона може призвести до взаємного блокування потоків або аварійного завершення.

Помилки в пріоритетах
int sgetI1( unsigned char **bp )
{
int i;

if ( flen == FLEN_ERROR ) return 0;
i = **bp;
if ( i > 127 ) i -= 256;
flen += 1;
*bp++;
return i;
}

Попередження PVS-Studio: V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwio.c 316

Помилка пов'язана з використанням инкремента. Для наочності перепишемо цей вираз, розставивши дужки:
*(bp++);

Тобто відбудеться зсув вмісту не за адресою bp, а самого покажчика, що в даному контексті безглуздо. Нижче за кодом є фрагменти виду *bp += N, з-за чого я зробив висновок, що це помилка.

Уникнути подібної помилки допомогла б розстановка дужок, що зробило б порядок обчислень більш зрозумілим. Також непоганий методикою є використання const для аргументів, які не повинні змінюватися.

Аналогічні попередження:
  • V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwio.c 354
  • V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwob.c 80
void CHitMemoryManager::load (IReader &packet)
{
....
if (!spawn_callback || !spawn_callback->m_object_callback)
if(!g_dedicated_server)
Level().client_spawn_manager().add(
delayed_object.m_object_id,m_object->ID(),callback);
#ifdef DEBUG
else {
if (spawn_callback && spawn_callback->m_object_callback) {
VERIFY(spawn_callback->m_object_callback == callback);
}
}
#endif // DEBUG
}

Попередження PVS-Studio: V563 It is possible that this 'else' branch must apply to the previous 'if' statement. hit_memory_manager.cpp 368

В цьому фрагменті гілка else відноситься до другого if з-за свого право-асоціативності, що не збігається з форматуванням коду. На щастя, даний випадок не відбивається на роботі програм, тим не менш, він може ускладнити процес налагодження і тестування.

Рекомендація проста — в більш-менш складних ветвлениях розставляйте фігурні дужки.
void HUD_SOUND_ITEM::PlaySound(HUD_SOUND_ITEM& hud_snd,
const Fvector& position,
const IGameObject* parent,
bool b_hud_mode,
bool looped,
u8 index)
{
....
hud_snd.m_activeSnd->snd.set_volume(
hud_snd.m_activeSnd->volume * b_hud_mode?psHUDSoundVolume:1.0 f);
}

Попередження PVS-Studio: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower than the priority '*' operator. hudsound.cpp 108

У тернарного умовного оператора пріоритет нижче, ніж у множення, тому порядок операцій буде наступним:
(hud_snd.m_activeSnd->volume * b_hud_mode)?psHUDSoundVolume:1.0 f

Очевидно, що правильний код повинен виглядати так:
hud_snd.m_activeSnd->volume * (b_hud_mode?psHUDSoundVolume:1.0 f)

Вирази, що містять тернарний оператор, кілька if-else гілок або операції ТА/АБО, — це ті випадки, коли краще поставити зайві дужки.

Аналогічні попередження:
  • V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower than the priority '+' operator. uihudstateswnd.cpp 487
  • V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower than the priority '+' operator. uicellcustomitems.cpp 106
Зайві порівняння
void CDestroyablePhysicsObject::OnChangeVisual()
{
if (m_pPhysicsShell){
if(m_pPhysicsShell)m_pPhysicsShell->Deactivate();
....
}
....
}

Попередження PVS-Studio: V571 Recurring check. The 'if (m_pPhysicsShell)' condition was already verified in line 32. destroyablephysicsobject.cpp 33

В даному прикладі двічі перевіряється m_pPhysicsShell. Швидше за все, друга перевірка зайва.
void CSE_ALifeItemPDA::STATE_Read(NET_Packet &tNetPacket,
u16 size)
{
....
if (m_wVersion > 89)

if ( (m_wVersion > 89)&&(m_wVersion < 98) )
{
....
}else{
....
}
}

Попередження PVS-Studio: V571 Recurring check. The 'm_wVersion > 89' condition was already verified in line 987. xrserver_objects_alife_items.cpp 989

Дуже дивний код. То тут забули вираз після if (m_wVersion > 89), то цілу серію else-if. Даний метод вимагає більш докладного розгляду розробником проекту.
void ELogCallback(void *context, LPCSTR txt)
{
....
bool bDlg = ('#'==txt[0])||((0!=txt[1])&&('#'==txt[1]));
if (bDlg){
int mt = ('!'==txt[0])||((0!=txt[1])&&('!'==txt[1]))?1:0;
....
}
}

Попередження PVS-Studio:
  • V590 Consider inspecting the '(0 != txt[1]) && ('#' == txt[1])' expression. The expression is excessive or contains a misprint. elog.cpp 29
  • V590 Consider inspecting the '(0 != txt[1]) && ('!' == txt[1])' expression. The expression is excessive or contains a misprint. elog.cpp 31
У виразах ініціалізації змінних bDlg mt перевірка (0 != txt[1]) є надлишковою. Якщо її опустити, вираження стануть читатися значно легше:
bool bDlg = ('#'==txt[0])||('#'==txt[1]);
int mt = ('!'==txt[0])||('!'==txt[1])?1:0;

Помилки в типи даних


float CRenderTarget::im_noise_time;

CRenderTarget::CRenderTarget()
{
....
param_blur = 0.f;
param_gray = 0.f;
param_noise = 0.f;
param_duality_h = 0.f;
param_duality_v = 0.f;
param_noise_fps = 25.f;
param_noise_scale = 1.f;

im_noise_time = 1/100;
im_noise_shift_w = 0;
im_noise_shift_h = 0;
....
}

Попередження PVS-Studio: V636 The '1 / 100' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gl_rendertarget.cpp 245

Значення виразу 1/100 дорівнює 0, так як виконується операція цілочисельного ділення. Щоб отримати значення 0.01 f, потрібно використовувати речовинний літерал, переписавши вираз: 1/100.0 f. Хоча можливо, що така поведінка було передбачено автором, і помилки тут немає.

CSpaceRestriction::merge (....) const
{
....
LPSTR S = xr_alloc<char>(acc_length);

for ( ; I != E; I++)
temp = strconcat(sizeof(S),S,*temp",",*(*I)->name());
....
}

Попередження PVS-Studio: V579 The strconcat function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the first argument. space_restriction.cpp 201

Функція strconcat, в якості першого параметра приймає довжину буфера. Буфер S оголошений, як LPSTR, тобто як покажчик на рядок. sizeof(S) буде дорівнює розміру покажчика в байтах, тобто sizeof(char *), а не кількості символів в рядку. Для обчислення довжини слід використовувати strlen(S).
class XRCDB_API MODEL
{
....
u32 status; // 0=ready, 1=init, 2=building
....
}

void MODEL::build (Fvector* V, int Vcnt, TRI* T, int Tcnt, 
build_callback* bc, void* bcp)
{
....
BTHREAD_params P = { this, V, Vcnt, T, Tcnt, bc, bcp };
thread_spawn(build_thread,"CDB-construction",0,&P);
while (S_INIT == status) Sleep(5);
....
}

Попередження PVS-Studio: V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. xrcdb.cpp 100

Компілятор може прибрати перевірку S_INIT == status в якості оптимізації, так як змінна status не модифікується в циклі. Для того, щоб уникнути подібної поведінки, використовувати volatile змінні або типи синхронізації даних між потоками.

Аналогічні попередження:
  • V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 23
  • V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 232
void CAI_Rat::UpdateCL()
{
....
if (!Useful()) {
inherited::UpdateCL ();
Exec_Look (Device.fTimeDelta);

CMonsterSquad *squad = monster_squad().get_squad(this);

if (squad && ((squad->GetLeader() != this &&
!squad->GetLeader()->g_Alive()) ||
squad->get_index(this) == u32(-1)))
squad->SetLeader(this);

....
}
....
}

Попередження PVS-Studio: V547 Expression 'squad->get_index(this) == u32(- 1)' is always false. The value range of unsigned char type: [0, 255]. ai_rat.cpp 480

Для того, щоб зрозуміти, чому це вираз завжди помилково, обчислимо значення окремих операндів. u32(-1) дорівнює 0xFFFFFFFF або 4294967295. Тип, що повертається методом squad->get_index (....), — u8, отже його максимальне значення — 0xFF або 255, що строго менше, ніж u32(-1). Відповідно, значенням такого порівняння завжди буде false. Даний код легко виправити, змінивши тип даних на u8:
squad->get_index(this) == u8(-1)

Та ж діагностика спрацьовує і для надлишкових порівнянь беззнакових змінних:
namespace ALife
{
typedef u64 _TIME_ID;
}
ALife::_TIME_ID CScriptActionCondition::m_tLifeTime;

IC bool CScriptEntityAction::CheckIfTimeOver()
{
return((m_tActionCondition.m_tLifeTime >= 0) &&
((m_tActionCondition.m_tStartTime +
m_tActionCondition.m_tLifeTime) < Device.dwTimeGlobal));
}

Попередження PVS-Studio: V547 Expression 'm_tActionCondition.m_tLifeTime >= 0' is always true. Unsigned type value is always >= 0. script_entity_action_inline.h 115

Змінна m_tLifeTime є беззнаковой, відповідно вона завжди більше або дорівнює нулю. Є це зайвої перевіркою або ж тут прихована помилка в логіці, судити розробнику.

Аналогічне попередження:

V547 Expression 'm_tActionCondition.m_tLifeTime < 0' is always false. Unsigned type value is never < 0. script_entity_action_inline.h 143
ObjectFactory::ServerObjectBaseClass *
CObjectItemScript::server_object (LPCSTR section) const
{
ObjectFactory::ServerObjectBaseClass *object = nullptr;

try {
object = m_server_creator(section);
}
catch(std::exception e) {
Msg("Exception [%s] raised while creating server object from "
"section [%s]", e.what(),section);
return (0);
}
....
}

Попередження PVS-Studio: V746 Type slicing. An exception should be caught by reference rather than by value. object_item_script.cpp 39

Функція std::exception::what() є віртуальною і може бути перевизначено у наслідуваних класах. В даному прикладі виняток ловиться за значенням, отже, екземпляр класу буде скопійований і вся інформація про полиморфном типі буде втрачена. Звертатися до what() в такому випадку безглуздо. Виняток варто перехоплювати за посиланням:
catch(const std::exception& e) {

Різне
void compute_cover_value (....)
{
....
float value [8];
....
if (value[0] < .999f) {
value[0] = value[0];
} 
....
}

Попередження PVS-Studio: V570 The 'value[0]' variable is assigned to itself. compiler_cover.cpp 260

Змінна value[0] присвоюється сама собі. Навіщо це робити — незрозуміло. Можливо, їй має бути надано інше значення.
void CActor::g_SetSprintAnimation(u32 mstate_rl,
MotionID &head,
MotionID &torso,
MotionID &legs)
{
SActorSprintState& sprint = m_anims->m_sprint;

bool jump = (mstate_rl&mcFall) ||
(mstate_rl&mcLanding) ||
(mstate_rl&mcLanding) ||
(mstate_rl&mcLanding2) ||
(mstate_rl&mcJump);
....
}

Попередження PVS-Studio: V501 There are identical sub-expressions '(mstate_rl & mcLanding)' to the left and to the right of the '||' operator. actoranimation.cpp 290

Найімовірніше тут просто зайва перевірка mstate_rl & mcLanding, але часто подібні попередження сигналізують про помилку в логіці і нерозглянутих значеннях enum.

Аналогічні попередження:
  • V501 There are identical sub-expressions 'HudItemData()' to the left and to the right of the '&&' operator. huditem.cpp 338
  • V501 There are identical sub-expressions 'list_idx == e_outfit' to the left and to the right of the '||' operator. uimptradewnd_misc.cpp 392
  • V501 There are identical sub-expressions '(D3DFMT_UNKNOWN == fTarget)' to the left and to the right of the '||' operator. hw.cpp 312
RELATION_REGISTRY::RELATION_MAP_SPOTS::RELATION_MAP_SPOTS()
{
....
spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location";
spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location";
....
}

Попередження PVS-Studio: V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 57, 58. relation_registry.cpp 58

Аналізатор виявив, що однією змінною присвоюються поспіль два значення. В даному випадку схоже, що це просто мертвий код і його варто видалити.
void safe_verify (....)
{
....
printf("FATAL ERROR (%s): failed to verify data\n");
....
}

Попередження PVS-Studio: V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 2. Present: 1. entry_point.cpp 41

У функцію printf передається недостатня кількість аргументів: формат '%s' вказує на те, що повинен бути переданий покажчик на рядок. Така ситуація може призвести до помилки доступу до пам'яті і екстреного завершення програми.

Висновок


Перевірка вихідного коду X-Ray Engine виявила велику кількість як зайвого або підозрілого коду, так і однозначно помилкових і небезпечних моментів. Варто відзначити, що статичний аналізатор відмінно допомагає виявляти помилки на ранньому етапі розробки, що значно полегшує життя програмісту і звільняє час на створення нових версій програми.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Pavel Belikov. Anomalies in X-Ray Engine.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.