Сергей Хренов

16 Апр 2018

Теги:

#CSharp #GameDev

Проверяем исходный C#-код Unity

16 Апр 2018

Автор: Сергей Хренов

Введение
Результаты проверки
Выводы

Недавно произошло долгожданное для многих событие - компания Unity Technologies разместила исходный C#-код игрового движка Unity для свободного скачивания на GitHub. Представлен код движка и редактора. Конечно, мы не могли пройти мимо, тем более, что в последнее время мы пишем не так много статей о проверке проектов на C#. Unity разрешает использовать предоставленные исходники только в справочных целях. Именно так и поступим. Испытаем последнюю на данный момент версию PVS-Studio 6.23 на коде Unity.

Введение

Ранее мы уже писали статью о проверке Unity. На тот момент для анализа было доступно не так много C#-кода: некоторые компоненты, библиотеки и примеры использования. Тем не менее, автору статьи удалось обнаружить довольно интересные ошибки.

Чем же Unity порадовал на этот раз? Я говорю "порадовал" и надеюсь, что не обижу этим авторов проекта. Тем более, что объем исходного C#-кода Unity, представленного на GitHub, составляет около 400 тысяч строк (без учета пустых) в 2058 файлах с расширением "cs". Это немало, и анализатор имел весьма широкое поле для деятельности.

Теперь о результатах. Перед началом анализа я немного упростил себе работу, включив режим отображения кодов по классификации CWE для найденных ошибок, а также активировав режим подавления предупреждений третьего уровня достоверности (Low). Эти настройки доступны в выпадающем меню PVS-Studio среды разработки Visual Studio, а также в параметрах анализатора. Избавившись таким образом от предупреждений с низкой достоверностью, я провел анализ исходного кода Unity, в результате которого было получено 181 предупреждение первого уровня достоверности (High) и 506 предупреждений второго уровня достоверности (Medium).

Я не стал изучать абсолютно все полученные предупреждения, так как их достаточно много. Разработчики или энтузиасты могут легко провести глубокий анализ, выполнив проверку Unity самостоятельно. Для этого у PVS-Studio предусмотрены триальный и бесплатный режимы использования. Также компании могут купить наш продукт и получить помимо лицензии быструю и подробную поддержку.

Судя по тому, что практически в каждой группе предупреждений мне с одной-двух попыток сразу удавалось найти реальную ошибку - их в Unity немало. И да, они разнообразные. Давайте изучим наиболее интересные ошибки.

Результаты проверки

Что-то не так с флагами

Предупреждение PVS-Studio: 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);
  ....
}

При комбинировании флагов перечисления MethodAttributes была допущена ошибка: значение Public использовано дважды. Возможно, виной тому неудачное форматирование кода.

Аналогичная ошибка допущена и в коде метода GenerateDeserialization:

V3001 There are identical sub-expressions 'MethodAttributes.Public' to the left and to the right of the '|' operator. SyncListStructProcessor.cs 309

Copy-Paste

Предупреждение 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);
}

Я привел фрагмент кода, предварительно отформатировав его, поэтому ошибка легко обнаруживается визуально: дважды производится сравнение с RenderTextureFormat.ARGBFloat. В оригинальном коде это выглядит несколько иначе:

Вероятно, в одном из двух одинаковых сравнений необходимо использовать другое значение перечисления RenderTextureFormat.

Двойная работа

Предупреждение PVS-Studio: 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;
  }
....
}

Переменной дважды присваивается значение true, так как Weaver.fail и fail - это одно и то же статическое поле класса Weaver. Возможно, грубой ошибки тут и нет, но код определенно требует внимания.

Без вариантов

Предупреждение PVS-Studio: 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;
}

Метод всегда возвращает false. Обратите внимание на комментарий в начале.

Забыли про результат

Предупреждение PVS-Studio: 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();
}

При конкатенации двух массивов discardedModifications и discardedRotationModifications забыли сохранить результат. Вероятно, программист предположил, что результат будет отражен сразу в массиве discardedModifications. Но это не так. В результате из метода возвращают исходный вариант массива discardedModifications. Код необходимо исправить следующим образом:

static public UndoPropertyModification[] Process(....)
{
  ....
  return discardedModifications.Concat(discardedRotationModifications)
                               .ToArray();
}

