Each generation, companies like Sony, Microsoft and Nintendo delight their consumers with new consoles and different games for them. Yet there is a caveat - some games exclusively run on their platforms. Whereas a console may be worth an expensive PC component or a full-fledged computer. So what can you do? Emulators come to the rescue here. The market is full of similar projects, some are released as open source. Let us turn our attention to Nintendo Switch emulators. On the network, Ryujinx and Yuzu projects are among most popular responses. Let's check the code of the Ryujinx emulator and find out how many interesting bugs can be found using static analysis.
Ryujinx (Ryujinx's name is based on the name "Ryujin" - the name of the Mythic (Sea God) Dragon) is a young open-source Nintendo Switch emulator project, written in C#. This emulator aims to provide superior accuracy and performance, a convenient interface.
Ryujinx project competes with its older brother Yuzu, written in C++, whose code was already covered in one of our articles. Each of these projects has its positive and negative sides. But let's leave the old man alone, and take a look at the young project with our static PVS-Studio code analyzer. The source code of the "Dragon" was taken from its official repository on GitHub.
Let's start reviewing Ryujinx project's errors with flaws that may cause NullReferenceException.
Developers often use variables whose value can be null without checking for this very null. Or something may happen, as in the case below.
V3095 The 'firmwareVersion' object was used before it was verified against null. Check lines: 1157, 1159. MainWindow.cs
private void HandleInstallerDialog(FileChooserDialog fileChooser){
....
string dialogTitle = $"Install Firmware {firmwareVersion.VersionString}";
if (firmwareVersion == null)
{
....
}
....
}firmwareVersion here is used before its check for null - this may result in a V3095 error. This message has been issued multiple times:
V3080 Possible null dereference. Consider inspecting 'firmwareVersion'. MainWindow.cs 605
public void LoadApplication(string path)
{
....
firmwareVersion = _contentManager.GetCurrentFirmwareVersion();
RefreshFirmwareLabel();
string message =
$"No installed firmware was found but Ryujinx was able to install firmware
{firmwareVersion.VersionString} from the provided game.
\nThe emulator will now start.";
....
}Here, the FirmWareVersion variable is used without a check. The GetCurrentFirmwareVersion method shows that we'll get null instead of a reference to an object. This can lead to an error as well.
public SystemVersion GetCurrentFirmwareVersion()
{
LoadEntries();
lock (_lock)
{
....
if (romfs.OpenFile(out IFile systemVersionFile,
"/file".ToU8Span(),
OpenMode.Read).IsSuccess())
{
return new SystemVersion(systemVersionFile.AsStream());
}
....
}
return null;
}Errors of this type are quite common for this project:
V3125 The 'Owner' object was used after it was verified against null. Check lines: 1084, 1082. KThread.cs 1084
private void FreeResources()
{
Owner?.RemoveThread(this);
if (_tlsAddress != 0 &&
Owner.FreeThreadLocalStorage(_tlsAddress) != KernelResult.Success)
{
....
}
....
}This time we see that we have a single check for null. Although the variable is used twice here. When we first encounter Owner, its method is invoked only when the variable is not null. In the second case, this nuance is forgotten. If Owner is null in the first case, the method won't be called. As for the second case, whoever tries to call the method will get NullReferenceException.
V3105 The 'result' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Client.cs 213
private byte[] Receive(int clientId, int timeout = 0)
{
....
var result = _client?.Receive(ref endPoint);
if (result.Length > 0)
{
....
}
....
}From this code fragment we see how a null-conditional operator is used to assign the result to the result variable. This variable isn't checked for null below. We may get an error in the line with the condition, as we can't estimate the null length.
V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'data' object Client.cs 254
public void ReceiveLoop(int clientId)
{
....
byte[] data = Receive(clientId);
if (data.Length == 0)
{
continue;
}
....
}Here the author assigns the function result to data. Let's look inside it and find out what it can return:
private byte[] Receive(int clientId, int timeout = 0)
{
....
var result = _client?.Receive(ref endPoint);
if (result.Length > 0)
{
....
}
return result;
....
}It seems like we've seen this code before, haven't we? The error I described above led to another one.
V3022 Expression 'result != KernelResult.Success' is always false. KMemoryRegionManager.cs 169
private KernelResult AllocatePagesImpl(....)
{
....
KernelResult result = pageList.AddRange(address, blockPagesCount);
if (result != KernelResult.Success)
....
}So, the first logic error tells us that the condition is always false. Why? Let's look inside the AddRange method.
public KernelResult AddRange(....)
{
....
return KernelResult.Success;
}We'll omit the method algorithm and focus on the result. return is called only once. So, there is only one possible value of the result variable. Either the method is not finished, or there has been a redundant check of the method result. We encountered the V3022 error many times in the project:
V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 26, 30. ZbcSetTableArguments.cs 26
public uint this[int index]
{
get
{
if (index == 0)
{
return element0;
}
else if (index == 1)
{
return element1;
}
else if (index == 2)
{
return element2;
}
else if (index == 2)
{
return element3;
}
throw new IndexOutOfRangeException();
}
}The error of recurring conditions. Possible reasons: good old copy-paste, or corny inattention. This is likely the second reason here. Typos with numbers 0, 1, 2 are frequent guests in programming. If you are interested in this topic, learn more details in the article.
V3022 Expression 'Base == null' is always false. Demangler.cs 2049
private BaseNode ParseExpression()
{
....
BaseNode Base = ParseExpression();
if (Base == null)
{
return null;
}
BaseNode subscript = ParseExpression();
if (Base == null)
{
return null;
}
....
}So above we see a similar error, Base is double-checked for null. This time, it was most likely the ill-fated copy-paste. Because of this, the same fragment contains the following error: 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
Most likely the second condition should have checked the subscript variable, which in turn would kill two birds with one stone:
BaseNode subscript = ParseExpression();
if (subscript == null)
{
return null;
}V3009 It's odd that this method always returns one and the same value of 'ResultCode.Success'. IApplicationFunctions.cs 116
public ResultCode GetDesiredLanguage(ServiceCtx context)
{
....
if (firstSupported > (int)SystemState.TitleLanguage.Chinese)
{
Logger.Warning?.Print(LogClass.ServiceAm,
"Application has zero supported languages");
context.ResponseData.Write(desiredLanguageCode);
return ResultCode.Success;
}
....
return ResultCode.Success;
}The Ryujinx project revealed several functions working with the ResultCode set of values. We've already seen one of them earlier. However, none of them used all the values, stopping only at Success. The developers may not have finished the job yet, or the functions have caught a bug. This is why a wrong result was used. We've already seen that other code that works with the results of these functions may cause errors or work incorrectly. Similar warnings in the project:
V3064 Potential division by zero. Consider inspecting denominator 'blockWidth'. AstcDecoder.cs 71
public AstcDecoder(
ReadOnlyMemory<byte> inputBuffer,
Memory<byte> outputBuffer,
int blockWidth,
int blockHeight,
int width,
int height,
int depth,
int levels,
int layers)
{
....
if ((uint)blockWidth > 12)
{
throw new ArgumentOutOfRangeException(nameof(blockWidth));
}
if ((uint)blockHeight > 12)
{
throw new ArgumentOutOfRangeException(nameof(blockHeight));
}
....
level.BlockCountX =
(level.ImageSizeX + blockWidth - 1) / blockWidth;
level.BlockCountY =
(level.ImageSizeY + blockHeight - 1) / blockHeight;
....
}This message warns about possible division by zero. The number range falls in range from 0 to 11 due to the check's condition. These variables still can be assigned 0. One has to secure this code fragment from such an error.
V3171 The value used as the size of an array could reach -1. Consider inspecting: deviceCount. AudioDevice.cs 133
public string[] ListAudioDeviceName()
{
int deviceCount = _sessions.Length;
if (!_isUsbDeviceSupported)
{
deviceCount--;
}
string[] result = new string[deviceCount];
....
}Guess where the error hides in this piece. If _sessions.Length is null, deviceCount may equal -1. This will cause an error when creating an array. To avoid this situation, the author should perform a check.
V3063 A part of conditional expression is always true if it is evaluated: value >= 0. NumberFormatter.cs 96
public static string FormatUint(uint value)
{
if (value <= MaxDecimal && value >= 0)
{
return value.ToString(CultureInfo.InvariantCulture) + "u";
}
....
}So, the analyzer tells us that the value >= 0 condition is always true. Here's a simple reason for this. The uint type range starts at 0, ends with 4294967295. That is, uint type variables are always greater or equal to 0. It follows that the value check is simply redundant. A few other similar situations have also been found:
V3139 Two or more case-branches perform the same actions. Demangler.cs 2251
private BaseNode ParseExpression()
{
....
case 'm':
_position += 2;
return ParseBinaryExpression("%");
case 'm':
_position += 2;
return ParseBinaryExpression("%");
....
}Good old switch operator. In this example, it's pretty large. But it's not about the confusion that may start at a certain point. There are two possibilities here. First - these two case branches must perform the same operation; we can merge branches. Second - only authors know about the error hiding here. There are 19 alike cases in the project.
V3022 Expression 'mainNca != null' is always true. ApplicationLoader.cs 272
public void LoadNsp(string nspFile)
{
....
if (mainNca == null)
{
Logger.Error?.Print(LogClass.Loader,
"Unable to load NSP: Could not find Main NCA");
return;
}
if (mainNca != null)
{
_device.Configuration.ContentManager.ClearAocData();
_device.Configuration.ContentManager.AddAocData(nsp,
nspFile,
mainNca.Header.TitleId,
_device.Configuration.FsIntegrityCheckLevel);
LoadNca(mainNca, patchNca, controlNca);
return;
}
....
}The analyzer tells us that the second condition in this place is always true. It's quite obvious—right before that, mainNca was checked for exactly the opposite value. Here comes the question - do we need the second check if the variable doesn't change?
V3022 Expression 'result == null' is always false. Demangler.cs 2906
private BaseNode ParseUnresolvedName(....)
{
....
BaseNode qualifier = ParseSimpleId();
if (qualifier == null)
{
return null;
}
if (result != null)
{
result = new QualifiedName(result, qualifier);
}
else if (isGlobal)
{
result = new GlobalQualifiedName(qualifier);
}
else
{
result = qualifier;
}
if (result == null)
{
return null;
}
....
}result is checked twice for null. However, the second check is always false. Why? The BaseNode class is definitely not null. And anyway, result is assigned objects of classes derived from this class. So it is a variable that was assigned a new class instance and then checked for null. I found other such cases in the project:
V3117 Constructor parameter 'context' is not used. IAccountServiceForAdministrator.cs 12
public IAccountServiceForAdministrator(ServiceCtx context,
AccountServiceFlag serviceFlag)
{
_applicationServiceServer = new ApplicationServiceServer(serviceFlag);
}The analyzer outputs quite a few V3117 warnings. Cases like this are caused by suspicious code. If the argument isn't used, then why pass it? Perhaps these functions are underperformed, or developers have simply reinsured. In the end, the parameters were not needed, but forgotten in code. There are quite a lot of similar code pieces in the project:
V3061 Parameter 'instruction' is always rewritten in method body before being used. EndConditionalBlock.cs 18
public static void Emit(byte[] instruction, CompilationContext context)
{
// 20000000
// Use the conditional begin instruction stored in the stack.
instruction = context.CurrentBlock.BaseInstruction;
....
}Here's a different case. The argument is passed and even used. But it's used with another value—instruction is overwritten right at the beginning. One should either not pass an unnecessary argument or make it optional if it still needs to be passed.
V3030 Recurring check. The 'setFlags' condition was already verified in line 139. InstEmitAluHelper.cs 141
public static void EmitGenericAluStoreA32(....)
{
Debug.Assert(value.Type == OperandType.I32);
if (rd == RegisterAlias.Aarch32Pc && setFlags)
{
if (setFlags)
{
// TODO: Load SPSR etc.
EmitBxWritePc(context, value);
}
else
{
EmitAluWritePc(context, value);
}
....
}
....
}It's controversial point here. From the analyzer's point of view, there's an extra check of the SetFlags variable here. However, from the developers' comments, this piece of code in the condition branches is incomplete. The author can't simply delete a redundant check—code in branches differs. One needs to deal with this code right now. The fragment may remain as it is, so there will be an error with unreachable code. It will be even more difficult to find this bug with the growth of code base.
V3138 String literal contains potential interpolated expression. Consider inspecting: keyHash. CacheCollection.cs 524
public void AddValue(ref Hash128 keyHash, byte[] value)
{
if (IsReadOnly)
{
Logger.Warning?.Print(LogClass.Gpu,
"Trying to add {keyHash} on a read-only cache, ignoring.");
....
}
....
}Here's a small error. Instead of an error message, we'll get the variable name where the error is stored. The developer forgot to specify a dollar sign ($), which enables string formatting.
V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. ShaderConfig.cs 413
private static TextureDescriptor[] GetTextureOrImageDescriptors(....)
{
....
foreach (var kv in dict.OrderBy(x => x.Key.Indexed)
.OrderBy(x => x.Key.Handle))
{
....
}
....
}We need to find out why the analyzer triggers here. To do this, we should look into how sorting works. OrderBy sorts the collection no matter if there were other sorts before it or not. In such a case, the result of dict.OrderBy(x => x.Key.Indexed).OrderBy(x => x.Key.Handle) equals dict.OrderBy(x => x.Key.Handle). To save the sorting attained earlier, the developer must use ThenBy. This will keep the primary sorting:
var kv in dict.OrderBy(x => x.Key.Indexed).ThenBy(x => x.Key.Handle)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
public override void PrintLeft(TextWriter writer)
{
foreach (BaseNode node in Nodes)
{
node.PrintLeft(writer);
}
}
public override void PrintRight(TextWriter writer)
{
foreach (BaseNode node in Nodes)
{
node.PrintLeft(writer);
}
}A prime example of beloved copy-paste. Both functions iterate collections and invoke PrintLeft for their elements. It would be fine if it were the only function of the BaseNodeclass. One simply could delete the redundant function. But BaseNode also has PrintRight. This means that the second function performs the wrong operation.
So, we checked the Ryujinx project with our analyzer. The results revealed many similar errors. While the project is still evolving, we expect developers to fix bugs and delight users with new features. In the meantime, if you are interested in static analysis checks of emulators, be sure to check out the article on Yuzu.