Discussing Errors in Unity3D's Open-Source Components

Discussing Errors in Unity3D's Open-Source
Components
Author: Ivan Kishchenko
Date: 23.08.2016
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:
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.
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!

Discussing Errors in Unity3D's Open-Source Components

More Related Content

What's hot

Viewers also liked

Similar to Discussing Errors in Unity3D's Open-Source Components

Recently uploaded

Discussing Errors in Unity3D's Open-Source Components