Artem Rovenskii

Apr 08 2022

Tags:

#CSharp #GameDev

PVS-Studio static analyzer to recheck Unity

Apr 08 2022

Author: Artem Rovenskii

Introduction
Check results
Conclusion

Unity is one of the most popular game engines. It helps create many excellent cross-platform projects. It's been 4 years since the last time we checked Unity's source code. Time has come again to see what interesting things we can find.

Introduction

A while ago, we checked Unity and wrote an article about it. Click here to read it. Unity is indeed a large project that thousands of developers use daily. And don't forget about all the users that spend their time playing games developed with Unity. I think that projects of this scale must be monitored regularly — errors in such projects can affect a large number of people.

In this article, I'll analyze the source code of the Unity engine and editor of version 2022.1.0b8. Let's go directly to the results of the check.

Check results

Issue 1

private void Draw(Rect windowRect)
{
  var rect = new Rect(....);
  ....
  if (m_NumFilteredVariants > 0)
  {
    ....        
    if (m_NumFilteredVariants > maxFilteredLength)
    {
      GUI.Label(....);
      rect.y += rect.height;
    }
  }
  else
  {
    GUI.Label(rect, "No variants with these keywords");
    rect.y += rect.height;                               // <=
  }

  rect.y = windowRect.height - kMargin - kSpaceHeight – 
    EditorGUI.kSingleLineHeight;                         // <=
  ....
}

V3008 The 'rect.y' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 370, 366. ShaderVariantCollectionInspector.cs 370

The analyzer reports that the same variable — rect.y — is assigned a value twice and the code does not use the variable between the assignments. If we take a closer look, we'll see that the value for this variable is produced a bit higher in the code, under the m_NumFilteredVariants > maxFilteredLength condition — and is also lost.

Consequently, all variable value changes, except for the last one, make no sense.

Issue 2

public static string FetchBuiltinDescription(....)
{
  return string.IsNullOrEmpty(version?.packageInfo?.description) ?
    string.Format(L10n.Tr(....), version.displayName) :
    version.packageInfo.description.Split(....)[0];
}

V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'version' object UpmPackageDocs.cs 38

The analyzer found two ways to address members of the same object. If the value of version is null, the IsNullOrEmpty method will return true. When the execution flow attempts to access displayName, a NullReferenceException exception will be thrown.

Issue 3

public void SetScaleFocused(Vector2 focalPoint,
                            Vector2 newScale,
                            bool lockHorizontal,
                            bool lockVertical)
{
  if (uniformScale)
    lockHorizontal = lockVertical = false;
  else
  {
    if (hZoomLockedByDefault)
      lockHorizontal = !lockHorizontal;

    if (hZoomLockedByDefault)
      lockVertical = !lockVertical;
  }
....
}

V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 930, 933. ZoomableArea.cs 930

The developers perform the same check twice. hZoomLockedByDefault is a field in a class. If we take a look at where this field is defined, we'll see the vZoomLockedByDefault field nearby.

internal class ZoomableArea
{
  ....
  // Zoom lock settings
  public bool hZoomLockedByDefault = false;
  public bool vZoomLockedByDefault = false;
  ....
}

It all looks like a simple typo.

Issue 4

private void UpdateTextFieldVisibility()
{
  if (showInputField)
  {
    ....
  }
  else if (inputTextField != null && inputTextField.panel != null)
  {
    if (inputTextField.panel != null)                         // <=
      inputTextField.RemoveFromHierarchy();

    inputTextField.UnregisterValueChangedCallback(OnTextFieldValueChange);
    inputTextField.UnregisterCallback<FocusOutEvent>(OnTextFieldFocusOut);
    inputTextField = null;
  }
}

V3022 Expression 'inputTextField.panel != null' is always true. BaseSlider.cs 648

The analyzer reports that the inputTextField.panel != null expression is always true.

Indeed — part of the condition above already contains an identical check. The authors could have intended to check something else, but made a mistake.

Issue 5

The analyzer detected the following code:

public enum EventType
{
  ....
  // Mouse button was released.
  MouseUp = 1,
  ....
  // Already processed event.
  Used = 12,
  ....
}
public static void MinMaxScroller(....)
{
  ....
  if (   Event.current.type == EventType.MouseUp 
      && Event.current.type == EventType.Used) 
  {
    scrollControlID = 0;
  }

  ....
}

V3022 Expression is always false. Probably the '||' operator should be used here. EditorGUIExt.cs 141

Here the analyzer found an expression that is always false. Whichever value the property returns, one of the comparisons is always false.

Below is a possible way to fix the code:

public static void MinMaxScroller(....)
{
  ....
  if (   Event.current.type == EventType.MouseUp 
      || Event.current.type == EventType.Used) 
  {
    scrollControlID = 0;
  }

  ....
}

Issue 6

