Анализ кода WolvenKit: что нужно знать перед созданием модов для Cyberpunk 2077

Все мы любим игры, но есть люди, которые любят в них не только играть, но ещё и создавать различные модификации для них. Сегодня посмотрим на 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.