Discussing Errors in Unity3D's Open-Source Components

Published October 20, 2016 by Ivan Kishchenko, posted by PVS-studio team
Do you see issues with this article? Let us know.
Advertisement

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.

image1.png


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:

  1. UI System- system for GUI development.
  2. Networking- system for implementing multiplayer mode.
  3. MemoryProfiler - system for profiling resources in use.
  4. XcodeAPI- component for interacting with the Xcode IDE.
  5. PlayableGraphVisualizer - system for project execution visualization.
  6. UnityTestTools - Unity3D testing utilities (no Unit tests included).
  7. AssetBundleDemo - project with AssetBundleServer's source files and demos for AssetBundle system.
  8. AudioDemos - demo projects for the audio system.
  9. NativeAudioPlugins - audio plugins (we are only interested in the demos for these plugins).
  10. 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:

  1. High - almost certainly an error.
  2. Medium - possible error or typo.
  3. 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.

image2.png

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\\_\\-\\.\\:\\,\\/\\@\\\\]"); // <=

When trying to instantiate the Regex class using this pattern, a System.ArgumentException exception will be thrown with the following message:

parsing \"[^A-Za-z0-9\\_\\-\\.\\:\\,\\/\\@\\]\" - Unrecognized escape sequence '

This message indicates that the pattern used is incorrect and that the Regex class cannot be instantiated using it. The programmer must have made a mistake when designing the pattern.



Possible access to an object using a null reference


V3080 Possible null dereference. Consider inspecting 't.staticFieldBytes'. MemoryProfiller CrawledDataUnpacker.cs 20

