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

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
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.

>
>
>
Analysis of the Yuzu Source Code Using …

Analysis of the Yuzu Source Code Using the PVS-Studio Static Code analyzer

Aug. 5, 2019

I'm Vladislav, at the moment I am doing an internship at PVS-Studio. As you know, the best way to get to know the product is to try it, and in my case to also flesh out an article from the obtained observations. I've always been interested in emulators of gaming platforms, the need for which is more and more felt with the release of new game consoles. Yuzu is the first Nintendo Switch emulator. With this project, we can make sure that PVS-Studio not only helps you find bugs in the code, but also makes it much readable and friendlier, and, with constant use, will help to avoid error occurrence in the code.

0651_Yuzu/image1.png

About the Project

Yuzu is an open source emulator that is distributed under the GPLv2 license for Windows and Linux (macOS build is no longer supported). The project was started in spring of the year 2017, when one of the Citra (which is an emulator of the portable game Nintendo 3DS console) authors, under the bunnei nickname began to explore Nintendo Switch. Due to likeness between Switch and 3ds, Yuzu is very similar to Citra. In January 2018, the Yuzu team was formed from several Citra developers, and it was decided to make the project open source. The emulator is written in C and C++, the graphical interface is implemented with the help of Qt5.

The size of the project is about 100,000 lines of code. To find bugs, I used PVS-Studio, the static code analyzer for programs written C, C++, C# and Java. Let's take a look at the interesting code errors I found during the review of this project in order to get to know PVS-Studio.

Dereference of a Potentially Null Pointer

0651_Yuzu/image2.png

V595 [CWE-476] The 'policy' pointer was utilized before it was verified against nullptr. Check lines: 114, 117. pcy_data.c 114

policy_data_new(POLICYINFO *policy, ....)
{
  ....
  if (id != NULL)
  {
    ret->valid_policy = id;
  }
  else 
  {
    ret->valid_policy = policy->policyid; // <=

    ....
  }

  if (policy != NULL) 
  {
    ....
  }
  ....
}

The pointer policy is first dereferenced, and then checked for NULL. This can mean one of two obvious things: undefined behavior will take place if the pointer is null, or the pointer can't be null and the program will always work correctly. If the first option is implied, the check should be made before dereferencing, whereas in the second option you can omit the redundant check. There is another not-so-obvious scenario: perhaps, policy can't be null pointer, if the id pointer is null. However, such interconnected code can confuse not only the analyzer, but also programmers. So you definitely shouldn't write like this.

Similar warnings:

  • V595 [CWE-476] The 'pkey->ameth' pointer was utilized before it was verified against nullptr. Check lines: 161, 180. a_sign.c 161
  • V595 [CWE-476] The 'curr->prev' pointer was utilized before it was verified against nullptr. Check lines: 1026, 1032. ssl_ciph.c 1026
  • V595 [CWE-476] The 's' pointer was utilized before it was verifiedagainst nullptr.Check lines: 1010, 1015. ssl_lib.c 1010

Suspicious Condition

V564 [CWE-480] The '|' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '||' operator. xbyak.h 1632

bool isExtIdx2();
....
int evex(..., bool Hi16Vidx = false)
{
  ....
  bool Vp = !((v ? v->isExtIdx2() : 0) | Hi16Vidx);
  ....
}

The isExtIdx2() function returns the value of the bool type, the Hi16Vidx variable is also of the bool type. The expression looks very suspicious, as if bitwise magic took place here, and then it magically turned into boolean logic. Most likely, the code that the author wanted to write looks as follows:

bool Vp = !((v ? v->isExtIdx2() : 0) || Hi16Vidx);

Actually, there's no error here. This code will work the same both with the |, and || operators. Nevertheless, such code made me think deeper and refactor it.

Impossible Condition

V547 [CWE-570] Expression 'module >= 2000' is always false. error.cpp 80

ResultCode Decode64BitError(u64 error)
{
  const auto description = (error >> 32) & 0x1FFF;
  auto module = error & 0x3FF;
  if (module >= 2000)
  {
    module -= 2000;
  }
  ....
 }