Не то проверили

Предупреждение PVS-Studio: 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;
}

В данном методе забыли предусмотреть ситуацию, когда переменная obj не будет равна null, но её не удастся привести к типу GameViewSize. Тогда переменная newResolution получит значение null, а отладочный вывод не будет произведен. Исправленный вариант кода мог бы иметь вид:

private static GameViewSize CastToGameViewSize(object obj)
{
  GameViewSize newResolution = obj as GameViewSize;
  if (newResolution == null)
  {
    Debug.LogError("Incorrect input");
  }
  return newResolution;
}

Недоработка

Предупреждение PVS-Studio: V3020 CWE-670 An unconditional 'return' within a loop. PolygonCollider2DEditor.cs 96

private void HandleDragAndDrop(Rect targetRect)
{
  ....
  foreach (....)
  {
    ....
    if (....)
    {
      ....
    }
    return;
  }
  ....
}

Цикл выполнит только одну итерацию, после чего метод завершит работу. Вероятны различные сценарии. Например, return должен находиться внутри блока if, либо где-то перед return пропущена директива continue. Вполне может быть, что ошибки тут и нет, но тогда следует сделать код более понятным.

Недостижимый код

Предупреждение PVS-Studio: 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
  ....
}

Две одинаковые проверки, следующие одна за другой. Очевидно, что в случае равенства buildingForEditor значению true, вторая проверка лишена смысла, так как в результате первой метод завершает работу. Если же значение buildingForEditor - false, то не будет выполнена ни then-ветвь первого оператора if, ни второго. Налицо ошибочная конструкция, требующая исправления.

Безусловное условие

Предупреждение PVS-Studio: 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];
}

Условие проверки индекса некорректно - результатом всегда будет false. Тем не менее, в случае передачи в метод GetParameter ошибочного индекса, исключение IndexOutOfRangeException все же будет выброшено, но уже при попытке доступа к элементу массива в блоке return. Правда, сообщение об ошибке будет несколько иным. Для того, чтобы код вел себя так, как ожидает разработчик, необходимо вместо оператора && в условии использовать ||:

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

Вероятно, вследствие использования методики Copy-Paste, в коде Unity присутствует ещё одна точно такая же ошибка:

Предупреждение PVS-Studio: V3022 CWE-570 Expression 'index < 0 && index >= parameters.Length' is always false. Animator.bindings.cs 711

И ещё одна похожая ошибка, также связанная с некорректным условием проверки индекса массива:

Предупреждение PVS-Studio: 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;
}

И в этом случае возможен выброс исключения IndexOutOfRangeException. Для исправления ошибки необходимо, как и в предыдущих фрагментах кода, использовать в условии оператор || вместо &&.

Просто странный код

На приведенный далее фрагмент кода указывают сразу два предупреждения:

Предупреждение PVS-Studio: V3022 CWE-571 Expression 'bRegisterAllDefinitions || (AudioSettings.GetSpatializerPluginName() == "GVR Audio Spatializer")' is always true. AudioExtensions.cs 463

Предупреждение PVS-Studio: 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;
  }
}

Выглядит, как незавершённый метод. Непонятно, почему его оставили в таком виде и не закомментировали бесполезные блоки кода. Всё, что метод делает в настоящее время:

if (!m_BuiltinDefinitionsRegistered)
{
  m_BuiltinDefinitionsRegistered = true;
}

Бесполезный метод

Предупреждение PVS-Studio: V3022 CWE-570 Expression 'PerceptionRemotingPlugin.GetConnectionState() != HolographicStreamerConnectionState.Disconnected' is always false. HolographicEmulationWindow.cs 171

private void Disconnect()
{
  if (PerceptionRemotingPlugin.GetConnectionState() !=
      HolographicStreamerConnectionState.Disconnected)
    PerceptionRemotingPlugin.Disconnect();
}

Для прояснения ситуации необходимо взглянуть на объявление метода PerceptionRemotingPlugin.GetConnectionState():

internal static HolographicStreamerConnectionState
GetConnectionState()
{
  return HolographicStreamerConnectionState.Disconnected;
}

Таким образом, вызов метода Disconnect() ни к чему не приводит.

