Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
The X-Ray Engine is a game engine, used in the S.T.A.L.K.E.R. game series. Its code was made public in September 16 2014, and since then, STALKER fans continue its development. A large project size, and a huge number of bugs in the games, gives us a wonderful chance to show what PVS-Studio is capable of.
X-Ray was created by a Ukrainian company, GSC GameWorld, for the game S.T.A.L.K.E.R.: Shadow of Chernobyl. This engine has a renderer supporting DirectX 8.1/9.0c/10/10.1/11, physical and sound engines, multiplayer, and an artificial intelligence system - A-Life. Later, the company was about to create a 2.0 version for their new game, but development was discontinued, and the source code was put out for public access.
This project is easily built with all of its dependencies in Visual Studio 2015. To do the analysis we used the engine source code 1.6v, from a repository on GitHub, and PVS-Studio 6.05 static code analyze, which can be downloaded from this link.
Let's start with errors related to copying code. The way they get to the code is usually the same: the code was copied, parts of variables were changed, and some remained forgotten. Such errors can quickly spread in the code base, and are very easy to overlook without a static code analyzer.
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 warning: V533 It is likely that the wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. mxqmetric.cpp 76
The analyzer detected that in the nested for loop, the variable i gets incremented, but another variable - j gets checked, which leads to an infinite loop. Most likely, a programmer just forgot to change it.
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 warnings:
In this fragment we see several conditional expressions in a row. Obviously, we need to replace the color_base with color_gray and color_add according to the code in the if branch.
/* 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 warning: 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
As in the previous example, two similar conditions are used here (strncmp(buff,"\\getpidr\\",9) == 0). It's hard to say for sure whether this is a mistake, or simply unreachable code, but it's worth revising for sure. Perhaps we should have blocks with getpidr/setpidr by analogy with getpdr/setpdr.
class RGBAMipMappedCubeMap
{
....
size_t height() const
{
return cubeFaces[0].height();
}
size_t width() const
{
return cubeFaces[0].height();
}
....
};PVS-Studio warning: V524 It is odd that the body of 'width' function is fully equivalent to the body of 'height' function. tpixel.h 1090
Methods height() and width() have the same body. Bearing in mind that we evaluate faces of a cube here, perhaps there is no error. But it's better to rewrite the method width() in the following way:
size_t width() const
{
return cubeFaces[0].width();
}C++ is a wonderful language that provides the programmer with many a possibility... to shoot yourself in the foot in a multitude of the cruelest ways. Undefined behavior, memory leaks, and of course, typos. And that's what will be discussed in this section.
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 warning: V591 Non-void function should return a value. _matrix33.h 435
At the end of the method there is no return *this. According to the standard, it will lead to undefined behavior. As the return value is a reference, it will probably lead to a program crash, upon attempting to access the return value.
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 warning: V575 The null pointer is passed into 'fclose' function. Inspect the first argument. ogg_enc.cpp 47
Quite an interesting example. The analyzer detected that the argument in the fclose is nullptr, which makes the function call meaningless. Presumably, the stream in was to be closed.
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 warnings:
In this fragment, we see that logical and bitwise operations get confused. The result will not be what the programmer expected: col will be always 0x01010101 regardless of the input data.
Correct variant:
DWORD a = (col >> 24) & 0x000000FF;
DWORD b = (col >> 16) & 0x000000FF;
DWORD g = (col >> 8) & 0x000000FF;
DWORD r = (col >> 0) & 0x000000FF;Another example of strange code:
VertexCache::VertexCache()
{
VertexCache(16);
}PVS-Studio warning: 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
Instead of calling a constructor from another, a new object of VertexCache gets created, and then destroyed, to initialize the instance. As a result, the members of the created object remain uninitialized.
BOOL CActor::net_Spawn(CSE_Abstract* DC)
{
....
m_States.empty();
....
}PVS-Studio warning: V530 The return value of function 'empty' is required to be utilized. actor_network.cpp 657
The analyzer warns that the value returned by the function is not used. It seems that the programmer confused the methods empty() and clear(): the empty() does not clear the array, but checks whether it is empty or not.
Such errors are quite common in various projects. The thing is that the name empty() is not very obvious: some view it as an action - deletion. To avoid such ambiguity, it's a good idea to add has, or is to the beginning of the method: it would be harder to confuse isEmpty() with clear().
A similar warning:
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 warning: V575 The 'memset' function processes '0' elements. Inspect the third argument. xrdebug.cpp 104
During the memset call the arguments got mixed up, and as a result the buffer isn't set to zero, as it was originally intended. This error can live in a project for quite a long time, because it is very difficult to detect. In such cases a static analyzer is of great help.
The correct use of memset:
memset(buffer, 0, capacity*lineCapacity);The following error is connected with incorrectly formed logical expression.
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 warning: V547 Expression is always true. Probably the '&&' operator should be used here. configs_dumper.cpp 262
The expression x != a || x != b is always true. Most likely, && was meant to be here instead of || operator.
More details on the topic of errors in logical expressions can be found in the article "Logical Expressions in C/C++. Mistakes Made by Professionals".http://www.viva64.com/en/b/0390/
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 warning: V674 The '0.5f' literal of the 'float' type is compared to a value of the 'int' type. boneprotections.cpp 54
The analyzer detected an integer comparison with a real constant. Perhaps, by analogy, the atof function, not atoi was supposed to be here, but in any case, this comparison should be rewritten so that it doesn't look suspicious. However, only the author of this code can say for sure if this code is erroneous or not.
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 warning: V595 The 'object' pointer was utilized before it was verified against nullptr. Check lines: 42, 47. level_bullet_manager_firetrace.cpp 42
The verification of the object pointer against nullptr occurs after the object->ID() is dereferenced. In the case where object is nullptr, the program will crash.
#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 warning: V718 The 'CoInitializeEx' function should not be called from 'DllMain' function. xrcore.cpp 205
In the DllMain, we cannot use a part of WinAPI function, including CoInitializeEx. You may read documentation on MSDN to be clear on this. There is probably no definite answer of how to rewrite this function, but we should understand that this situation is really dangerous, because it can cause thread deadlock, or a program crash.
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 warning: V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwio.c 316
The error is related to increment usage. To make this expression more clear, let's rewrite it, including the brackets:
*(bp++);So we'll have a shift not of the content by bp address, but the pointer itself, which is meaningless in this context. Further on in the code there are fragments of *bp += N type, made me think that this is an error.
Placing parentheses could help to avoid this error and make the evaluation more clear. Also a good practice is to use const for arguments that should not changed.
Similar warnings:
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 warning: V563 It is possible that this 'else' branch must apply to the previous 'if' statement. hit_memory_manager.cpp 368
In this fragment the else branch is related to the second if because of its right-associativity, which doesn't coincide with the code formatting. Fortunately, this doesn't affect the work of the program in any way, but nevertheless, it can make the debugging and testing process much more complicated.
So the recommendation is simple - put curly brackets in more, or less, complex branches.
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 warning: 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
A ternary conditional operator has a lower precedence than the multiplication operator, that's why the order of operations will be as follows:
(hud_snd.m_activeSnd->volume * b_hud_mode)?psHUDSoundVolume:1.0fApparently, the correct code should be the following:
hud_snd.m_activeSnd->volume * (b_hud_mode?psHUDSoundVolume:1.0f)Expressions containing a ternary operator, several if-else branches, or operations AND/OR, are all cases where it's better to put extra brackets.
Similar warnings:
void CDestroyablePhysicsObject::OnChangeVisual()
{
if (m_pPhysicsShell){
if(m_pPhysicsShell)m_pPhysicsShell->Deactivate();
....
}
....
}PVS-Studio warning: V571 Recurring check. The 'if (m_pPhysicsShell)' condition was already verified in line 32. destroyablephysicsobject.cpp 33
In this case m_pPhysicsShell gets checked twice. Most likely, the second check is redundant.
void CSE_ALifeItemPDA::STATE_Read(NET_Packet &tNetPacket,
u16 size)
{
....
if (m_wVersion > 89)
if ( (m_wVersion > 89)&&(m_wVersion < 98) )
{
....
}else{
....
}
}PVS-Studio warning: V571 Recurring check. The 'm_wVersion > 89' condition was already verified in line 987. xrserver_objects_alife_items.cpp 989
This code is very strange. In this fragment we see that a programmer either forgot an expression after if (m_wVersion > 89), or a whole series of else-if. This method requires a more scrutinizing review.
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 warnings:
The check (0 != txt[1]) is excessive in the expressions of initialization of the bDlg and mt variables. If we omit it, the expression will be much easier to read.
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 warning: 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
The value of the expression 1/100 is 0, since it is an operation of integer division. To get the value 0.01f, we need to use a real literal and rewrite the expression: 1/100.0f. Although, there is still a chance that such behavior was meant to be here, and there is no error.
CSpaceRestriction::merge(....) const
{
....
LPSTR S = xr_alloc<char>(acc_length);
for ( ; I != E; ++I)
temp = strconcat(sizeof(S),S,*temp,",",*(*I)->name());
....
}PVS-Studio warning: 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
The function strconcat gets the buffer size as the first parameter. S buffer is declared as a LPSTR, i.e. as a pointer to a string. sizeof(S) will be equal to the pointer size in bites, namely sizeof(char *), not the number of symbols in the string. To evaluate the length we should use 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 warning: V712 Be advised that compiler may delete this cycle, or make it infinite. Use volatile variable(s) or synchronization primitives to avoid this. xrcdb.cpp 100
The compiler can remove the check S_INIT == status as a measure of optimization, because the status variable isn't modified in the loop. To avoid such behavior, we should use volatile variables, or types of data synchronization between the threads.
Similar warnings:
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 warning: V547 Expression 'squad->get_index(this) == u32(- 1)' is always false. The value range of unsigned char type: [0, 255]. ai_rat.cpp 480
To understand why this expression is always false, let's evaluate values of individual operands. u32(-1) is 0xFFFFFFFF or 4294967295. The type, returned by the method squad->get_index(....), is u8, thus its maximum value is 0xFF or 255, which is strictly less than u32(-1). Consequently, the result of such comparison will always be false. This code can be easily fixed, if we change the data type to u8:
squad->get_index(this) == u8(-1)The same diagnostic is triggered for redundant comparisons of unsigned variables.
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 warning: V547 Expression 'm_tActionCondition.m_tLifeTime >= 0' is always true. Unsigned type value is always >= 0. script_entity_action_inline.h 115
The variable m_tLifeTime is unsigned, thus it is always greater than or equal to zero. It's up to the developer to say if it is an excessive check, or an error in the logic of the program.
The same warning:
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 warning: V746 Type slicing. An exception should be caught by reference rather than by value. object_item_script.cpp 39
The function std::exception::what() is virtual, and can be overridden in inherited classes. In this example, the exception is caught by value, hence the class instance will be copied, and all information about the polymorphic type will be lost. Accessing what() is meaningless in this case. The exception should be caught by reference:
catch(const std::exception& e) {void compute_cover_value (....)
{
....
float value [8];
....
if (value[0] < .999f) {
value[0] = value[0];
}
....
}PVS-Studio warning: V570 The 'value[0]' variable is assigned to itself. compiler_cover.cpp 260
The variable value[0] is assigned to itself. It's unclear, why this should be. Perhaps a different value should be assigned to it.
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 warning: V501 There are identical sub-expressions '(mstate_rl & mcLanding)' to the left and to the right of the '||' operator. actoranimation.cpp 290
Most probably, we have an extra check mstate_rl & mcLanding, but quite often such warnings indicate an error in the logic and enum values that weren't considered.
Similar warnings:
RELATION_REGISTRY::RELATION_MAP_SPOTS::RELATION_MAP_SPOTS()
{
....
spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location";
spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location";
....
}PVS-Studio warning: V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 57, 58. relation_registry.cpp 58
The analyzer detected that the same variable is assigned with two values in a row. In this case, it seems that it's just dead code, and it should be removed.
void safe_verify(....)
{
....
printf("FATAL ERROR (%s): failed to verify data\n");
....
}PVS-Studio warning: V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 2. Present: 1. entry_point.cpp 41
An insufficient number or arguments is passed to the printpf function: the format '%s' shows that the pointer to a string should be passed. Such a situation can lead to a memory access error, and to program termination.
The analysis of X-Ray Engine detected a good number of both redundant and suspicious code, as well as erroneous and hazardous moments. It is worth noting that a static analyzer is of great help in detecting errors during the early stages of development, which significantly simplifies the life of a programmer and provides more time for creation of new versions of your applications.
0
0
0
0