private List<T> GetChildrenRecursively(....)
{
  if (result == null)
    result = new List<T>();
  if (m_Children.Any())
  {
    var children = sorted ? (....)m_Children.OrderBy(c => c.key)
                                            .OrderBy(c => c.m_Priority) 
                          : m_Children;
    foreach (var child in children)
      child.GetChildrenRecursively(sorted, result);
  }
  else if (value != null)
    result.Add(value);
  return result;
}

V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. MenuService.cs 499

The analyzer detected that this code calls OrderBy twice in a row.

I found this warning quite interesting. Of course, calling OrderBy twice is not an error pattern. Most likely, this is a code fragment that may cause an error if someone misunderstands how this code works. If the developer intended to sort the collection first by key, and they by priority, then this code will produce an error. Why?

Let's have a look. In this code, the two OrderBy calls will sort the collection first by priority, and then by key. It's not sufficiently clear, is it? I think that here, instead of the second OrderBy call, calling ThenBy would be a good idea. This way, the sorting wouldn't be done "vice versa". ThenBy will be easier to read and won't raise any extra questions. For details, read the following note.

By the way, PVS-Studio found one more similar suspicious code fragment: V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. SearchSelector.cs 177

Issue 7

public void IconSectionGUI(NamedBuildTarget namedBuildTarget,....)
{
  ....
  if (platformUsesStandardIcons)
  {
    var selectedDefault = (m_SelectedPlatform < 0);
    // Set default platform variables
    BuildPlatform platform = null;
    namedBuildTarget = NamedBuildTarget.Standalone;
    ....
  }
  ....
}

V3061 Parameter 'namedBuildTarget' is always rewritten in method body before being used. PlayerSettingsIconsEditor.cs 396

This code fragment is fairly unusual. The method's first parameter is overwritten before being used. What's more, this parameter is used only inside the if (platformUsesStandardIcons) condition. As a result, the value passed to the method is always lost.

Issue 8

internal void BeginNamingNewAsset(....)
{
  m_State.m_NewAssetIndexInList = m_LocalAssets.IndexOfNewText(....);
  if (m_State.m_NewAssetIndexInList != -1)
  {
    Frame(instanceID, true, false);
    GetRenameOverlay().BeginRename(newAssetName, instanceID, 0f);
  }
  else
  {
    Debug.LogError("Failed to insert new asset into list");
  }

  Repaint();
}

V3022 Expression 'm_State.m_NewAssetIndexInList != -1' is always true. ObjectListArea.cs 511

The analyzer detected an expression that is always true. m_State.m_NewAssetIndexInList is assigned a value that the IndexOfNewText method returns. Let's take a look at this method's implementation:

public int IndexOfNewText(....)
{
  int idx = 0;
  if (m_ShowNoneItem)
    idx++;

  for (; idx < m_FilteredHierarchy.results.Length; ++idx)
  {
    FilteredHierarchy.FilterResult r = m_FilteredHierarchy.results[idx];
                    
    if (foldersFirst && r.isFolder && !isCreatingNewFolder)
      continue;
                    
    if (foldersFirst && !r.isFolder && isCreatingNewFolder)
      break;
                    
    string propertyPath = AssetDatabase.GetAssetPath(r.instanceID);
    if (EditorUtility.NaturalCompare(....) > 0)
    {
      return idx;
    }
  }
  return idx;
}

You can notice that the method returns idx that is always greater than or equal to 0.

As a result, the else branch is never executed. The error could hide inside the IndexOfNewText method. The developers had expected the method to be able to return -1.

Issue 9

public static Overlay CreateOverlay(Type type)
{
  ....
  if (overlay == null)
  {
    Debug.LogWarning("Overlay of type {type} can not be instantiated." + ....);
    return null;
  }
  ....
}

V3138 String literal contains potential interpolated expression. Consider inspecting: type. OverlayUtilities.cs 116

PVS-Studio indicates that the string interpolation character is missing. Such mistakes often complicate any attempts to search for problems in code, because default error messages will contain inaccurate information.

Issue 10

int GetCurveAtPosition(Vector2 viewPos, out Vector2 closestPointOnCurve)
{
  ....
  for (int i = m_DrawOrder.Count - 1; i >= 0; --i)
  {
    CurveWrapper wrapper = GetCurveWrapperFromID(m_DrawOrder[i]);

    if (wrapper.hidden || wrapper.readOnly || wrapper.curve.length == 0)
      continue;
    ....
  }
}

V3080 Possible null dereference. Consider inspecting 'wrapper'. CurveEditor.cs 1889

The analyzer detected a code fragment that can lead to dereferencing a reference whose value is null.

The GetCurveWrapperFromID method can return null:

internal CurveWrapper GetCurveWrapperFromID(int curveID)
{
  if (m_AnimationCurves == null)
    return null;

  int index;
  if (curveIDToIndexMap.TryGetValue(curveID, out index))
    return m_AnimationCurves[index];

  return null;
}