С тем же методом PerceptionRemotingPlugin.GetConnectionState() связана ещё одна ошибка:

Предупреждение PVS-Studio: V3022 CWE-570 Expression 'PerceptionRemotingPlugin.GetConnectionState() == HolographicStreamerConnectionState.Connected' is always false. HolographicEmulationWindow.cs 177

private bool IsConnectedToRemoteDevice()
{
  return PerceptionRemotingPlugin.GetConnectionState() ==
         HolographicStreamerConnectionState.Connected;
}

Результат работы метода эквивалентен следующему:

private bool IsConnectedToRemoteDevice()
{
  return false;
}

Как видим, среди предупреждений V3022 нашлось немало интересных. Вероятно, если потратить ещё какое-то время, можно увеличить список. Но давайте двигаться дальше.

Не по формату

Предупреждение 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));
  ....
}

Ошибки нет, но код, как говорится, "с запахом". Вероятно, ранее сообщение было более информативным, наподобие такого: "Negative path index {0} is invalid.". Затем его упростили, но параметр index для метода Format убрать забыли. Конечно, это не то же самое, как забытый параметр для указанного спецификатора вывода в строку, то есть конструкция вида String.Format("Negative path index {0} is invalid."). В таком случае было бы выброшено исключение. Но и в нашем случае необходима аккуратность при рефакторинге. Код нужно исправить так:

public void SetPath(....)
{
  if (index < 0)
    throw new ArgumentOutOfRangeException(
      "Negative path index is invalid.");
  ....
}

Подстрока подстроки

Предупреждение PVS-Studio: 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");
}

Поиск подстроки "UnityEngine.SetupCoroutine" в условии лишен всякого смысла, так как перед этим производится поиск "UnityEngine.". Таким образом, следует удалить последнюю проверку, либо уточнить правильность подстрок.

Ещё одна подобная ошибка:

Предупреждение PVS-Studio: 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;
}

Размер имеет значение

Предупреждение PVS-Studio: 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;
}

Условие для проверки допустимого размера страницы ошибочно. Вместо оператора || необходимо использовать &&. Исправленный код:

public override bool IsValid()
{
  ....
  return base.IsValid()
    && (pageSize >= 1 && pageSize <= 1000)
    && totalFilters <= 10;
}

Возможно деление на ноль

Предупреждение PVS-Studio: 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));
  ....
}

Проблема может возникнуть при передаче в метод значения width = 1. В самом методе это никак не проверяется. Метод GenerateColorTexture вызывается в коде всего один раз с параметром 100:

void OnEnable()
{
  if (s_ColorTexture == null)
    s_ColorTexture = GenerateColorTexture(100);
  ....
}

Так что ошибки пока нет. Но, на всякий случай, в методе GenerateColorTexture следует предусмотреть возможность передачи некорректного значения ширины.

Парадоксальная проверка

Предупреждение PVS-Studio: 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(....);
    ....
  }
}

Вероятно, вследствие опечатки, выполнение данного кода гарантирует использование нулевой ссылки m_Parent. Исправленный код:

public void ShowPopup()
{
  if (m_Parent != null)
  {
    ....
    Rect r = m_Parent.borderSize.Add(....);
    ....
  }
}

Точно такая же ошибка встречается и далее в коде:

Предупреждение PVS-Studio: 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(....);
    ....
}

А вот ещё одна интересная ошибка, которая может привести к доступу по нулевой ссылке вследствие некорректной проверки:

Предупреждение PVS-Studio: 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);
  ....
}

Мне кажется, что разработчики Unity довольно часто допускают ошибки, связанные с неправильным использованием операторов || и && в условиях. В данном случае, если objects будет иметь значение null, то это приведет к проверке второй части условия (objects != null || objects.Length >= 1), что повлечет за собой непредвиденный выброс исключения. Ошибку необходимо исправить следующим образом:

public TypeSelection(string typeName, Object[] objects)
{
  System.Diagnostics.Debug.Assert(objects != null &&
                                  objects.Length >= 1);
  ....
}

Рано обнулили

Предупреждение PVS-Studio: 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));
  }
  ....
}

В данном случае выброс исключения (доступ по нулевой ссылке m_RowRects) произойдет при формировании строки сообщения для другого исключения. Код можно исправить, например, так:

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

