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