Geekly Articles each Day
📜 ⬆️ ⬇️

Checking the source C # Unity code

Picture 5

Recently, a long-awaited event for many people happened - Unity Technologies has posted the original C # code of the Unity game engine for free download on GitHub. Introduced engine code and editor. Of course, we could not pass by, especially since lately we have written not so many articles on checking C # projects. Unity allows you to use the provided source code for reference purposes only. That is exactly what we will do. Let's test the latest version of PVS-Studio 6.23 on the Unity code.

Introduction


We have previously written an article on Unity verification. At that time, not much C # code was available for analysis: some components, libraries and usage examples. Nevertheless, the author of the article managed to detect some rather interesting errors.

What is Unity pleased this time? I say "pleased" and I hope that I will not offend the authors of the project. Moreover, the volume of the original Unity C # code presented on GitHub is about 400 thousand lines (excluding empty) in 2058 files with the extension “cs”. This is quite a lot, and the analyzer had a very wide field of activity.

Now about the results. Before starting the analysis, I simplified my work a bit by turning on the CWE classification display codes for the errors found, and also activating the third confidence level warning suppression mode (Low). These settings are available in the PVS-Studio drop-down menu of the Visual Studio development environment, as well as in the analyzer settings. Having thus gotten rid of low confidence warnings, I analyzed the Unity source code, which resulted in 181 warnings of the first confidence level (High) and 506 warnings of the second confidence level (Medium).
')
I didn’t study absolutely all received warnings, as there are quite a lot of them. Developers or enthusiasts can easily conduct in-depth analysis by running the Unity check themselves. To do this, PVS-Studio provides trial and free use modes. Companies can also buy our product and get quick and detailed support besides the license.

Judging by the fact that in almost every group of warnings I managed to find a real mistake from one or two attempts at once - there are a lot of them in Unity. And yes, they are diverse. Let's explore the most interesting bugs.

Test results


Something is wrong with the flags

PVS-Studio warning : V3001 There are identical sub-expressions 'MethodAttributes.Public' operator. SyncListStructProcessor.cs 240
MethodReference GenerateSerialization() { .... MethodDefinition serializeFunc = new MethodDefinition("SerializeItem", MethodAttributes.Public | MethodAttributes.Virtual | MethodAttributes.Public | // <= MethodAttributes.HideBySig, Weaver.voidType); .... }

An error was made when combining the MethodAttributes enumeration flags: the Public value was used twice. Perhaps the reason for this is bad code formatting.

A similar error was made in the GenerateDeserialization method code:
Copy paste

PVS-Studio warning : V3001 There are identical sub-expressions' format == RenderTextureFormat.ARGBFloat operator. RenderTextureEditor.cs 87
 public static bool IsHDRFormat(RenderTextureFormat format) { Return (format == RenderTextureFormat.ARGBHalf || format == RenderTextureFormat.RGB111110Float || format == RenderTextureFormat.RGFloat || format == RenderTextureFormat.ARGBFloat || format == RenderTextureFormat.ARGBFloat || format == RenderTextureFormat.RFloat || format == RenderTextureFormat.RGHalf || format == RenderTextureFormat.RHalf); }

I gave the code snippet, having preformatted it, so the error is easily detected visually: a comparison with the RenderTextureFormat.ARGBFloat is made twice. In the original code, it looks a little different:

Picture 3


Probably, in one of two identical comparisons it is necessary to use a different value of the RenderTextureFormat enumeration.

Double work

PVS-Studio warning : V3008 CWE-563 The 'fail' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1633, 1632. UNetWeaver.cs 1633
 class Weaver { .... public static bool fail; .... static public bool IsValidTypeToGenerate(....) { .... if (....) { .... Weaver.fail = true; fail = true; return false; } return true; } .... }

The variable is set to true twice, because Weaver.fail and fail are the same static field of the Weaver class. Perhaps a blunder is not here, but the code definitely needs attention.

No options

PVS-Studio Warning: V3009 CWE-393 It's one odd that this method returns. ProjectBrowser.cs 1417
 // Returns true if we should early out of OnGUI bool HandleCommandEventsForTreeView() { .... if (....) { .... if (....) return false; .... } return false; }

The method always returns false . Pay attention to the comment at the beginning.

Forgot about the result

PVS-Studio warning : V3010 CWE-252 The return value of the function 'Concat' is required to be utilized. AnimationRecording.cs 455
 static public UndoPropertyModification[] Process(....) { .... discardedModifications.Concat(discardedRotationModifications); return discardedModifications.ToArray(); }

When concatenating two arrays, discardedModifications and discardedRotationModifications forgot to save the result. Probably, the programmer suggested that the result will be reflected immediately in the discardedModifications array. But it is not. As a result, the original array of the discardedModifications is returned from the method. The code needs to be corrected as follows:
 static public UndoPropertyModification[] Process(....) { .... return discardedModifications.Concat(discardedRotationModifications) .ToArray(); }

Not checked

PVS-Studio Warning: V3019 CWE-697; Check variables 'obj', 'newResolution'. GameViewSizesMenuItemProvider.cs 104
 private static GameViewSize CastToGameViewSize(object obj) { GameViewSize newResolution = obj as GameViewSize; if (obj == null) { Debug.LogError("Incorrect input"); return null; } return newResolution; }

In this method, we forgot to foresee a situation where the obj variable will not be null , but it cannot be converted to the GameViewSize type. Then the variable newResolution will receive a null value, and debugging output will not be produced. The corrected version of the code could be:
 private static GameViewSize CastToGameViewSize(object obj) { GameViewSize newResolution = obj as GameViewSize; if (newResolution == null) { Debug.LogError("Incorrect input"); } return newResolution; }

Imperfection

PVS-Studio warning : V3020 CWE-670 An unconditional 'return' within a loop. PolygonCollider2DEditor.cs 96
 private void HandleDragAndDrop(Rect targetRect) { .... foreach (....) { .... if (....) { .... } return; } .... }

The loop will perform only one iteration, after which the method will complete the work. Different scenarios are likely. For example, return must be inside an if block, or the continue directive is missing before return . It may well be that there is no error here, but then you should make the code more understandable.

Unreachable code

PVS-Studio Warning: V3021 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the statement is a senseless customScript Assembly.cs 179
 public bool IsCompatibleWith(....) { .... if (buildingForEditor) return IsCompatibleWithEditor(); if (buildingForEditor) buildTarget = BuildTarget.NoTarget; // Editor .... }

Two identical checks, one after the other. Obviously, in the case of equality of the buildingForEditor to true , the second check is meaningless, as a result of the first method completes the work. If the value of the buildingForEditor is false , then neither the then-branch of the first if statement nor the second one will be executed. There is an erroneous design requiring correction.

Unconditional condition

PVS-Studio warning : V3022 CWE-570 Expression 'index <0 && index> = parameters.Length' is always false. AnimatorControllerPlayable.bindings.cs 287
 public AnimatorControllerParameter GetParameter(int index) { AnimatorControllerParameter[] param = parameters; if (index < 0 && index >= parameters.Length) throw new IndexOutOfRangeException( "Index must be between 0 and " + parameters.Length); return param[index]; }

The index check condition is incorrect - the result will always be false . However, in the case of an erroneous index being passed to the GetParameter method, an IndexOutOfRangeException exception will still be thrown, but already when attempting to access an array element in a return block. True, the error message will be somewhat different. In order for the code to behave as the developer expects, it is necessary instead of the && operator in the condition to use ||:
 public AnimatorControllerParameter GetParameter(int index) { AnimatorControllerParameter[] param = parameters; if (index < 0 || index >= parameters.Length) throw new IndexOutOfRangeException( "Index must be between 0 and " + parameters.Length); return param[index]; }

Probably due to the use of the Copy-Paste technique, another exact error is present in the Unity code:

PVS-Studio warning : V3022 CWE-570 Expression 'index <0 && index> = parameters.Length' is always false. Animator.bindings.cs 711

And one more similar error, also related to the incorrect condition for checking the array index:

PVS-Studio warning : V3022 CWE-570 Expression 'handle.valueIndex <0 && handle.valueIndex> = list.Length' is always false. StyleSheet.cs 81
 static T CheckAccess<T>(T[] list, StyleValueType type, StyleValueHandle handle) { T value = default(T); if (handle.valueType != type) { Debug.LogErrorFormat(.... ); } else if (handle.valueIndex < 0 && handle.valueIndex >= list.Length) { Debug.LogError("Accessing invalid property"); } else { value = list[handle.valueIndex]; } return value; }

And in this case, a throw exception IndexOutOfRangeException. To correct the error, it is necessary, as in the previous code fragments, to use the operator in the condition || instead of &&.

Just a weird code

Two warnings immediately indicate the following code snippet:

PVS-Studio Warning: V3022 CWE-571 Expression 'bRegisterAllDefinitions || (AudioSettings.GetSpatializerPluginName () == "GVR Audio Spatializer") 'is always true. AudioExtensions.cs 463

PVS-Studio Warning: V3022 CWE-571 Expression 'bRegisterAllDefinitions || (AudioSettings.GetAmbisonicDecoderPluginName () == "GVR Audio Spatializer") 'is always true. AudioExtensions.cs 467
 // This is where we register our built-in spatializer extensions. static private void RegisterBuiltinDefinitions() { bool bRegisterAllDefinitions = true; if (!m_BuiltinDefinitionsRegistered) { if (bRegisterAllDefinitions || (AudioSettings.GetSpatializerPluginName() == "GVR Audio Spatializer")) { } if (bRegisterAllDefinitions || (AudioSettings.GetAmbisonicDecoderPluginName() == "GVR Audio Spatializer")) { } m_BuiltinDefinitionsRegistered = true; } }

Looks like an unfinished method. It is not clear why he was left in this form and did not comment out useless blocks of code. All that the method is doing at the moment:
 if (!m_BuiltinDefinitionsRegistered) { m_BuiltinDefinitionsRegistered = true; }

Useless method

PVS-Studio warning : V3022 CWE-570 Expression 'PerceptionRemotingPlugin.GetConnectionState ()! = HolographicStreamerConnectionState.Disconnected' is always false. HolographicEmulationWindow.cs 171
 private void Disconnect() { if (PerceptionRemotingPlugin.GetConnectionState() != HolographicStreamerConnectionState.Disconnected) PerceptionRemotingPlugin.Disconnect(); }

To clarify the situation, you need to look at the declaration of the PerceptionRemotingPlugin.GetConnectionState () method:
 internal static HolographicStreamerConnectionState GetConnectionState() { return HolographicStreamerConnectionState.Disconnected; }

Thus, calling Disconnect () does not lead to anything.

There is one more error associated with the same PerceptionRemotingPlugin.GetConnectionState () method:

PVS-Studio warning : V3022 CWE-570 Expression 'PerceptionRemotingPlugin.GetConnectionState () == HolographicStreamerConnectionState.Connected' is always false. HolographicEmulationWindow.cs 177
 private bool IsConnectedToRemoteDevice() { return PerceptionRemotingPlugin.GetConnectionState() == HolographicStreamerConnectionState.Connected; }

The result of the method is equivalent to the following:
 private bool IsConnectedToRemoteDevice() { return false; }

As you can see, among the warnings V3022 found a lot of interesting. Probably, if you spend some more time, you can increase the list. But let's move on.

Not in format

PVS-Studio warning : V3025 CWE-685 Incorrect format. A different number of formatted items is expected while calling 'Format' function. Arguments not used: index. Physics2D.bindings.cs 2823
 public void SetPath(....) { if (index < 0) throw new ArgumentOutOfRangeException( String.Format("Negative path index is invalid.", index)); .... }

There is no error, but the code, as they say, "with the smell." Probably the earlier message was more informative, like this: "Negative path index {0} is invalid." . Then it was simplified, but the index parameter for the Format method was forgotten. Of course, this is not the same as a forgotten parameter for the specified output specifier in a string, that is, a structure of the form String.Format (“Negative path index {0} is invalid.”) . In that case, an exception would be thrown. But in our case, careful refactoring is required. The code needs to be fixed like this:
 public void SetPath(....) { if (index < 0) throw new ArgumentOutOfRangeException( "Negative path index is invalid."); .... }

Substring substring

PVS-Studio warning : V3053 An excessive expression. Examine the substrings 'UnityEngine.' and 'UnityEngine.SetupCoroutine'. StackTrace.cs 43
 static bool IsSystemStacktraceType(object name) { string casted = (string)name; return casted.StartsWith("UnityEditor.") || casted.StartsWith("UnityEngine.") || casted.StartsWith("System.") || casted.StartsWith("UnityScript.Lang.") || casted.StartsWith("Boo.Lang.") || casted.StartsWith("UnityEngine.SetupCoroutine"); }

Searching for the substring "UnityEngine.SetupCoroutine" in the condition is meaningless, since before this a search for "UnityEngine." Is performed. Thus, you should delete the last check, or clarify the correct substrings.

Another similar error:

PVS-Studio warning : V3053 An excessive expression. Examine the substrings 'Windows.dll' and 'Windows.'. AssemblyHelper.cs 84
 static private bool CouldBelongToDotNetOrWindowsRuntime(string assemblyPath) { return assemblyPath.IndexOf("mscorlib.dll") != -1 || assemblyPath.IndexOf("System.") != -1 || assemblyPath.IndexOf("Windows.dll") != -1 || // <= assemblyPath.IndexOf("Microsoft.") != -1 || assemblyPath.IndexOf("Windows.") != -1 || // <= assemblyPath.IndexOf("WinRTLegacy.dll") != -1 || assemblyPath.IndexOf("platform.dll") != -1; }

Size matters

PVS-Studio warning : V3063 CWE-571 A part of the conditional expression is always true if it is evaluated: pageSize <= 1000. UNETInterface.cs 584
 public override bool IsValid() { .... return base.IsValid() && (pageSize >= 1 || pageSize <= 1000) && totalFilters <= 10; }

The condition for checking the valid page size is wrong. Instead of the operator || nessesary to use &&. Corrected code:
 public override bool IsValid() { .... return base.IsValid() && (pageSize >= 1 && pageSize <= 1000) && totalFilters <= 10; }

Division by zero is possible

PVS-Studio Warning: V3064 CWE-369 Potential division by zero. Consider inspecting denominator '(float) (width - 1)'. ClothInspector.cs 249
 Texture2D GenerateColorTexture(int width) { .... for (int i = 0; i < width; i++) colors[i] = GetGradientColor(i / (float)(width - 1)); .... }

The problem may occur when passing to the method the value width = 1 . In the method itself, this is not verified. The GenerateColorTexture method is called in the code only once with the parameter 100:
 void OnEnable() { if (s_ColorTexture == null) s_ColorTexture = GenerateColorTexture(100); .... }

So there is no error yet. But, just in case, the GenerateColorTexture method should provide the ability to transfer incorrect width values.

Paradoxical check

PVS-Studio warning : V3080 CWE-476 Possible null dereference. Consider inspecting 'm_Parent'. EditorWindow.cs 449
 public void ShowPopup() { if (m_Parent == null) { .... Rect r = m_Parent.borderSize.Add(....); .... } }

Probably due to a typo, the execution of this code guarantees the use of the m_Parent null reference. Corrected code:
 public void ShowPopup() { if (m_Parent != null) { .... Rect r = m_Parent.borderSize.Add(....); .... } }

Exactly the same error occurs later in the code:

PVS-Studio warning : V3080 CWE-476 Possible null dereference. Consider inspecting 'm_Parent'. EditorWindow.cs 470
 internal void ShowWithMode(ShowMode mode) { if (m_Parent == null) { .... Rect r = m_Parent.borderSize.Add(....); .... }

And here is another interesting error that can lead to access via a null link due to incorrect verification:

PVS-Studio warning : V3080 CWE-476 Possible null dereference. Consider inspecting 'objects'. TypeSelectionList.cs 48
 public TypeSelection(string typeName, Object[] objects) { System.Diagnostics.Debug.Assert(objects != null || objects.Length >= 1); .... }

It seems to me that Unity developers quite often make mistakes due to improper use of operators || and && in terms. In this case, if objects will be null , this will check the second part of the condition (objects! = Null || objects.Length> = 1) , which will cause an unexpected exception to be thrown. The error must be corrected as follows:
 public TypeSelection(string typeName, Object[] objects) { System.Diagnostics.Debug.Assert(objects != null && objects.Length >= 1); .... }

Early zeroed

PVS-Studio warning : V3080 CWE-476 Possible null dereference. Consider inspecting 'm_RowRects'. TreeViewControlGUI.cs 272
 public override void GetFirstAndLastRowVisible(....) { .... if (rowCount != m_RowRects.Count) { m_RowRects = null; throw new InvalidOperationException(string.Format("....", rowCount, m_RowRects.Count)); } .... }

In this case, an exception is thrown (access via the null link m_RowRects ) will occur when a message string is formed for another exception. The code can be fixed, for example, like this:
 public override void GetFirstAndLastRowVisible(....) { .... if (rowCount != m_RowRects.Count) { var m_RowRectsCount = m_RowRects.Count; m_RowRects = null; throw new InvalidOperationException(string.Format("....", rowCount, m_RowRectsCount)); } .... }

Again check error

PVS-Studio warning : V3080 CWE-476 Possible null dereference. Consider inspecting 'additionalOptions'. MonoCrossCompile.cs 279
 static void CrossCompileAOT(....) { .... if (additionalOptions != null & additionalOptions.Trim().Length > 0) arguments += additionalOptions.Trim() + ","; .... }

Due to the fact that & is used in the condition, the second part of the condition will always be checked, regardless of the result of checking the first part. If the variable additionalOptions is null , an exception is inevitable. The error must be corrected by using the && operator instead of &.

As you can see, among the warnings with the number V3080 there are quite insidious errors.

Late check

PVS-Studio warning : V3095 CWE-476 The 'element' object was used against null. Check lines: 101, 107. StyleContext.cs 101
 public override void OnBeginElementTest(VisualElement element, ....) { if (element.IsDirty(ChangeType.Styles)) { .... } if (element != null && element.styleSheets != null) { .... } .... }

The variable element is used without first checking for null inequality. At the same time further in the code such verification is performed. The code probably needs to be fixed this way:
 public override void OnBeginElementTest(VisualElement element, ....) { if (element != null) { if (element.IsDirty(ChangeType.Styles)) { .... } if (element.styleSheets != null) { .... } } .... }

There are 18 more such errors in the code. I will list the first 10:
Invalid Equals method

PVS-Studio Warning: V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CurveEditorSelection.cs 74
 public override bool Equals(object _other) { CurveSelection other = (CurveSelection)_other; return other.curveID == curveID && other.key == key && other.type == type; }

The overload of the Equals method is done carelessly. It is necessary to take into account the possibility of obtaining null as a parameter, since this may lead to an exception being thrown, which was not provided for in the calling code. Also throwing an exception will result in the situation when it is not possible to bring _other to the CurveSelection type. Code requires correction. A good example of implementing Equals overload is provided in the documentation .

There are other similar errors in the code:
And again about checking for null inequality

PVS-Studio warning : V3125 CWE-476 The camera has been verified against null. Check lines: 184, 180. ARBackgroundRenderer.cs 184
 protected void DisableARBackgroundRendering() { .... if (camera != null) camera.clearFlags = m_CameraClearFlags; // Command buffer camera.RemoveCommandBuffer(CameraEvent.BeforeForwardOpaque, m_CommandBuffer); camera.RemoveCommandBuffer(CameraEvent.BeforeGBuffer, m_CommandBuffer); }

When the camera variable is first used, it is checked for null inequality. But then they forget to make it by code. The corrected version could be:
 protected void DisableARBackgroundRendering() { .... if (camera != null) { camera.clearFlags = m_CameraClearFlags; // Command buffer camera.RemoveCommandBuffer(CameraEvent.BeforeForwardOpaque, m_CommandBuffer); camera.RemoveCommandBuffer(CameraEvent.BeforeGBuffer, m_CommandBuffer); } }

Another similar error:

PVS-Studio Warning: V3125 CWE-476 The 'item' object has been verified against null. Check lines: 88, 85. TreeViewForAudioMixerGroups.cs 88
 protected override Texture GetIconForItem(TreeViewItem item) { if (item != null && item.icon != null) return item.icon; if (item.id == kNoneItemID) // <= return k_AudioListenerIcon; return k_AudioGroupIcon; }

An error was made, leading in some cases to access via a null link. Fulfillment of the condition in the first if block provides an exit from the method. However, if this does not happen, then there are no guarantees that the item link is nonzero. Corrected code:
 protected override Texture GetIconForItem(TreeViewItem item) { if (item != null) { if (item.icon != null) return item.icon; if (item.id == kNoneItemID) return k_AudioListenerIcon; } return k_AudioGroupIcon; }

There are 12 more similar errors in the code. I will list the first 10:
The choice was small

PVS-Studio warning : V3136 CWE-691 Constant expression in switch statement. HolographicEmulationWindow.cs 261
 void ConnectionStateGUI() { .... HolographicStreamerConnectionState connectionState = PerceptionRemotingPlugin.GetConnectionState(); switch (connectionState) { .... } .... }

And here the PerceptionRemotingPlugin.GetConnectionState () method was to blame. We have already encountered it when we studied the warnings of V3022 :
 internal static HolographicStreamerConnectionState GetConnectionState() { return HolographicStreamerConnectionState.Disconnected; }

The method will return a constant. Very strange code. It is necessary to pay close attention to it.

findings


Picture 6


I think this can be stopped, otherwise the article will become boring and lengthy. I repeat, I brought errors that immediately caught my eye. The Unity code undoubtedly contains a greater number of erroneous or incorrect constructions that require correction. The difficulty lies in the fact that many of the issued warnings are very controversial and only the author of the code can make an accurate “diagnosis” in each specific case.

In general, one can say about the Unity project that it is rich in errors, but considering the size of its code base (400 thousand lines), everything is not so bad. Nevertheless, I hope that the authors will not neglect the tools of code analysis to improve the quality of their product.

Use PVS-Studio and the reckless code!



, : Sergey Khrenov. Checking the Unity C# Source Code

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/353640/

More articles:


All Articles