Jump to content
  • Advertisement


  • Content Count

  • Joined

  • Last visited

Community Reputation

1471 Excellent

About Code_Analysis

  • Rank

Personal Information

  1. Our company develops, promotes, and sells the PVS-Studio static code analyzer for C/C++ programmers. However, our collaboration with customers is not limited solely to selling PVS-Studio licenses. For example, we often take on contract projects as well. Due to NDAs, we're not usually allowed to reveal details about this work, and you might not be familiar with the projects names, anyway. But this time, we think you'll be excited by our latest collaboration. Together with Epic Games, we 're working on the Unreal Engine project. This is what we're going to tell you about in this article: https://www.unrealengine.com/blog/how-pvs-studio-team-improved-unreal-engines-code    
  2. Code_Analysis

    12 mistakes of The Powder Toy Simulator

  3. On March 19, 2014, Unreal Engine 4 was made publicly available. Subscription costs only $19 per month. The source codes have also been published at the github repository. Since that moment, we have received quite a number of e-mails, twitter messages, etc., people asking to check this game engine. So we are fulfilling our readers' request in this article; let's see what interesting bugs the PVS-Studio static code analyzer has found in the project's source code. Unreal Engine The Unreal Engine is a game engine developed by Epic Games, first illustrated in the 1998 first-person shooter game Unreal. Although primarily developed for the first-person shooters, it has been successfully used in a variety of other genres, including stealth, MMORPGs, and other RPGs. With its code written in C++, Unreal Engine features a high degree of portability and is a tool used by many game developers today. The official website: https://www.unrealengine.com/ The Wikipedia article: Unreal Engine. Analysis methodology for an nmake-based project There exist certain difficulties regarding analysis of the Unreal Engine project. To check it, we had to use a new feature recently introduced in PVS-Studio Standalone. Because of that, we had to postpone the publication of this article a bit so that it would follow the release of the new PVS-Studio version with this feature. I guess many would like to try it: it allows programmers to easily check projects that make use of complex or non-standard build systems. PVS-Studio's original working principle is as follows:You open a project in Visual Studio.Click the "Start" button.The Visual Studio-integrated plugin collects all the necessary information: which files need to be analyzed, which macros are to be expanded, where the header files location, and so on.The plugin launches the analyzer module itself and outputs the analysis results.What's special about Unreal Engine 4 is that it is an nmake-based project, therefore it can't be checked by the PVS-Studio plugin. Let me explain this point. Unreal Engine is implemented as a Visual Studio project, but the build is done with nmake. It means that the plugin cannot know which files are compiled with which switches. Therefore, analysis is impossible. To be exact, it is possible, but it will be somewhat of an effort (see the documentation section, "Direct integration of the analyzer into build automation systems"). And here's PVS-Studio Standalone coming to help! It can work in two modes: You obtain preprocessed files in any way and let the tool check them.Its monitoring compiler calls and get all the necessary information. It is the second mode that we are interested in now. This is how the check of Unreal Engine was done: We launched PVS-Studio Standalone.Clicked "Compiler Monitoring".Then we clicked "Start Monitoring" and made sure the compiler call monitoring mode was on.We opened the Unreal Engine project in Visual Studio and started the project build. The monitoring window indicated that the compiler calls were being tapped.When the build was finished, we clicked Stop Monitoring, and after that the PVS-Studio analyzer was launched. The diagnostic messages were displayed in the PVS-Studio Standalone window. Hint. It is more convenient to use Visual Studio instead of the PVS-Studio Standalone's editor to work with the analysis report. You only need to save the results into a log file and then open it in the Visual Studio environment (Menu->PVS-Studio->Open/Save->Open Analysis Report). All that and many other things are described in detail in the article "PVS-Studio Now Supports Any Build System under Windows and Any Compiler. Easy and Right Out of the Box". Do read this article please before you start experimenting with PVS-Studio Standalone! Analysis results I found the Unreal Engine project's code very high-quality. For example, developers employ static code analysis during the development, which is hinted at by the following code fragments: // Suppress static code analysis warning about a // potential comparison of two constants CA_SUPPRESS(6326); .... // Suppress static code analysis warnings about a // potentially ill-defined loop. BlendCount > 0 is valid. CA_SUPPRESS(6294) .... #if USING_CODE_ANALYSIS These code fragments prove that they use a static code analyzer integrated into Visual Studio. To find out more about this tool, see the article Visual Studio 2013 Static Code Analysis in depth: What? When and How? The project authors may also use some other analyzers, but I can't say for sure. So their code is pretty good. Since they use static code analysis tools during the development, PVS-Studio has not found many suspicious fragments. However, just like any other large project, this one does have some bugs, and PVS-Studio can catch some of them. So let's find out what it has to show us. Typos static bool PositionIsInside(....) { return Position.X >= Control.Center.X - BoxSize.X * 0.5f && Position.X = Control.Center.Y - BoxSize.Y * 0.5f && Position.Y >= Control.Center.Y - BoxSize.Y * 0.5f; } PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'Position.Y >= Control.Center.Y - BoxSize.Y * 0.5f' to the left and to the right of the '&&' operator. svirtualjoystick.cpp 97 Notice that the Position.Y variable is compared to the Control.Center.Y - BoxSize.Y * 0.5f expression twice. This is obviously a typo; the '-' operator should be replaced with '+' in the last line. Here's one more similar mistake in a condition: void FOculusRiftHMD::PreRenderView_RenderThread( FSceneView& View) { .... if (View.StereoPass == eSSP_LEFT_EYE || View.StereoPass == eSSP_LEFT_EYE) .... } PVS-Studio's diagnostic message: V501 There are identical sub-expressions 'View.StereoPass == eSSP_LEFT_EYE' to the left and to the right of the '||' operator. oculusrifthmd.cpp 1453 It seems that the work with Oculus Rift is not well tested yet. Let's go on. struct FMemoryAllocationStats_DEPRECATED { .... SIZE_T NotUsed5; SIZE_T NotUsed6; SIZE_T NotUsed7; SIZE_T NotUsed8; .... }; FMemoryAllocationStats_DEPRECATED() { .... NotUsed5 = 0; NotUsed6 = 0; NotUsed6 = 0; NotUsed8 = 0; .... } PVS-Studio's diagnostic message: V519 The 'NotUsed6' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 86, 88. memorybase.h 88 Structure members are initialized here. A typo causes the NotUsed6 member to be initialized twice, while the NotUsed7 member remains uninitialized. However, the _DEPRECATED() suffix in the function name tells us this code is not of much interest anymore. Here are two other fragments where one variable is assigned a value twice:V519 The HighlightText variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 204, 206. srichtextblock.cpp 206V519 The TrackError.MaxErrorInScaleDueToScale variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1715, 1716. animationutils.cpp 1716 Null pointers I pretty often come across null pointer dereferencing errors in error handlers. No wonder: these fragments are difficult and uninteresting to test. In Unreal Engine, you can find a null pointer dereferencing error in an error handler too: bool UEngine::CommitMapChange( FWorldContext &Context ) { .... LevelStreamingObject = Context.World()->StreamingLevels[j]; if (LevelStreamingObject != NULL) { .... } else { check(LevelStreamingObject); UE_LOG(LogStreaming, Log, TEXT("Unable to handle streaming object %s"), *LevelStreamingObject->GetName()); } .... } PVS-Studio's diagnostic message: V522 Dereferencing of the null pointer 'LevelStreamingObject' might take place. unrealengine.cpp 10768 We want to print the object name when an error occurs. But the object doesn't exist. Here's another fragment with null pointer dereferencing. It's all much more interesting here. Perhaps the error appeared because of an incorrect merge. Anyway, the comment proves that the code is incomplete: void FStreamingPause::Init() { .... if( GStreamingPauseBackground == NULL && GUseStreamingPause ) { // @todo UE4 merge andrew // GStreamingPauseBackground = new FFrontBufferTexture(....); GStreamingPauseBackground->InitRHI(); } } PVS-Studio's diagnostic message: V522 Dereferencing of the null pointer 'GStreamingPauseBackground' might take place. streamingpauserendering.cpp 197 A few more words about null pointers Almost in every program I check, I get a pile of V595 warnings (examples). These warnings indicate the following trouble: A pointer is dereferenced first and only then is checked for being null. That's not always an error, but this code is highly suspicious and needs to be checked anyway! The V595 diagnostic helps us reveal slip-ups like this: /** * Global engine pointer. * Can be 0 so don't use without checking. */ ENGINE_API UEngine* GEngine = NULL; bool UEngine::LoadMap( FWorldContext& WorldContext, FURL URL, class UPendingNetGame* Pending, FString& Error ) { .... if (GEngine->GameViewport != NULL) { ClearDebugDisplayProperties(); } if( GEngine ) { GEngine->WorldDestroyed( WorldContext.World() ); } .... } PVS-Studio's diagnostic message: V595 The 'GEngine' pointer was utilized before it was verified against nullptr. Check lines: 9714, 9719. unrealengine.cpp 9714 Notice the comment. The global variable GEngine may be equal to zero, so it must be checked before it can be used. And there is such a check indeed in the function LoadMap(): if( GEngine ) Unfortunately, this check is executed only after the pointer has been already used: if (GEngine->GameViewport != NULL) There were quite a number of V595 warnings for the project (about 82). I guess many of them are false positives, so I won't litter the article with the samples and cite them in a separate list: ue-v595.txt. Excess variable declaration This error is pretty nice. It is about mistakenly declaring a new variable instead of using an already existing one. void FStreamableManager::AsyncLoadCallback(....) { .... FStreamable* Existing = StreamableItems.FindRef(TargetName); .... if (!Existing) { // hmm, maybe it was redirected by a consolidate TargetName = ResolveRedirects(TargetName); FStreamable* Existing = StreamableItems.FindRef(TargetName); } if (Existing && Existing->bAsyncLoadRequestOutstanding) .... } PVS-Studio's diagnostic message: V561 It's probably better to assign value to 'Existing' variable than to declare it anew. Previous declaration: streamablemanager.cpp, line 325. streamablemanager.cpp 332 I suspect the code must look like this: // hmm, maybe it was redirected by a consolidate TargetName = ResolveRedirects(TargetName); Existing = StreamableItems.FindRef(TargetName); Errors in function calls bool FRecastQueryFilter::IsEqual( const INavigationQueryFilterInterface* Other) const { // @NOTE: not type safe, should be changed when // another filter type is introduced return FMemory::Memcmp(this, Other, sizeof(this)) == 0; } PVS-Studio's diagnostic message: V579 The Memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. pimplrecastnavmesh.cpp 172 The comment warns us that it is dangerous to use Memcmp(). But actually it all is even worse than the programmer expects. The point is that the function compares only a part of the object. The sizeof(this) operator returns the pointer size; that is, the function will compare the first 4 bytes in a 32-bit program and 8 bytes in a 64-bit program. The correct code should look as follows: return FMemory::Memcmp(this, Other, sizeof(*this)) == 0; But that's not the only trouble with the Memcmp() function. Have a look at the following code fragment: D3D11_STATE_CACHE_INLINE void GetBlendState( ID3D11BlendState** BlendState, float BlendFactor[4], uint32* SampleMask) { .... FMemory::Memcmp(BlendFactor, CurrentBlendFactor, sizeof(CurrentBlendFactor)); .... } PVS-Studio's diagnostic message: V530 The return value of function 'Memcmp' is required to be utilized. d3d11statecacheprivate.h 547 The analyzer was surprised at finding the Memcmp() function's result not being used anywhere. And this is an error indeed. As far as I get it, the programmer wanted to copy the data, not compare them. If so, the Memcpy() function should be used: FMemory::Memcpy(BlendFactor, CurrentBlendFactor, sizeof(CurrentBlendFactor)); A variable assigned to itself enum ECubeFace; ECubeFace CubeFace; friend FArchive& operatorGetMembers().GetAllocatedSize()) : 0; } PVS-Studio's diagnostic message: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. materialshared.h 224 This code is pretty complicated. To make the explanation clearer, I have composed a simplified artificial sample: return A() + B() + C() + uniform ? UniformSize() : 0; A certain size is being calculated in this code. Depending on the value of the uniform variable, either UniformSize() or 0 should be added. But the code actually works in quite a different way. The priority of the addition operators '+' is higher than that of the '?:' operator. So here's what we get: return (A() + B() + C() + uniform) ? UniformSize() : 0; A similar issue can be found in Unreal Engine's code. I suspect the program calculates something different than the programmer wanted it to. Mess-up with enum I didn't feel like describing this case at first as I would have to cite quite a large piece of code. But then I overcame my laziness, so please be patient too. namespace EOnlineSharingReadCategory { enum Type { None = 0x00, Posts = 0x01, Friends = 0x02, Mailbox = 0x04, OnlineStatus = 0x08, ProfileInfo = 0x10, LocationInfo = 0x20, Default = ProfileInfo|LocationInfo, }; } namespace EOnlineSharingPublishingCategory { enum Type { None = 0x00, Posts = 0x01, Friends = 0x02, AccountAdmin = 0x04, Events = 0x08, Default = None, }; inline const TCHAR* ToString (EOnlineSharingReadCategory::Type CategoryType) { switch (CategoryType) { case None: { return TEXT("Category undefined"); } case Posts: { return TEXT("Posts"); } case Friends: { return TEXT("Friends"); } case AccountAdmin: { return TEXT("Account Admin"); } .... } } The analyzer generates a few V556 warnings at once on this code. The reason is that the switch operator has a variable of the EOnlineSharingReadCategory::Type type as its argument. At the same time, case operators work with values of a different type, EOnlineSharingPublishingCategory::Type. A logical error const TCHAR* UStructProperty::ImportText_Internal(....) const { .... if (*Buffer == TCHAR('\"')) { while (*Buffer && *Buffer != TCHAR('\"') && *Buffer != TCHAR('\n') && *Buffer != TCHAR('\r')) { Buffer++; } if (*Buffer != TCHAR('\"')) .... } PVS-Studio's diagnostic message: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 310, 312. propertystruct.cpp 310 The programmer intended to skip all text in double quotes. The algorithm was meant to be like this:Once the program comes across a double quote, a loop is started.The loop keeps skipping characters until stumbling across the next double quote.The error is about the pointer failing to be referenced to the next character after the first double quote is found. As a result, the second double quote is found right away, too, and the loop doesn't start. Here is simpler code to clarify the point: if (*p == '\"') { while (*p && *p != '\"') p++; } To fix the error, you need to change the code in the following way: if (*p == '\"') { p++; while (*p && *p != '\"') p++; } Suspicious shift class FMallocBinned : public FMalloc { .... /* Used to mask off the bits that have been used to lookup the indirect table */ uint64 PoolMask; .... FMallocBinned(uint32 InPageSize, uint64 AddressLimit) { .... PoolMask = ( ( 1 FORCENOINLINE void Set(....) { .... if ( DefinitionPtr == NULL ) { WidgetStyleValues.Add( PropertyName, MakeShareable( new DefinitionType( InStyleDefintion ) ) ); } else { WidgetStyleValues.Add( PropertyName, MakeShareable( new DefinitionType( InStyleDefintion ) ) ); } } PVS-Studio's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. slatestyle.h 289 Miscellaneous What's left is just diverse subtle issues which are not very interesting to discuss. So let me just cite a few code fragments and corresponding diagnostic messages. void FNativeClassHeaderGenerator::ExportProperties(....) { .... int32 NumByteProperties = 0; .... if (bIsByteProperty) { NumByteProperties; } .... } PVS-Studio's diagnostic message: V607 Ownerless expression 'NumByteProperties'. codegenerator.cpp 633 static void GetModuleVersion( .... ) { .... char* VersionInfo = new char[InfoSize]; .... delete VersionInfo; .... } PVS-Studio's diagnostic message: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] VersionInfo;'. windowsplatformexceptionhandling.cpp 107 const FSlateBrush* FSlateGameResources::GetBrush( const FName PropertyName, ....) { .... ensureMsgf(BrushAsset, TEXT("Could not find resource '%s'"), PropertyName); .... } PVS-Studio's diagnostic message: V510 The 'EnsureNotFalseFormatted' function is not expected to receive class-type variable as sixth actual argument. slategameresources.cpp 49 Conclusions Using the static analyzer integrated into Visual Studio does make sense but it is not enough. The authors should consider using specialized tools in addition to it, for example our analyzer PVS-Studio. If you compare PVS-Studio to VS2013's analyzer, the former detects 6 times more bugs. Here you have the proof: Comparison of static code analyzers: CppCat, Cppcheck, PVS-Studio and Visual Studio;Comparison methodology. I invite all those who want their code to be high-quality to try our code analyzer. P.S. I should also mention that the errors described in this article (except for microoptimizations) could theoretically have been found by the lightweight analyzer CppCat as well. A one-year license for CppCat costs $250; annual renewal costs $200. But it wouldn't do in this particular case because it is lightweight and lacks the necessary functionality to monitoring compiler launches, which is a crucial requirement when checking Unreal Engine. However, the CppCat analyzer may well satisfy the authors of small projects.
  4. Hello, we are the developers of the PVS-Studio static code analyzer. We have created a new software product CppCat and are going to tell you about it in this article. Some time ago we imagined that we had never had PVS-Studio yet retained the experience of developing static analysis tools for C/C++ code. Our minds thus refreshed, we made a new static analyzer just the way we wanted it to be - simple and easy to use. You will also be glad to know that it costs $250 per installation. Background of the new product We always did our best to keep PVS-Studio easy to use and understand. But it inevitably grew to lose its simplicity as it acquired new functionality. For example, such is the ID field in the error table: some find it useful while others are confused by it. People mix it up with the diagnostic number and wonder why there can be IDs like 3, 7, 23, 25 - and what about the rest? The answer is simple and obvious: they are hidden; for example, the "64-bit" diagnostic set is turned off. Similar issues are with the settings as well. It is important in the case of large projects to have the option of choosing among preprocessors (Clang or Visual C++). It allows a user to greatly enhance the speed of analysis for certain projects. But those who are only getting started with the tool may be confused by this option. They select Clang and expect to get warnings generated by it. Some even send us indignant letters blaming us for selling an expensive add-on for Clang. Just in case, follow this link to find out how exactly we utilize Clang. You know, it's difficult to sell our analyzer to a programmer who mistakenly thinks that it is just a wrapper for Clang . This is how a vague setting gives rise to great confusion. We programmers are smart and sensible guys. But when it comes to learning new tools, we often resemble ordinary users. And it's quite okay: there's just too much information around us and you usually don't have enough time and strength to spend on learning every technological innovation. So, although programmers' tools are pretty intricate products, learning them should be as simple as handling a calculator is. Otherwise, you risk your tool just remaining unnoticed. People just won't have enough time to comprehend it. And that's what often happens to PVS-Studio. It's entirely our own fault that PVS-Studio has grown so complicated. And we have failed to find a way of returning the interface to the simplicity it possessed before. So, we created a new product: CppCat. This tool is so simple and streamlined that you will never get lost among its settings. Learning to use it takes less time than reading this article. Yes, it lacks quite a few features, but those are still there in PVS-Studio that will also remain. We just offer a cheaper carrot grater to those who don't need an expensive multi-function food processor. Now, let's point out the main idea once again. We've created an easy to learn and use tool for static analysis of C/C++ code. It will make your first steps into the static analysis methodology as simple as possible. And thanks to its simplicity and relatively low price, CppCat should go on to become a popular tool which will be as indespencible to the software developers, as a safety net for the rope-walker! Functionality The CppCat tool is designed to detect suspicious fragments in program code written in C/C++. The main functions of the analyzer are: Analysis of projects; Automatic analysis of files after compilation. That's all. The analyzer does exactly what it is designed for: checking the code and telling the programmer which fragments need closer examination. The tool can only work in Visual Studio (2010, 2012 and 2013). We decided to drop using the phrase "the analyzer detects bugs" too often. You see, any analyzer produces false positives. But when the analyzer generates warnings on correct code, it doesn't mean that you don't have to do anything with that code at all. We came across an interesting association in one article. The analyzer reveals code fragments with the "smell". "Smelly" code is not necessarily incorrect. It just means that it contains some anomalies that may cause some confusion when the code is maintained by other developers. "Smelly" code may lead to errors after refactoring: a programmer responsible for modifying the code may fail to understand how certain functions work and bring in some defects by a mistake. Thus, we believe that every code fragment that CppCat finds suspicious must be reviewed and improved. By clearing the code of its "smell", you will greatly help your coworkers. However, warnings do need to be suppressed sometimes. We offer a number of methods to do that - see the CppCat manual. Readers may still wonder: in what ways exactly is CppCat different from PVS-Studio? Here's the answer in the form of a summary table: Table 1. Comparing features of PVS-Studio against CppCat. On one hand, CppCat lacks much. But on the other, it retains all the functions necessary for everyday use in your work. The main ideological difference between CppCat and PVS-Studio is the following: CppCat licenses are individual. You install it on your computer and start using it. It doesn't provide functionality useful in team work - for example, you cannot set CppCat to run night checks. This and other functions of that kind are provided by PVS-Studio. PVS-Studio is intended for large, complex projects. CppCat can check large projects too - there's no restriction concerning the project size. It's just that CppCat doesn't have some of the additional functions and ships under a different licensing policy. CppCat can be used in companies (and large ones as well) in the same way - but you just need to purchase several licenses (and we offer discounts for buying several licenses at a time!). Pricing policy It's very simple with the prices. One copy of the product costs $250. The license is not floating - it is bound to one computer (hardware ID). We offer discounts depending on how many licenses you buy at a time: 1 - 4 licenses: $250.00 5 - 24 licenses: $225.00 25+ licenses: $212.50 The license is valid during 1 year. On the expiration, it can be renewed at 80% of the initial price (i.e. $200). Downloading, trying and purchasing Nothing complicated about that too: Visit the product website: http://www.cppcat.com Support is provided via e-mail: team@cppcat.com The full-function trial version can be used for 7 days. And what about PVS-Studio? We go on developing PVS-Studio as well. It is still relevant and all its aspects, as well as the pricing policy, remain the same. Conclusion We are launching a sale concurrently with our new product's release. You get a 5% discount when buying CppCat during 5 days (up to January, 19). Follow this link for the bargain.  
  5. Code_Analysis

    Grounded Pointers

    Thank you. Written by stupidity. I'm a little fix this.
  6. Code_Analysis

    Wade Not In Unknown Waters: Part One

    This article is about C++. "We decided to write several small posts on how C/C++" - I mean, there will be several articles. Some will be about C++, the other about C/C++.
  7. The Visual Studio development environment allows you to perform static code analysis. This analysis is very useful and easy-to-use. However, we should understand that Visual Studio performs a huge number of functions. It means that each of its functions taken separately cannot compare to specialized tools. The code refactoring and coloring functions are not as good as in Visual Assist. The function of integrated image editing is naturally worse than that in Adobe Photoshop or CorelDRAW. The same is true for the static code analysis function as well. But this all is theorization. Let's move to practice and see what interesting things the PVS-Studio analyzer has managed to find in Visual Studio 2012 folders. We didn't actually plan to check the source files included into Visual Studio. It happened by chance: many header files underwent some changes in Visual Studio 2012 because of support for the new language standard C++11. We have faced the task to make sure that the PVS-Studio analyzer can handle these header files. Continue: http://www.viva64.com/en/b/0163/
  8. Code_Analysis

    100 bugs in Open Source C/C++ projects

    First step in Linux directoion: we support MinGW. Example of test MinGW project: http://www.viva64.com/en/b/0154/
  9. PVS-Studio 4.21 now Integrating with Microsoft Team Foundation Server 2005/2008/2010 - http://www.viva64.com/en/d/0006/
  • Advertisement

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!