Все мы любим игры, но есть люди, которые любят в них не только играть, но ещё и создавать различные модификации для них. Сегодня посмотрим на WolvenKit — один из инструментов для создания модов для Cyberpunk 2077.

Введение

Я думаю, что большинство из вас ставили моды в свои любимые игры. И на это есть множество причин. Добавить новые геймплейные механики, скины, улучшить графику, да и просто привнести больше веселья в игру. Например, у меня такой игрой был The Elder Scrolls V: Skyrim. И я точно такой не один. Это же как раз та игра, где Драконорождённый мог сражаться с паровозиком Томасом, а не с драконами.

Кто-то даже сам создавал модификации для игр. Сегодня мы посмотрим на проект для создания модов для игр с точки зрения кода. А конкретнее, взглянем на ошибки и подозрительные места в коде, обнаруженные статическим анализатором. Этот проект был найден мной на просторах GitHub. WolvenKit — это инструмент для моддинга Cyberpunk 2077 с открытым исходным кодом, написанный на C#.

Для анализа же мы будем использовать статический анализатор кода PVS-Studio 7.32. Получить триальный ключ и попробовать последнюю версию анализатора можно с помощью этой страницы.

Готовы? Тогда поехали!

Ошибки и странности

Issue 1

Любая программа начинается с установки, так и WolvenKit не исключение. На GitHub также доступен WolvenKit.Installer. Он тоже написан на C#. Я решил не оставлять его в стороне и проанализировать. Да, кода там мало, но это не помешало найти одну ошибку.

public async Task InstallAsync(....)
{
  ....
  try
  {
    ....
  }
  catch (Octokit.ApiException)
  {
    var apiInfo = ghClient.GetLastApiInfo();
    var rateLimit = apiInfo?.RateLimit;
    var howManyRequestsCanIMakePerHour = rateLimit?.Limit;
    var howManyRequestsDoIHaveLeft = rateLimit?.Remaining;
    var whenDoesTheLimitReset = rateLimit?.Reset;

     _logger.LogInformation(
    $"[Update] {howManyRequestsDoIHaveLeft}/{howManyRequestsCanIMakePerHour}" +
    $" - reset: {whenDoesTheLimitReset 
    ?? whenDoesTheLimitReset.Value.ToLocalTime()}");
    return false;
  }
}

Предупреждение PVS-Studio: V3105 The 'whenDoesTheLimitReset' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. LibraryService.cs 332

Ошибка закралась в обработчик исключений :) Анализатор говорит нам о том, что переменная whenDoesTheLimitReset используется, хотя получила значение с помощью оператора '?.', а значит может быть равна null. При этом мы видим проверку на null с помощью оператора '??'. Вот только переменная как раз используется, если её значение равно null. В итоге получим исключение типа NullReferenceException. Интересно, из-за чего возникла данная ошибка. Может разработчик не знал, как работает оператор '??', или эта ошибка возникла из-за невнимательности.

Issue 2

А теперь переходим к ошибкам основного проекта.

public AppImpl()
{
  ....
  // load oodle
  var settingsManager = Locator.Current.GetService();
  if (   settingsManager.IsHealthy() 
      && !Oodle.Load(settingsManager?.GetRED4OodleDll()))
  {
    throw new FileNotFoundException($"{Core.Constants.Oodle} not found.");
  }
}

Предупреждение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'settingsManager' object App.xaml.cs 48

Ещё одна ошибка с null. Переменная settingsManager дважды используется в условии оператора if. Причём сначала без проверки на null, а во второй раз с проверкой, с помощью '?.' Можно подумать, что IsHealthy — это метод расширения, и исключения не возникнет, но нет, это обычный экземплярный метод.

Issue 3

public EFileReadErrorCodes ReadBuffer(RedBuffer buffer)
{
  ....
  data.Uk1 = _reader.ReadUInt32();
  data.Uk2 = _reader.ReadUInt32();
  data.Uk3 = _reader.ReadUInt32();
  var numBrck = _reader.ReadUInt32();
  var numSurf = _reader.ReadUInt32();
  var numProb = _reader.ReadUInt32();
  var numFact = _reader.ReadUInt32();
  var numTetr = _reader.ReadUInt32();
  data.Uk4 = _reader.ReadUInt32();
  data.Uk5 = _reader.ReadUInt32();
  data.Uk5 = _reader.ReadUInt32();      // <=

  data.Bounds.Min.X = _reader.ReadSingle();
  ....
}

Предупреждение PVS-Studio: V3008 The 'data.Uk5' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 35, 34. CGIDataReader.cs 35

А вот пример часто встречающейся ошибки. Свойству data.Uk5 дважды присваивается значение. Видимо, разработчик просто копировал одну строчку и изменял цифру, а в последней строчке забыл. И вряд ли это лишняя строчка, так как имеется свойство data.Uk6.

