Discussing Errors in Unity3D's Open-Source Components

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:

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:

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:

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:

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!

By Ivan Kishchenko