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.
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.
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:
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.
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.
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 )
....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.
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:
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:
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.