# 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\. ![0651_Yuzu/image1.png](https://import.viva64.com/docx/blog/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](https://import.viva64.com/docx/blog/0651_Yuzu/image2.png) [V595](https://pvs-studio.com/en/docs/warnings/v595/) \[[CWE\-476](https://cwe.mitre.org/data/definitions/476.html)\] The 'policy' pointer was utilized before it was verified against nullptr\. Check lines: 114, 117\. pcy\_data\.c 114 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v595/) \[[CWE\-476](https://cwe.mitre.org/data/definitions/476.html)\] The 'pkey\-\>ameth' pointer was utilized before it was verified against nullptr\. Check lines: 161, 180\. a\_sign\.c 161 * [V595](https://pvs-studio.com/en/docs/warnings/v595/) \[[CWE\-476](https://cwe.mitre.org/data/definitions/476.html)\] The 'curr\-\>prev' pointer was utilized before it was verified against nullptr\. Check lines: 1026, 1032\. ssl\_ciph\.c 1026 * [V595](https://pvs-studio.com/en/docs/warnings/v595/) \[[CWE\-476](https://cwe.mitre.org/data/definitions/476.html)\] The 's' pointer was utilized before it was verifiedagainst nullptr\.Check lines: 1010, 1015\. ssl\_lib\.c 1010 ## Suspicious Condition [V564](https://pvs-studio.com/en/docs/warnings/v564/) \[[CWE\-480](https://cwe.mitre.org/data/definitions/480.html)\] The '\|' operator is applied to bool type value\. You've probably forgotten to include parentheses or intended to use the '\|\|' operator\. xbyak\.h 1632 ```cpp 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-570](https://cwe.mitre.org/data/definitions/570.html)\] Expression 'module \>\= 2000' is always false\. error\.cpp 80 ```cpp 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](https://import.viva64.com/docx/blog/0651_Yuzu/image3.png) ## Another Impossible Condition [V547](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-570](https://cwe.mitre.org/data/definitions/570.html)\] Expression 'side \!\= MBEDTLS\_ECDH\_OURS' is always false\. ecdh\.c 192 ```cpp 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_\. ```cpp 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: ```cpp .... if( side == MBEDTLS_ECDH_OURS ) .... ``` ## Copy\-Pasted For Operator The analyzer issued warnings for each of these _for_ operators\. [V621](https://pvs-studio.com/en/docs/warnings/v621/) \[[CWE\-835](https://cwe.mitre.org/data/definitions/835.html)\] 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 ```cpp 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](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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\. ```cpp 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](https://pvs-studio.com/en/blog/terms/0012/) 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v501/) \[[CWE\-570](https://cwe.mitre.org/data/definitions/570.html)\] There are identical sub\-expressions '\(c \=\= ' '\)' to the left and to the right of the '\|\|' operator\. a\_print\.c 77 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-571](https://cwe.mitre.org/data/definitions/571.html)\] Expression 'i \=\= 0' is always true\. bf\_buff\.c 187 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-571](https://cwe.mitre.org/data/definitions/571.html)\] Expression _'ptr_ \!\= NULL' is always true\. bss\_acpt\.c 356 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-571](https://cwe.mitre.org/data/definitions/571.html)\] Expression '\(ca\_ret \= check\_ca\(x\)\) \!\= 2' is always true\. v3\_purp\.c 756 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v1001/) \[[CWE\-563](https://cwe.mitre.org/data/definitions/563.html)\] The 'current\_value' variable is assigned but is not used by the end of the function\. gl\_state\.cpp 30 ```cpp template 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](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-570](https://cwe.mitre.org/data/definitions/570.html)\] Expression 'ok \=\= 0' is always false\. gostr341001\.c 133 * [V547](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-571](https://cwe.mitre.org/data/definitions/571.html)\] Expression 'ps \>\= 1' is always true\. ui\_openssl\_win\.c 286 * [V547](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-570](https://cwe.mitre.org/data/definitions/570.html)\] Expression 'w \> 6' is always false\. ecp\.c 1395 * [V547](https://pvs-studio.com/en/docs/warnings/v547/) \[[CWE\-571](https://cwe.mitre.org/data/definitions/571.html)\] 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\.