.... = packedSnapshot.typeDescriptions.Where(t => t.staticFieldBytes != null & t.staticFieldBytes.Length > 0 // <= )....

An object is accessed after a null check. However, it is accessed regardless of the check result, which may cause throwing NullReferenceException. The programmer must have intended to use the conditional AND operator (&&) but made a typo and wrote the logical AND operator (&) instead.



Accessing an object before a null check


V3095 The 'uv2.gameObject' object was used before it was verified against null. Check lines: 1719, 1731. UnityEngine.Networking NetworkServer.cs 1719

if (uv2.gameObject.hideFlags == HideFlags.NotEditable || uv2.gameObject.hideFlags == HideFlags.HideAndDontSave) continue; .... if (uv2.gameObject == null) continue;

An object is first accessed and only then is it tested for null. If the reference to the object is found to be null, we are almost sure to get NullReferenceException before reaching the check.

Besides that error, the analyzer found 2 more similar ones:

  • V3095 The 'm_HorizontalScrollbarRect' object was used before it was verified against null. Check lines: 214, 220. UnityEngine.UI ScrollRect.cs 214
  • V3095 The 'm_VerticalScrollbarRect' object was used before it was verified against null. Check lines: 215, 221. UnityEngine.UI ScrollRect.cs 215


Two if statements with the same condition and the unconditional return statement in the then block

It's quite an interesting issue, which is a perfect illustration of how mighty copy-paste is; a classical example of a typo.

V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless UnityEngine.UI StencilMaterial.cs 64

if (!baseMat.HasProperty("_StencilReadMask")) { Debug.LogWarning(".... _StencilReadMask property", baseMat); return baseMat; } if (!baseMat.HasProperty("_StencilReadMask")) // <= { Debug.LogWarning(".... _StencilWriteMask property", baseMat); return baseMat; }

The programmer must have copy-pasted a code fragment but forgot to change the condition.

Based on this typo, I'd say that the second check was meant to look like this:

if (!baseMat.HasProperty("_StencilWriteMask"))

Instantiating an exception class without further using the instance

V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new ApplicationException(FOO). AssetBundleDemo AssetBundleManager.cs 446

if (bundleBaseDownloadingURL.ToLower().StartsWith("odr://")) { #if ENABLE_IOS_ON_DEMAND_RESOURCES Log(LogType.Info, "Requesting bundle " + ....); m_InProgressOperations.Add( new AssetBundleDownloadFromODROperation(assetBundleName) ); #else new ApplicationException("Can't load bundle " + ....); // <= #endif }

Class ApplicationException is created but not used in any way. The programmer must have wanted an exception to be thrown but forgot to add the throw keyword when forming the exception.



Unused arguments in a string formatting method

As we all know, the number of {N} format items used for string formatting must correspond to the number of arguments passed to the method.

V3025 Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: port. AssetBundleDemo AssetBundleServer.cs 59

Console.WriteLine("Starting up asset bundle server.", port); // <= Console.WriteLine("Port: {0}", port); Console.WriteLine("Directory: {0}", basePath);

Judging by the logic of this code, it seems that the programmer forgot to remove the argument in the first line. This typo is not critical from the technical viewpoint and it won't cause any error, but it still carries no meaning.

A loop that may become infinite under certain conditions

V3032 Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. AssetBundleDemo AssetBundleServer.cs 16

Process masterProcess = Process.GetProcessById((int)processID); while (masterProcess == null || !masterProcess.HasExited) // <= { Thread.Sleep(1000); }

The programmer must have intended the loop to iterate until completion of an external process but didn't take into account the fact that the masterProcess variable might initially have the value null if the process was not found, which would cause an infinite loop. To make this algorithm work properly, you need to access the process using its identifier at each iteration:

while (true) { Process masterProcess = Process.GetProcessById((int)processID); if (masterProcess == null || masterProcess.HasExited) // <= break; Thread.Sleep(1000); }

Unsafe event initialization


The analyzer detected a potentially unsafe call to an event handler, which may result in throwing NullReferenceException.

V3083 Unsafe invocation of event 'unload', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. AssetBundleDemo AssetBundleManager.cs 47

internal void OnUnload() { m_AssetBundle.Unload(false); if (unload != null) unload(); // <= }

In this code, the unload field is tested for null and then this event is called. The null check allows you to avoid throwing an exception in case the event has no subscribers at moment when it is being called.

Imagine, however, that the event has one subscriber. At the point between the null check and the call to the event handler, the subscriber may unsubscribe from the event, for example, in another thread. To protect your code in this situation, you can fix it in the following way:

internal void OnUnload() { m_AssetBundle.Unload(false); unload?.Invoke(); // <= }

This solution will help you make sure that testing of the event for null and calling to its handler will be executed as one statement, making the event call safe.



Part of a logical expression always true or false


V3063 A part of conditional expression is always false: connId < 0. UnityEngine.Networking ConnectionArray.cs 59

public NetworkConnection Get(int connId) { if (connId < 0) { return m_LocalConnections[Mathf.Abs(connId) - 1]; } if (connId < 0 || connId > m_Connections.Count) // <= { ... return null; } return m_Connections[connId]; }

The connId < 0 expression will always evaluate to false the second time it is checked in the get function, since the function always terminates after the first check. Therefore, evaluating this expression for the second time does not make sense.

The analyzer found one more similar error.

public bool isServer { get { if (!m_IsServer) { return false; } return NetworkServer.active && m_IsServer; // <= } }

You surely know that this property can be easily simplified to the following:

public bool isServer { get { return m_IsServer && NetworkServer.active; } }

Besides these two examples, there are 6 more issues of that kind:

  • V3022 Expression 'm_Peers == null' is always false. UnityEngine.Networking NetworkMigrationManager.cs 710
  • V3022 Expression 'uv2.gameObject == null' is always false. UnityEngine.Networking NetworkServer.cs 1731
  • V3022 Expression 'newEnterTarget != null' is always true. UnityEngine.UI BaseInputModule.cs 147
  • V3022 Expression 'pointerEvent.pointerDrag != null' is always false. UnityEngine.UI TouchInputModule.cs 227
  • V3063 A part of conditional expression is always true: currentTest != null. UnityTestTools TestRunner.cs 237
  • V3063 A part of conditional expression is always false: connId < 0. UnityEngine.Networking ConnectionArray.cs 86


Conclusion


Just like any other project, this one contains a number of errors and typos. As you have probably noticed, PVS-Studio is especially good at catching typos.

You are also welcome to try our static analyzer with your own or someone else's project in C/C++/C#.

Thank you all for reading! May your code stay bugless!

Cancel Save
0 Likes 6 Comments

Comments

TheChubu

I rather like the table with lines of code, false/positives and such. They're nice metrics to have in mind.

October 20, 2016 10:00 PM
Alberth

Instead of publishing an endless series of articles advertising over and over again that PVS can find bugs that humans missed, fix the bugs, and make a PR for the project.

October 21, 2016 05:16 AM
swiftcoder

Instead of publishing an endless series of articles advertising over and over again that PVS can find bugs that humans missed, fix the bugs, and make a PR for the project.

They usually do.

This comes up on pretty much every PVS article - maybe the articles could link to the patch requests to head off this line of attack?

November 04, 2016 04:33 PM
nhold

Instead of publishing an endless series of articles advertising over and over again that PVS can find bugs that humans missed, fix the bugs, and make a PR for the project.

They usually do.

This comes up on pretty much every PVS article - maybe the articles could link to the patch requests to head off this line of attack?

That would be awesome, I do see some changes in the referenced source but not sure if it was because of Ivans pull requests.

December 12, 2016 10:34 PM
mr_tawan

I wonder if this is something should be publicly published....

December 13, 2016 10:32 AM
DemonDar

I would like having a tool for resolving coding dependencies. In example today I was able to merge 4 classes into 1 class that is even simpler than any of the before classes: the reason is that with requirements changes and evolving code base each of the 4 classes changed slightly until they become basically reciprocal referencing, just a big blob of linked classes. A depdency graph could have helped to spot that immediatly, instead I spent a pair of hours narrowing down that and refactoring the code to simplfy it. I know already of static analizer and I would just run them periodically as long as my boss provide the license to them :D useful tools for a small set o problems. But I would like also tools for solving totally different sets of problems.

January 18, 2017 04:25 PM
You must log in to join the conversation.
Don't have a GameDev.net account? Sign up!
Advertisement