The constant 0x3FF = 1023. Let's look at the next line, we won't enter this condition. The value module cannot exceed 2000. Perhaps, the value of the constant changed during the development process.

0651_Yuzu/image3.png

Another Impossible Condition

V547 [CWE-570] Expression 'side != MBEDTLS_ECDH_OURS' is always false. ecdh.c 192

int mbedtls_ecdh_get_params(.... , mbedtls_ecdh_side side )
{
  ....

  if( side == MBEDTLS_ECDH_THEIRS )
    return( mbedtls_ecp_copy( &ctx->Qp, &key->Q ) );

  if( side != MBEDTLS_ECDH_OURS )
  {
    ....
  }
  ....
}

The function handles keys, whose values are stored in mbedtls_ecdh_side.

typedef enum
{
    MBEDTLS_ECDH_OURS,   
    MBEDTLS_ECDH_THEIRS, 
} mbedtls_ecdh_side;

As we can see, we'll never be able to handle the value, equal to MBEDTLS_ECDH_OURS as it's checked for inequality, whereas there are only two values and we haven't got to the first if, so it'll never be true. Most likely, it would be right to add else to the first if. Or to check for equality:

....
if( side == MBEDTLS_ECDH_OURS )
  ....

Copy-Pasted For Operator

The analyzer issued warnings for each of these for operators.

V621 [CWE-835] Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. curve25519.c 646

static void fe_invert(....)
{
  ....
  for (i = 1; i < 1; ++i) 
  {
    fe_sq(t0, t0);
  }
  ....
  for (i = 1; i < 1; ++i) 
  {
    fe_sq(t0, t0);
  }
  ....

  for (i = 1; i < 1; ++i) 
  {
    fe_sq(t0, t0);
  }
  ....
}

Most likely, it is a humdrum copy-paste and the loops had to execute at least one iteration.

Data Alignment

V802 On 64-bit platform, structure size can be reduced from 32 to 24 bytes by rearranging the fields according to their sizes in decreasing order. engine.h 256

struct option_w
{
    const wchar_t* name;
    int has_arg;
    int *flag;
    int val;
};

In this case, we can reduce the structure size by 8 bytes by rearranging fields in the descending order on a 64-bit platform (e.g.'WIN64, MSVC'), where the pointer size is 8 bytes. As the pointer size is 8 bytes, the size of the int variable is 4, the structure with the fields in this sequence will take 24 bytes, not 32.

struct option_w
{
  const wchar_t* name;
  int *flag;
  int val;
  int has_arg;

};

I'd like to give a general recommendation: arrange data fields in structures in the descending order of their size, as with some data models in systems, where the application will be used, such order can give significant acceleration of working with memory.

There were other 286 such warnings, here are some of them:

  • V802 On 64-bit platform, structure size can be reduced from 56 to 48 bytes by rearranging the fields according to their sizes in decreasing order. vulkan_core.h 2255
  • V802 On 64-bit platform, structure size can be reduced from 64 to 56 bytes by rearranging the fields according to their sizes in decreasing order. vulkan_core.h 2428
  • V802 On 64-bit platform, structure size can be reduced from 24 to 16 bytes by rearranging the fields according to their sizes in decreasing order. vulkan.hpp 35306

We fight not only with errors, but also with redundant code

This project contains quite a lot of redundant code, which, in my opinion, relates to the fact that developers were inattentive when they were changing its operational logic and made typos.

Example 1.

V501 [CWE-570] There are identical sub-expressions '(c == ' ')' to the left and to the right of the '||' operator. a_print.c 77

ASN1_PRINTABLE_type(const unsigned char *s, int len)
{
  ....
  if (!(((c >= 'a') && (c <= 'z')) ||
  ((c >= 'A') && (c <= 'Z')) ||
  (c == ' ') ||
  ((c >= '0') && (c <= '9')) ||
  (c == ' ') || (c == '\'') ||
   ....
  (c == '=') || (c == '?')))
  {
    ....
  }
  ....
}