Issue 4

public void Load()
{
  ....
  for (....; sectorIndex < pgc.BufferTableSectors.Count; ....)  // <=
  {
    if (   pgc.SectorEntries == null 
        || pgc.SectorEntries.Count <= sectorIndex 
        || pgc.SectorEntries[sectorIndex] == null)
    {
      throw new ArgumentNullException();
    }

    var sectorHash = pgc.SectorEntries[sectorIndex]!.SectorHash;
    if (!_entries.ContainsKey(sectorHash))
    {
      _entries[sectorHash] = new();
    }

    if (   pgc.BufferTableSectors == null                    // <=
        || pgc.BufferTableSectors.Count <= sectorIndex 
        || pgc.BufferTableSectors[sectorIndex] == null)
    {
      throw new ArgumentNullException();
    }
  }
}

Предупреждение PVS-Studio: V3095 The 'pgc.BufferTableSectors' object was used before it was verified against null. Check lines: 43, 56. GeometryCacheService.cs 43

Анализатор обнаружил использование свойства pgc.BufferTableSectors перед проверкой этого же свойства на равенство null.

Issue 5

private void LoadInfo()
{
  if (_projectManager.ActiveProject is null)
  {
    return;
  }

  var path = Path.Combine(Environment.CurrentDirectory,
                          "Resources",
                          "soundEvents.json");
  path = Path.Combine(_projectManager.ActiveProject.ResourcesDirectory,
                      "info.json");
    if (File.Exists(path))
    {
      ....
    }
}

Предупреждение PVS-Studio: V3008 The 'path' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 82, 81. SoundModdingViewModel.cs 82

А вот это вряд ли ошибка, но точно является подозрительным фрагментом кода. Переменной path два раза присваивается значение. Возможно, одно из них осталось после отладки или это просто артефакт разработки.

Issue 6

private bool InsertArrayItem(IRedArray ira, int index, IRedType item)
{
  var iraType = ira.GetType();
  if (iraType.IsGenericType)
  {
    var arrayType = iraType.GetGenericTypeDefinition();
    if (   arrayType == typeof(CArray<>) 
        || arrayType == typeof(CStatic<>) 
        && ira.Count < ira.MaxSize)
    {
      ....
    }
  }
}

Предупреждение PVS-Studio: V3130 Priority of the '&&' operator is higher than that of the '||' operator. Possible missing parentheses. ChunkViewModel.cs 3053

Ещё один подозрительный фрагмент. Сложно сказать — ошибка или нет, но стоит задуматься над этим кодом. Анализатор сообщает про возможную ошибку с приоритетом операторов. Как мы знаем, приоритет оператора '&&' выше, чем '||'. Получается, что выражение ira.Count < ira.MaxSize вычисляется только с arrayType == typeof(CStatic<>), а не со всем OR выражением . Если разработчик действительно хотел такой логики, то для читаемости стоит взять AND выражение в скобки.

Issue 7

private void AddCurrent(worldNodeData current)
{
  ....
  if (Parent?.Data is DataBuffer db && db.Buffer.Data is IRedType irt)
  {
    if (irt is IRedArray ira && ira.InnerType.IsAssignableTo(current.GetType()))
    {
      var indexx = Parent.GetIndexOf(this) + 1;
      if (indexx == -1 || indexx > ira.Count)      // <=
      {
        indexx = ira.Count;
      }
      ira.Insert(indexx, current);
    }
  }
}

Предупреждение PVS-Studio: V3063 A part of conditional expression is always false if it is evaluated: indexx == -1. ChunkViewModel.cs 4201

Анализатор говорит о том, что выражение indexx == -1 всегда false. Почему он так решил и правильно ли это? Давайте разбираться. Переменная indexx получает своё значение из выражения Parent.GetIndexOf(this) + 1. Взглянем на метод GetIndexOf.

public int GetIndexOf(ChunkViewModel child)
{
  if (child.NodeIdxInParent > -1)
  {
    return child.NodeIdxInParent;
  }
  
  for (var i = 0; i < Properties.Count; i++)
  {
    if (ReferenceEquals(Properties[i], child))
    {
      child.NodeIdxInParent = i;
      return i;
    }
  }
  return -1;
}

Как видим, самое минимальное, что метод может вернуть — это -1. А в нашем выражении к возвращаемому значению метода прибавляют 1. Получается, что анализатор прав. Тут либо не нужно прибавлять 1, либо нужно как-то условие изменить.

Анализатор по ходу кода подсчитывает возможные значения для переменных и накладываемые ограничения. Этот механизм называется Data Flow. Именно благодаря этому анализатор и нашёл такую ошибку.

Issue 8

public static Dictionary GetPropertiesFor(....)
{
  ....
  foreach (var appearance in appearancesArr)
  {
    details[....] = appearance.AppearanceName.ToString()!;
    details[....] = appearance.IsPlayer == true ? "True" : "False";
    details[....] = ParseGameEntityReference(appearance?.PuppetRef);

    counter++;
  }
  ....
}

Предупреждение PVS-Studio: V3095 The 'appearance' object was used before it was verified against null. Check lines: 547, 548. NodeProperties.cs 547

Простая ошибка. Есть 3 строчки, в которых используется переменная appearance. При этом в последней строчке appearance используется с оператором '?.', то есть потенциально переменная может иметь значение null.

Issue 9

public int UncookTask(FileSystemInfo[] paths, UncookTaskOptions options)
{
  ....
  if (options.outpath == null)       // <=
  {
    _loggerService.Error("Please fill in an output path.");
    return ERROR_BAD_ARGUMENTS;
  }

  if (   options.meshExportType != null
      && string.IsNullOrEmpty(options.meshExportMaterialRepo) 
      && options.outpath is null)     // <=
  {
    _loggerService.Error("When using --mesh-export-type, the --outpath" +
    " or the --mesh-export-material-repo must be specified.");
    return ERROR_INVALID_COMMAND_LINE;
  }
}

Предупреждение PVS-Studio: V3022 Expression 'options.meshExportType != null && string.IsNullOrEmpty(options.meshExportMaterialRepo) && options.outpath is null' is always false. UncookTask.cs 44

В данном случае анализатор считает, что всё это большое выражение всегда false. И это действительно так. Все подвыражения соединены через '&&', и при этом последнее подвыражение уже было в предыдущем операторе if с последующим выходом из метода.

Issue 10

private static string TryGetGameInstallDir()
{
  if (   programName?.ToString() is string n 
      && installLocation?.ToString() is string l)
  {
    if (n.Contains(gameName) || n.Contains(gameName))  // <= 
    {
      exePath = Directory.GetFiles(l, exeName, SearchOption.AllDirectories)
                         .First();
    }
  }
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'n.Contains(gameName)' to the left and to the right of the '||' operator. Oodle.cs 488

Анализатор обнаружил одинаковые подвыражения слева и справа от оператора '||'. Возможно, во втором подвыражении нужно использовать переменную l.

Issue 11

public void Extract(Stream output, bool decompressBuffers)
{
  if (Archive is not { } ar)
  {
    throw new InvalidParsingException(
      $"{Archive.ArchiveAbsolutePath} is not a cyberpunk77 archive.");
  }

  ar.CopyFileToStream(output, NameHash64, decompressBuffers);
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'Archive'. FileEntry.cs 87

По сути, Archive is not { } — это то же самое, что и Archive is not object или Archive is null. И если эта проверка в результате даёт true, то при создании исключения об ошибке получим NullReferenceExceprion.

Issue 12

public ButtonCursorStateView()
{
  HoverStateName = "Hover";
  PressStateName = "Hover";
  DefaultStateName = "Default";

  PostConstruct();
}

Предупреждение PVS-Studio: V3091 Empirical analysis. It is possible that a typo is present inside the string literal: "Hover". The 'Hover' word is suspicious. ButtonCursorStateView.cs 34

Немного эвристического анализа. Кажется, анализатор нашёл неудачный copy-paste и в PressStateName нужно присвоить "Press".

Issue 13

public static int GetCompressedBufferSizeNeeded(int count)
{
  var n = (((count + 0x3ffff + ((uint)((count + 0x3ffff) >> 0x3f) & 0x3ffff))
    >> 0x12) * 0x112) + count;
  //var n  = OodleNative.GetCompressedBufferSizeNeeded((long)count);
  return (int)n;
}

Предупреждение PVS-Studio: V3134 Shift by 63 bits is greater than the size of 'Int32' type of expression '(count + 0x3ffff)'. Oodle.cs 397

И последний подозрительный фрагмент. В данном случае разработчик хочет выполнить сдвиг на 0x3f, что эквивалентно 63. Вот только это больше, чем размер типа выражения, к которому применяется сдвиг. Это выражение имеет тип Int32 с размером 32. Так как в C# сдвиги выполняются циклически, сдвиг на 63 будет эквивалентен сдвигу на 31.

Заключение

Вот мы и прошлись по всем ошибкам и подозрительным местам в проекте WolvenKit. В статье я привёл не все ошибки, т.к. многие из них повторяются, но я обязательно сообщу обо всех разработчикам с помощью issue на GitHub.

Удачи!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Artem Rovenskii. WolvenKit code analysis: things to know before modding Cyberpunk 2077.