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




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.

Рисунок 6

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

Рисунок 2

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.

Рисунок 4

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.



Use PVS-Studio to search for bugs in C, C++, C# and Java

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;


Bugs Found

Checked Projects
361
Collected Errors
13 428