Ryujinx: re-checking the Nintendo Switch emulator using PVS-Studio

• About the project
• Error analysis
  • Inattention, typos, copy-paste
  • NullReferenceException and more
  • null from the past
• Conclusion

Nintendo Switch's popularity hasn't waned, and with exclusive games winning awards, the desire to play them is only growing. However, not everyone has the opportunity to try out this portable gaming console. Ryujinx — a Nintendo Switch emulator — solves this problem. Today we are going to check its code using the PVS-Studio analyzer.

About the project

Ryujinx is a Nintendo Switch emulator. According to the developers, it aims to provide excellent accuracy, performance, and a user-friendly interface. The project is written in C#. It's being actively developed and available in the GitHub repository.

Among the Nintendo Switch emulators, Ryujinx also has competitors — for example, the Yuzu project written in C++. It was reviewed in one of our articles on project checking.

In the PVS-Studio blog, there was also an article in which we checked the Ryujinx emulator. Although, a lot of time has passed since then: many new features have appeared and brought new errors. Let's talk about them today.

Error analysis

Inattention, typos, copy-paste

Such mistakes are easy to miss. If you are lucky, they may not affect the program performance, but otherwise they may change its logic.

Issue 1

So, let's start with an attentiveness test.

I'm going to show you a large code fragment. Can you find a catch there?

public static PrimitiveType Convert(this PrimitiveTopology topology)
{
  switch (topology)
  {
    case PrimitiveTopology.Points:
      return PrimitiveType.Points;
    case PrimitiveTopology.Lines:
       return PrimitiveType.Lines;
    case PrimitiveTopology.LineLoop:
       return PrimitiveType.LineLoop;
    case PrimitiveTopology.LineStrip:
       return PrimitiveType.LineStrip;
    case PrimitiveTopology.Triangles:
       return PrimitiveType.Triangles;
    case PrimitiveTopology.TriangleStrip:
       return PrimitiveType.TriangleStrip;
    case PrimitiveTopology.TriangleFan:
       return PrimitiveType.TriangleFan;
    case PrimitiveTopology.Quads:
       return PrimitiveType.Quads;
    case PrimitiveTopology.QuadStrip:
       return PrimitiveType.QuadStrip;
    case PrimitiveTopology.Polygon:
       return PrimitiveType.TriangleFan;
    case PrimitiveTopology.LinesAdjacency:
       return PrimitiveType.LinesAdjacency;
    case PrimitiveTopology.LineStripAdjacency:
       return PrimitiveType.LineStripAdjacency;
    case PrimitiveTopology.TrianglesAdjacency:
       return PrimitiveType.TrianglesAdjacency;
    case PrimitiveTopology.TriangleStripAdjacency:
       return PrimitiveType.TriangleStripAdjacency;
    case PrimitiveTopology.Patches:
       return PrimitiveType.Patches;
  }
  ....
}

The error is not so difficult to notice if you examine this code fragment closely. However, would you like to do it while working and not knowing that there is an error?

If this made your eyes dazzled, I'll give you a hint:

switch (topology)
{
  ....
  case PrimitiveTopology.TriangleFan:
    return PrimitiveType.TriangleFan;
  ....
  case PrimitiveTopology.Polygon:
    return PrimitiveType.TriangleFan;
  ....
}

This is a classic copy-paste error. The second case section should return the PrimitiveType.Polygon value. Let's make sure that it's in the PrimitiveType enumeration:

public enum PrimitiveType
{
  ....
  TriangleFan = 6,
  ....
  Polygon = 9,
  ....
}

Copy-paste is convenient and fast, but as the fragment gets bigger and bigger, the possibility of making mistakes also grows.

Issue 2

Let's take a look at a suspicious piece of code:

private void YesButton_Clicked(object sender, EventArgs args)
{
  ....
  Window.Functions = _mainWindow.Window.Functions =
    WMFunction.All & WMFunction.Close;
  ....
}

The PVS-Studio warning: V3182 The result of 'WMFunction.All & WMFunction.Close' expression is '0'. It is possible that the '|' operator should be used instead. UpdateDialog.cs 69

As we can see from the warning, the analyzer complains about the '&' operator. To understand why, let's take a look at the WMFunction enumeration:

[Flags]
public enum WMFunction
{
  All = 0x1,
  Resize = 0x2,
  Move = 0x4,
  Minimize = 0x8,
  Maximize = 0x10,
  Close = 0x20
}

We are dealing with bit flags, but in this case the implementation of combining them doesn't work: the result of the bitwise AND (&) operation for the WMFunction.All and WMFunction.Close values will be equal to zero.

The analyzer immediately gives us a hint on how to solve this problem. The correct implementation of combining flags may look as follows:

Window.Functions = _mainWindow.Window.Functions =
  WMFunction.All | WMFunction.Close;

Issue 3

Another interesting case:

private BaseNode ParseSpecialName(....)
{
  switch (....)
  {
    case 'C':
      _position += 2;
      BaseNode firstType = ParseType();
      if (   firstType == null 
          || ParseNumber(true).Length == 0 
          || !ConsumeIf("_"))
      {
        return null;
      }
      BaseNode secondType = ParseType();
      return new CtorVtableSpecialName(secondType, firstType); // <=
  }
}

The PVS-Studio warning: V3066 Possible incorrect order of arguments passed to 'CtorVtableSpecialName' constructor: 'secondType' and 'firstType'. Demangler.cs 803

The order of arguments looks strange, but let's not guess and check the method:

public CtorVtableSpecialName(BaseNode firstType, BaseNode secondType) :
  base(NodeType.CtorVtableSpecialName)
{
  _firstType  = firstType;
  _secondType = secondType;
}

At first glance, the order of parameters when called really seems wrong, but a programmer could have done it on purpose.

However, how to understand whether the developers intended to write like this or made a mistake? Comments could have helped, however there are none.

Note. In addition to the new warnings, there are still unfixed errors from the last check:

• V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Demangler.cs 2043
• V3013 It is odd that the body of 'PrintLeft' function is fully equivalent to the body of 'PrintRight' function (10, line 18). PackedTemplateParameter.cs 10

You can find the analysis of these errors in this article.

NullReferenceException and more

Suspicious interactions with potential null references are a common pattern.

Issue 4

Let's take a look at the dangerous course of action:

public Result Initialize(....)
{
  ....
  if (type == ThreadType.User)
  {
    if (owner.AllocateThreadLocalStorage(out _tlsAddress) 
        != Result.Success)
    {
      return KernelResult.OutOfMemory;
    }

    MemoryHelper.FillWithZeros(owner.CpuMemory, 
                               _tlsAddress,
                               KTlsPageInfo.TlsEntrySize);
  }

  if (owner != null)
  {
    Owner = owner;
    owner.IncrementReferenceCount();
    owner.IncrementThreadCount();
    ....
  }
  else
  {
    is64Bits = true;
  }
}

The PVS-Studio warning: V3095 The 'owner' object was used before it was verified against null. Check lines: 169, 174. KThread.cs 169

We are interested in the owner variable. It is checked for null, but before that, it is used without checking. This code looks suspicious.

The question is: is there an error here, or is there some kind of connection between the owner and type variables, and there will be no exception? It's hard to say from an outside perspective, developers should know better. Anyway, the code is still suspicious.

By the way, the analyzer did not work quite correctly, as it pointed to the owner.CpuMemory access, not owner.AllocateThreadLocalStorage. Here we have something to work on. :)

Issue 5

And now vice versa. Let's take a look at the following code:

private void RenderLoop()
{
  ....
  _renderer?.Window?.SetAntiAliasing(....);
  _renderer?.Window?.SetScalingFilter(....);
  _renderer?.Window?.SetScalingFilterLevel(....);
  ....
  _renderer.Window.SetSize(....);
  ....
}

The PVS-Studio warning: V3125 The '_renderer.Window' object was used after it was verified against null. Check lines: 882, 877. AppHost.cs 882

This time the developer checks _renderer.Window using the '?.' null-conditional operator, but then, the check is omitted.

It's hard to say whether the developer wanted this or just forgot to add the check.

Note. In addition to the new errors, the project still contains unfixed errors from the last check:

• V3095 The 'IManager.NsdSettings' object was used before it was verified against null. Check lines: 37, 41. FqdnResolver.cs 37
• V3125 The 'Owner' object was used after it was verified against null. Check lines: 1308, 1306. KThread.cs 1308
• V3105 The 'result' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Client.cs 214

Issue 6

Let's look at the following method, which the analyzer found suspicious:

public (uint, bool) GetFormat(....)
{
  TextureSpecializationState state = 
    GetTextureSpecState(stageIndex, handle, cbufSlot).Value;
  ....
}

At first glance, the issue is not visible. However, the devil, as they say, is in the details. Note the warning:

V3080 Possible null dereference of method return value. Consider inspecting: GetTextureSpecState(...). ShaderSpecializationState.cs 408

Let's follow the path of the analyzer user and look at the method from the inside:

private Box<....> GetTextureSpecState(....)
{
  TextureKey key = new TextureKey(stageIndex, handle, cbufSlot);
  if (_textureSpecialization.TryGetValue(key,
        out Box<TextureSpecializationState> state))
  {
    return state;
  }
  return null;
}

Since there is a null return possibility, it's a good idea to add a check.

It's worth noting that such code occurs several times:

• V3080 Possible null dereference of method return value. Consider inspecting: GetTextureSpecState(...). ShaderSpecializationState.cs 408
• V3080 Possible null dereference of method return value. Consider inspecting: GetTextureSpecState(...). ShaderSpecializationState.cs 420
• V3080 Possible null dereference of method return value. Consider inspecting: GetTextureSpecState(...). ShaderSpecializationState.cs 431

If you want to learn more about NullReferenceException, I recommend you reading this article. For example, you can check if you remember all the listed ways of encountering NRE.

null from the past

Unfixed errors of the past caused new ones.

Issue 7

The following warning was described in the previous article about this project check:

public void LoadApplication(string path, bool isFirmwareTitle)
{
  ....
  firmwareVersion = _contentManager.GetCurrentFirmwareVersion();
  string message = $"{firmwareVersion.VersionString}.....";
  GtkDialog.CreateInfoDialog($"{firmwareVersion.VersionString} .... ");
  ....
}

The PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'firmwareVersion'. MainWindow.cs 734

The issue is that the FirmwareVersion variable can have the null value, but there is no check before it's used. Let's take a look at the GetCurrentFirmwareVersion method:

public SystemVersion GetCurrentFirmwareVersion()
{
  LoadEntries();
  lock (_lock)
  {
    ....
    if (....)
    {
      return new SystemVersion(systemVersionFile.AsStream());
    }
    ....
  }
  return null; // <=
}

There is a possibility of returning null, which may cause an error.

In addition to the previously mentioned warning, a new one has emerged:

public async Task<bool> LoadGuestApplication(){
  ....
  firmwareVersion = ContentManager.GetCurrentFirmwareVersion();


  await ContentDialogHelper.CreateInfoDialog(
    LocaleManager.Instance.UpdateAndGetDynamicValue(
      ....,  
      firmwareVersion.VersionString),                  // <=
    LocaleManager.Instance.UpdateAndGetDynamicValue(
      ...., 
      firmwareVersion.VersionString),                  // <=
    ....);               
  ....
}

The PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'firmwareVersion'. AppHost.cs 541

It's worth adding that now there is a null check for firmwareVersion in the code below:

public async Task<bool> LoadGuestApplication()
{
  ....
  firmwareVersion = ContentManager.GetCurrentFirmwareVersion();
  ....
  Logger.Notice.Print(
    LogClass.Application, 
    $"Using Firmware Version: {firmwareVersion?.VersionString}");
}

Conclusion

We hope this article will help the developers improve their project.

It's worth mentioning that about two years have passed since the last check, but some errors have not been fixed. We recommend the Ryujinx developers to pay attention to them and read the previous article.

If you are interested in articles about checking game projects, I suggest you reading our blog.

And if you'd like to check your code using PVS-Studio, you can download and try it here.