• Anomalies in X-Ray Engine

General and Gameplay Programming

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.

Introduction

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.

Copy-paste

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 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: 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 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(); } Improper use of C++ 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 struct _matrix33 { public: typedef _matrix33Self; 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: 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 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". 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_distflags.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. Precedence errors 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: 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 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.0f Apparently, 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: V502 Perhaps the '?:' operator works in a different way than was expected. The '?:' operator has a lower priority than the '+' operator. uihudstateswnd.cpp 487 V502 Perhaps the '?:' operator works in a different way than was expected. The '?:' operator has a lower priority than the '+' operator. uicellcustomitems.cpp 106 Unnecessary comparisons 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: 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 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; Errors in data types 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(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: V712 Be advised that compiler may delete this cycle or make it infinite. 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 infinite. 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 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) { Miscelleneous 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: 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 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. Conclusion 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. By Pavel Belikov
 
 0   Report Article 
 Sign in to follow this   Followers 0 Go to articles General and Gameplay Programming 
 User Feedback 3 Comments 0 Reviews FRex 1781 Posted January 23, 2017 These articles make me wonder how anything works in games at all. Glitches from stuff like the && in colors and so on is understandable. But dereferencing a nullptr. WTF. Share this comment Link to comment Share on other sites tyhender 168 Posted January 25, 2017 These articles make me wonder how anything works in games at all. Glitches from stuff like the && in colors and so on is understandable. But dereferencing a nullptr. WTF. Haha. Even that a lot of errors weren't fixed before S.T.A.L.K.E.R's release, gmae still worked good.  The another thing, is that those errors were made by a big(then) game company, imaginee how much bugs are there in Engines made by indie companies/one man. Share this comment Link to comment Share on other sites TheChubu 9480 Posted January 28, 2017 "Be advised that this project is not sanctioned by GSC Game World in any way – and they remain the copyright holders of all the original source code."   lmao, so they just put the code with a BSD licence and hoped for the best. GSC doesn't exists anymore, who knows who can green light it... Share this comment Link to comment Share on other sites Create an account or sign in to comment You need to be a member in order to leave a comment Create an account Sign up for a new account in our community. It's easy! Register a new account Sign in Already have an account? Sign in here. Sign In Now 
•  
 
 Advertisement 
 Advertisement Latest Featured Articles 0 Effect: Area Light Shadows Part 1: PCSS By Vilem Otte Monday at 01:59 AM 0 Color Palettes By gdarchive September 10 1 Ray Tracing - Part 1 By Bacterius August 8 1 Contact-hardening Soft Shadows Made Fast By maxest September 2 3 A Brief Introduction to Lerp By mattdesl August 21 Featured Blogs Summer Baseball Challenge 2018 - Part 2 (Game Graphics) By Rutin in Rutin's Dev Blog    2 Warfront Infinite Dev Blog #18: Decals, New Camera System and Turret Models By EddieK in Warfront Infinite Dev Blog    4 Leadwerks Software to Provide VR Services to NASA By Josh Klint in Leadwerks Developer Blog    4 cross platform development: IDE or do it yourself By GamerX1221 in GamerX1221's Blog    0 Update 1.2 Submitted to Apple – In memory of Sarah Curtis By SOS-CC in SOS-CC Mobile Game Development    8 Popular Now 9 Quaternion, why divide angle by 2? By mathematicalStarted Tuesday at 06:03 AM 33 Semi-complete Newbie By Ryan WoodStarted Tuesday at 12:43 AM 13 why we use matrix and only multiplication as operator in transforms? By moeen kStarted Monday at 12:11 PM 13 How to manage inventory system if there are too many classes? By MarkNefedovStarted Monday at 11:37 AM 10 Optimization Newbie: What program language should i learn? By GerhartStarted Monday at 02:49 AM Advertisement GameDev.net GameDev.net Articles GameDev.net Event Coverage GameDev.net Forums GameDev.net Blogs GameDev.net Gallery GameDev.net News GameDev.net Projects GDNet Chat All Activity Search In Everywhere This Category This Article More options... Find results that contain... All of my search term words Any of my search term words Find results in... Content titles and body Content titles only Home Articles Programming General and Gameplay Programming Anomalies in X-Ray Engine 
 
 
 × Existing user? Sign In Sign Up Browse Back Articles & Tutorials Back All Categories Audio Business Game Design Industry Programming Visual Arts Columns Back GameDev Unboxed Event Coverage Back All Events Game Developers Conference Power Up Digital Games Conference GameDev.Market Links News Podcasts Back All Podcasts Game Dev Loadout Archive Community Back Beginners Back Beginners Group Beginners Forum Beginners Resources Blogs Calendar Chat Forums Back All Forums Audio Business Game Design Programming Visual Arts Community GameDev Challenges Affiliates Topical Workshops Gallery Groups Back For Beginners GameDev Challenges All Groups Projects Back All Projects Games Game Assets Game Mods Developer Tools Store Forums Back All Forums For Beginners Audio Back Music and Sound FX Games Career Development Business Back Games Career Development Production and Management Games Business and Law Game Design Back Game Design and Theory Writing for Games Programming Back Artificial Intelligence Engines and Middleware General and Gameplay Programming Graphics and GPU Programming Math and Physics Networking and Multiplayer Visual Arts Back 2D and 3D Art Critique and Feedback Community Back GameDev Challenges GDNet Lounge GDNet Comments, Suggestions, and Ideas Coding Horrors Your Announcements Hobby Project Classifieds Indie Showcase Affiliates Back NeHe Productions AngelCode Topical Workshops Careers Back Contractors Hobby Projects Game Jobs Back Browse on GameDev.Jobs Post a Job Members Back Subscriptions Chat Guidelines Leaderboard Online Users Awards Search Back All Activity My Activity Streams Back Latest Topics Featured Blogs Search var ipsDebug=false;var CKEDITOR_BASEPATH='//www.gamedev.net/applications/core/interface/ckeditor/ckeditor/';var ipsSettings={cookie_path:"/",cookie_prefix:"ips4_",cookie_ssl:true,upload_imgURL:"",message_imgURL:"",notification_imgURL:"",baseURL:"//www.gamedev.net/",jsURL:"//www.gamedev.net/applications/core/interface/js/js.php",csrfKey:"b7a6119e2379b54a23d802372922f1b7",antiCache:"14de000c45",disableNotificationSounds:false,useCompiledFiles:true,links_external:true,memberID:0,analyticsProvider:"ga",viewProfiles:true,mapProvider:'google',mapApiKey:"AIzaSyAeT7tk3vnWWmbgVISkLpbhkQvekG19rHM",}; ips.setSetting('date_format',jQuery.parseJSON('"mm\/dd\/yy"'));ips.setSetting('date_first_day',jQuery.parseJSON('0'));ips.setSetting('remote_image_proxy',jQuery.parseJSON('1'));ips.setSetting('ipb_url_filter_option',jQuery.parseJSON('"none"'));ips.setSetting('url_filter_any_action',jQuery.parseJSON('"allow"'));ips.setSetting('bypass_profanity',jQuery.parseJSON('0'));ips.setSetting('emoji_style',jQuery.parseJSON('"native"'));ips.setSetting('emoji_shortcodes',jQuery.parseJSON('"1"'));ips.setSetting('emoji_ascii',jQuery.parseJSON('"1"'));ips.setSetting('emoji_cache',jQuery.parseJSON('"1"'));ips.setSetting('quickSearchDefault',jQuery.parseJSON('"all"'));ips.setSetting('quickSearchMinimum',jQuery.parseJSON('3'));ips.setSetting('quickSearchShowAdv',jQuery.parseJSON('true'));ips.setSetting('quickSearchIn',jQuery.parseJSON('"title"')); { "@context": "http://schema.org", "@type": "Article", "url": "https://www.gamedev.net/articles/programming/general-and-gameplay-programming/anomalies-in-x-ray-engine-r4440/", "discussionUrl": "https://www.gamedev.net/articles/programming/general-and-gameplay-programming/anomalies-in-x-ray-engine-r4440/", "mainEntityOfPage": "https://www.gamedev.net/articles/programming/general-and-gameplay-programming/anomalies-in-x-ray-engine-r4440/", "name": "Anomalies in X-Ray Engine", "headline": "Anomalies in X-Ray Engine", "text": "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. \n \n\n\nIntroduction\n\nX-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. \n\nThis 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.\n\nCopy-paste\n\nLet\u0027s 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. \n\n\n\nMxMatrix\u0026amp; MxQuadric::homogeneous(MxMatrix\u0026amp; H) const\n{\n ....\n unsigned int i, j;\n\n for(i=0; iPVS-Studio warning: V533 It is likely that the wrong variable is being incremented inside the \u0027for\u0027 operator. Consider reviewing \u0027i\u0027. mxqmetric.cpp 76\n\nThe 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. \n\nvoid CBaseMonster::settings_read(CInifile const * ini,\n LPCSTR section, \n SMonsterSettings \u0026amp;data)\n{\n ....\n if (ini-\u0026gt;line_exist(ppi_section,\"color_base\"))\n sscanf(ini-\u0026gt;r_string(ppi_section,\"color_base\"), \"%f,%f,%f\", \n \u0026amp;data.m_attack_effector.ppi.color_base.r, \n \u0026amp;data.m_attack_effector.ppi.color_base.g, \n \u0026amp;data.m_attack_effector.ppi.color_base.b); \n if (ini-\u0026gt;line_exist(ppi_section,\"color_base\"))\n sscanf(ini-\u0026gt;r_string(ppi_section,\"color_gray\"), \"%f,%f,%f\", \n \u0026amp;data.m_attack_effector.ppi.color_gray.r, \n \u0026amp;data.m_attack_effector.ppi.color_gray.g, \n \u0026amp;data.m_attack_effector.ppi.color_gray.b);\n if (ini-\u0026gt;line_exist(ppi_section,\"color_base\"))\n sscanf(ini-\u0026gt;r_string(ppi_section,\"color_add\"), \"%f,%f,%f\", \n \u0026amp;data.m_attack_effector.ppi.color_add.r, \n \u0026amp;data.m_attack_effector.ppi.color_add.g, \n \u0026amp;data.m_attack_effector.ppi.color_add.b);\n ....\n}\n\nPVS-Studio warnings:\n\n\nV581 The conditional expressions of the \u0027if\u0027 operators situated alongside each other are identical. Check lines: 445, 447. base_monster_startup.cpp 447 \n \n \nV581 The conditional expressions of the \u0027if\u0027 operators situated alongside each other are identical. Check lines: 447, 449. base_monster_startup.cpp 449 \n \n\nIn 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. \n\n/* process a single statement */\nstatic void ProcessStatement(char *buff, int len)\n{\n ....\n if (strncmp(buff,\"\\\\pauthr\\\\\",8) == 0)\n {\n ProcessPlayerAuth(buff, len);\n } else if (strncmp(buff,\"\\\\getpidr\\\\\",9) == 0)\n {\n ProcessGetPid(buff, len);\n } else if (strncmp(buff,\"\\\\getpidr\\\\\",9) == 0)\n {\n ProcessGetPid(buff, len);\n } else if (strncmp(buff,\"\\\\getpdr\\\\\",8) == 0)\n {\n ProcessGetData(buff, len);\n } else if (strncmp(buff,\"\\\\setpdr\\\\\",8) == 0)\n {\n ProcessSetData(buff, len);\n } \n}\n\nPVS-Studio warning: V517 The use of \u0027if (A) {...} else if (A) {...}\u0027 pattern was detected. There is a probability of logical error presence. Check lines: 1502, 1505. gstats.c 1502\n\nAs in the previous example, two similar conditions are used here (strncmp(buff,\"\\\\getpidr\\\\\",9) == 0). It\u0027s hard to say for sure whether this is a mistake, or simply unreachable code, but it\u0027s worth revising for sure. Perhaps we should have blocks with getpidr/setpidr by analogy with getpdr/setpdr.\n\n\nclass RGBAMipMappedCubeMap\n{\n ....\n size_t height() const\n {\n return cubeFaces[0].height();\n }\n\n size_t width() const\n {\n return cubeFaces[0].height();\n }\n ....\n};\n\nPVS-Studio warning: V524 It is odd that the body of \u0027width\u0027 function is fully equivalent to the body of \u0027height\u0027 function. tpixel.h 1090\n\nMethods 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\u0027s better to rewrite the method width() in the following way:\n\nsize_t width() const\n{\n return cubeFaces[0].width();\n}\n\nImproper use of C++\n\nC++ 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\u0027s what will be discussed in this section. \n\n\n\ntemplate \nstruct _matrix33\n{\npublic:\n typedef _matrix33Self;\n typedef Self\u0026amp; SelfRef;\n ....\n IC SelfRef sMTxV(Tvector\u0026amp; R, float s1, const Tvector\u0026amp; V1) const\n {\n R.x = s1*(m[0][0] * V1.x + m[1][0] * V1.y + m[2][0] * V1.z);\n R.y = s1*(m[0][1] * V1.x + m[1][1] * V1.y + m[2][1] * V1.z);\n R.z = s1*(m[0][2] * V1.x + m[1][2] * V1.y + m[2][2] * V1.z);\n }\n ....\n}\n\n\nPVS-Studio warning: V591 Non-void function should return a value. _matrix33.h 435\n\nAt 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. \n\nETOOLS_API int __stdcall ogg_enc(....)\n{\n ....\n FILE *in, *out = NULL;\n ....\n input_format *format;\n ....\n in = fopen(in_fn, \"rb\");\n\n if(in == NULL) return 0;\n\n format = open_audio_file(in, \u0026amp;enc_opts);\n if(!format){\n fclose(in);\n return 0;\n };\n\n out = fopen(out_fn, \"wb\");\n if(out == NULL){\n fclose(out);\n return 0;\n } \n ....\n}\n\nPVS-Studio warning: V575 The null pointer is passed into \u0027fclose\u0027 function. Inspect the first argument. ogg_enc.cpp 47\n\nQuite 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. \n\nvoid NVI_Image::ABGR8_To_ARGB8()\n{\n // swaps RGB for all pixels\n assert(IsDataValid());\n assert(GetBytesPerPixel() == 4);\n UINT hxw = GetNumPixels();\n for (UINT i = 0; i \u0026gt; 24) \u0026amp;\u0026amp; 0x000000FF;\n DWORD b = (col \u0026gt;\u0026gt; 16) \u0026amp;\u0026amp; 0x000000FF;\n DWORD g = (col \u0026gt;\u0026gt; 8) \u0026amp;\u0026amp; 0x000000FF;\n DWORD r = (col \u0026gt;\u0026gt; 0) \u0026amp;\u0026amp; 0x000000FF;\n col = (a \n\nPVS-Studio warnings:\n\n\n\nV560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 170 \n \nV560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 171 \n \nV560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 172 \n \nV560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 173 \n\nIn 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. \n\nCorrect variant:\n\nDWORD a = (col \u0026gt;\u0026gt; 24) \u0026amp; 0x000000FF;\nDWORD b = (col \u0026gt;\u0026gt; 16) \u0026amp; 0x000000FF;\nDWORD g = (col \u0026gt;\u0026gt; 8) \u0026amp; 0x000000FF;\nDWORD r = (col \u0026gt;\u0026gt; 0) \u0026amp; 0x000000FF;\n\n\t\n\nAnother example of strange code:\n\nVertexCache::VertexCache()\n{\n VertexCache(16);\n}\n\nPVS-Studio warning: V603 The object was created but it is not being used. If you wish to call constructor, \u0027this-\u0026gt;VertexCache::VertexCache(....)\u0027 should be used. vertexcache.cpp 6\n\nInstead 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. \n\nBOOL CActor::net_Spawn(CSE_Abstract* DC)\n{\n ....\n m_States.empty();\n ....\n}\n\t\nPVS-Studio warning: V530 The return value of function \u0027empty\u0027 is required to be utilized. actor_network.cpp 657\n\nThe 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.\n\nSuch 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\u0027s a good idea to add has, or is to the beginning of the method: it would be harder to confuse isEmpty() with clear(). \n\nA similar warning:\n\nV530 The return value of function \u0027unique\u0027 is required to be utilized. uidragdroplistex.cpp 780\n\nsize_t xrDebug::BuildStackTrace(EXCEPTION_POINTERS* exPtrs,\n char *buffer,\n size_t capacity,\n size_t lineCapacity)\n{\n memset(buffer, capacity*lineCapacity, 0);\n ....\n}\n\nPVS-Studio warning: V575 The \u0027memset\u0027 function processes \u00270\u0027 elements. Inspect the third argument. xrdebug.cpp 104\n\nDuring the memset call the arguments got mixed up, and as a result the buffer isn\u0027t 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. \n\nThe correct use of memset:\n\nmemset(buffer, 0, capacity*lineCapacity);\n\nThe following error is connected with incorrectly formed logical expression. \n\nvoid configs_dumper::dumper_thread(void* my_ptr)\n{\n ....\n DWORD wait_result = WaitForSingleObject(\n this_ptr-\u0026gt;m_make_start_event, INFINITE);\n while ( wait_result != WAIT_ABANDONED) ||\n (wait_result != WAIT_FAILED))\n ....\n}\n\nPVS-Studio warning: V547 Expression is always true. Probably the \u0027\u0026amp;\u0026amp;\u0027 operator should be used here. configs_dumper.cpp 262\n\nThe expression \u0027x != a || x != b\u0027 is always true. Most likely, \u0026amp;\u0026amp; was meant to be here instead of || operator. \n\nMore details on the topic of errors in logical expressions can be found in the article \"Logical Expressions in C/C++. Mistakes Made by Professionals\". \n\nvoid SBoneProtections::reload(const shared_str\u0026amp; bone_sect, \n IKinematics* kinematics)\n{\n ....\n CInifile::Sect \u0026amp;protections = pSettings-\u0026gt;r_section(bone_sect);\n for (CInifile::SectCIt i=protections.Data.begin();\n protections.Data.end() != i; ++i) \n {\n string256 buffer;\n BoneProtection BP;\n ....\n BP.BonePassBullet = (BOOL) (\n atoi( _GetItem(i-\u0026gt;second.c_str(), 2, buffer) )\u0026gt;0.5f);\n ....\n }\n}\n\nPVS-Studio warning: V674 The \u00270.5f\u0027 literal of the \u0027float\u0027 type is compared to a value of the \u0027int\u0027 type. boneprotections.cpp 54\n\nThe 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\u0027t look suspicious. However, only the author of this code can say for sure if this code is erroneous or not. \n\nclass IGameObject :\n public virtual IFactoryObject,\n public virtual ISpatial,\n public virtual ISheduled,\n public virtual IRenderable,\n public virtual ICollidable\n{\npublic:\n ....\n virtual u16 ID() const = 0;\n ....\n}\n\nBOOL CBulletManager::test_callback(\n const collide::ray_defs\u0026amp; rd,\n IGameObject* object,\n LPVOID params)\n{\n bullet_test_callback_data* pData = \n (bullet_test_callback_data*)params;\n SBullet* bullet = pData-\u0026gt;pBullet;\n\n if( (object-\u0026gt;ID() == bullet-\u0026gt;parent_id) \u0026amp;\u0026amp; \n (bullet-\u0026gt;fly_distflags.ricochet_was)) return FALSE;\n\n BOOL bRes = TRUE;\n if (object){\n ....\n }\n \n return bRes;\n}\n\nPVS-Studio warning: V595 The \u0027object\u0027 pointer was utilized before it was verified against nullptr. Check lines: 42, 47. level_bullet_manager_firetrace.cpp 42\n\nThe verification of the object pointer against nullptr occurs after the object-\u0026gt;ID() is dereferenced. In the case where object is nullptr, the program will crash. \n\n#ifdef _EDITOR\nBOOL WINAPI DllEntryPoint(....)\n#else\nBOOL WINAPI DllMain(....)\n#endif\n{\n switch (ul_reason_for_call)\n {\n ....\n case DLL_THREAD_ATTACH:\n if (!strstr(GetCommandLine(), \"-editor\"))\n CoInitializeEx(NULL, COINIT_MULTITHREADED);\n timeBeginPeriod(1);\n break;\n ....\n }\n return TRUE;\n}\n\nPVS-Studio warning: V718 The \u0027CoInitializeEx\u0027 function should not be called from \u0027DllMain\u0027 function. xrcore.cpp 205\n\nIn 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. \n\nPrecedence errors \n\nint sgetI1( unsigned char **bp )\n{\n int i;\n\n if ( flen == FLEN_ERROR ) return 0;\n i = **bp;\n if ( i \u0026gt; 127 ) i -= 256;\n flen += 1;\n *bp++;\n return i;\n}\n\nPVS-Studio warning: V532 Consider inspecting the statement of \u0027*pointer++\u0027 pattern. Probably meant: \u0027(*pointer)++\u0027. lwio.c 316\nThe error is related to increment usage. To make this expression more clear, let\u0027s rewrite it, including the brackets: \n\n*(bp++);\n\nSo we\u0027ll 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. \n\nPlacing 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. \n\nSimilar warnings:\n\n\nV532 Consider inspecting the statement of \u0027*pointer++\u0027 pattern. Probably meant: \u0027(*pointer)++\u0027. lwio.c 354 \n V532 Consider inspecting the statement of \u0027*pointer++\u0027 pattern. Probably meant: \u0027(*pointer)++\u0027. lwob.c 80 \n\nvoid CHitMemoryManager::load (IReader \u0026amp;packet)\n{\n ....\n if (!spawn_callback || !spawn_callback-\u0026gt;m_object_callback)\n if(!g_dedicated_server)\n Level().client_spawn_manager().add(\n delayed_object.m_object_id,m_object-\u0026gt;ID(),callback);\n#ifdef DEBUG\n else {\n if (spawn_callback \u0026amp;\u0026amp; spawn_callback-\u0026gt;m_object_callback) {\n VERIFY(spawn_callback-\u0026gt;m_object_callback == callback);\n }\n }\n#endif // DEBUG\n}\n\nPVS-Studio warning: V563 It is possible that this \u0027else\u0027 branch must apply to the previous \u0027if\u0027 statement. hit_memory_manager.cpp 368\n\nIn this fragment the else branch is related to the second if because of its right-associativity, which doesn\u0027t coincide with the code formatting. Fortunately, this doesn\u0027t affect the work of the program in any way, but nevertheless, it can make the debugging and testing process much more complicated. \n\nSo the recommendation is simple - put curly brackets in more, or less, complex branches. \nvoid HUD_SOUND_ITEM::PlaySound(HUD_SOUND_ITEM\u0026amp; hud_snd,\n const Fvector\u0026amp; position,\n const IGameObject* parent,\n bool b_hud_mode,\n bool looped,\n u8 index)\n{\n ....\n hud_snd.m_activeSnd-\u0026gt;snd.set_volume(\n hud_snd.m_activeSnd-\u0026gt;volume * b_hud_mode?psHUDSoundVolume:1.0f);\n}\n\nPVS-Studio warning: V502 Perhaps the \u0027?:\u0027 operator works in a different way than it was expected. The \u0027?:\u0027 operator has a lower priority than the \u0027*\u0027 operator. hudsound.cpp 108\n\nA ternary conditional operator has a lower precedence than the multiplication operator, that\u0027s why the order of operations will be as follows:\n\n(hud_snd.m_activeSnd-\u0026gt;volume * b_hud_mode)?psHUDSoundVolume:1.0f\n\nApparently, the correct code should be the following: \n\nhud_snd.m_activeSnd-\u0026gt;volume * (b_hud_mode?psHUDSoundVolume:1.0f)\n\nExpressions containing a ternary operator, several if-else branches, or operations AND/OR, are all cases where it\u0027s better to put extra brackets. \n\nSimilar warnings:\n\n\nV502 Perhaps the \u0027?:\u0027 operator works in a different way than was expected. The \u0027?:\u0027 operator has a lower priority than the \u0027+\u0027 operator. uihudstateswnd.cpp 487 \n V502 Perhaps the \u0027?:\u0027 operator works in a different way than was expected. The \u0027?:\u0027 operator has a lower priority than the \u0027+\u0027 operator. uicellcustomitems.cpp 106 \n\nUnnecessary comparisons\n\nvoid CDestroyablePhysicsObject::OnChangeVisual()\n{\n if (m_pPhysicsShell){\n if(m_pPhysicsShell)m_pPhysicsShell-\u0026gt;Deactivate();\n ....\n }\n ....\n}\n\nPVS-Studio warning: V571 Recurring check. The \u0027if (m_pPhysicsShell)\u0027 condition was already verified in line 32. destroyablephysicsobject.cpp 33\n\nIn this case m_pPhysicsShell gets checked twice. Most likely, the second check is redundant. \n\nvoid CSE_ALifeItemPDA::STATE_Read(NET_Packet \u0026amp;tNetPacket,\n u16 size)\n{\n ....\n if (m_wVersion \u0026gt; 89)\n\n if ( (m_wVersion \u0026gt; 89)\u0026amp;\u0026amp;(m_wVersion \n\nPVS-Studio warning: V571 Recurring check. The \u0027m_wVersion \u0026gt; 89\u0027 condition was already verified in line 987. xrserver_objects_alife_items.cpp 989\n\nThis code is very strange. In this fragment we see that a programmer either forgot an expression after if (m_wVersion \u0026gt; 89), or a whole series of else-if. This method requires a more scrutinizing review. \n\nvoid ELogCallback(void *context, LPCSTR txt)\n{\n ....\n bool bDlg = (\u0027#\u0027==txt[0])||((0!=txt[1])\u0026amp;\u0026amp;(\u0027#\u0027==txt[1]));\n if (bDlg){\n int mt = (\u0027!\u0027==txt[0])||((0!=txt[1])\u0026amp;\u0026amp;(\u0027!\u0027==txt[1]))?1:0;\n ....\n }\n}\n\nPVS-Studio warnings: \n\n\n\nV590 Consider inspecting the \u0027(0 != txt[1]) \u0026amp;\u0026amp; (\u0027#\u0027 == txt[1])\u0027 expression. The expression is excessive or contains a misprint. elog.cpp 29 \n \nV590 Consider inspecting the \u0027(0 != txt[1]) \u0026amp;\u0026amp; (\u0027!\u0027 == txt[1])\u0027 expression. The expression is excessive or contains a misprint. elog.cpp 31 \n\nThe 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. \n\nbool bDlg = (\u0027#\u0027==txt[0])||(\u0027#\u0027==txt[1]);\nint mt = (\u0027!\u0027==txt[0])||(\u0027!\u0027==txt[1])?1:0;\n\nErrors in data types\n\n\n\nfloat CRenderTarget::im_noise_time;\n\nCRenderTarget::CRenderTarget()\n{\n ....\n param_blur = 0.f;\n param_gray = 0.f;\n param_noise = 0.f;\n param_duality_h = 0.f;\n param_duality_v = 0.f;\n param_noise_fps = 25.f;\n param_noise_scale = 1.f;\n\n im_noise_time = 1/100;\n im_noise_shift_w = 0;\n im_noise_shift_h = 0;\n ....\n}\n\nPVS-Studio warning: V636 The \u00271 / 100\u0027 expression was implicitly cast from \u0027int\u0027 type to \u0027float\u0027 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\n\nThe 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.\n\n\nCSpaceRestriction::merge(....) const\n{\n ....\n LPSTR S = xr_alloc(acc_length);\n \n for ( ; I != E; ++I)\n temp = strconcat(sizeof(S),S,*temp,\",\",*(*I)-\u0026gt;name());\n ....\n}\n\nPVS-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\n\nThe 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).\n\nclass XRCDB_API MODEL\n{\n ....\n u32 status; // 0=ready, 1=init, 2=building\n ....\n}\n\nvoid MODEL::build (Fvector* V, int Vcnt, TRI* T, int Tcnt, \n build_callback* bc, void* bcp)\n{\n ....\n BTHREAD_params P = { this, V, Vcnt, T, Tcnt, bc, bcp };\n thread_spawn(build_thread,\"CDB-construction\",0,\u0026amp;P);\n while (S_INIT == status) Sleep(5);\n ....\n}\n\nPVS-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\n\nThe compiler can remove the check S_INIT == status as a measure of optimization, because the status variable isn\u0027t modified in the loop. To avoid such behavior, we should use volatile variables, or types of data synchronization between the threads. \n\nSimilar warnings:\n\n\nV712 Be advised that compiler may delete this cycle or make it infinite. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 23 \n V712 Be advised that compiler may delete this cycle or make it infinite. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 232 \n\nvoid CAI_Rat::UpdateCL()\n{\n ....\n if (!Useful()) {\n inherited::UpdateCL ();\n Exec_Look (Device.fTimeDelta);\n\n CMonsterSquad *squad = monster_squad().get_squad(this);\n\n if (squad \u0026amp;\u0026amp; ((squad-\u0026gt;GetLeader() != this \u0026amp;\u0026amp;\n !squad-\u0026gt;GetLeader()-\u0026gt;g_Alive()) ||\n squad-\u0026gt;get_index(this) == u32(-1)))\n squad-\u0026gt;SetLeader(this);\n\n ....\n }\n ....\n}\n\nPVS-Studio warning: V547 Expression \u0027squad-\u0026gt;get_index(this) == u32(- 1)\u0027 is always false. The value range of unsigned char type: [0, 255]. ai_rat.cpp 480\n\nTo understand why this expression is always false, let\u0027s evaluate values of individual operands. u32(-1) is 0xFFFFFFFF or 4294967295. The type, returned by the method squad-\u0026gt;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:\n\nsquad-\u0026gt;get_index(this) == u8(-1)\n\nThe same diagnostic is triggered for redundant comparisons of unsigned variables. \n\nnamespace ALife\n{\n typedef u64 _TIME_ID;\n}\nALife::_TIME_ID CScriptActionCondition::m_tLifeTime;\n\nIC bool CScriptEntityAction::CheckIfTimeOver()\n{\n return((m_tActionCondition.m_tLifeTime \u0026gt;= 0) \u0026amp;\u0026amp;\n ((m_tActionCondition.m_tStartTime +\n m_tActionCondition.m_tLifeTime) \n\nPVS-Studio warning: V547 Expression \u0027m_tActionCondition.m_tLifeTime \u0026gt;= 0\u0027 is always true. Unsigned type value is always \u0026gt;= 0. script_entity_action_inline.h 115\n\nThe variable m_tLifeTime is unsigned, thus it is always greater than or equal to zero. It\u0027s up to the developer to say if it is an excessive check, or an error in the logic of the program. \nThe same warning:\n\nV547 Expression \u0027m_tActionCondition.m_tLifeTime \n\nObjectFactory::ServerObjectBaseClass *\nCObjectItemScript::server_object (LPCSTR section) const\n{\n ObjectFactory::ServerObjectBaseClass *object = nullptr;\n\n try {\n object = m_server_creator(section);\n }\n catch(std::exception e) {\n Msg(\"Exception [%s] raised while creating server object from \"\n \"section [%s]\", e.what(),section);\n return (0);\n }\n ....\n}\n\n\nPVS-Studio warning: V746 Type slicing. An exception should be caught by reference rather than by value. object_item_script.cpp 39\n\nThe 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:\n\n catch(const std::exception\u0026amp; e) {\n\n\nMiscelleneous \n\nvoid compute_cover_value (....)\n{\n ....\n float value [8];\n ....\n if (value[0] \n\nPVS-Studio warning: V570 The \u0027value[0]\u0027 variable is assigned to itself. compiler_cover.cpp 260\n\nThe variable value[0] is assigned to itself. It\u0027s unclear, why this should be. Perhaps a different value should be assigned to it. \n\nvoid CActor::g_SetSprintAnimation(u32 mstate_rl,\n MotionID \u0026amp;head,\n MotionID \u0026amp;torso,\n MotionID \u0026amp;legs)\n{\n SActorSprintState\u0026amp; sprint = m_anims-\u0026gt;m_sprint;\n \n bool jump = (mstate_rl\u0026amp;mcFall) ||\n (mstate_rl\u0026amp;mcLanding) ||\n (mstate_rl\u0026amp;mcLanding) ||\n (mstate_rl\u0026amp;mcLanding2) ||\n (mstate_rl\u0026amp;mcJump);\n ....\n}\n\nPVS-Studio warning: V501 There are identical sub-expressions \u0027(mstate_rl \u0026amp; mcLanding)\u0027 to the left and to the right of the \u0027||\u0027 operator. actoranimation.cpp 290\n\nMost probably, we have an extra check mstate_rl \u0026amp; mcLanding, but quite often such warnings indicate an error in the logic and enum values that weren\u0027t considered. \n\nSimilar warnings:\n\n\nV501 There are identical sub-expressions \u0027HudItemData()\u0027 to the left and to the right of the \u0027\u0026amp;\u0026amp;\u0027 operator. huditem.cpp 338 \n V501 There are identical sub-expressions \u0027list_idx == e_outfit\u0027 to the left and to the right of the \u0027||\u0027 operator. uimptradewnd_misc.cpp 392 \n V501 There are identical sub-expressions \u0027(D3DFMT_UNKNOWN == fTarget)\u0027 to the left and to the right of the \u0027||\u0027 operator. hw.cpp 312 \n\nRELATION_REGISTRY::RELATION_MAP_SPOTS::RELATION_MAP_SPOTS()\n{\n ....\n spot_names[ALife::eRelationTypeWorstEnemy] = \"enemy_location\";\n spot_names[ALife::eRelationTypeWorstEnemy] = \"enemy_location\";\n ....\n}\n\nPVS-Studio warning: V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 57, 58. relation_registry.cpp 58\n\nThe analyzer detected that the same variable is assigned with two values in a row. In this case, it seems that it\u0027s just dead code, and it should be removed.\n\nvoid safe_verify(....)\n{\n ....\n printf(\"FATAL ERROR (%s): failed to verify data\\n\");\n ....\n}\n\nPVS-Studio warning: V576 Incorrect format. A different number of actual arguments is expected while calling \u0027printf\u0027 function. Expected: 2. Present: 1. entry_point.cpp 41\n\nAn insufficient number or arguments is passed to the printpf function: the format \u0027%s\u0027 shows that the pointer to a string should be passed. Such a situation can lead to a memory access error, and to program termination. \n\nConclusion\n\n\n\nThe 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. \n\nBy Pavel Belikov\n", "articleBody": "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. \n \n\n\nIntroduction\n\nX-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. \n\nThis 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.\n\nCopy-paste\n\nLet\u0027s 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. \n\n\n\nMxMatrix\u0026amp; MxQuadric::homogeneous(MxMatrix\u0026amp; H) const\n{\n ....\n unsigned int i, j;\n\n for(i=0; iPVS-Studio warning: V533 It is likely that the wrong variable is being incremented inside the \u0027for\u0027 operator. Consider reviewing \u0027i\u0027. mxqmetric.cpp 76\n\nThe 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. \n\nvoid CBaseMonster::settings_read(CInifile const * ini,\n LPCSTR section, \n SMonsterSettings \u0026amp;data)\n{\n ....\n if (ini-\u0026gt;line_exist(ppi_section,\"color_base\"))\n sscanf(ini-\u0026gt;r_string(ppi_section,\"color_base\"), \"%f,%f,%f\", \n \u0026amp;data.m_attack_effector.ppi.color_base.r, \n \u0026amp;data.m_attack_effector.ppi.color_base.g, \n \u0026amp;data.m_attack_effector.ppi.color_base.b); \n if (ini-\u0026gt;line_exist(ppi_section,\"color_base\"))\n sscanf(ini-\u0026gt;r_string(ppi_section,\"color_gray\"), \"%f,%f,%f\", \n \u0026amp;data.m_attack_effector.ppi.color_gray.r, \n \u0026amp;data.m_attack_effector.ppi.color_gray.g, \n \u0026amp;data.m_attack_effector.ppi.color_gray.b);\n if (ini-\u0026gt;line_exist(ppi_section,\"color_base\"))\n sscanf(ini-\u0026gt;r_string(ppi_section,\"color_add\"), \"%f,%f,%f\", \n \u0026amp;data.m_attack_effector.ppi.color_add.r, \n \u0026amp;data.m_attack_effector.ppi.color_add.g, \n \u0026amp;data.m_attack_effector.ppi.color_add.b);\n ....\n}\n\nPVS-Studio warnings:\n\n\nV581 The conditional expressions of the \u0027if\u0027 operators situated alongside each other are identical. Check lines: 445, 447. base_monster_startup.cpp 447 \n \n \nV581 The conditional expressions of the \u0027if\u0027 operators situated alongside each other are identical. Check lines: 447, 449. base_monster_startup.cpp 449 \n \n\nIn 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. \n\n/* process a single statement */\nstatic void ProcessStatement(char *buff, int len)\n{\n ....\n if (strncmp(buff,\"\\\\pauthr\\\\\",8) == 0)\n {\n ProcessPlayerAuth(buff, len);\n } else if (strncmp(buff,\"\\\\getpidr\\\\\",9) == 0)\n {\n ProcessGetPid(buff, len);\n } else if (strncmp(buff,\"\\\\getpidr\\\\\",9) == 0)\n {\n ProcessGetPid(buff, len);\n } else if (strncmp(buff,\"\\\\getpdr\\\\\",8) == 0)\n {\n ProcessGetData(buff, len);\n } else if (strncmp(buff,\"\\\\setpdr\\\\\",8) == 0)\n {\n ProcessSetData(buff, len);\n } \n}\n\nPVS-Studio warning: V517 The use of \u0027if (A) {...} else if (A) {...}\u0027 pattern was detected. There is a probability of logical error presence. Check lines: 1502, 1505. gstats.c 1502\n\nAs in the previous example, two similar conditions are used here (strncmp(buff,\"\\\\getpidr\\\\\",9) == 0). It\u0027s hard to say for sure whether this is a mistake, or simply unreachable code, but it\u0027s worth revising for sure. Perhaps we should have blocks with getpidr/setpidr by analogy with getpdr/setpdr.\n\n\nclass RGBAMipMappedCubeMap\n{\n ....\n size_t height() const\n {\n return cubeFaces[0].height();\n }\n\n size_t width() const\n {\n return cubeFaces[0].height();\n }\n ....\n};\n\nPVS-Studio warning: V524 It is odd that the body of \u0027width\u0027 function is fully equivalent to the body of \u0027height\u0027 function. tpixel.h 1090\n\nMethods 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\u0027s better to rewrite the method width() in the following way:\n\nsize_t width() const\n{\n return cubeFaces[0].width();\n}\n\nImproper use of C++\n\nC++ 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\u0027s what will be discussed in this section. \n\n\n\ntemplate \nstruct _matrix33\n{\npublic:\n typedef _matrix33Self;\n typedef Self\u0026amp; SelfRef;\n ....\n IC SelfRef sMTxV(Tvector\u0026amp; R, float s1, const Tvector\u0026amp; V1) const\n {\n R.x = s1*(m[0][0] * V1.x + m[1][0] * V1.y + m[2][0] * V1.z);\n R.y = s1*(m[0][1] * V1.x + m[1][1] * V1.y + m[2][1] * V1.z);\n R.z = s1*(m[0][2] * V1.x + m[1][2] * V1.y + m[2][2] * V1.z);\n }\n ....\n}\n\n\nPVS-Studio warning: V591 Non-void function should return a value. _matrix33.h 435\n\nAt 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. \n\nETOOLS_API int __stdcall ogg_enc(....)\n{\n ....\n FILE *in, *out = NULL;\n ....\n input_format *format;\n ....\n in = fopen(in_fn, \"rb\");\n\n if(in == NULL) return 0;\n\n format = open_audio_file(in, \u0026amp;enc_opts);\n if(!format){\n fclose(in);\n return 0;\n };\n\n out = fopen(out_fn, \"wb\");\n if(out == NULL){\n fclose(out);\n return 0;\n } \n ....\n}\n\nPVS-Studio warning: V575 The null pointer is passed into \u0027fclose\u0027 function. Inspect the first argument. ogg_enc.cpp 47\n\nQuite 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. \n\nvoid NVI_Image::ABGR8_To_ARGB8()\n{\n // swaps RGB for all pixels\n assert(IsDataValid());\n assert(GetBytesPerPixel() == 4);\n UINT hxw = GetNumPixels();\n for (UINT i = 0; i \u0026gt; 24) \u0026amp;\u0026amp; 0x000000FF;\n DWORD b = (col \u0026gt;\u0026gt; 16) \u0026amp;\u0026amp; 0x000000FF;\n DWORD g = (col \u0026gt;\u0026gt; 8) \u0026amp;\u0026amp; 0x000000FF;\n DWORD r = (col \u0026gt;\u0026gt; 0) \u0026amp;\u0026amp; 0x000000FF;\n col = (a \n\nPVS-Studio warnings:\n\n\n\nV560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 170 \n \nV560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 171 \n \nV560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 172 \n \nV560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 173 \n\nIn 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. \n\nCorrect variant:\n\nDWORD a = (col \u0026gt;\u0026gt; 24) \u0026amp; 0x000000FF;\nDWORD b = (col \u0026gt;\u0026gt; 16) \u0026amp; 0x000000FF;\nDWORD g = (col \u0026gt;\u0026gt; 8) \u0026amp; 0x000000FF;\nDWORD r = (col \u0026gt;\u0026gt; 0) \u0026amp; 0x000000FF;\n\n\t\n\nAnother example of strange code:\n\nVertexCache::VertexCache()\n{\n VertexCache(16);\n}\n\nPVS-Studio warning: V603 The object was created but it is not being used. If you wish to call constructor, \u0027this-\u0026gt;VertexCache::VertexCache(....)\u0027 should be used. vertexcache.cpp 6\n\nInstead 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. \n\nBOOL CActor::net_Spawn(CSE_Abstract* DC)\n{\n ....\n m_States.empty();\n ....\n}\n\t\nPVS-Studio warning: V530 The return value of function \u0027empty\u0027 is required to be utilized. actor_network.cpp 657\n\nThe 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.\n\nSuch 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\u0027s a good idea to add has, or is to the beginning of the method: it would be harder to confuse isEmpty() with clear(). \n\nA similar warning:\n\nV530 The return value of function \u0027unique\u0027 is required to be utilized. uidragdroplistex.cpp 780\n\nsize_t xrDebug::BuildStackTrace(EXCEPTION_POINTERS* exPtrs,\n char *buffer,\n size_t capacity,\n size_t lineCapacity)\n{\n memset(buffer, capacity*lineCapacity, 0);\n ....\n}\n\nPVS-Studio warning: V575 The \u0027memset\u0027 function processes \u00270\u0027 elements. Inspect the third argument. xrdebug.cpp 104\n\nDuring the memset call the arguments got mixed up, and as a result the buffer isn\u0027t 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. \n\nThe correct use of memset:\n\nmemset(buffer, 0, capacity*lineCapacity);\n\nThe following error is connected with incorrectly formed logical expression. \n\nvoid configs_dumper::dumper_thread(void* my_ptr)\n{\n ....\n DWORD wait_result = WaitForSingleObject(\n this_ptr-\u0026gt;m_make_start_event, INFINITE);\n while ( wait_result != WAIT_ABANDONED) ||\n (wait_result != WAIT_FAILED))\n ....\n}\n\nPVS-Studio warning: V547 Expression is always true. Probably the \u0027\u0026amp;\u0026amp;\u0027 operator should be used here. configs_dumper.cpp 262\n\nThe expression \u0027x != a || x != b\u0027 is always true. Most likely, \u0026amp;\u0026amp; was meant to be here instead of || operator. \n\nMore details on the topic of errors in logical expressions can be found in the article \"Logical Expressions in C/C++. Mistakes Made by Professionals\". \n\nvoid SBoneProtections::reload(const shared_str\u0026amp; bone_sect, \n IKinematics* kinematics)\n{\n ....\n CInifile::Sect \u0026amp;protections = pSettings-\u0026gt;r_section(bone_sect);\n for (CInifile::SectCIt i=protections.Data.begin();\n protections.Data.end() != i; ++i) \n {\n string256 buffer;\n BoneProtection BP;\n ....\n BP.BonePassBullet = (BOOL) (\n atoi( _GetItem(i-\u0026gt;second.c_str(), 2, buffer) )\u0026gt;0.5f);\n ....\n }\n}\n\nPVS-Studio warning: V674 The \u00270.5f\u0027 literal of the \u0027float\u0027 type is compared to a value of the \u0027int\u0027 type. boneprotections.cpp 54\n\nThe 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\u0027t look suspicious. However, only the author of this code can say for sure if this code is erroneous or not. \n\nclass IGameObject :\n public virtual IFactoryObject,\n public virtual ISpatial,\n public virtual ISheduled,\n public virtual IRenderable,\n public virtual ICollidable\n{\npublic:\n ....\n virtual u16 ID() const = 0;\n ....\n}\n\nBOOL CBulletManager::test_callback(\n const collide::ray_defs\u0026amp; rd,\n IGameObject* object,\n LPVOID params)\n{\n bullet_test_callback_data* pData = \n (bullet_test_callback_data*)params;\n SBullet* bullet = pData-\u0026gt;pBullet;\n\n if( (object-\u0026gt;ID() == bullet-\u0026gt;parent_id) \u0026amp;\u0026amp; \n (bullet-\u0026gt;fly_distflags.ricochet_was)) return FALSE;\n\n BOOL bRes = TRUE;\n if (object){\n ....\n }\n \n return bRes;\n}\n\nPVS-Studio warning: V595 The \u0027object\u0027 pointer was utilized before it was verified against nullptr. Check lines: 42, 47. level_bullet_manager_firetrace.cpp 42\n\nThe verification of the object pointer against nullptr occurs after the object-\u0026gt;ID() is dereferenced. In the case where object is nullptr, the program will crash. \n\n#ifdef _EDITOR\nBOOL WINAPI DllEntryPoint(....)\n#else\nBOOL WINAPI DllMain(....)\n#endif\n{\n switch (ul_reason_for_call)\n {\n ....\n case DLL_THREAD_ATTACH:\n if (!strstr(GetCommandLine(), \"-editor\"))\n CoInitializeEx(NULL, COINIT_MULTITHREADED);\n timeBeginPeriod(1);\n break;\n ....\n }\n return TRUE;\n}\n\nPVS-Studio warning: V718 The \u0027CoInitializeEx\u0027 function should not be called from \u0027DllMain\u0027 function. xrcore.cpp 205\n\nIn 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. \n\nPrecedence errors \n\nint sgetI1( unsigned char **bp )\n{\n int i;\n\n if ( flen == FLEN_ERROR ) return 0;\n i = **bp;\n if ( i \u0026gt; 127 ) i -= 256;\n flen += 1;\n *bp++;\n return i;\n}\n\nPVS-Studio warning: V532 Consider inspecting the statement of \u0027*pointer++\u0027 pattern. Probably meant: \u0027(*pointer)++\u0027. lwio.c 316\nThe error is related to increment usage. To make this expression more clear, let\u0027s rewrite it, including the brackets: \n\n*(bp++);\n\nSo we\u0027ll 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. \n\nPlacing 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. \n\nSimilar warnings:\n\n\nV532 Consider inspecting the statement of \u0027*pointer++\u0027 pattern. Probably meant: \u0027(*pointer)++\u0027. lwio.c 354 \n V532 Consider inspecting the statement of \u0027*pointer++\u0027 pattern. Probably meant: \u0027(*pointer)++\u0027. lwob.c 80 \n\nvoid CHitMemoryManager::load (IReader \u0026amp;packet)\n{\n ....\n if (!spawn_callback || !spawn_callback-\u0026gt;m_object_callback)\n if(!g_dedicated_server)\n Level().client_spawn_manager().add(\n delayed_object.m_object_id,m_object-\u0026gt;ID(),callback);\n#ifdef DEBUG\n else {\n if (spawn_callback \u0026amp;\u0026amp; spawn_callback-\u0026gt;m_object_callback) {\n VERIFY(spawn_callback-\u0026gt;m_object_callback == callback);\n }\n }\n#endif // DEBUG\n}\n\nPVS-Studio warning: V563 It is possible that this \u0027else\u0027 branch must apply to the previous \u0027if\u0027 statement. hit_memory_manager.cpp 368\n\nIn this fragment the else branch is related to the second if because of its right-associativity, which doesn\u0027t coincide with the code formatting. Fortunately, this doesn\u0027t affect the work of the program in any way, but nevertheless, it can make the debugging and testing process much more complicated. \n\nSo the recommendation is simple - put curly brackets in more, or less, complex branches. \nvoid HUD_SOUND_ITEM::PlaySound(HUD_SOUND_ITEM\u0026amp; hud_snd,\n const Fvector\u0026amp; position,\n const IGameObject* parent,\n bool b_hud_mode,\n bool looped,\n u8 index)\n{\n ....\n hud_snd.m_activeSnd-\u0026gt;snd.set_volume(\n hud_snd.m_activeSnd-\u0026gt;volume * b_hud_mode?psHUDSoundVolume:1.0f);\n}\n\nPVS-Studio warning: V502 Perhaps the \u0027?:\u0027 operator works in a different way than it was expected. The \u0027?:\u0027 operator has a lower priority than the \u0027*\u0027 operator. hudsound.cpp 108\n\nA ternary conditional operator has a lower precedence than the multiplication operator, that\u0027s why the order of operations will be as follows:\n\n(hud_snd.m_activeSnd-\u0026gt;volume * b_hud_mode)?psHUDSoundVolume:1.0f\n\nApparently, the correct code should be the following: \n\nhud_snd.m_activeSnd-\u0026gt;volume * (b_hud_mode?psHUDSoundVolume:1.0f)\n\nExpressions containing a ternary operator, several if-else branches, or operations AND/OR, are all cases where it\u0027s better to put extra brackets. \n\nSimilar warnings:\n\n\nV502 Perhaps the \u0027?:\u0027 operator works in a different way than was expected. The \u0027?:\u0027 operator has a lower priority than the \u0027+\u0027 operator. uihudstateswnd.cpp 487 \n V502 Perhaps the \u0027?:\u0027 operator works in a different way than was expected. The \u0027?:\u0027 operator has a lower priority than the \u0027+\u0027 operator. uicellcustomitems.cpp 106 \n\nUnnecessary comparisons\n\nvoid CDestroyablePhysicsObject::OnChangeVisual()\n{\n if (m_pPhysicsShell){\n if(m_pPhysicsShell)m_pPhysicsShell-\u0026gt;Deactivate();\n ....\n }\n ....\n}\n\nPVS-Studio warning: V571 Recurring check. The \u0027if (m_pPhysicsShell)\u0027 condition was already verified in line 32. destroyablephysicsobject.cpp 33\n\nIn this case m_pPhysicsShell gets checked twice. Most likely, the second check is redundant. \n\nvoid CSE_ALifeItemPDA::STATE_Read(NET_Packet \u0026amp;tNetPacket,\n u16 size)\n{\n ....\n if (m_wVersion \u0026gt; 89)\n\n if ( (m_wVersion \u0026gt; 89)\u0026amp;\u0026amp;(m_wVersion \n\nPVS-Studio warning: V571 Recurring check. The \u0027m_wVersion \u0026gt; 89\u0027 condition was already verified in line 987. xrserver_objects_alife_items.cpp 989\n\nThis code is very strange. In this fragment we see that a programmer either forgot an expression after if (m_wVersion \u0026gt; 89), or a whole series of else-if. This method requires a more scrutinizing review. \n\nvoid ELogCallback(void *context, LPCSTR txt)\n{\n ....\n bool bDlg = (\u0027#\u0027==txt[0])||((0!=txt[1])\u0026amp;\u0026amp;(\u0027#\u0027==txt[1]));\n if (bDlg){\n int mt = (\u0027!\u0027==txt[0])||((0!=txt[1])\u0026amp;\u0026amp;(\u0027!\u0027==txt[1]))?1:0;\n ....\n }\n}\n\nPVS-Studio warnings: \n\n\n\nV590 Consider inspecting the \u0027(0 != txt[1]) \u0026amp;\u0026amp; (\u0027#\u0027 == txt[1])\u0027 expression. The expression is excessive or contains a misprint. elog.cpp 29 \n \nV590 Consider inspecting the \u0027(0 != txt[1]) \u0026amp;\u0026amp; (\u0027!\u0027 == txt[1])\u0027 expression. The expression is excessive or contains a misprint. elog.cpp 31 \n\nThe 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. \n\nbool bDlg = (\u0027#\u0027==txt[0])||(\u0027#\u0027==txt[1]);\nint mt = (\u0027!\u0027==txt[0])||(\u0027!\u0027==txt[1])?1:0;\n\nErrors in data types\n\n\n\nfloat CRenderTarget::im_noise_time;\n\nCRenderTarget::CRenderTarget()\n{\n ....\n param_blur = 0.f;\n param_gray = 0.f;\n param_noise = 0.f;\n param_duality_h = 0.f;\n param_duality_v = 0.f;\n param_noise_fps = 25.f;\n param_noise_scale = 1.f;\n\n im_noise_time = 1/100;\n im_noise_shift_w = 0;\n im_noise_shift_h = 0;\n ....\n}\n\nPVS-Studio warning: V636 The \u00271 / 100\u0027 expression was implicitly cast from \u0027int\u0027 type to \u0027float\u0027 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\n\nThe 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.\n\n\nCSpaceRestriction::merge(....) const\n{\n ....\n LPSTR S = xr_alloc(acc_length);\n \n for ( ; I != E; ++I)\n temp = strconcat(sizeof(S),S,*temp,\",\",*(*I)-\u0026gt;name());\n ....\n}\n\nPVS-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\n\nThe 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).\n\nclass XRCDB_API MODEL\n{\n ....\n u32 status; // 0=ready, 1=init, 2=building\n ....\n}\n\nvoid MODEL::build (Fvector* V, int Vcnt, TRI* T, int Tcnt, \n build_callback* bc, void* bcp)\n{\n ....\n BTHREAD_params P = { this, V, Vcnt, T, Tcnt, bc, bcp };\n thread_spawn(build_thread,\"CDB-construction\",0,\u0026amp;P);\n while (S_INIT == status) Sleep(5);\n ....\n}\n\nPVS-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\n\nThe compiler can remove the check S_INIT == status as a measure of optimization, because the status variable isn\u0027t modified in the loop. To avoid such behavior, we should use volatile variables, or types of data synchronization between the threads. \n\nSimilar warnings:\n\n\nV712 Be advised that compiler may delete this cycle or make it infinite. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 23 \n V712 Be advised that compiler may delete this cycle or make it infinite. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 232 \n\nvoid CAI_Rat::UpdateCL()\n{\n ....\n if (!Useful()) {\n inherited::UpdateCL ();\n Exec_Look (Device.fTimeDelta);\n\n CMonsterSquad *squad = monster_squad().get_squad(this);\n\n if (squad \u0026amp;\u0026amp; ((squad-\u0026gt;GetLeader() != this \u0026amp;\u0026amp;\n !squad-\u0026gt;GetLeader()-\u0026gt;g_Alive()) ||\n squad-\u0026gt;get_index(this) == u32(-1)))\n squad-\u0026gt;SetLeader(this);\n\n ....\n }\n ....\n}\n\nPVS-Studio warning: V547 Expression \u0027squad-\u0026gt;get_index(this) == u32(- 1)\u0027 is always false. The value range of unsigned char type: [0, 255]. ai_rat.cpp 480\n\nTo understand why this expression is always false, let\u0027s evaluate values of individual operands. u32(-1) is 0xFFFFFFFF or 4294967295. The type, returned by the method squad-\u0026gt;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:\n\nsquad-\u0026gt;get_index(this) == u8(-1)\n\nThe same diagnostic is triggered for redundant comparisons of unsigned variables. \n\nnamespace ALife\n{\n typedef u64 _TIME_ID;\n}\nALife::_TIME_ID CScriptActionCondition::m_tLifeTime;\n\nIC bool CScriptEntityAction::CheckIfTimeOver()\n{\n return((m_tActionCondition.m_tLifeTime \u0026gt;= 0) \u0026amp;\u0026amp;\n ((m_tActionCondition.m_tStartTime +\n m_tActionCondition.m_tLifeTime) \n\nPVS-Studio warning: V547 Expression \u0027m_tActionCondition.m_tLifeTime \u0026gt;= 0\u0027 is always true. Unsigned type value is always \u0026gt;= 0. script_entity_action_inline.h 115\n\nThe variable m_tLifeTime is unsigned, thus it is always greater than or equal to zero. It\u0027s up to the developer to say if it is an excessive check, or an error in the logic of the program. \nThe same warning:\n\nV547 Expression \u0027m_tActionCondition.m_tLifeTime \n\nObjectFactory::ServerObjectBaseClass *\nCObjectItemScript::server_object (LPCSTR section) const\n{\n ObjectFactory::ServerObjectBaseClass *object = nullptr;\n\n try {\n object = m_server_creator(section);\n }\n catch(std::exception e) {\n Msg(\"Exception [%s] raised while creating server object from \"\n \"section [%s]\", e.what(),section);\n return (0);\n }\n ....\n}\n\n\nPVS-Studio warning: V746 Type slicing. An exception should be caught by reference rather than by value. object_item_script.cpp 39\n\nThe 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:\n\n catch(const std::exception\u0026amp; e) {\n\n\nMiscelleneous \n\nvoid compute_cover_value (....)\n{\n ....\n float value [8];\n ....\n if (value[0] \n\nPVS-Studio warning: V570 The \u0027value[0]\u0027 variable is assigned to itself. compiler_cover.cpp 260\n\nThe variable value[0] is assigned to itself. It\u0027s unclear, why this should be. Perhaps a different value should be assigned to it. \n\nvoid CActor::g_SetSprintAnimation(u32 mstate_rl,\n MotionID \u0026amp;head,\n MotionID \u0026amp;torso,\n MotionID \u0026amp;legs)\n{\n SActorSprintState\u0026amp; sprint = m_anims-\u0026gt;m_sprint;\n \n bool jump = (mstate_rl\u0026amp;mcFall) ||\n (mstate_rl\u0026amp;mcLanding) ||\n (mstate_rl\u0026amp;mcLanding) ||\n (mstate_rl\u0026amp;mcLanding2) ||\n (mstate_rl\u0026amp;mcJump);\n ....\n}\n\nPVS-Studio warning: V501 There are identical sub-expressions \u0027(mstate_rl \u0026amp; mcLanding)\u0027 to the left and to the right of the \u0027||\u0027 operator. actoranimation.cpp 290\n\nMost probably, we have an extra check mstate_rl \u0026amp; mcLanding, but quite often such warnings indicate an error in the logic and enum values that weren\u0027t considered. \n\nSimilar warnings:\n\n\nV501 There are identical sub-expressions \u0027HudItemData()\u0027 to the left and to the right of the \u0027\u0026amp;\u0026amp;\u0027 operator. huditem.cpp 338 \n V501 There are identical sub-expressions \u0027list_idx == e_outfit\u0027 to the left and to the right of the \u0027||\u0027 operator. uimptradewnd_misc.cpp 392 \n V501 There are identical sub-expressions \u0027(D3DFMT_UNKNOWN == fTarget)\u0027 to the left and to the right of the \u0027||\u0027 operator. hw.cpp 312 \n\nRELATION_REGISTRY::RELATION_MAP_SPOTS::RELATION_MAP_SPOTS()\n{\n ....\n spot_names[ALife::eRelationTypeWorstEnemy] = \"enemy_location\";\n spot_names[ALife::eRelationTypeWorstEnemy] = \"enemy_location\";\n ....\n}\n\nPVS-Studio warning: V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 57, 58. relation_registry.cpp 58\n\nThe analyzer detected that the same variable is assigned with two values in a row. In this case, it seems that it\u0027s just dead code, and it should be removed.\n\nvoid safe_verify(....)\n{\n ....\n printf(\"FATAL ERROR (%s): failed to verify data\\n\");\n ....\n}\n\nPVS-Studio warning: V576 Incorrect format. A different number of actual arguments is expected while calling \u0027printf\u0027 function. Expected: 2. Present: 1. entry_point.cpp 41\n\nAn insufficient number or arguments is passed to the printpf function: the format \u0027%s\u0027 shows that the pointer to a string should be passed. Such a situation can lead to a memory access error, and to program termination. \n\nConclusion\n\n\n\nThe 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. \n\nBy Pavel Belikov\n", "dateCreated": "2016-06-20T11:35:34+0000", "datePublished": "2017-01-22T22:30:00+0000", "dateModified": "2017-01-22T22:30:00+0000", "pageStart": 1, "pageEnd": 1, "author": { "@type": "Person", "name": "PVS-studio team", "image": "https://www.gamedev.net/uploads/profile/photo-thumb-229739.png", "url": "https://www.gamedev.net/profile/229739-pvs-studio-team/" }, "publisher": { "@type": "Organization", "name": "PVS-studio team", "image": "https://www.gamedev.net/uploads/profile/photo-thumb-229739.png", "logo": { "@type": "ImageObject", "url": "https://www.gamedev.net/uploads/profile/photo-thumb-229739.png" }, "url": "https://www.gamedev.net/profile/229739-pvs-studio-team/" }, "interactionStatistic": [ { "@type": "InteractionCounter", "interactionType": "http://schema.org/ViewAction", "userInteractionCount": 6666 }, { "@type": "InteractionCounter", "interactionType": "http://schema.org/FollowAction", "userInteractionCount": 6 }, { "@type": "InteractionCounter", "interactionType": "http://schema.org/ReviewAction", "userInteractionCount": 0 }, { "@type": "InteractionCounter", "interactionType": "http://schema.org/CommentAction", "userInteractionCount": 3 } ], "image": { "@type": "ImageObject", "url": "https://www.gamedev.net/uploads/e6e9340bdaf7a52c7e94b25d6e0030bc.png", "width": 250, "height": 150 }, "aggregateRating": { "@type": "AggregateRating", "ratingValue": 5, "ratingCount": 4, "reviewCount": 0, "bestRating": "5" }, "commentCount": 3 } { "@context": "http://www.schema.org", "@type": "WebSite", "name": "GameDev.net", "url": "https://www.gamedev.net/", "potentialAction": { "type": "SearchAction", "query-input": "required name=query", "target": "https://www.gamedev.net/search/?q={query}" }, "inLanguage": [ { "@type": "Language", "name": "English (USA)", "alternateName": "en-US" } ] } { "@context": "http://www.schema.org", "@type": "Organization", "name": "GameDev.net", "url": "https://www.gamedev.net/", "address": { "@type": "PostalAddress", "streetAddress": "", "addressLocality": null, "addressRegion": null, "postalCode": null, "addressCountry": null } } { "@context": "http://schema.org", "@type": "BreadcrumbList", "itemListElement": [ { "@type": "ListItem", "position": 1, "item": { "@id": "https://www.gamedev.net/articles/", "name": "Articles" } }, { "@type": "ListItem", "position": 2, "item": { "@id": "https://www.gamedev.net/articles/programming/", "name": "Programming" } }, { "@type": "ListItem", "position": 3, "item": { "@id": "https://www.gamedev.net/articles/programming/general-and-gameplay-programming/", "name": "General and Gameplay Programming" } } ] } { "@context": "http://schema.org", "@type": "ContactPage", "url": "https://www.gamedev.net/contact/" } Important Information By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.   I accept We are the game development community. Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up! Sign me up!