Снова ошибка при проверке

Предупреждение PVS-Studio: 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() + ",";  
  ....
}

Из-за того, что в условии использован оператор &, вторая часть условия будет проверена всегда, вне зависимости от результата проверки первой части. В случае, если переменная additionalOptions будет иметь значение null, неизбежен выброс исключения. Ошибку необходимо исправить, использовав оператор && вместо &.

Как видим, среди предупреждений с номером V3080 присутствуют довольно коварные ошибки.

Запоздалая проверка

Предупреждение PVS-Studio: 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)
  {
    ....
  }
  ....
}

Переменную element используют без предварительной проверки на неравенство null. При этом далее в коде такая проверка выполняется. Код, вероятно, необходимо исправить таким образом:

public override void OnBeginElementTest(VisualElement element, ....)
{
  if (element != null)
  {
    if (element.IsDirty(ChangeType.Styles))
    {
      ....
    }

    if (element.styleSheets != null)
    {
      ....
    }
  }
  ....
}

В коде есть ещё 18 подобных ошибок. Приведу списком первые 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
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

Некорректный метод Equals

Предупреждение PVS-Studio: 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;
}

Перегрузка метода Equals выполнена небрежно. Необходимо учесть возможность получения null в качестве параметра, так как это может привести к выбросу исключения, которое не было предусмотрено в вызывающем коде. Также к выбросу исключения приведет ситуация, когда не удастся привести _other к типу CurveSelection. Код требует исправления. Хороший пример реализации перегрузки Equals приведен в документации.

В коде есть и другие подобные ошибки:

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

И снова о проверке на неравенство null

Предупреждение PVS-Studio: 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);
}

При первом использовании переменной camera, её проверяют на неравенство null. А вот далее по коду это сделать забывают. Исправленный вариант мог бы иметь вид:

protected void DisableARBackgroundRendering()
{
  ....
  if (camera != null)
  {
    camera.clearFlags = m_CameraClearFlags;

    // Command buffer
    camera.RemoveCommandBuffer(CameraEvent.BeforeForwardOpaque,
                               m_CommandBuffer);
    camera.RemoveCommandBuffer(CameraEvent.BeforeGBuffer,
                               m_CommandBuffer);
  }
}

Ещё одна подобная ошибка:

Предупреждение PVS-Studio: 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;
}

Допущена ошибка, приводящая в некоторых случаях к доступу по нулевой ссылке. Выполнение условия в первом блоке if обеспечивает выход из метода. Однако если этого не происходит, то нет гарантий, что ссылка item ненулевая. Исправленный вариант кода:

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

В коде есть ещё 12 аналогичных ошибок. Приведу списком первые 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

Выбор оказался невелик

Предупреждение PVS-Studio: V3136 CWE-691 Constant expression in switch statement. HolographicEmulationWindow.cs 261

void ConnectionStateGUI()
{
  ....
  HolographicStreamerConnectionState connectionState =
    PerceptionRemotingPlugin.GetConnectionState();
  switch (connectionState)
  {
    ....
  }
  ....
}

И тут оказался виноват метод PerceptionRemotingPlugin.GetConnectionState(). Мы уже сталкивались с ним, когда изучали предупреждения V3022:

internal static HolographicStreamerConnectionState
  GetConnectionState()
{
  return HolographicStreamerConnectionState.Disconnected;
}

Метод вернет константу. Очень странный код. Необходимо обратить на него пристальное внимание.

Выводы

Думаю, на этом можно остановиться, иначе статья станет скучной и затянутой. Повторюсь, я привел ошибки, которые мне сразу бросились в глаза. Код Unity, несомненно, содержит большее число ошибочных или некорректных конструкций, требующих исправления. Трудность состоит в том, что многие из выданных предупреждений носят весьма спорный характер и точный "диагноз" в каждом конкретном случае способен поставить только автор кода.

В целом о проекте Unity можно сказать, что он богат на ошибки, но с учётом объема его кодовой базы (400 тысяч строк), всё не так плохо. Тем не менее, надеюсь, что авторы не будут пренебрегать инструментами анализа кода для улучшения качества своего продукта.

Используйте PVS-Studio и безбажного всем кода!

#CSharp #GameDev