To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (an extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Nintendo Switch: drop test of the Ryuji…

Nintendo Switch: drop test of the Ryujinx emulator's source code

Jun 30 2021
Author:

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.

0840_Ryujinx/image1.png

Introduction

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.

Potential null reference exception

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:

  • V3095 The '_profileEntry.Text' object was used before it was verified against null. Check lines: 34, 40. ProfileDialog.cs 34
  • V3095 The 'owner' object was used before it was verified against null. Check lines: 161, 166. KThread.cs 161
  • V3095 The 'owner' object was used before it was verified against null. Check lines: 1084, 1103. KThread.cs 1084
  • V3095 The '_nsdSettings' object was used before it was verified against null. Check lines: 44, 48. FqdnResolver.cs 44
  • V3095 The 'texture' object was used before it was verified against null. Check lines: 354, 362. TextureBindingsManager.cs 354

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:

  • V3080 Possible null dereference. Consider inspecting 'region'. KMemoryManager.cs 46
  • V3080 Possible null dereference. Consider inspecting 'node'. KPageTableBase.cs 2250
  • V3080 Possible null dereference. Consider inspecting 'node'. KPageTableBase.cs 2316
  • V3080 Possible null dereference. Consider inspecting 'node'. KPageTableBase.cs 2408
  • V3080 Possible null dereference. Consider inspecting 'dimension'. Demangler.cs 361

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.

Logic errors

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:

  • V3022 Expression 'result != KernelResult.Success' is always false. KProcess.cs 639
  • V3022 Expression 'TerminationRequested' is always false. KThread.cs 375
  • V3022 Expression 'resultCode == ResultCode.Success' is always true. IManagerForApplication.cs 32
  • V3022 Expression 'resultCode == ResultCode.Success' is always true. IManagerForSystemService.cs 32
  • V3022 Expression 'result != ResultCode.Success' is always false. IApplicationFunctions.cs 376

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:

  • V3009 It's odd that this method always returns one and the same value of 'ResultCode.Success'. IAddOnContentManager.cs 52
  • V3009 It's odd that this method always returns one and the same value of 'ResultCode.Success'. ISystemSettingsServer.cs 30
  • V3009 It's odd that this method always returns one and the same value of 'Status.Success'. ConsumerBase.cs 131
  • V3009 It's odd that this method always returns one and the same value of 'ResultCode.Success'. IBinder.cs 14
  • V3009 It's odd that this method always returns one and the same value of 'true'. AstcDecoder.cs 307

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.

Redundant code

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:

  • V3063 A part of conditional expression is always false if it is evaluated: backendDisconnected. SoundIoHardwareDeviceDriver.cs 68
  • V3063 A part of conditional expression is always true if it is evaluated: info != null. SynchronizationManager.cs 132
  • V3063 A part of conditional expression is always false if it is evaluated: flush. TextureManager.cs 942

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:

  • V3022 Expression 'result == null' is always false. Demangler.cs 2848
  • V3022 Expression 'result == null' is always false. Demangler.cs 2833
  • V3022 Expression 'result == null' is always false. Demangler.cs 3094
  • V3022 Expression 'result == null' is always false. Demangler.cs 2930

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:

  • V3117 Constructor parameter 'context' is not used. IAccountServiceForApplication.cs 13
  • V3117 Constructor parameter 'context' is not used. IAccountServiceForSystemService.cs 11
  • V3117 Constructor parameter 'context' is not used. IDeliveryCacheStorageService.cs 12
  • V3117 Constructor parameter 'memory' is not used. NvHostAsGpuDeviceFile.cs 17
  • V3117 Constructor parameter 'condition' is not used. IfBlock.cs 17

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)

Copy-paste

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.

Conclusion

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.

Popular related articles
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept