# 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\.

![0423_Unity3D/image1.png](https://import.viva64.com/docx/blog/0423_Unity3D/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](https://bitbucket.org/unity-technologies/):

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

![0423_Unity3D/image3.png](https://import.viva64.com/docx/blog/0423_Unity3D/image3.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](https://pvs-studio.com/en/docs/warnings/v3057/) Invalid regular expression pattern in constructor\. Inspect the first argument\. AssetBundleDemo ExecuteInternalMono\.cs 48

```cpp
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: 

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 't\.staticFieldBytes'\. MemoryProfiller CrawledDataUnpacker\.cs 20

```cpp
.... = 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](https://pvs-studio.com/en/docs/warnings/v3095/) The 'uv2\.gameObject' object was used before it was verified against null\. Check lines: 1719, 1731\. UnityEngine\.Networking NetworkServer\.cs 1719

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3095/) The 'm\_HorizontalScrollbarRect' object was used before it was verified against null\. Check lines: 214, 220\. UnityEngine\.UI ScrollRect\.cs 214
* [V3095](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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

```cpp
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:

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



**Instantiating an exception class without further using the instance**

[V3006](https://pvs-studio.com/en/docs/warnings/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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3025/) Incorrect format\. A different number of format items is expected while calling 'WriteLine' function\. Arguments not used: port\. AssetBundleDemo AssetBundleServer\.cs 59

```cpp
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](https://pvs-studio.com/en/docs/warnings/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

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3083/) Unsafe invocation of event 'unload', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AssetBundleDemo AssetBundleManager\.cs 47

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always false: connId < 0\. UnityEngine\.Networking ConnectionArray\.cs 59

```cpp
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\.

```cpp
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:

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

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

* [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'm\_Peers \=\= null' is always false\. UnityEngine\.Networking NetworkMigrationManager\.cs 710
* [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'uv2\.gameObject \=\= null' is always false\. UnityEngine\.Networking NetworkServer\.cs 1731
* [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'newEnterTarget \!\= null' is always true\. UnityEngine\.UI BaseInputModule\.cs 147
* [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'pointerEvent\.pointerDrag \!\= null' is always false\. UnityEngine\.UI TouchInputModule\.cs 227
* [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always true: currentTest \!\= null\. UnityTestTools TestRunner\.cs 237
* [V3063](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/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\!