Checking the source C # code Unity

    Picture 5

    Recently, a long-awaited event for many people happened - Unity Technologies posted the original C # code for the Unity game engine for free download on GitHub. The engine and editor code is presented. Of course, we could not pass by, especially since recently we have not written so many articles about checking projects in C #. Unity permits the use of the provided sources for reference purposes only. That's exactly what we’ll do. We will test the latest version of PVS-Studio 6.23 on Unity code.

    Introduction


    We previously wrote an article on Unity Validation. At that time, not many C # -codes were available for analysis: some components, libraries, and usage examples. Nevertheless, the author of the article was able to detect quite interesting errors.

    What did Unity please this time? I say “pleased” and I hope that I will not offend the authors of the project. Moreover, the size of the original Unity C # code, presented on GitHub , is about 400 thousand lines (excluding empty ones) in 2058 files with the extension “cs”. This is 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 mode for displaying codes according to the CWE classification for the errors found, and also activating the mode for suppressing warnings of the third level of reliability (Low). These settings are available in the PVS-Studio drop-down menu in the Visual Studio development environment, as well as in the analyzer settings. Having got rid of warnings with low confidence in this way, I analyzed the Unity source code, as a result of which I received 181 warnings of the first confidence level (High) and 506 warnings of the second confidence level (Medium).

    I did not study absolutely all the warnings I received, since there are a lot of them. Developers or enthusiasts can easily do in-depth analysis by doing a Unity test on their own. For this, PVS-Studio provides trial and free use modes. Also, companies can buy our product and receive, in addition to a license, quick and detailed support.

    Judging by the fact that in almost every warning group, from one or two attempts, I immediately managed to find a real error - there are a lot of them in Unity. And yes, they are diverse. Let's study the most interesting mistakes.

    Validation Results


    Something is wrong with the flags

    PVS-Studio Warning: V3001 There are identical sub-expressions 'MethodAttributes.Public' to the left and to the right of the '|' 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 flags of the MethodAttributes enumeration : the Public value was used twice. Perhaps this is due to poor code formatting.

    A similar error was made in the code of the GenerateDeserialization method :
    • V3001 There are identical sub-expressions 'MethodAttributes.Public' to the left and to the right of the '|' operator. SyncListStructProcessor.cs 309

    Copy-Paste

    Warning PVS-Studio: V3001 There are identical sub-expressions 'format == RenderTextureFormat.ARGBFloat' to the left and to the right of the '||' 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 a snippet of code, having previously formatted it, so the error is easily detected visually: a comparison is made twice with RenderTextureFormat.ARGBFloat . In the original code, it looks a bit different:

    Picture 3


    Probably, in one of two identical comparisons it is necessary to use another 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 , since Weaver.fail and fail are the same static field in the Weaver class . Perhaps there is no gross error here, but the code definitely needs attention.

    No Options

    PVS-Studio Warning: V3009 CWE-393 It's odd that this method always returns one and the same value of 'false'. 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 the result.

    PVS-Studio Warning: V3010 CWE-252 The return value of 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 this is not so. As a result, the original version of the discardedModifications array is returned from the method . The code needs to be fixed as follows:
    static public UndoPropertyModification[] Process(....)
    {
      ....
      return discardedModifications.Concat(discardedRotationModifications)
                                   .ToArray();
    }

    Checked something wrong

    PVS-Studio Warning: V3019 CWE-697 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. 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, they forgot to foresee a situation where the obj variable will not be null , but it cannot be converted to a GameViewSize type . Then the newResolution variable will get null , and the debug 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;
    }

    Flaw

    Warning-Studio of PVS: V3020 the 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 the if block , or somewhere before the return the continue directive is missing . It may well be that there is no error here, but then you should make the code more understandable.

    Inaccessible 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 second 'if' statement is senseless CustomScriptAssembly.cs 179
    public bool IsCompatibleWith(....)
    {
      ....
      if (buildingForEditor)
        return IsCompatibleWithEditor();
      if (buildingForEditor)
        buildTarget = BuildTarget.NoTarget; // Editor
      ....
    }

    Two identical checks, one after another. Obviously, if buildingForEditor is set to true , the second check is meaningless, as the first method terminates. If the buildingForEditor value is false , then neither the then-branch of the first if statement nor the second one will be executed . There is an erroneous design that needs to be fixed.

    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 condition for checking the index is incorrect - the result will always be false . However, if an invalid index is passed to the GetParameter method , an IndexOutOfRangeException exception will still be thrown, but already when trying to access an array element in the return block . True, the error message will be slightly different. In order for the code to behave as the developer expects, it is necessary to use || instead of the && operator:
    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 Copy-Paste methodology, another exact same error is present in Unity code:

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

    And another 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[] 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;
    }

    Again, throwing an IndexOutOfRangeException is possible . 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 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;
      }
    }

    It looks like an incomplete method. It is unclear why he was left in this form and did not comment on useless blocks of code. All the method is currently doing:
    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, take a look at the declaration of the PerceptionRemotingPlugin.GetConnectionState () method :
    internal static HolographicStreamerConnectionState
    GetConnectionState()
    {
      return HolographicStreamerConnectionState.Disconnected;
    }

    Thus, a call to the Disconnect () method does not lead to anything. Another error is 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 there were many interesting. Probably, if you spend some more time, you can increase the list. But let's move on.

    Not formatted

    Warning PVS-Studio: V3025 CWE-685 Incorrect format. A different number of format 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, is “with a smell”. Probably before the message was more informative, like this: «path by Negative index {0 is invalid}.» . Then it was simplified, but they forgot to remove the index parameter for the Format method . Of course, this is not the same as a forgotten parameter for the specified string output specifier, that is, a construction of the form String.Format ("Negative path index {0} is invalid.") . In that case, an exception would be thrown. But in our case, accuracy with refactoring is also necessary. 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");
    }

    The search for the substring "UnityEngine.SetupCoroutine" in the condition makes no sense, since before this the search for "UnityEngine." Is performed. Thus, you should delete the last check, or clarify the correctness of the 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 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 acceptable page size is erroneous. Instead of the operator || nessesary to use &&. Corrected Code:
    public override bool IsValid()
    {
      ....
      return base.IsValid()
        && (pageSize >= 1 && pageSize <= 1000)
        && totalFilters <= 10;
    }

    Possible division by zero.

    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 the value width = 1 to the method . In the method itself, this is not checked in any way. The GenerateColorTexture method is called in the code only once with 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 transmit an incorrect width value.

    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 a null reference m_Parent . 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 associated with the misuse of the || and && in conditions. In this case, if objects is null , this will lead to the verification of the second part of the condition (objects! = Null || objects.Length> = 1) , which will entail an unexpected exception throw. The error must be corrected as follows:
    public TypeSelection(string typeName, Object[] objects)
    {
      System.Diagnostics.Debug.Assert(objects != null &&
                                      objects.Length >= 1);
      ....
    }

    Early nullified

    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 generated 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 error while checking

    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 & operator 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 additionalOptions variable is null , throwing 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.

    Belated Check

    PVS-Studio Warning: V3095 CWE-476 The 'element' object was used before it was verified 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 element variable is used without first checking for null inequality . Moreover, further in the code such verification is performed. The code probably needs to be fixed in this way:
    public override void OnBeginElementTest(VisualElement element, ....)
    {
      if (element != null)
      {
        if (element.IsDirty(ChangeType.Styles))
        {
          ....
        }
        if (element.styleSheets != null)
        {
          ....
        }
      }
      ....
    }

    There are another 18 such errors in the code. I will list the first 10:
    • V3095 CWE-476 The 'property' object was used before it was verified against null. Check lines: 5137, 5154. EditorGUI.cs 5137
    • V3095 CWE-476 The 'exposedPropertyTable' object was used before it was verified against null. Check lines: 152, 154. ExposedReferenceDrawer.cs 152
    • V3095 CWE-476 The 'rectObjs' object was used before it was verified against null. Check lines: 97, 99. RectSelection.cs 97
    • V3095 CWE-476 The 'm_EditorCache' object was used before it was verified against null. Check lines: 134, 140. EditorCache.cs 134
    • V3095 CWE-476 The 'setup' object was used before it was verified against null. Check lines: 43, 47. TreeViewExpandAnimator.cs 43
    • V3095 CWE-476 The 'response.job' object was used before it was verified against null. Check lines: 88, 99. AssetStoreClient.cs 88
    • V3095 CWE-476 The 'compilationTask' object was used before it was verified against null. Check lines: 1010, 1011. EditorCompilation.cs 1010
    • V3095 CWE-476 The 'm_GenericPresetLibraryInspector' object was used before it was verified against null. Check lines: 35, 36. CurvePresetLibraryInspector.cs 35
    • Предупреждение PVS-Studio: V3095 CWE-476 The 'Event.current' object was used before it was verified against null. Check lines: 574, 620. AvatarMaskInspector.cs 574
    • V3095 CWE-476 The 'm_GenericPresetLibraryInspector' object was used before it was verified against null. Check lines: 31, 32. ColorPresetLibraryInspector.cs 31

    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;
    }

    Overloading the Equals method is done carelessly. It is necessary to take into account the possibility of getting null as a parameter, since this can lead to throwing an exception that was not provided in the calling code. Also, the release of exclusion lead a situation where not manage to bring _other the type CurveSelection . The code needs to be fixed. A good example of implementing an Equals overload is in the documentation .

    There are other similar errors in the code:
    • V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. SpritePackerWindow.cs 40
    • V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. PlatformIconField.cs 28
    • V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ShapeEditor.cs 161
    • V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ActiveEditorTrackerBindings.gen.cs 33
    • V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ProfilerFrameDataView.bindings.cs 60

    And again about checking for null inequality

    PVS-Studio warning : V3125 CWE-476 The 'camera' object was used after it was 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 using the camera variable for the first time , it is checked for null inequality . But then they forget to do it by code. The corrected version could have the form:
    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 was used after it was 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 has been made that in some cases leads 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 is no guarantee that the item link is non-zero. Corrected version of the 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:
    • V3125 CWE-476 The 'element' object was used after it was verified against null. Check lines: 132, 107. StyleContext.cs 132
    • V3125 CWE-476 The 'mi.DeclaringType' object was used after it was verified against null. Check lines: 68, 49. AttributeHelper.cs 68
    • V3125 CWE-476 The 'label' object was used after it was verified against null. Check lines: 5016, 4999. EditorGUI.cs 5016
    • V3125 CWE-476 The 'Event.current' object was used after it was verified against null. Check lines: 277, 268. HostView.cs 277
    • V3125 CWE-476 The 'bpst' object was used after it was verified against null. Check lines: 96, 92. BuildPlayerSceneTreeView.cs 96
    • V3125 CWE-476 The 'state' object was used after it was verified against null. Check lines: 417, 404. EditorGUIExt.cs 417
    • V3125 CWE-476 The 'dock' object was used after it was verified against null. Check lines: 370, 365. WindowLayout.cs 370
    • V3125 CWE-476 The 'info' object was used after it was verified against null. Check lines: 234, 226. AssetStoreAssetInspector.cs 234
    • V3125 CWE-476 The 'platformProvider' object was used after it was verified against null. Check lines: 262, 222. CodeStrippingUtils.cs 262
    • V3125 CWE-476 The 'm_ControlPoints' object was used after it was verified against null. Check lines: 373, 361. EdgeControl.cs 373

    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 already came across it when we studied the warnings of V3022 :
    internal static HolographicStreamerConnectionState
      GetConnectionState()
    {
      return HolographicStreamerConnectionState.Disconnected;
    }

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

    conclusions


    Picture 6


    I think you can stop here, otherwise the article will become boring and lengthy. I repeat, I cited errors that immediately struck me. Unity code certainly contains more erroneous or incorrect constructs that need to be fixed. The difficulty lies in the fact that many of the warnings issued are highly controversial and only the author of the code can make an accurate “diagnosis” in each case.

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

    Use PVS-Studio and code which is careless to all!



    If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Checking the Unity C # Source Code

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

    Also popular now: