Geekly Articles each Day
📜 ⬆️ ⬇️

Analyzing errors in open components of Unity3D

Unity3D is one of the most promising and fast-developing game engines at the moment. From time to time, new libraries and components appear in the official repository, many of which until recently were not available as source files. Unfortunately, the Unity3D development team provided at the mercy of the public not all the source code of the project, but only some of its components, libraries and demos. In this article we will try to find all sorts of errors and typos in them using the PVS-Studio static analyzer.


Introduction


We decided to check all the components, libraries and demos written in C #, whose source code is provided in the official Unity3D developer repository :

  1. UI System - a system for implementing a graphical interface.
  2. Networking - a system for the implementation of multiplayer.
  3. MemoryProfiler - a system for profiling the resources used.
  4. XcodeAPI is a component for interacting with the Xcode development environment.
  5. PlayableGraphVisualizer is a visualization system for project execution.
  6. UnityTestTools - Unity3D testing utilities (without Unit tests).
  7. AssetBundleDemo is a project containing the AssetBundleServer source code and demonstrating the use of the AssetBundle system.
  8. AudioDemos - projects that demonstrate the use of an audio system.
  9. NativeAudioPlugins - audio plugins (we are only interested in the code that demonstrates their use).
  10. GraphicsDemos - projects that demonstrate the use of the graphics system.

It would be very interesting to look at the source code of the engine core itself, but apart from the developers, no one has this opportunity yet. Therefore, today on our operating table only a small part of the source code of the engine, which we can check. The most interesting projects for us are: the new UI system, designed to implement a more flexible graphical interface with respect to the old clumsy GUI, and the network library, which faithfully served us until the advent of UNet.

Also of no less interest is MemoryProfiler, as a powerful and flexible tool for profiling resources and loads.
')

Found bugs and suspicious places


All warnings issued by the analyzer can be divided into 3 levels:

  1. High is the most likely error.
  2. Medium - a possible error or typo.
  3. Low - a warning of an unlikely possible error or typo.

We will consider only high and medium levels.

The table below shows the list of verified projects and the final result of the audit for all projects. The columns “Project Name” and “Number of Lines of Code”, I think, are clear to everyone and should not cause any questions, but the purpose of the column “Analyzer Triggers” should be explained. It contains information on the number of analyzer triggerings. Positive responses are those that directly or indirectly indicate errors or typos in the code. False alarms - False messages analyzers that point to the correct sections of the code, suspecting that they contain errors or typos. As already mentioned earlier, all the triggers are divided into 3 levels. We will consider only high and medium, since the low level mainly contains informational messages or improbable errors.



As a result of testing 10 projects, 16 high-level warnings were received, 75% of which correctly pointed out problem areas in the code, and 18 medium-level responses, 39% of which correctly pointed out problem areas. The quality of the code should be recognized as high, since the analyzer finds on average only one error per 2000 lines of code. This is a good result.

So, we are done with statistics. We now proceed to the consideration of directly detected errors and typos.

Erroneous regex

V3057 Invalid regular expression patern in constructor. Inspect the first argument. AssetBundleDemo ExecuteInternalMono.cs 48

private static readonly Regex UnsafeCharsWindows = new Regex("[^A-Za-z0-9\\_\\-\\.\\:\\,\\/\\@\\\\]");

When we try to create an instance of the Regex class with this pattern, we will get a System.ArgumentException exception with the message:
parsing \ "[^ A-Za-z0-9 \\ _ \\ - \\. \\: \\, \\ / \\ @ \\] \" - Unrecognized escape sequence '

This suggests that this pattern is incorrect, and a Regex object cannot be created with this pattern. Apparently, the programmer made a mistake in his design.

It is possible to access the object with a zero reference.

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

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

After checking the object for null, it is accessed. In this case, the appeal occurs regardless of the result of the check. This can lead to a NullReferenceException exception. Most likely, the programmer planned to use the conditional and && operator, but because of a typo, the logical and & operator is used.

Accessing an object before checking it for null

V3095 The 'uv2.gameObject' object 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;

At first, the object is accessed, and only then is it checked for null . Most likely, if the object reference turns out to be equal to null , then we will get a NullReferenceException exception , without having reached the check.

In addition to the above error, the analyzer detected 2 more similar alarms, which under certain circumstances can cause exceptions:


The if statement with the same condition is already encountered , containing in the then part an unconditional return statement

A very interesting trigger that shows all the power of copy-paste, a classic 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 statement is a 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; }

It is likely that the programmer copied part of the code and forgot to change the test conditions after insertion.

Based on this typo, we can say that the second check should be:

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

Creating an instance of an exception class without further use

V3006 The object was not used. The 'throw' keyword could be missing: 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 }

The ApplicationException class is created, but not used at all. Most likely, the programmer wanted to throw an exception, but forgot to add a throw statement before the created exception.

Unused arguments when formatting a string

As you know, when formatting strings, the number of expressions of type {N} must correspond to the number of arguments passed.

V3025 Incorrect format. A different number of items is expected while calling the '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);

In this case, judging by the meaning of this piece of code, the programmer most likely forgot to remove the argument from the first line. From a technical point of view, this typo is not critical and will not cause an error, but at the same time does not carry any semantic meaning.

A cycle that can turn into an eternal one under certain conditions.

V3032 Waiting for Compiler may be optimized for 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); }

Most likely, the programmer wanted to wait in the cycle of the completion of the external process, but did not take into account that the variable masterProcess may initially assume null if the process is not found, which will cause an infinite loop. For the correct operation of this algorithm, it is necessary to request a process by its identifier for each iteration of the cycle:

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

Unsafe event initiation

The analyzer detected a potentially unsafe event event call. A NullReferenceException exception may occur.

V3083 Unsafe invocation of event 'unload', NullReferenceException is possible. Consider assigning an 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 part of the code, a check is made on the null of the unload field, and then the event is called. Checking for null will avoid exceptions if nobody has signed up for the event at the time of its call.

However, imagine that an event has one subscriber. And in the moment between checking for null and directly calling the event handler, there is a possibility that the event will be unsubscribed, for example, in another thread. To protect yourself in this situation, you can do this:

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

Thus, checking the event for null and calling its handler will be performed as a single command, which will ensure that the event is safely invoked.

Part of a logical expression is always true or false.

V3063 A part of the 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 expression connId <0 in the second test of the get function will always be false , because, using this expression in the first test, the function is always exited. Based on this, in the second test this expression does not carry any semantic and functional load.

Also found another similar error.

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

I think it is not necessary to say that this property can be easily simplified to the form:

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

In addition to the above two examples, 6 similar errors were found in the projects:

Conclusion


As in any other projects, there were no errors and typos. As you can see, PVS-Studio is the most successful at finding typos.

In turn, you can also check your own, or any other project written in C / C ++ / C # using this static analyzer.

Thank you all for your attention! I wish you a reckless program.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Ivan Kishchenko. Discussing Errors in Unity3D's Open-Source Components .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/308336/

More articles:


All Articles