PVS-studio team

  • Content count

  • Joined

  • Last visited

Community Reputation

1886 Excellent

About PVS-studio team

  • Rank

Personal Information

  1. Introduction CryEngine is a game engine created by the German company Crytek in the year 2002, and originally used in the first-person shooter Far Cry. There are a lot of great games made on the basis of different versions of CryEngine, by many studios who have licensed the engine: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve and many others. In March 2016 the Crytek company announced the release of the new CryEngine V, and soon after, posted the source code on GitHub. To perform the source code analysis, we used the PVS-Studio for Linux. Now, it has become even more convenient for the developers of cross-platform projects to track the quality of their code, with one static analysis tool. The Linux version can be downloaded as an archive, or a package for a package manager. You can set up the installation and update for the majority of distributions, using our repository. This article only covers the general analysis warnings, and only the "High" certainty level (there are also "Medium" and "Low"). To be honest, I didn't even examine all of the "High" level warnings, because there was already enough material for an article after even a quick look. I started working on the article several times over a period of a few months, so I can say with certainty that the bugs described here have living in the code for some months already. Some of the bugs that had been found during the previous check of the project, also weren't fixed. It was very easy to download and check the source code in Linux. Here is a list of all necessary commands: mkdir ~/projects && cd ~/projects git clone https://github.com/CRYTEK/CRYENGINE.git cd CRYENGINE/ git checkout main chmod +x ./download_sdks.py ./download_sdks.py pvs-studio-analyzer trace -- \ sh ./cry_waf.sh build_linux_x64_clang_profile -p gamesdk pvs-studio-analyzer analyze \ -l /path/to/PVS-Studio.lic \ -o ~/projects/CRYENGINE/cryengine.log \ -r ~/projects/CRYENGINE/ \ -C clang++-3.8 -C clang-3.8 \ -e ~/projects/CRYENGINE/Code/SDKs \ -j4 plog-converter -a GA:1,2 -t tasklist \ -o ~/projects/CRYENGINE/cryengine_ga.tasks \ ~/projects/CRYENGINE/cryengine.log The report file cryengine_ga.tasks can be opened and viewed in QtCreator. What did we manage to find in the source code of CryEngine V? A strange Active() function V501 There are identical sub-expressions to the left and to the right of the '==' operator: bActive == bActive LightEntity.h 124 void SetActive(bool bActive) { if (bActive == bActive) return; m_bActive = bActive; OnResetState(); } The function does nothing because of a typo. It seems to me that if there was a contest, "Super Typo", this code fragment would definitely take first place. I think this error has every chance to get into the section, "C/C++ bugs of the month". But that's not all, here is a function from another class: V501 There are identical sub-expressions 'm_staticObjects' to the left and to the right of the '||' operator. FeatureCollision.h 66 class CFeatureCollision : public CParticleFeature { public: CRY_PFX2_DECLARE_FEATURE public: CFeatureCollision(); .... bool IsActive() const { return m_terrain || m_staticObjects || m_staticObjects; } .... bool m_terrain; bool m_staticObjects; bool m_dynamicObjects; }; The variable m_staticObjects is used twice in the function IsActive(), although there is an unused variable m_dynamicObjects. Perhaps, it was this variable that was meant to be used. Code above has no bugs V547 Expression 'outArrIndices < 0' is always false. Unsigned type value is never < 0. CGFLoader.cpp 881 static bool CompactBoneVertices(...., DynArray& outArrIndices, ....) // begin(), &meshData), (uint8)std::distance(meshData....->....begin(), &chunk), (uint8)std::distance(meshData.m_instances.begin(), &instance) }; memcpy(hashableData, pCREGeomCache, sizeof(pCREGeomCache)); .... } Pay attention to the arguments of the memcpy() function. The programmer plans to copy the object pCREGeomCache to the array hashableData, but he accidentally copies not the size of the object, but the size of the pointer using the sizeof operator. Due to the error, the object is not copied completely, only 4 or 8 bytes. V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'this' class object. ClipVolumeManager.cpp 145 void CClipVolumeManager::GetMemoryUsage(class ICrySizer* pSizer) const { pSizer->AddObject(this, sizeof(this)); for (size_t i = 0; i < m_ClipVolumes.size(); ++i) pSizer->AddObject(m_ClipVolumes.m_pVolume); } A similar mistake was made when the programmer evaluated the size of this pointer instead of the size of a class. Correct variant: sizeof(*this). V530 The return value of function 'release' is required to be utilized. ClipVolumes.cpp 492 vector m_jitteredDepthPassArray; void CClipVolumesStage::PrepareVolumetricFog() { .... for (int32 i = 0; i < m_jitteredDepthPassArray.size(); ++i) { m_jitteredDepthPassArray.release(); } m_jitteredDepthPassArray.resize(depth); for (int32 i = 0; i < depth; ++i) { m_jitteredDepthPassArray = CryMakeUnique(); m_jitteredDepthPassArray->SetViewport(viewport); m_jitteredDepthPassArray->SetFlags(....); } .... } If we look at the documentation for the class std::unique_ptr, the release() function should be used as follows: std::unique_ptr up(new Foo()); Foo* fp = up.release(); delete fp; Most likely, it was meant to use the reset() function instead of the release() one. V549 The first argument of 'memcpy' function is equal to the second argument. ObjectsTree_Serialize.cpp 1135 void COctreeNode::LoadSingleObject(....) { .... float* pAuxDataDst = pObj->GetAuxSerializationDataPtr(....); const float* pAuxDataSrc = StepData(....); memcpy(pAuxDataDst, pAuxDataDst, min(....) * sizeof(float)); .... } It was forgotten, to pass pAuxDataSrc to the memcpy() function. Instead of this, the same variable pAuxDataDst is used as both source and destination. No one is immune to errors. By the way, those who are willing, may test their programming skills and attentiveness, by doing a quiz on the detection of similar bugs: q.viva64.com. Strange code V501 There are identical sub-expressions to the left and to the right of the '||' operator: val == 0 || val == - 0 XMLCPB_AttrWriter.cpp 363 void CAttrWriter::PackFloatInSemiConstType(float val, ....) { uint32 type = PFSC_VAL; if (val == 0 || val == -0) // pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef(); for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // next = pmd; .... } One more code fragment that remained uncorrected since the last project check. But it is still unclear if this is a formatting error, or a mistake in logic. About pointers V522 Dereferencing of the null pointer 'pCEntity' might take place. BreakableManager.cpp 2396 int CBreakableManager::HandlePhysics_UpdateMeshEvent(....) { CEntity* pCEntity = 0; .... if (pmu && pSrcStatObj && GetSurfaceType(pSrcStatObj)) { .... if (pEffect) { .... if (normal.len2() > 0) pEffect->Spawn(true, pCEntity->GetSlotWorldTM(...); // GetPhysicalProxy()) return 1; } .... } The analyzer detected null pointer dereference. The code of the function is written or refactored in such a way that there is now a branch of code, where the pointer pCEntity will be, initialized by a zero. Now let's have a look at the variant of a potential dereference of a null pointer. V595 The 'pTrack' pointer was utilized before it was verified against nullptr. Check lines: 60, 61. AudioNode.cpp 60 void CAudioNode::Animate(SAnimContext& animContext) { .... const bool bMuted = gEnv->IsEditor() && (pTrack->GetFlags() & IAnimTrack::eAnimTrackFlags_Muted); if (!pTrack || pTrack->GetNumKeys() == 0 || pTrack->GetFlags() & IAnimTrack::eAnimTrackFlags_Disabled) { continue; } .... } The author of this code first used the pointer pTrack, but its validity is checked on the next line of code before the dereference. Most likely, this is not how the program should work. There were a lot of V595 warnings, they won't really fit into the article. Very often, such code is a real error, but thanks to luck, the code works correctly. V571 Recurring check. The 'if (rLightInfo.m_pDynTexture)' condition was already verified in line 69. ObjMan.cpp 70 // Safe memory helpers #define SAFE_RELEASE(p){ if (p) { (p)->Release(); (p) = NULL; } } void CObjManager::UnloadVegetationModels(bool bDeleteAll) { .... SVegetationSpriteLightInfo& rLightInfo = ....; if (rLightInfo.m_pDynTexture) SAFE_RELEASE(rLightInfo.m_pDynTexture); .... } In this fragment there is no serious error, but it is not necessary to write extra code, if the corresponding checks are already included in the special macro. One more fragment with redundant code: V571 Recurring check. The 'if (m_pSectorGroups)' condition was already verified in line 48. PartitionGrid.cpp 50 V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. SystemInit.cpp 4045 class CLvlRes_finalstep : public CLvlRes_base { .... for (;; ) { if (*p == '/' || *p == '\\' || *p == 0) { char cOldChar = *p; *p = 0; // create zero termination _finddata_t fd; bool bOk = FindFile(szFilePath, szFile, fd); if (bOk) assert(strlen(szFile) == strlen(fd.name)); *p = cOldChar; // get back the old separator if (!bOk) return; memcpy((void*)szFile, fd.name, strlen(fd.name)); //
  2. Anomalies in X-Ray Engine

    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; ir_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 > 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
  3. Unity3D is one of the most promising and rapidly developing game engines to date. Every now and then, the developers upload new libraries and components to the official repository, many of which weren't available in as open-source projects until recently. Unfortunately, the Unity3D developer team allowed the public to dissect only some of the components, libraries, and demos employed by the project, while keeping the bulk of its code closed. In this article, we will try to find bugs and typos in those components with the help of PVS-Studio static analyzer. Introduction We decided to check all the components, libraries, and demos in C#, whose source code is available in the Unity3D developer team's official repository: UI System- system for GUI development. Networking- system for implementing multiplayer mode. MemoryProfiler - system for profiling resources in use. XcodeAPI- component for interacting with the Xcode IDE. PlayableGraphVisualizer - system for project execution visualization. UnityTestTools - Unity3D testing utilities (no Unit tests included). AssetBundleDemo - project with AssetBundleServer's source files and demos for AssetBundle system. AudioDemos - demo projects for the audio system. NativeAudioPlugins - audio plugins (we are only interested in the demos for these plugins). GraphicsDemos - demo projects for the graphics system. I wish we could take a look at the source files of the engine's kernel itself, but, unfortunately, no one except the developers themselves have access to it currently. So, what we've got on our operating table today is just a small part of the engine's source files. We are most interested in the UI system designed for implementing a more flexible GUI than the older, clumsy one, and the networking library, which served us hand and foot before UNet's release. We are also as much interested in MemoryProfiler, which is a powerful and flexible tool for resource and load profiling. Errors and suspicious fragments found All warnings issued by the analyzer are grouped into 3 levels: High - almost certainly an error. Medium - possible error or typo. Low - unlikely error or typo. We will be discussing only the high and medium levels in this article. The table below presents the list of projects we have checked and analysis statistics across all the projects. The columns "Project name" and "Number of LOC" are self-explanatory, but the "Issued Warnings" column needs some explanation. It contains information about all the warnings issued by the analyzer. Positive warnings are warnings that directly or indirectly point to real errors or typos in the code. False warnings, or false positives, are those that interpret correct code as faulty. As I already said, all warnings are grouped into 3 levels. We will be discussing only the high- and medium-level warnings, since the low level mostly deals with information messages or unlikely errors. For the 10 projects checked, the analyzer issued 16 high-level warnings, 75% of which correctly pointed to real defects in the code, and 18 medium-level warnings, 39% of which correctly pointed to real defects in the code. The code is definitely of high quality, as the average ratio of errors found to the number of LOC is one error per 2000 lines of code, which is a good result. Now that we are done with the statistics, let's see what errors and typos we managed to find. Incorrect regular expression V3057 Invalid regular expression pattern in constructor. Inspect the first argument. AssetBundleDemo ExecuteInternalMono.cs 48 private static readonly Regex UnsafeCharsWindows = new Regex("[^A-Za-z0-9\\_\\-\\.\\:\\,\\/\\@\\\\]"); //
  4. In May 2016, German game-development company Crytek made a decision to upload the source code of their game engine CryEngine V to Github. The engine is written in C++ and has immediately attracted attention of both the open-source developer community and the team of developers of PVS-Studio static analyzer who regularly scan the code of open-source projects to estimate its quality. A lot of great games were created by a number of video-game development studios using various versions of CryEngine, and now the engine has become available to even more developers. This article gives an overview of errors found in the project by PVS-Studio static analyzer. Introduction CryEngine is a game engine developed by German company Crytek in 2002 and originally used in first-person shooter Far Cry. A lot of great games were created by a number of video-game development studios using various licensed versions of CryEngine: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve, and many others. In March 2016, Crytek announced a release date for their new engine CryEngine V and uploaded its source code to Github soon after. The project's source code was checked by PVS-Studio static analyzer, version 6.05. This is a tool designed for detecting software errors in program source code in C, C++, and C#. The only true way of using static analysis is to regularly scan code on developers' computers and build-servers. However, in order to demonstrate PVS-Studio's diagnostic capabilities, we run single-time checks of open-source projects and then write articles about errors found. If we like a project, we might scan it again a couple of years later. Such recurring checks are in fact the same as single-time checks since the code accumulates a lot of changes during that time. For our checks, we pick projects that are simply popular and wide-known as well as projects suggested by our readers via e-mail. That's why CryEngine V was by no means the first game engine among those scanned by our analyzer. Other engines that we have already checked include: Unreal Engine 4 (first check, second check, third check) Check of Godot Engine Check of Serious Engine Check of X-Ray Engine Check of Xenko Engine We also checked CryEngine 3 SDK once. I'd like to elaborate on the check of Unreal Engine 4 engine in particular. Using that project as an example allowed us to demonstrate in every detail what the right way of using static analysis on a real project should look like, covering the whole process from the phase of integrating the analyzer into the project to the phase of cutting warnings to zero with subsequent control over bug elimination in new code. Our work on Unreal Engine 4 project developed into collaboration with Epic Games company, in terms of which our team fixed all the defects found in the engine's source code and wrote a joint article with Epic Games on the accomplished work (it was posted on Unreal Engine Blog). Epic Games also purchased a PVS-Studio license to be able to maintain the quality of their code on their own. Collaboration of this kind is something that we would like to try with Crytek, too. Analyzer-report structure In this article, I'd like to answer a few frequently asked questions concerning the number of warnings and false positives, for example, "What is the ratio of false positives?" or "Why are there so few bugs in so large a project?" To begin with, all PVS-Studio warnings are classified into three severity levels: High, Medium, and Low. The High level holds the most critical warnings, which are almost surely real errors, while the Low level contains the least critical warnings or warnings that are very likely to be false positives. Keep in mind that the codes of errors do not tie them firmly to particular severity levels: distribution of warnings across the levels very much depends on the context. This is how the warnings of the General Analysis module are distributed across the severity levels for CryEngine V project: High: 576 warnings; Medium: 814 warnings, Low: 2942 warnings. Figure 1 shows distribution of the warnings across the levels in the form of a pie chart. Figure 1 - Percentage distribution of warnings across severity levels It is impossible to include all the warning descriptions and associated code fragments in an article. Our articles typically discuss 10-40 commented cases; some warnings are given as a list; and most have to be left unexamined. In the best-case scenario, project authors, after we inform them, ask for a complete analysis report for close study. The bitter truth is that in most cases the number of High-level warnings alone is more than enough for an article, and CryEngine V is no exception. Figure 2 shows the structure of the High-level warnings issued for this project. Figure 2 - Structure of High-level warnings Let's take a closer look at the sectors of this chart: Described in the article (6%) - warnings cited in the article and accompanied by code fragments and commentary. Presented as a list (46%) - warnings cited as a list. These warnings refer to the same pattern as some of the errors already discussed, so only the warning text is given. False Positives (8%) - a certain ratio of false positives we have taken into account for future improvement of the analyzer. Other (40%) - all the other warnings issued. These include warnings that we had to leave out so that the article wouldn't grow too large, non-critical warnings, or warnings whose validity could be estimated only by a member of the developer team. As our experience of working on Unreal Engine 4 has shown, such code still "smells" and those warnings get fixed anyway. Analysis results Annoying copy-paste V501 There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z - q2.v.z entitynode.cpp 93 bool CompareRotation(const Quat& q1, const Quat& q2, float epsilon) { return (fabs_tpl(q1.v.x - q2.v.x) SRenderingPassInfo::SRenderingPassInfo(....)' should be used. i3dengine.h 2589 SRenderingPassInfo() : pShadowGenMask(NULL) , nShadowSide(0) , nShadowLod(0) , nShadowFrustumId(0) , m_bAuxWindow(0) , m_nRenderStackLevel(0) , m_eShadowMapRendering(static_cast(SHADOW_MAP_NONE)) , m_bCameraUnderWater(0) , m_nRenderingFlags(0) , m_fZoomFactor(0.0f) , m_pCamera(NULL) , m_nZoomInProgress(0) , m_nZoomMode(0) , m_pJobState(nullptr) { threadID nThreadID = 0; gEnv->pRenderer->EF_Query(EFQ_MainThreadList, nThreadID); m_nThreadID = static_cast(nThreadID); m_nRenderFrameID = gEnv->pRenderer->GetFrameID(); m_nRenderMainFrameID = gEnv->pRenderer->GetFrameID(false); } SRenderingPassInfo(threadID id) { SRenderingPassInfo(); // GetSpawnData(i); const float amount = spawn.amount; const int spawnedBefore = int(spawn.spawned); const float endTime = spawn.delay + HasDuration() ? spawn.duration : fHUGE; .... } The function above seems to measure time in a wrong way. The precedence of the addition operator is higher than that of the ternary operator :?, so the value 0 or 1 is added to spawn.delay first, and then the value spawn.duration or fHUGE is written into the endTime variable. This error is quite a common one. To learn more about interesting patterns of errors involving operation precedence collected from the PVS-Studio bug database, see my article: Logical Expressions in C/C++. Mistakes Made by Professionals. V634 The priority of the '*' operation is higher than that of the 'GetLightProfileCounts().ResetFrameTicks(); if (passInfo.IsGeneralPass() && m_pPartManager) m_pPartManager->Update(); .... } The m_pPartManager pointer is dereferenced and then checked. Example 2 V595 The 'gEnv->p3DEngine' pointer was utilized before it was verified against nullptr. Check lines: 1477, 1480. gameserialize.cpp 1477 bool CGameSerialize::LoadLevel(....) { .... // can quick-load if (!gEnv->p3DEngine->RestoreTerrainFromDisk()) return false; if (gEnv->p3DEngine) { gEnv->p3DEngine->ResetPostEffects(); } .... } The gEnv->p3DEngine pointer is dereferenced and then checked. Example 3 V595 The 'pSpline' pointer was utilized before it was verified against nullptr. Check lines: 158, 161. facechannelkeycleanup.cpp 158 void FaceChannel::CleanupKeys(....) { CFacialAnimChannelInterpolator backupSpline(*pSpline); // Create the key entries array. int numKeys = (pSpline ? pSpline->num_keys() : 0); .... } The pSpline pointer is dereferenced and then checked. Miscellaneous V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. mergedmeshrendernode.cpp 999 static inline void ExtractSphereSet(....) { .... switch (statusPos.pGeom->GetType()) { if (false) { case GEOM_CAPSULE: statusPos.pGeom->GetPrimitive(0, &cylinder); } if (false) { case GEOM_CYLINDER: statusPos.pGeom->GetPrimitive(0, &cylinder); } for (int i = 0; i < 2 && ....; ++i) { .... } break; .... } This fragment is probably the strangest of all found in CryEngine V. Whether or not the case label will be selected does not depend on the if statement, even in case of if (false). In the switch statement, an unconditional jump to the label occurs if the condition of the switch statement is met. Without the break statement, one could use such code to "bypass" irrelevant statements, but, again, maintaining such obscure code isn't easy. One more question is, why does the same code execute when jumping to the labels GEOM_CAPSULE and GEOM_CYLINDER? V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_action.cpp 143 typedef CryStringT<char> string; // The actual fragment name. string m_fragName; //! cast to C string. const value_type* c_str() const { return m_str; } const value_type* data() const { return m_str; }; void LogError(const char* format, ...) const { .... } void QueueAction(const UpdateContext& context) { .... ErrorReporter(*this, context).LogError("....'%s'", m_fragName); .... } When it is impossible to specify the number and types of all acceptable parameters to a function, one puts ellipsis (...) at the end of the list of parameters in the function declaration, which means "and perhaps a few more". Only POD (Plain Old Data) types can be used as actual parameters to the ellipsis. If an object of a class is passed as an argument to a function's ellipsis, it almost always signals the presence of a bug. In the code above, it is the contents of the object that get to the stack, not the pointer to a string. Such code results in forming "gibberish" in the buffer or a crash. The code of CryEngine V uses a string class of its own, and it already has an appropriate method, c_str(). The fixed version: LogError("....'%s'", m_fragName.c_str(); A few more suspicious fragments: V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 1339 V510 The 'Format' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 2648 V510 The 'CryWarning' function is not expected to receive class-type variable as sixth actual argument. crypak.cpp 3324 V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. crypak.cpp 3333 V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4864 V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4931 V510 The 'Format' function is not expected to receive class-type variable as third actual argument. featuretester.cpp 1727 V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1314 int CTriMesh::Slice(....) { .... bop_meshupdate *pmd = new bop_meshupdate, *pmd0; pmd->pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef(); for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // next = pmd; .... } This code is very strange. The programmer put a semicolon after the for loop, while the code formatting suggests that it should have a body. V535 The variable 'j' is being used for this loop and for the outer loop. Check lines: 3447, 3490. physicalworld.cpp 3490 void CPhysicalWorld::SimulateExplosion(....) { .... for(j=0;jnIslands;j++) // GetBreakability() == 2 && pCollision->idmat[0] != pCollision->idmat[1] && (energy = pMat->GetBreakEnergy()) > 0 && pCollision->mass[0] * 2 > energy && .... pMat->GetHitpoints()
  5. The first-person shooter 'Serious Sam' celebrated its release anniversary on March, 2016. In honor of this, the game developers form the Croatian company Croteam decided to open the source code for the game engine, Serious Engine 1 v.1.10. It provoked the interest of a large number of developers, who got an opportunity to have a look at the code and improve it. I have also decided to participate in the code improvement, and wrote an article reviewing the bugs that were found by PVS-Studio analyzer. Introduction Serious Engine is a game engine developed by a Croatian company Croteam. V 1.1o, and was used in the games 'Serious Sam Classic: The First Encounter', and 'Serious Sam Classic: The Second Encounter'. Later on, the Croteam Company released more advanced game engines - Serious Engine 2, Serious Engine 3, and Serious Engine 4; the source code of Serious Engine version 1.10 was officially made open and available under the license GNU General Public License v.2 The project is easily built in Visual Studio 2013, and checked by PVS-Studio 6.02 static analyzer. Typos! V501 There are identical sub-expressions to the left and to the right of the '==' operator: tp_iAnisotropy == tp_iAnisotropy gfx_wrapper.h 180 class CTexParams { public: inline BOOL IsEqual( CTexParams tp) { return tp_iFilter == tp.tp_iFilter && tp_iAnisotropy == tp_iAnisotropy && //
  6. Sony C#/.NET Component Set Analysis

    Some of you may know that we have recently released version 6.00 of our analyzer, that now has C# support. The ability to scan C# projects increases the number of open-source projects we can analyze. This article is about one such check. This time it is a project, developed by Sony Computer Entertainment (SCEI). What have we checked? Sony Computer Entertainment is a video game company. Being a branch of Sony Corporation, it specializes in video games and game consoles. This company develops video games, hardware and software for PlayStation consoles. Authoring Tools Framework (ATF) is a set of C#/.NET components for making tools on Windows(R). ATF has been used by most Sony Computer Entertainment first party game studios to make custom tools. This component set is used by such studios as Naughty Dog, Guerrilla Games and Quantic Dream. Tools developed with these program components were used during the creation of such well know games as 'The Last of Us' and 'Killzone'. ATF is an open-source project which is available at this GitHub repository. Analysis tool To perform the source code analysis, we used PVS-Studio static code analyzer. This tool scans projects written in C/C++/C#. Every diagnostic message has a detailed description in the documentation with examples of incorrect code and possible ways to fix the bugs. Quite a number of the diagnostic descriptions have a link to corresponding sections of error base, where you can see information about the bugs that were found in real projects with the help of these diagnostics. You can download the analyzer here and run it on your (or somebody's) code. Examples of errors public static void DefaultGiveFeedback(IComDataObject data, GiveFeedbackEventArgs e) { .... if (setDefaultDropDesc && (DropImageType)e.Effect != currentType) { if (e.Effect != DragDropEffects.None) { SetDropDescription(data, (DropImageType)e.Effect, e.Effect.ToString(), null); } else { SetDropDescription(data, (DropImageType)e.Effect, e.Effect.ToString(), null); } .... } } Analyzer warning: V3004 The 'then' statement is equivalent to the 'else' statement. Atf.Gui.WinForms.vs2010 DropDescriptionHelper.cs 199 As you see in the code, the same method with similar arguments will be called, in spite of the e.Effect != DragDropEffects.None being true or not. It's hard to suggest any ways to fix this code fragment, without being a developer of this code, but I think it is clear that this fragment needs a more thorough revision. What exactly should be fixed, is a question which should be directed to the author of this code. Let's look at the following code fragment: public ProgressCompleteEventArgs(Exception progressError, object progressResult, bool cancelled) { ProgressError = ProgressError; ProgressResult = progressResult; Cancelled = cancelled; } Analyzer warning: V3005 The 'ProgressError' variable is assigned to itself. Atf.Gui.Wpf.vs2010 StatusService.cs 24 It was supposed that during the method call, the properties would get values, passed as arguments; at the same time the names of properties and parameters differ only in the first letter case. As a result - ProgressError property is assigned to itself instead of being given the progressError parameter. Quite interesting here, is the fact that it's not the only case where capital and small letters get confused. Several of the projects we've checked have the same issues. We suspect that we'll find a new error pattern typical for C# programs soon. There is a tendency to initialize properties in a method, where the names of parameters differ from the names of the initialized properties by just one letter. As a result, we have errors like this. The next code fragment is probably not erroneous, but it looks rather strange, to say the least. public double Left { get; set; } public double Top { get; set; } public void ApplyLayout(XmlReader reader) { .... FloatingWindow window = new FloatingWindow( this, reader.ReadSubtree()); .... window.Left = window.Left; window.Top = window.Top; .... } Analyzer warning: V3005 The 'window.Left' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 706 | V3005 The 'window.Top' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 707 In the analyzer warnings you can see that the window object properties Left and Top are assigned to themselves. In some cases this variant is perfectly appropriate, for example when the property access method has special logic. But there is no additional logic for these properties, and so it is unclear why the code is written this way. Next example: private static void OnBoundPasswordChanged(DependencyObject d, DependencyPropertyChangedEventArgs e) { PasswordBox box = d as PasswordBox; if (d == null || !GetBindPassword(d)) { return; } // avoid recursive updating by ignoring the box's changed event box.PasswordChanged -= HandlePasswordChanged; .... } Analyzer warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'd', 'box'. Atf.Gui.Wpf.vs2010 PasswordBoxBehavior.cs 38 We have already seen quite a number of errors of this type in the C# projects that we checked. By casting an object to a compatible type using as operator the programmer gets a new object, but further in the code the source object is compared to null. This code can work correctly, if you are sure that the d object will always be compatible with the PasswordBox type. But it is not so (for now or if there are more changes in the program); you can easily get NullReferenceException in code that used to work correctly. So in any case this code needs to be reviewed. In the following example, conversely, the programmer clearly tried to make the code as secure as possible, although it's not quite clear what for. public Rect Extent { get { return _extent; } set { if (value.Top < -1.7976931348623157E+308 || value.Top > 1.7976931348623157E+308 || value.Left < -1.7976931348623157E+308 || value.Left > 1.7976931348623157E+308 || value.Width > 1.7976931348623157E+308 || value.Height > 1.7976931348623157E+308) { throw new ArgumentOutOfRangeException("value"); } _extent = value; ReIndex(); } } Analyzer warning: V3022 Expression is always false. Atf.Gui.Wpf.vs2010 PriorityQuadTree.cs 575 This condition will always be false. Let's look into the code and see why. This is an implementation of the property that has Rect type, hence value also has Rect type. Top, Left, Width, Height are properties of this type, that have double type. This code checks if these property values exceed the range of values, that the double type takes. We also see that "magic numbers" are used here for comparison, not constants, defined in the double type. That's why this condition will always be false, since the double type values are always within the value range. Apparently, the programmer wanted to secure the program from a non-standard implementation of double type in the compiler. Nevertheless, it looks rather strange, so it was reasonable for the analyzer to issue a warning, suggesting that the programmer double-check the code. Let's go on. public DispatcherOperationStatus Status { get; } public enum DispatcherOperationStatus { Pending, Aborted, Completed, Executing } public object EndInvoke(IAsyncResult result) { DispatcherAsyncResultAdapter res = result as DispatcherAsyncResultAdapter; if (res == null) throw new InvalidCastException(); while (res.Operation.Status != DispatcherOperationStatus.Completed || res.Operation.Status == DispatcherOperationStatus.Aborted) { Thread.Sleep(50); } return res.Operation.Result; } Analyzer warning: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. Atf.Gui.Wpf.vs2010 SynchronizeInvoke.cs 74 The condition of the while loop is redundant, it could be simplified by removing the second subexpression. Then the loop can be simplified in the following way: while (res.Operation.Status != DispatcherOperationStatus.Completed) Thread.Sleep(50); Next example, quite an interesting one: private Vec3F ProjectToArcball(Point point) { float x = (float)point.X / (m_width / 2); // Scale so bounds map // to [0,0] - [2,2] float y = (float)point.Y / (m_height / 2); x = x - 1; // Translate 0,0 to the center y = 1 - y; // Flip so +Y is up if (x < -1) x = -1; else if (x > 1) x = 1; if (y < -1) y = -1; else if (y > 1) y = 1; .... } Analyzer warning: V3041 The 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;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 216 | V3041 The 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;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 217 This is one of those cases where it's very hard for a third-party developer to say for sure if there is an error in this code or not. On the one hand - integer division with implicit casting to a real type looks strange. On the other hand, sometimes it can be done deliberately, regardless of the precision loss. It's difficult to say what was meant here. Perhaps the programmer didn't want to lose the precision of the code, but it will still occur as a result of m_width / 2 operation. In this case we should rewrite the code in the following way: float x = point.X / ((float)m_width / 2); On the other hand, there is a possibility that an integer number was meant to be written to x, as further on we see operations of comparison with integer values. But in this case, there was no need to do explicit casting to float type. float x = point.X / (m_width / 2); Our analyzer continues developing and obtaining new diagnostics. The next error was found with the help of our new diagnostic. But as this diagnostic wasn't in the release version of the analyzer, there will be no link to the documentation, but I hope the idea is clear: public static QuatF Slerp(QuatF q1, QuatF q2, float t) { double dot = q2.X * q1.X + q2.Y * q1.Y + q2.Z * q1.Z + q2.W * q1.W; if (dot < 0) q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W; .... } Analyzer warning: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Atf.Core.vs2010 QuatF.cs 282 You can see that a sum of several products is evaluated and the result is written to the dot variable. After that if the dot value is negative, there is inversion of all values of this operation. More precisely, the inversion was meant to be here, judging by the code formatting. In reality, only X property of q1 will be inverted, all other properties will be inverted regardless of the value of dot variable. Solution for this problem is curly brackets: if (dot < 0) { q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W; } Let's go on. public float X; public float Y; public float Z; public void Set(Matrix4F m) { .... ww = -0.5 * (m.M22 + m.M33); if (ww >= 0) { if (ww >= EPS2) { double wwSqrt = Math.Sqrt(ww); X = (float)wwSqrt; ww = 0.5 / wwSqrt; Y = (float)(m.M21 * ww); Z = (float)(m.M31 * ww); return; } } else { X = 0; Y = 0; Z = 1; return; } X = 0; ww = 0.5 * (1.0f - m.M33); if (ww >= EPS2) { double wwSqrt = Math.Sqrt(ww); Y = (float)wwSqrt; //
  7. Not All is Fine in the Morrowind Universe

    Guys, if you have suggestions about the projects that can be checked by our analyzer, please write to us! We are ready to fight:). We have started supporting C# from the 6.00 version, so C# open-source projects are also welcome.
  8. Not All is Fine in the Morrowind Universe

    Thank you for pointing to a new repository. We will check the new code very soon.    I have some news regarding the new source code. We scanned it and found very minor errors. So, congratulations to OpenMW! 
  9. I have checked the OpenMW project by PVS-Studio and written this tiny article. Very few bugs were found, so OpenMW team can be proud of their code. OpenMW OpenMW is an attempt to reconstruct the popular RPG Morrowind, a full-blown implementation of all of the game's specifics with open source code. To run OpenMW, you will need an original Morrowind disk. The source code can be downloaded from https://code.google.com/p/openmw/ Suspicious fragments found Fragment No. 1 std::string getUtf8(unsigned char c, ToUTF8::Utf8Encoder& encoder, ToUTF8::FromType encoding) { .... conv[0xa2] = 0xf3; conv[0xa3] = 0xbf; conv[0xa4] = 0x0; conv[0xe1] = 0x8c; conv[0xe1] = 0x8c; mData.mFlags & ESM::MagicEffect::NoDuration) ) Fragment No. 3 void Clothing::blank() { mData.mType = 0; mData.mWeight = 0; mData.mValue = 0; mData.mEnchant = 0; mParts.mParts.clear(); mName.clear(); mModel.clear(); mIcon.clear(); mIcon.clear(); mEnchant.clear(); mScript.clear(); } PVS-Studio diagnostic message: V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 48, 49. components loadclot.cpp 49 The mIcon object is cleared twice. The second clearing is redundant or something else should have been cleared instead. Fragment No. 4 void Storage::loadDataFromStream( ContainerType& container, std::istream& stream) { std::string line; while (!stream.eof()) { std::getline( stream, line ); .... } .... } PVS-Studio diagnostic message: V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression. components translation.cpp 45 When working with the std::istream class, calling the eof() function to terminate the loop is not enough. If a failure occurs when reading data, the call of the eof() function will return false all the time. To terminate the loop in this case, you need an additional check of the value returned by fail(). Fragment No. 5 class Factory { .... bool getReadSourceCache() { return mReadSourceCache; } bool getWriteSourceCache() { return mReadSourceCache; } .... bool mReadSourceCache; bool mWriteSourceCache; .... }; PVS-Studio diagnostic message: V524 It is odd that the body of 'getWriteSourceCache' function is fully equivalent to the body of 'getReadSourceCache' function. components factory.hpp 209 I guess the getWriteSourceCache() function should look like this: bool getWriteSourceCache() { return mWriteSourceCache; } Fragments No. 6, 7, 8 std::string rangeTypeLabel(int idx) { const char* rangeTypeLabels [] = { "Self", "Touch", "Target" }; if (idx >= 0 && idx = 0 && idx < 3) A similar defect was found in two other fragments:V557 Array overrun is possible. The value of 'idx' index could reach 143. esmtool labels.cpp 391V557 Array overrun is possible. The value of 'idx' index could reach 27. esmtool labels.cpp 475 Fragment No. 9 enum UpperBodyCharacterState { UpperCharState_Nothing, UpperCharState_EquipingWeap, UpperCharState_UnEquipingWeap, .... }; bool CharacterController::updateWeaponState() { .... if((weaptype != WeapType_None || UpperCharState_UnEquipingWeap) && animPlaying) .... } PVS-Studio diagnostic message: V560 A part of conditional expression is always true: UpperCharState_UnEquipingWeap. openmw character.cpp 949 This condition is very strange. In its current form, it can be reduced to if (animPlaying). Something is obviously wrong with it. Fragments No. 10, 11 void World::clear() { mLocalScripts.clear(); mPlayer->clear(); .... if (mPlayer) .... } PVS-Studio diagnostic message: V595 The 'mPlayer' pointer was utilized before it was verified against nullptr. Check lines: 234, 245. openmw worldimp.cpp 234 Similar defect: V595 The mBody pointer was utilized before it was verified against nullptr. Check lines: 95, 99. openmw physic.cpp 95 Fragment No. 12 void ExprParser::replaceBinaryOperands() { .... if (t1==t2) mOperands.push_back (t1); else if (t1=='f' || t2=='f') mOperands.push_back ('f'); else std::logic_error ("failed to determine result operand type"); } PVS-Studio diagnostic message: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw logic_error(FOO); components exprparser.cpp 101 The keyword throw is missing. The fixed code should look like this: throw std::logic_error ("failed to determine result operand type"); Conclusion Purchase a PVS-Studio for in your team, and you will save huge amount of time usually spent on eliminating typos and diverse bugs.
  10. Not All is Fine in the Morrowind Universe

    Thank you for the feedback! We reqularly check PVS-Studio with the help of PVS-Studio. So, you can be sure, it has no errors:)  Also, we has an article that desribes how we test our tool http://www.viva64.com/en/a/0047/#ID0EMQAE
  11. Not All is Fine in the Morrowind Universe

    Thank you for the feedback. We will be more accurate next time and ask the editor to check our article once more. Also, I will tell you a small story, that I hope you will find funny. The word "blatant" if you transliterate it in Russian sounds like the word from the jail world. This word means a person/group that is conntected with criminality and has some respect. I am not a linguist but it looks luke this word has interesting etiminology:)
  12. Not All is Fine in the Morrowind Universe

    Thank you for your constructive feedback. In the nearest future, we are planning to recheck the source code of OpenMV and compare the dynamics of the first bug report and the second bug report. Right now our tool does't provide any interesting statistics reposrts, but it is a good idea.    Speaking about interesting bugs statistics in open-source projects, I advise you to review this article http://www.viva64.com/en/b/0300/ It should be interesting.
  13. Not All is Fine in the Morrowind Universe

    Thank you for pointing to a new repository. We will check the new code very soon. 
  14. The majority of the projects we report about in these articles contain dozens of PVS-Studio analyzer warnings. Of course we choose just a small portion of data from the analyzer report to be in our articles. There are some projects though, where the quantity of warnings is not that high and the number of some interesting "bloomers" is just not enough for an article. Usually these are small projects, which ceased developing. Today I'm going to tell you about Appleseed project check, the code of which we found quite high-quality, from the point of view of the analyzer. Introduction Appleseed is a modern, open source, physically-based rendering engine designed to produce photorealistic images, animations and visual effects. It provides individuals and small studios with an efficient, reliable suite of tools built on robust foundations and open technologies. This project contains 700 source code files. Our PVS-Studio analyzer found just several warnings of 1st and 2nd level that could be of interest to us. Check Results V670 The uninitialized class member m_s0_cache is used to initialize the m_s1_element_swapper member. Remember that members are initialized in the order of their declarations inside a class. animatecamera cache.h 1009 class DualStageCache : public NonCopyable { .... S1ElementSwapper m_s1_element_swapper; //