Rechecking Unity with the PVS-Studio static analyzer

Unity is one of the most popular game engines. It creates a lot of great cross-platform projects. It has been almost 4 years since our last review of its source code. It’s time to find out what you can find interesting this time.

Introduction

We have previously released article about checking Unity. I think you will be interested to get acquainted with it. It’s a really big project that thousands of developers use every day. And don’t forget about the users who spend their time playing games developed with Unity. I believe that projects of this scale should not be left unattended for a long time, because mistakes in them can affect a large number of people.

We will analyze the source code of the Unity engine and the version editor 2022.1.0b8. Let’s go directly to the test results.

Test 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 a value is assigned twice in a row, and the variable is not used between assignments. If you look closely, you can see that the value generated for this variable is slightly higher under the condition m_NumFilteredVariants > maxFilteredLength will also be lost.

It turns out that all changes in the value of the variable, except for the last one, do not make 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 access members of the same object. If the value version will nullthen the method IsNullOrEmpty will return true, and when referring to displayName an exception will be thrown NullReferenceException.

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

Developers perform the same check twice in a row. *hZoomLockedByDefault is a class field and if we look at the place of its declaration, we will see that there is a field nearby vZoomLockedByDefault.

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

Everything looks like a banal typo, which is exactly what the analyzer points out.

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 parser reports that the expression inputTextField.panel != null always true.

Indeed, part of the condition above already contains such a check. Perhaps they wanted to check something else, but they made a mistake.

Issue 5

The following code was found:

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. Editor GUIExt.cs 141

In this case, the analyzer has found an expression that is always false. Whatever value the property returns, one of the comparisons will be false.

Possible corrected 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 has detected a situation in the code where two consecutive calls OrderBy.

As for me, this is a very interesting warning. Naturally, two calls OrderBy are not an error pattern. It’s more of a code that can lead to an error if you misunderstand how it works. If the programmer wanted to sort the collection first by key and then by priority, then in this case there will be an error. Why?

Let’s figure it out. This code has two calls OrderBy sort the collection first by priority, and then by key. Not really obvious, is it? I think that instead of the second OrderBy it is better to use ThenByso that sorting is not done in reverse. Writing with ThenBy will be better read and will not cause unnecessary questions. For details, I suggest you go to a small note.

By the way, PVS-Studio found another similar suspicious place: 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

Pretty weird piece of code. The first method parameter is overwritten before it is used. Moreover, this parameter is used only inside the condition if (platformUsesStandardIcons). As a result, the value that is 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 found an expression that is always true. m_State.m_NewAssetIndexInList the return value of the method is assigned IndexOfNewText. Let’s take a look at the implementation of this method:

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 see that the method returns idxwhich is always greater than or equal to 0.

As a result of the branch else will never be executed. Perhaps the error may be in the method IndexOfNewText. When using it, it was expected that it could 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 inspection: type. OverlayUtilities.cs 116

PVS-Studio indicates that a string interpolation character is missing. Such shortcomings often complicate the search for a problem, since the received message 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 has detected a code fragment that can dereference a reference whose value is equal to null.

Method GetCurveWrapperFromID 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 return value of the method is written to a variable wrapper. After that, the reference is dereferenced, which may result in an exception being thrown. Perhaps the developer is completely sure that in his case the method will never return nullbut this code is definitely worth paying attention to.

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 signals that the expression $”{material.shader.name}/” always not null. I think it’s pretty hard to disagree with this statement. Therefore, checking for null using the ‘??’ operator is useless.

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 indicates that a value is assigned to a local variable, but after that it is not used anywhere. You can also notice that the code starting from if (s_RayCastHits.Length > 0), doesn’t do anything meaningful. All assignments in this fragment are made to local variables, which 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 indicates that targetItem result in a derived type GameObjectTreeViewItem using the operator as. However, in line with null they check not the received link, but the original one.

If it doesn’t work, convert with asthen hierarchyTargetItem will contain the value null. And when transmitting hierarchyTargetItem in IsDropTargetUserModifiable inside the method, everyone’s favorite will be thrown out NullReferenceException.

Simplified code for this method:

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

It is worth noting that hierarchyTargetItem used even earlier as the second argument when calling the delegate m_CustomDragHandling and method DoDragScenes. In the first case, it is not clear what methods the delegate points to and, as a result, whether it is possible to dereference a null reference. In the second case, in the method DoDragScenes always checking for null, so no exception will be thrown. The code for this method can be found 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 drew attention to a suspicious code in which two if follow each other under the same conditions. It could be assumed that the second if – just extra code that does not affect anything. This assumption will be erroneous, since when forming the value refScale value is used scale. Therefore, the second block still affects the result.

It is worth noting that the value uniformScaling does not change inside conditional blocks. That is, all calculations could be written under one if.

Conclusion

I guess it was not in vain that I decided to check this project again. Re-checking Unity allowed us to find several rather interesting places. Which of the following are errors and which are just shortcomings? The developers will have to judge this. From the outside, alas, it can be difficult to assess the criticality of a warning.

In any case, thanks to the Unity team for their hard work! I would like to believe that this article helped to make a small contribution to the quality of the project.

You can also check your project with the PVS-Studio analyzer. To do this, you can take a trial key from us at site.

If you want to share this article with an English-speaking audience, please use the translation link: Artem Rovenskii. PVS-Studio static analyzer to recheck Unity.