PVS-Studio noticed unnecessary (c == ' '), which is repeated one line after.

Example 2.

V547 [CWE-571] Expression 'i == 0' is always true. bf_buff.c 187

buffer_write(BIO *b, const char *in, int inl)
{
  ....  

  for (;;) 
  {
    i = BIO_read(b->next_bio, out, outl);
    if (i <= 0) 
    {
      BIO_copy_next_retry(b);
      if (i < 0)
      {
        return ((num > 0) ? num : i);
      }
      if (i == 0)
      {
        return (num);
      }
    }
  ....
}

In this code fragment, there is a redundant check i==0. If we got to this code block, the check i<=0 has already been made and resulted in true, the same as the i<0 check, resulting in false, which means 0 can be the only value of i.

Example 3.

V547 [CWE-571] Expression 'ptr != NULL' is always true. bss_acpt.c 356

acpt_ctrl(....)
{
  {
  if (ptr != NULL) 
  {
    if (num == 0) 
    {
      b->init = 1;
      free(data->param_addr);
      data->param_addr = strdup(ptr);
     }
     else if (num == 1) 
     {
     data->accept_nbio = (ptr != NULL);
    ....
  }
}

Here comes the contrariety. Many cases lack the ptr != NULL check on order to avoid undefined behavior due to the null pointer dereference, on the contrary, in this case the check was redundant.

Example 4.

V547 [CWE-571] Expression '(ca_ret = check_ca(x)) != 2' is always true. v3_purp.c 756

int ca_ret;
if ((ca_ret = check_ca(x)) != 2)
{
....
}
check_ca(const X509 *x)
{
  if (ku_reject(x, KU_KEY_CERT_SIGN))
  {
    return 0;
  }
  if (x->ex_flags & EXFLAG_BCONS) 
  {
    ....
  }
  else if (....) 
  {
    return 5;
  }
  return 0;
  }
}

The check_ca function never returns 2. As a result, we have a large code fragment, which will never be executed. Perhaps, the developer has removed the block of code with this return from check_ca but forgot to remove it from another part of the program.

Example 5.

V1001 [CWE-563] The 'current_value' variable is assigned but is not used by the end of the function. gl_state.cpp 30

template <typename T1, typename T2>
bool UpdateTie(T1 current_value, const T2 new_value) 
{
  const bool changed = current_value != new_value;
  current_value = new_value;
  return changed;
}

In this fragment the analyzer indicates that the copy of the current_value variable, which we handle in the UpdateTie function isn't used after assigning the new_value value to it. Accordingly, we can safely remove this line of code.

In total, 19 more warnings of this kind were found in the project, here are PVS-Studio warnings about some of them:

  • V547 [CWE-570] Expression 'ok == 0' is always false. gostr341001.c 133
  • V547 [CWE-571] Expression 'ps >= 1' is always true. ui_openssl_win.c 286
  • V547 [CWE-570] Expression 'w > 6' is always false. ecp.c 1395
  • V547 [CWE-571] Expression 'ssl->minor_ver == 3' is always true. ssl_cli.c 3195

Conclusion

On the one hand, as the open source project, it contains a small number or errors, especially since a small team of developers is working on it. On the other hand, the emulator is a small brother of Citra, which can run almost all custom and many commercial 3ds games and, by the way, contains ready-made fragments from there. I'm sure in future, users will get much functional and less bugs.

This emulator is currently under active work and there is a community of moderators who can be contacted through the site.

Popular related articles
Free PVS-Studio for those who develops open source projects

Date: 12.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…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: 11.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…
Appreciate Static Code Analysis!

Date: 10.16.2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
The way static analyzers fight against false positives, and why they do it

Date: 03.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…
The Ultimate Question of Programming, Refactoring, and Everything

Date: 04.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…
PVS-Studio for Java

Date: 01.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…
Static analysis as part of the development process in Unreal Engine

Date: 06.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 ROI

Date: 01.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…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: 07.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 Last Line Effect

Date: 05.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…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
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