Анализ кода 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<bool> 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<ISettingsManager>(); 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<string, string> 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.

Удачи!