Вступление
X-Ray был создан украинской компанией GSC GameWorld для игры S.T.A.L.K.E.R.: Тень Чернобыля. Движок включает рендер с поддержкой DirectX 8.1/9.0c/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::Sect &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.5f);
....
}
}Предупреждение PVS-Studio: V674 The '0.5f' 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.0f);
}Предупреждение PVS-Studio: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '*' operator. hudsound.cpp 108
У тернарного условного оператора приоритет ниже, чем у умножения, поэтому порядок операций будет следующим:
(hud_snd.m_activeSnd->volume * b_hud_mode)?psHUDSoundVolume:1.0fОчевидно, что правильный код должен выглядеть так:
hud_snd.m_activeSnd->volume * (b_hud_mode?psHUDSoundVolume:1.0f)Выражения, содержащие тернарный оператор, несколько if-else ветвей или операции И/ИЛИ, — это те случаи, когда лучше поставить лишние скобки.
Аналогичные предупреждения:
- V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. uihudstateswnd.cpp 487
- V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' 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.01f, нужно использовать вещественный литерал, переписав выражение: 1/100.0f. Хотя возможно, что данное поведение было предусмотрено автором, и ошибки здесь нет.
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.
Комментарии (31)
hdfan2
20.06.2016 15:58+18— Папа, а правда, что рядом с нами когда-то была АЭС?
— Правда, сынок, — сказал папа и погладил сына по голове.
— А правда, что, когда для неё писали софт, то в одном месте забыли проверить указатель перед разыменованием?
— Правда, сынок, — сказал папа и погладил сына по второй голове.
elmm
20.06.2016 16:47+1Вот бы бесезда бы свои движки гоняла через анализаторы. По ощущением, багов там должно быть заметно больше.
IgeNiaI
20.06.2016 17:28-2Так они еще и движок сами писали. То-то они сделали на порядок меньше, чем обещали, да и то, что вышло, было малоиграбельно.
Кстати, а почему они решили все сами делать? Я могу понять серьезные компании, выпустившие не одну игру, или просто с кучей денег, но здесь ни того, ни другого.
bano-notit
20.06.2016 17:43+4Они написали меньше потому, что компания, которая занималась продвижением игры в массы и подготовкой масс к игре сказала, что, например, машины никому не нужны, изломы тоже подождут, локация «генераторы» никому не будет интересной, так же как и город. Просто этой компании нужно было выпустить игру поскорее, чтобы срубить бабла побыстрее. Поэтому в игре отсутствуют некоторые мобы, возможности и много чего интересного. Этим же сокращением сроков можно объяснить и баги с ошибки в коде, который кстати в самом сталкере может и не использоваться, а был только как заготовка для продажи движка другим компаниям.
А на счёт того, что хотели свой движок… Ну просто захотелось выпендрится. И кстати на тот момент у них получилось, потому что их движок поддерживал последний DirectX, а другие забугорские движки нет.
И всё же жаль, что заморозили S.T.A.L.K.E.R. 2. До сих пор жду…IgeNiaI
21.06.2016 02:44Они делали игру 6 лет. Думаю, если б не делали свой движок, успели бы доделать мобов и локации, да багов было бы меньше.
bano-notit
21.06.2016 13:29Но при этом они сделали так как хотели, потому что движок был сделан именно для этой игры. Брали бы они другой движок, он был бы тяжелее в коде, не соответствовал бы «изыскам» и вносить туда «изыски» было бы труднее. А так у них есть свой продукт, в котором они как рыба в воде и доделывать там могли что захотят.
Приведу пример. Вот у вас есть дом. Вы можете делать там всё что захотите, хоть бензопилой стены рубать. А если вы снимаете дом, то Вы там не хозяин, вы там только жилец, и если там будут какие-то изменения, то вам надо будет их согласовывать с владельцем. Да и на крайней случай владелец в крайний момент может Вас тупо выгнать, потому что Вы ему не нравитесь или потому что его левая нога так захотела.
Для них иметь свой движок было необходимостью. Они его сделали.
И знаете, если бы не было той компании, то они бы успели в срок со всеми мобами, но компании не понравились все фишки, по этому большую часть времени разрабы занимались вырезательстом и склеиванием кусков.
Tujh
21.06.2016 14:54Первоначально у них уже был Vital engine для игры Venom, собственной же разработки. Так что «сталкеровский» движок создавался не «с нуля», да и команда имела опыт. Более вероятно выглядит то, что задержка в выпуске возникла из-за того, что изначально была сделана ставка на DX8, при том, что на дворе уже не то, что маячили, а были готовы игры на DX9, и последующее переписывание на новый рендер с добавление плюшек, шейдеров и всего остального, а так как переписывание шло параллельно с изучением, вот и получилось, что получилось.
DeLuxis
21.06.2016 06:35+5Выпендриться?!
В то время еще не было такого разнообразия движков и компании вынуждены были держать на затворках свои разработки.
Это сейчас есть 100500 движков на любой цвет и вкус, а тогда такого разнообразия не было.
Идея игры кстати появилась во время работ и экспериментов над ранними версии движка, который кстати создавался силами одного человека ради фана.
Error1024
20.06.2016 20:04+3А много вы знаете движков с открытым миром и динамическим освещением доступные на момент начала разработки игры?
Да и сейчас динамическое освещение в играх большая редкость.
Лично мне графика сталкера кажеться более живой именно из-за освещения и в целом того как X-Ray рендерит.
Современные игры не смотря на супер детализацию графики выглядят для меня пластмассовыми, скорее всего из-за «мертвого», намертво запечённого в Ligth map-ах света.IgeNiaI
21.06.2016 02:51+2На тот момент Valve уже доделали Source и представили его публике, взявшись за работу над Half-Life 2. И там уже была нормальная поддержка больших расстояний и физика была лучше, чем у некоторых современных движков. По мне так явно лучше таковой в сталкере.
За то время, пока они делали игру, NCsoft успели сделать Lineage 2, на старом Unreal Engine, с динамическим освещением и куда большим миром. К моменту выхода сталкера мир вырос еще в пару раз.
За это же время Epic Games выпустили UT2003 и UT2004, опять же, с хорошей физикой, отличным динамическим освещением, наличием транспорта, в том числе и летающего, а также наличием больших карт, да и оптимизация лучше, чем в Source и гораздо лучше, чем в X-Ray
nitrocaster
21.06.2016 11:05В современных играх как раз наблюдается тенденция отказа от лайтмапов в пользу полностью динамического освещения, поскольку железо становится быстрее и считать на нем можно все больше. Графика в сталкере кажется живой во многом благодаря лайтмапам.
mamkaololosha
20.06.2016 20:12+1Игра вышла 9 лет назад. Д.Е.В.Я.Т.Ь.! Это у тебя такой консерватизм-дибилизм?
> а почему они решили все сами делать
В свой движок новая фича добавляется за месяц. В чужой за полгода или релиз-патч. Да, они могли купить Q3, но большинство фичей им бы пришлось и так и так добавлять самим. Темболее на 2005й год (когда разработка ждалкера была закончена и программисты ушли в 4а геймс) — ни у кого кроме валв-рокстар и прочих топов, не было такого тека, вообще ниукого.
Sirikid
20.06.2016 21:31Так и не прошел ЧН из-за бага, Квестовый NPC просто не реагировал на меня (а может это было из-за кряка }:) )
YaakovTooth
20.06.2016 22:34+1Нет, там после релиза патчи выходили чаще, чем я себе кофе варю. С последним патчем всё проходится отлично.
IgeNiaI
21.06.2016 02:55+1Проходится, но не отлично. Месяц назад перепрошел на патче 1.0006. Баг на баге, часть квестовых маркеров просто не исчезала после выполнения квеста, другая часть была за пределами карт, еще у части квестов просто не срабатывали триггеры и они так и остались висеть в списке. К счастью, хоть главный квест не сломался.
Tujh
21.06.2016 13:08Ни чего подобного. У меня на патче 1.0003 игра проходилась, а на 1.0004 и позднее просто вылетала в произвольные моменты на Янтаре (после того, как проводишь учёного выжившего в крушении вертолёта) и отказывалась загружать сохранённые игры на этой карте.
Sirikid
21.06.2016 18:06Это как раз на Янтаре было, когда со сталкерами надо было идти куда-то вглубь
Grief
21.06.2016 06:00+2Здравствуйте, я — программист, но не пишу на плюсах профессионально, предпочитая для зарабатывания денег языки более «пластмассовые», навроде java и python, где цена ошибки не так высока, дебаг прост, а стектрейсы из логов и сообщения об ошибках практически всегда имеют смысл, а не представляют из себя «ошибка по адресу-хекс-код, память cannot be read», тем не менее, иногда пописываю на плюсах «для души», для общего развития так сказать. И вот вопрос у меня есть, есть отличный компилятор GCC, есть Intel C++ Compiler, есть MSVC++ (или как он там называется правильно), и в каждом из них есть некая подсистема, которая напоминает модуль статического анализа. Я говорю о предупреждениях: к примеру, отсутствующий возврат из не-void функции в GCC будет обнаружен при включенном -Werror=return-type. Повторюсь, я не специалист по плюсам, но пройдемся по статье:
V533 (инкремент «не той» переменной в for-цикле) — найти for-циклы в AST, проверить на пересечение множества перменных из условия и действия. Я из головы пишу сейчас — естественно, поразмыслив, можно усложнить проверку, например оценить характер изменения переменных и «направлений» условий, то есть, скажем, заставить i++ требовать условия i < X, а для i > Y — выбрасывать ошибку, однако, это не rocket-science. Да, работа, да, возможно, сложная, но не матан.
V581 — вообще смешно — сравнить на равество условия последовательных ифов. Вы ведь тоже не можете оценить значение всех сайд-эффектов? Думается мне, это в статике в плюсах вообще невозможно в общем случае, это под силу только динамическим анализаторам, ну может только в каких-то частных случаях если — статически.
V517 — то же самое.
V524 — то же самое. Тело функции, конечно, как правило, длиннее условия, но когда оперируешь AST, а не строками текста, отступы, форматирование и прочее — не играют роли, препроцессор уже прошел, предоптимизация выкинула очевидный «мертвый» код, все < и > «повернуты» в одну сторону и т.д. — красота!
V591 — как я уже говорил, в GCC это работает из коробки как минимум.
V575 — …
Вообще и так длинно получилось, суть, я думаю, и так ясна.
Конечно, есть и сложные для обнаружения проблемы, но в статье не описана ни одна из них, хотя, быть может, я и недооцениваю. Хм, скажем так, по сравнению с логикой самого GCC в статье я не нашел ни одной хоть сколько бы то ни было алгоритмически сложной в поиске ошибки.
И вот, собственно, вопрос, почему ребята, которые пишут GCC не добавили все это внутрь их компилятора? Или майки в свой? Ну ладно, первые — волонтеры, вторые — бюрократы, но как насчет интеловцев? Они-то знают толк? Или нет?
Я вижу здесь два варианта:
1. Добавление всех этих проверок в компилятор ни сколько не увеличит количество проблем и качество работы программ, написанных на плюсах. Улучшит качество кода? Да! Но очень плохо написанный код может работать очень хорошо, а в очень красивом, как дипломная работа студента, коде — быть скрытая ошибка. Вы много продуктов проверяли и многие из них работают хорошо. Относительно — хорошо. В этом мире все относительно. По крайней мере я не встречал идеального кода еще в своей жизни. И идеальных программ тоже. Я не часто лазаю по чужому коду, но вот хотел в tmux добавить true color когда-то, но так и не осилил, там просто ад внутри, копипаста, какие-то хаки под разные платформы, наслоения функциональности, и это — чистый си. А программка-то работает как часы, да и true color к ней кто-то прикрутил и без моего участия.
2. Я что-то не понимаю в этом мире :)Sirikid
21.06.2016 06:44+1Ричи и Страуструп доверяли своим пользователям и давали эффективный инструмент, думаю отчасти поэтому первые компиляторы не делали анализа кода. Никогда не смотрел с такой стороны, но имхо естественно что компилятор компилирует, анализатор анализирует а 0 можно разыменовать.
Andrey2008
21.06.2016 10:56+1Вообще и так длинно получилось, суть, я думаю, и так ясна.
Если честно, то нет. Я не понял этот большой комментарий. Поэтому возможно отвечу мимо.
Инструменты статического анализа всегда будут лучше, чем компиляторы. В этом смысл их существования. Условный пример: Paint и Photoshop. Photoshop будет существовать до тех пор, пока он позволяет круче редактировать картинки, чем Paint. И за это он собственно берёт деньги.
Нет, не все так просто как кажется. Анализатор как раз пытается учесть различные сайд-эффекты, иначе он будет выдавать слишком много ложных срабатываний. Вообще ценность анализатора не в том, чтобы выдать больше сообщений, а в том, чтобы сообразить, когда следует промолчать.
Почему это не добавляют в GCC и иные компиляторы? На самом деле добавляют, но не спеша. Ценность статических анализаторов в том, что они бегут впереди компиляторов. Плюс далеко не все диагностики полезно делать в компиляторе. Ведь компилятор решает и другие задачи. Например, компилятор должен быстро работать. А многие диагностики как раз существенно замедляют работу. И если потерпеть и подождать статический анализатор (которые, например, работает ночью) можно, то для компилятора такое замедление недопустимо.
EvgeniyRyzhkov
21.06.2016 11:20Главная задача компилятора — проверить минимально код на отсутствие самых «тупых» ошибок и максимально быстро сгенерировать код. Эта задача противоречит задаче «сделать максимально глубокий анализ и найти все возможные проблемы».
Аргумент о том, что «вроде не сложно сделать анализатор — чего ж они не встроят в компилятор» не выдерживает критики. Со стороны чужая работа всегда кажется проще, чем она видна изнутри.
Так, например, в анализаторе люди думать, что самое сложное — это найти и выдать ошибку. Это не так. Самое сложное «не выдать ложное срабатывание». Поэтому в любой диагностике несколько десятков исключений, когда диагностике срабатывать не надо. Могут ли разработчики компилятора делать эту работу? Могут. Но у них и своей хватает.
juwagn
21.06.2016 06:01Вот бы на Java такой анализатор, ещё и на JavaScript, купил бы, очень была бы нужная вещь.
bano-notit
21.06.2016 13:35JSLint Вам в помощь, JSHint, если хотите, чтобы ваш код могли прочитать другие люди.
inoyakaigor
Какая стыдоба!
alexander007
«А и х** с ним, прибыток сбросил и нормально»
mamkaololosha
Указанный код вызывается 10%… 0% всего игрового времени. Не удивительно что в него часто не залезали и часто не тестировали.
FoxCanFly
10%… 0%? Особенно умножение матрицы на вектор, ага. Да и среди других примеров встречаются явно часто используемые функции.
mamkaololosha
Нуйптваюмать, ага-ага, ну на смотри:
https://github.com/OpenXRay/xray-16/search?utf8=%E2%9C%93&q=homogeneous
https://github.com/OpenXRay/xray-16/search?utf8=%E2%9C%93&q=settings_read
RGBAMipMappedCubeMap — если там куб или квадрат, то пофиг
https://github.com/OpenXRay/xray-16/search?utf8=%E2%9C%93&q=sMTxV
https://github.com/OpenXRay/xray-16/search?utf8=?&q=ogg_enc
https://github.com/OpenXRay/xray-16/search?utf8=%E2%9C%93&q=ABGR8_To_ARGB8
https://github.com/OpenXRay/xray-16/search?utf8=?&q=dumper_thread
https://github.com/OpenXRay/xray-16/search?utf8=%E2%9C%93&q=compute_cover_value
Andrey2008
Это логично, что ошибки в основном находятся в редкоиспользумом/неиспользуемом коде. В часто используемых они находятся ночными отладками, воплями руководства и вставления перьев программистам в одно место. А при регулярном использовании статического анализа могли бы находиться сразу после внесения их в код. Не все, конечно, но большое количество. Впрочем, это не интересно и скучно. Да здравствует кофе, ночные отладки и героические победы над багами! :)