We are looking for anomalies in the X-Ray Engine
X-Ray Engine is a game engine that is used in STALKER games on September 16, 2014. Its source code was made publicly available, and since then fans have been developing it. The large size of the project, a huge number of bugs in the games - all this has an excellent demonstration of the capabilities of the PVS-Studio static code analyzer.
X-Ray was created by the Ukrainian company GSC GameWorld for the game STALKER: Shadow of Chernobyl. The engine includes a render with support for DirectX 8.1 / 9.0c / 10 / 10.1 / 11, physical and sound engines, multiplayer and A-Life artificial intelligence system. Subsequently, the company created the version 2.0 engine for its new game, but development was discontinued and the source codes leaked to the network.
')
The project, along with all its dependencies, is easily assembled in Visual Studio 2015. For verification, we used the source code of the 1.6 engine from the repository on GitHub and the PVS-Studio 6.04 static code analyzer, which can be downloaded by reference .
To begin, consider the errors associated with copying code. The scenario of their occurrence in different cases is usually similar: they copied the code, changed some of the variables, and forgot a few. Such errors can quickly spread through the code base, and it is very easy to skip them without a static analyzer.
PVS-Studio warning : V533 It is likely that a variable is being increased inside the 'for' operator. Consider reviewing 'i'. mxqmetric.cpp 76
The analyzer found that in the nested for loop, the variable i is incremented, and the variable j is checked, which leads to an infinite loop. Most likely, when copying it they simply forgot to change it.
PVS-Studio warnings :
In this fragment, several identical conditional expressions are used in succession. Obviously, it is necessary to replace color_base with color_gray and color_add in accordance with the code in the if branch body .
PVS-Studio warning : V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 1502, 1505. gstats.c 1502
As in the previous example, two identical conditions are used here ( strncmp (buff, "\\ getpidr \\", 9) == 0 ). It is difficult to say for sure whether this is an error or just an unreachable code, but you should definitely pay attention to this. It is possible that there should be blocks with getpidr / setpidr by analogy with getpdr / setpdr .
PVS-Studio warning : V524 It’s odd that it’s a function of the body’s width. tpixel.h 1090
The height () and width () methods have the same body. Given that the dimensions of the faces of the cube are calculated, perhaps there is no error here. But it is better to rewrite the width () method as follows:
C ++ is a wonderful language that gives a programmer many opportunities ... to shoot yourself a leg in a particularly cruel way. Indefinite behavior, memory leaks and, of course, typos - errors of this kind will be discussed in the current section.
PVS-Studio warning : V591 Non-void function should return a value. _matrix33.h 435
At the end of the method skipped return * this . By standard, this code will lead to undefined behavior. Since the return value is a reference, it is likely to cause the program to crash when trying to access the return value.
PVS-Studio warning : V575 The null pointer is passed into the 'fclose' function. Inspect the first argument. ogg_enc.cpp 47
Quite an interesting example. The analyzer detected that the argument in the fclose call is nullptr , which makes the function call meaningless. It can be assumed that they should have closed the in stream .
PVS-Studio warnings:
In this section of the code logical and bit operations are confused. The result will not be what the programmer expected: col will always be 0x01010101 regardless of the input data.
The correct option is:
Another example of strange code:
PVS-Studio warning : V603 The object was not used. If you wish to call the constructor, 'this-> VertexCache :: VertexCache (....)' should be used. vertexcache.cpp 6
Instead of calling one constructor from another to initialize the instance, a new object of type VertexCache will be created and immediately destroyed. As a result, the members of the object being created will remain uninitialized.
PVS-Studio warning : V530 The return value of the function is 'empty' is required to be utilized. actor_network.cpp 657
The analyzer warns that the return value is not used. It seems that the programmer has confused the methods empty () and clear () : empty () does not clear the array, but checks whether it is empty.
Such errors are often found in various projects. The problem is that the name empty () is not obvious: some perceive it as an action - deletion. In order to avoid such ambiguity it is better to add the verb has, is to the beginning of the method: indeed, isEmpty () with clear () is difficult to confuse.
Similar warning:
V530 uidragdroplistex.cpp 780
PVS-Studio warning : V575 The 'memset' function processes '0' elements. Inspect the third argument. xrdebug.cpp 104
When memset was called, the arguments were mixed up in places and, as a result, the buffer is not reset, as originally intended. Such an error can live in a project for a very long time, since it is very difficult to detect. In such places a static analyzer comes to the rescue of the programmer.
Proper use of memset :
The following error is related to an incorrectly composed logical expression.
PVS-Studio warning : V547 Expression is always true. Probably the '&&' operator should be used here. configs_dumper.cpp 262
Expressions of the form 'x! = A || x! = b 'are always true. Most likely instead of the operator || the && operator was implied.
More information about errors in logical expressions can be found in the article " Logical expressions in C / C ++. How professionals are mistaken ."
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 a comparison of an integer value with a real constant. It is possible that, by analogy, the atof function should be used instead of atoi , in the other case it is worth rewriting this comparison so that it does not look suspicious. However, to say for sure whether this example is erroneous or not, can only the developer who wrote it.
PVS-Studio warning : V595 The 'object' pointer was used before it was verified against nullptr. Check lines: 42, 47. level_bullet_manager_firetrace.cpp 42
Checking the object pointer for equality nullptr comes after dereferencing object-> ID () . In the case where object is nullptr, this will cause the program to crash.
PVS-Studio warning : V718 The 'CoInitializeEx' function should not be called from the 'DllMain' function. xrcore.cpp 205
In the body of DllMain you cannot use part of the WinAPI functions, including CoInitializeEx. You can verify this by reading the MSDN documentation . It is impossible to give any definite advice on how to rewrite this function, but it is worth understanding that this situation is dangerous, since it can lead to a mutual blocking of threads or an abnormal termination.
PVS-Studio warning : V532 Consider inspecting the statement of '* pointer ++' pattern. Probably meant: '(* pointer) ++'. lwio.c 316
Error associated with the use of increment. For clarity, we rewrite this expression by placing brackets:
That is, there will be a shift not in the content at bp, but in the pointer itself, which is meaningless in this context. Below the code there are fragments of the form * bp + = N , which is why I concluded that this is a mistake.
To avoid such an error would help the arrangement of brackets, which would make the order of calculations more understandable. Also a good technique is the use of const for arguments that should not change.
Similar warnings:
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 refers to the second if if due to its right associativity, which does not coincide with the code formatting. Fortunately, this case does not affect the work of the program, however, it can complicate the process of debugging and testing.
The recommendation is simple - put curly braces in more or less complex branches.
PVS-Studio warning : V502 Perhaps the '?:' Operator works in a different way. The '?:' Operator has a lower limit than the '*' operator. hudsound.cpp 108
The ternary conditional operator has lower priority than multiplication, so the order of operations will be as follows:
Obviously, the correct code should look like this:
Expressions containing a ternary operator, several if-else branches or AND / OR operations are those cases when it is better to put extra brackets.
Similar warnings:
PVS-Studio warning : V571 Recurring check. The 'if (m_pPhysicsShell)' condition was already verified in line 32. destroyableysyssobject.cpp 33
In this example, m_pPhysicsShell is double checked. Most likely, the second check is superfluous.
PVS-Studio warning : V571 Recurring check. The 'm_wVersion> 89' condition was already verified in line 987. xrserver_objects_alife_items.cpp 989
Very strange code. Whether the expression was forgotten here after if (m_wVersion> 89) , or the whole series of else-if . This method requires more detailed consideration by the project developer.
PVS-Studio warnings :
In the initialization expressions for the variables bDlg and mt, the check (0! = Txt [1]) is redundant. If you omit it, expressions will become much easier to read:
PVS-Studio warning : V636 The ' 1/100 ' expression was implicitly cast from 'int' type to 'float' type. Consider using a fractional part. An example: double A = (double) (X) / Y ;. gl_rendertarget.cpp 245
The value of the 1/100 expression is 0, since the integer division operation is performed. To get the value of 0.01f, you need to use the real literal by rewriting the expression: 1 / 100.0f. Although it is possible that this behavior was provided by the author, and there is no error here.
PVS-Studio warning : V579 It is possibly a mistake. Inspect the first argument. space_restriction.cpp 201
The strconcat function takes the length of the buffer as the first parameter. Buffer S is declared as LPSTR , that is, as a pointer to a string. sizeof (S) will be equal to the size of the pointer in bytes, that is, sizeof (char *) , and not the number of characters in the string. To calculate the length, use strlen (S) .
PVS-Studio warning : V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable (s) or synchronization primitives to avoid this. xrcdb.cpp 100
The compiler can remove the S_INIT == status check as an optimization, since the status variable is not modified in a loop. To avoid this behavior, you need to use volatile variables or types of data synchronization between threads.
Similar warnings:
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
In order to understand why this expression is always false, let's calculate the values of individual operands. u32 (-1) is 0xFFFFFFFF or 4294967295. The type returned by the squad-> get_index (....) method is u8, hence its maximum value is 0xFF or 255, which is strictly less than u32 (-1). Accordingly, the value of such a comparison will always be false . This code is easy to fix by changing the data type to u8:
The same diagnostics also work for redundant comparisons of unsigned variables:
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 m_tLifeTime variable is unsigned; accordingly, it is always greater than or equal to zero. Is this an extra check or is there an error in logic here, judged by the developer.
Similar warning:
V547 Expression 'm_tActionCondition.m_tLifeTime <0' is always false. Unsigned type value is never <0. script_entity_action_inline.h 143
PVS-Studio warning : V746 Type slicing. By reference rather than by value. object_item_script.cpp 39
The std :: exception :: what () function is virtual and can be redefined in inherited classes. In this example, the exception is caught by value, therefore, an instance of the class will be copied and all information about the polymorphic type will be lost. Refer to what () in this case is meaningless. The exception is to intercept the link:
PVS-Studio warning : V570 The 'value [0]' variable is assigned to itself. compiler_cover.cpp 260
The variable value [0] is assigned to itself. Why do this - it is not clear. Perhaps she should have been assigned a different value.
PVS-Studio warning : V501 There are identical sub-expressions '(mstate_rl & mcLanding)' to the left and the right of the '||' operator. actoranimation.cpp 290
Most likely there is just an extra check of mstate_rl & mcLanding , but often such warnings signal an error in the logic and unexamined enum values.
Similar warnings:
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 has found that two values are assigned to one variable in a row. In this case, it looks like it's just a dead code and should be deleted.
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 of arguments are passed to the printf function: the format '% s' indicates that a pointer must be passed to the string. Such a situation may lead to a memory access error and emergency program termination.
Checking the source code X-Ray Engine revealed a large number of both superfluous or suspicious code, as well as uniquely erroneous and dangerous moments. It is worth noting that the static analyzer perfectly helps to detect errors at an early stage of development, which makes life much easier for the programmer and frees up time to create new versions of the application.
If you want to share this article with an English-speaking audience, then please use the link to the translation: Pavel Belikov. Anomalies in X-Ray Engine .
Introduction
X-Ray was created by the Ukrainian company GSC GameWorld for the game STALKER: Shadow of Chernobyl. The engine includes a render with support for DirectX 8.1 / 9.0c / 10 / 10.1 / 11, physical and sound engines, multiplayer and A-Life artificial intelligence system. Subsequently, the company created the version 2.0 engine for its new game, but development was discontinued and the source codes leaked to the network.
')
The project, along with all its dependencies, is easily assembled in Visual Studio 2015. For verification, we used the source code of the 1.6 engine from the repository on GitHub and the PVS-Studio 6.04 static code analyzer, which can be downloaded by reference .
Copy-paste
To begin, consider the errors associated with copying code. The scenario of their occurrence in different cases is usually similar: they copied the code, changed some of the variables, and forgot a few. Such errors can quickly spread through the code base, and it is very easy to skip them without a static 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 a variable is being increased inside the 'for' operator. Consider reviewing 'i'. mxqmetric.cpp 76
The analyzer found that in the nested for loop, the variable i is incremented, and the variable j is checked, which leads to an infinite loop. Most likely, when copying it they simply 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' are located alongside each other are identical. Check lines: 445, 447. base_monster_startup.cpp 447
- V581 The conditional expressions of the 'if' are located alongside each other are identical. Check lines: 447, 449. base_monster_startup.cpp 449
In this fragment, several identical conditional expressions are used in succession. Obviously, it is necessary to replace color_base with color_gray and color_add in accordance with the code in the if branch body .
/* 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 possibility of logical error presence. Check lines: 1502, 1505. gstats.c 1502
As in the previous example, two identical conditions are used here ( strncmp (buff, "\\ getpidr \\", 9) == 0 ). It is difficult to say for sure whether this is an error or just an unreachable code, but you should definitely pay attention to this. It is possible that there should be 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’s odd that it’s a function of the body’s width. tpixel.h 1090
The height () and width () methods have the same body. Given that the dimensions of the faces of the cube are calculated, perhaps there is no error here. But it is better to rewrite the width () method as follows:
size_t width() const { return cubeFaces[0].width(); } Incorrect use of C ++
C ++ is a wonderful language that gives a programmer many opportunities ... to shoot yourself a leg in a particularly cruel way. Indefinite behavior, memory leaks and, of course, typos - errors of this kind will be discussed in the current 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 { Rx = s1*(m[0][0] * V1.x + m[1][0] * V1.y + m[2][0] * V1.z); Ry = s1*(m[0][1] * V1.x + m[1][1] * V1.y + m[2][1] * V1.z); Rz = 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 skipped return * this . By standard, this code will lead to undefined behavior. Since the return value is a reference, it is likely to cause the program to crash when trying 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 the 'fclose' function. Inspect the first argument. ogg_enc.cpp 47
Quite an interesting example. The analyzer detected that the argument in the fclose call is nullptr , which makes the function call meaningless. It can be assumed that they should have closed the in stream .
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 the conditional expression is always true: 0x000000FF. nvi_image.cpp 170
- V560 A part of the conditional expression is always true: 0x000000FF. nvi_image.cpp 171
- V560 A part of the conditional expression is always true: 0x000000FF. nvi_image.cpp 172
- V560 A part of the conditional expression is always true: 0x000000FF. nvi_image.cpp 173
In this section of the code logical and bit operations are confused. The result will not be what the programmer expected: col will always be 0x01010101 regardless of the input data.
The correct option is:
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 not used. If you wish to call the constructor, 'this-> VertexCache :: VertexCache (....)' should be used. vertexcache.cpp 6
Instead of calling one constructor from another to initialize the instance, a new object of type VertexCache will be created and immediately destroyed. As a result, the members of the object being created will remain uninitialized.
BOOL CActor::net_Spawn(CSE_Abstract* DC) { .... m_States.empty(); .... } PVS-Studio warning : V530 The return value of the function is 'empty' is required to be utilized. actor_network.cpp 657
The analyzer warns that the return value is not used. It seems that the programmer has confused the methods empty () and clear () : empty () does not clear the array, but checks whether it is empty.
Such errors are often found in various projects. The problem is that the name empty () is not obvious: some perceive it as an action - deletion. In order to avoid such ambiguity it is better to add the verb has, is to the beginning of the method: indeed, isEmpty () with clear () is difficult to confuse.
Similar warning:
V530 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
When memset was called, the arguments were mixed up in places and, as a result, the buffer is not reset, as originally intended. Such an error can live in a project for a very long time, since it is very difficult to detect. In such places a static analyzer comes to the rescue of the programmer.
Proper use of memset :
memset(buffer, 0, capacity*lineCapacity); The following error is related to an incorrectly composed 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
Expressions of the form 'x! = A || x! = b 'are always true. Most likely instead of the operator || the && operator was implied.
More information about errors in logical expressions can be found in the article " Logical expressions in C / C ++. How professionals are mistaken ."
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 a comparison of an integer value with a real constant. It is possible that, by analogy, the atof function should be used instead of atoi , in the other case it is worth rewriting this comparison so that it does not look suspicious. However, to say for sure whether this example is erroneous or not, can only the developer who wrote it.
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 used before it was verified against nullptr. Check lines: 42, 47. level_bullet_manager_firetrace.cpp 42
Checking the object pointer for equality nullptr comes after dereferencing object-> ID () . In the case where object is nullptr, this will cause the program to 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 the 'DllMain' function. xrcore.cpp 205
In the body of DllMain you cannot use part of the WinAPI functions, including CoInitializeEx. You can verify this by reading the MSDN documentation . It is impossible to give any definite advice on how to rewrite this function, but it is worth understanding that this situation is dangerous, since it can lead to a mutual blocking of threads or an abnormal termination.
Priority 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
Error associated with the use of increment. For clarity, we rewrite this expression by placing brackets:
*(bp++); That is, there will be a shift not in the content at bp, but in the pointer itself, which is meaningless in this context. Below the code there are fragments of the form * bp + = N , which is why I concluded that this is a mistake.
To avoid such an error would help the arrangement of brackets, which would make the order of calculations more understandable. Also a good technique is the use of const for arguments that should not change.
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 refers to the second if if due to its right associativity, which does not coincide with the code formatting. Fortunately, this case does not affect the work of the program, however, it can complicate the process of debugging and testing.
The recommendation is simple - put curly braces 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. The '?:' Operator has a lower limit than the '*' operator. hudsound.cpp 108
The ternary conditional operator has lower priority than multiplication, so the order of operations will be as follows:
(hud_snd.m_activeSnd->volume * b_hud_mode)?psHUDSoundVolume:1.0f Obviously, the correct code should look like this:
hud_snd.m_activeSnd->volume * (b_hud_mode?psHUDSoundVolume:1.0f) Expressions containing a ternary operator, several if-else branches or AND / OR operations are those cases when it is better to put extra brackets.
Similar warnings:
- V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '+' operator. uihudstateswnd.cpp 487
- V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '+' operator. uicellcustomitems.cpp 106
Extra 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. destroyableysyssobject.cpp 33
In this example, m_pPhysicsShell is double checked. Most likely, the second check is superfluous.
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
Very strange code. Whether the expression was forgotten here after if (m_wVersion> 89) , or the whole series of else-if . This method requires more detailed consideration by the project developer.
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 misprint. elog.cpp 29
- V590 Consider inspecting the '(0! = Txt [1]) && ('! '== txt [1])' expression. The expression is misprint. elog.cpp 31
In the initialization expressions for the variables bDlg and mt, the check (0! = Txt [1]) is redundant. If you omit it, expressions will become 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 using a fractional part. An example: double A = (double) (X) / Y ;. gl_rendertarget.cpp 245
The value of the 1/100 expression is 0, since the integer division operation is performed. To get the value of 0.01f, you need to use the real literal by rewriting the expression: 1 / 100.0f. Although it is possible that this behavior was provided by the author, and there is no error here.
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 It is possibly a mistake. Inspect the first argument. space_restriction.cpp 201
The strconcat function takes the length of the buffer as the first parameter. Buffer S is declared as LPSTR , that is, as a pointer to a string. sizeof (S) will be equal to the size of the pointer in bytes, that is, sizeof (char *) , and not the number of characters in the string. To calculate the length, 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 infinity. Use volatile variable (s) or synchronization primitives to avoid this. xrcdb.cpp 100
The compiler can remove the S_INIT == status check as an optimization, since the status variable is not modified in a loop. To avoid this behavior, you need to use volatile variables or types of data synchronization between threads.
Similar warnings:
- Or make it infinity. Use volatile variable (s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 23
- Or make it infinity. Use volatile variable (s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 232
void CAI_Rat::UpdateCL() { .... if (!Useful()) { inherited::UpdateCL (); Exec_Look (Device.fTimeDelta); CMonsterSquad *squad = monster_squad().get_squad(this); if (squad && ((squad->GetLeader() != this && !squad->GetLeader()->g_Alive()) || squad->get_index(this) == u32(-1))) squad->SetLeader(this); .... } .... } PVS-Studio 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
In order to understand why this expression is always false, let's calculate the values of individual operands. u32 (-1) is 0xFFFFFFFF or 4294967295. The type returned by the squad-> get_index (....) method is u8, hence its maximum value is 0xFF or 255, which is strictly less than u32 (-1). Accordingly, the value of such a comparison will always be false . This code is easy to fix by changing the data type to u8:
squad->get_index(this) == u8(-1) The same diagnostics also work 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 m_tLifeTime variable is unsigned; accordingly, it is always greater than or equal to zero. Is this an extra check or is there an error in logic here, judged by the developer.
Similar 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. By reference rather than by value. object_item_script.cpp 39
The std :: exception :: what () function is virtual and can be redefined in inherited classes. In this example, the exception is caught by value, therefore, an instance of the class will be copied and all information about the polymorphic type will be lost. Refer to what () in this case is meaningless. The exception is to intercept the link:
catch(const std::exception& e) { miscellanea
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. Why do this - it is not clear. Perhaps she should have been assigned a different value.
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 the right of the '||' operator. actoranimation.cpp 290
Most likely there is just an extra check of mstate_rl & mcLanding , but often such warnings signal an error in the logic and unexamined enum values.
Similar warnings:
- V501 There are identical sub-expressions "HudItemData ()" huditem.cpp 338
- V501 There are identical sub-expressions 'list_idx == e_outfit' 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 has found that two values are assigned to one variable in a row. In this case, it looks like it's just a dead code and should be deleted.
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 of arguments are passed to the printf function: the format '% s' indicates that a pointer must be passed to the string. Such a situation may lead to a memory access error and emergency program termination.
Conclusion
Checking the source code X-Ray Engine revealed a large number of both superfluous or suspicious code, as well as uniquely erroneous and dangerous moments. It is worth noting that the static analyzer perfectly helps to detect errors at an early stage of development, which makes life much easier for the programmer and frees up time to create new versions of the application.
If you want to share this article with an English-speaking audience, then please use the link to the translation: Pavel Belikov. Anomalies in X-Ray Engine .
Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.
Source: https://habr.com/ru/post/303664/
All Articles