WolvenKit code analysis: things to know before modding Cyberpunk 2077

We're all keen on playing games, but some of us also enjoy creating various mods. Today, let's take a peek at WolvenKit, one of the modding tools for Cyberpunk 2077.

Intro

I think most of us have installed mods in the closest-to-our-heart games. And there are plenty of reasons for that: we want to add some new gameplay mechanics and skins, enhance the graphics, and just bring more hilarity to the game. For instance, that's what I did in The Elder Scrolls V: Skyrim. And I guess I'm not the only one. I mean, this is exactly the sort of game where players could transform dragons into Thomas the Tank Engine, and Dovahkiin could fight this creature.

Some even created their own mods for the games. Today, we're going to check the project for game modding and see how its code is set up. Let's take a look at the errors and suspicious code fragments that the static analyzer has detected. I found this project on GitHub. WolvenKit is an open-source modding tool for Cyberpunk 2077 written in C#.

We use the PVS-Studio 7.32 static code analyzer. If you're interested, you can get a free trial key on this page and evaluate the latest analyzer version.

Ready? Steady? Go!

Errors and stranger things

Issue 1

As with any program, we need to install WolvenKit before using it. WolvenKit.Installer is also available on GitHub. It's also written in C#. I've decided not to leave it aside and analyze it. There's not much code, but the analyzer still managed to detect a bug.

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

The PVS-Studio warning: V3105 The 'whenDoesTheLimitReset' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. LibraryService.cs 332

The error lurks in the exception handler :) The analyzer reports that developers use the whenDoesTheLimitReset variable, although it gets its value using the '?.' operator. This could mean that it may be equal to null. And we see a check for null using the '??' operator. However, the variable will be really used if its value is null. As a result, we get a NullReferenceException. Hmm, what caused the error? Developers might not know how the '??' operator works, or they just didn't spot it in the code.

Issue 2

Let's look at the bugs in the main project.

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

The PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'settingsManager' object App.xaml.cs 48

Here's another null error. Developers used the settingsManager variable twice in the condition of the if statement. They initially used it without null checking, and then with the check via '?.'. We might assume that IsHealthy is an extension method, so the exception won't be thrown. Unfortunately, it's a regular instance method.

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

The PVS-Studio warning: V3008 The 'data.Uk5' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 35, 34. CGIDataReader.cs 35

Here's an example of a common error. The data.Uk5 property is assigned a value twice. Developers might have copied a line, change the digit, but then forgotten about it in the last line. It's hardly a redundant line, since there's the data.Uk6 property.

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

The PVS-Studio warning: V3095 The 'pgc.BufferTableSectors' object was used before it was verified against null. Check lines: 43, 56. GeometryCacheService.cs 43

The analyzer has detected that developers used the pgc.BufferTableSectors property before checking it for 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))
    {
      ....
    }
}

The PVS-Studio warning: V3008 The 'path' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 82, 81. SoundModdingViewModel.cs 82

It's probably not an error but a suspicious code fragment. The path variable is assigned a value twice. Developers might have left one of these values after debugging, or it might just be the development artifact.

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

The PVS-Studio warning: V3130 Priority of the '&&' operator is higher than that of the '||' operator. Possible missing parentheses. ChunkViewModel.cs 3053

There's another suspicious code fragment. It's tough to say for sure if it's a bug or not, but it's worth taking a peek at this code. The analyzer warns about a possible issue related to the operator precedence. As we all know, the '&&' operator takes precedence over '||'. It means that the ira.Count < ira.MaxSize expression is evaluated only with arrayType == typeof(CStatic<>), not with the entire OR expression. If developers wanted it, it'd be a good idea to put the AND expression in parentheses for readability.

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

The PVS-Studio warning: V3063 A part of conditional expression is always false if it is evaluated: indexx == -1. ChunkViewModel.cs 4201

The analyzer has detected that the indexx == -1 expression is always false. Why does the analyzer warn about this? Is it correct? Let's dive in. The indexx variable gets its value from the Parent.GetIndexOf(this) + 1 expression. Look at the GetIndexOf method.

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

As we can see, the minimum return value from the method is -1. In the expression, developers added 1 to the return method value. So, the analyzer is right. Developers either probably didn't need to add 1, or they should've tweaked the condition.

The analyzer traverses the code and tracks possible values for variables and constraints. This mechanism is called Data Flow. This is how the analyzer spots these kinds of errors.

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

The PVS-Studio warning: V3095 The 'appearance' object was used before it was verified against null. Check lines: 547, 548. NodeProperties.cs 547

Here's a simple error: three lines use the appearance variable. In the last line, developers used appearance with the '?.' operator. So, the variable may be 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;
  }
}

The PVS-Studio warning: V3022 Expression 'options.meshExportType != null && string.IsNullOrEmpty(options.meshExportMaterialRepo) && options.outpath is null' is always false. UncookTask.cs 44

The analyzer warns that the large expression is always false. And that's really true. All the subexpressions are connected via '&&', and the last one has already been in the previous if statement, followed by the method exit.

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

The PVS-Studio warning: V3001 There are identical sub-expressions 'n.Contains(gameName)' to the left and to the right of the '||' operator. Oodle.cs 488

The analyzer has detected the subexpressions that are identical to the ones on either side of the '||' operator. Developers may need to use the l variable in the second subexpression.

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

The PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'Archive'. FileEntry.cs 87

Essentially, Archive is not { } is the same as Archive is not object or Archive is null. If the check is true, developers get a NullReferenceExceprion when an error exception is thrown.

Issue 12

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

  PostConstruct();
}

The PVS-Studio warning: V3091 Empirical analysis. It is possible that a typo is present inside the string literal: "Hover". The 'Hover' word is suspicious. ButtonCursorStateView.cs 34

Let's bring in some heuristic analysis. It seems that the analyzer has detected the failed copy-paste, and developers should assign* "Press"* to PressStateName.

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

The PVS-Studio warning: V3134 Shift by 63 bits is greater than the size of 'Int32' type of expression '(count + 0x3ffff)'. Oodle.cs 397

And here's the last suspicious fragment. Developers wanted to shift to 0x3f, which is equal to 63. However, it exceeds the size of the expression type to which the shift operation is applied. This expression is of the Int32 type, which contains 32 bits. Since shifts are circular in C#, the shift of 63 is equal to that of 31.

Outro

We've covered all the bugs and suspicious code fragments in the WolvenKit project! While I haven't listed every single bug in this article due to some being repetitive, I'll be reporting all of them directly to the developers via the Issue on GitHub.

Stay tuned for updates and happy developing!