The method's return value is stored to the wrapper variable. Then the link is dereferenced — and that can cause an exception. The developer could have been confident that the method would never return null, but nevertheless, this code needs a closer look.

Issue 11

internal static void MaterialShaderReferences(....)
{
  var material = context.target as Material;
  if (material == null || !material.shader)
    return;

  indexer.AddReference(context.documentIndex, "shader", material.shader);

  if (!indexer.settings.options.properties)
    return;

  var ownerPropertyType = typeof(Shader);
  var shaderName = $"{material.shader.name}/" ?? string.Empty;   // <=
  ....
}

V3022 Expression '$"{material.shader.name}/"' is always not null. The operator '??' is excessive. IndexerExtensions.cs 190

The analyzer warns that the $"{material.shader.name}/" is always not null. It's hard to disagree with this statement. Consequently, using the '??' operator and conducting a null check is unnecessary.

Issue 12

static int CountIntersections(....)
{
  ....
  int hitLength = s_RayCastHits.Length;
  float maxDist = 0;
  if (hitLength > 0)
    maxDist = s_RayCastHits[s_RayCastHits.Length - 1].distance;

  physicsScene.Raycast(....);
  if (s_RayCastHits.Length > 0)
  {
    float len = length - s_RayCastHits[0].distance;
    if (len > maxDist)
    {
      maxDist = len;                                 // <=
    }
  }

  return hitLength + s_RayCastHits.Length;
}

V3137 The 'maxDist' variable is assigned but is not used by the end of the function. TreeAOImporter.cs 142

The analyzer points out that the local variable is assigned a value but then this value is never used. You may have also noticed that starting with if (s_RayCastHits.Length > 0), the code does not do anything meaningful. All assignments in this code fragment are done through local variables that do not affect the return value in any way.

Issue 13

public override DragAndDropVisualMode DoDrag(....)
{
  var hierarchyTargetItem = targetItem as GameObjectTreeViewItem;

  if (m_CustomDragHandling != null)
  {
    DragAndDropVisualMode dragResult = 
      m_CustomDragHandling(parentItem as GameObjectTreeViewItem,
                           hierarchyTargetItem,
                           ....);
    ....
  }
  DragAndDropVisualMode dragSceneResult =
    DoDragScenes(parentItem as GameObjectTreeViewItem,
                 hierarchyTargetItem,
                 ....);

  if (   targetItem != null 
      && !IsDropTargetUserModifiable(hierarchyTargetItem, dropPos)) // <=
  {
    return DragAndDropVisualMode.Rejected;
  }
  ....
}

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'targetItem', 'hierarchyTargetItem'. AssetOrGameObjectTreeViewDragging.cs 153

The analyzer points out that the developer casts targetItem to GameObjectTreeViewItem by use of the as operator. However, then the original reference — instead of the resulting reference — is checked for null.

If the conversion by use of the as operator fails, hierarchyTargetItem will contain null. Passing the hierarchyTargetItem null value to IsDropTargetUserModifiable will cause the NullReferenceException exception that everyone loves so much.

This method's simplified code looks as follows:

static bool IsDropTargetUserModifiable(GameObjectTreeViewItem targetItem, ....)
{
  if (targetItem.isSceneHeader && !targetItem.scene.isLoaded)
    return false;
  ....
}

It's worth noting that hierarchyTargetItem is used earlier as a second argument when the m_CustomDragHandling delegate and the DoDragScenes method are called. In the first case, it's unclear which methods the delegate points to, and, consequently, whether dereferencing a null reference may happen. In the second case, the DoDragScenes method always does a null check, so no exception will be thrown. You can find this method's code here.

Issue 14

static Vector3 ResizeHandlesGUI(....)
{
  ....
  Vector3 scale = Vector3.one; 
  ....
  if (uniformScaling)                                 // <=
  {
    float refScale = (xHandle == 1 ? scale.y : scale.x);
    scale = Vector3.one * refScale;
  }

  if (uniformScaling)                                 // <=
  {
    float refScale = (xHandle == 1 ? scale.y : scale.x);
    scale = Vector3.one * refScale;
  }
  ....
}

V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 722, 728. BuiltinTools.cs 722

The analyzer found a suspicious code fragment where two if blocks with identical conditions follow one another. One could assume that the second if block is redundant code that does not affect anything. But this assumption is not quite correct, because the code uses the scale value to form the refScale value. This means, the second block still affects the result.

It's worth noting that the uniformScaling does not change inside the conditional blocks. This means, all calculations could be placed under one if.

Conclusion

Looks like checking this project again was a good idea. I found several code fragments that were definitely worth my attention. Which of those are errors and which are just flaws? It's upon developers to decide. From the outside, alas, it can be difficult to determine whether a warning is critical.

In any case, I thank the Unity team for their hard work! I want to believe that this article made a small contribution to the project's quality.

You can also download PVS-Studio and check your project. To do this, you can get a trial key on our website.

#CSharp #GameDev