# Анализ исходного кода проекта Yuzu c помощью статического анализатора PVS\-Studio Меня зовут Владислав, и в данный момент я прохожу стажировку в компании PVS\-Studio\. Как известно, лучший способ ознакомится с продуктом \- попробовать его, а заодно оформить свои наблюдения в виде статьи\. Меня всегда очень интересовали эмуляторы игровых платформ, потребность в которых всё больше и больше ощущается с выходом новых игровых консолей\. Yuzu \- это первый Nintendo Switch эмулятор\. На примере данного проекта, мы можем убедиться, что PVS\-Studio помогает не только найти баги в коде, но и сделать его гораздо читабельнее и дружелюбнее, а также, при постоянном использовании, поможет избежать появления ситуаций, в которых могут возникать ошибки\. ![0651_Yuzu_ru/image1.png](https://import.viva64.com/docx/blog/0651_Yuzu_ru/image1.png) ## О проекте Yuzu \- эмулятор с открытым исходным кодом, он распространяется по лицензии GPLv2 для Windows и Linux \(Сборка под macOS перестала поддерживаться\)\. Проект был начат весной 2017 года, когда человек под ником bunnei, один из авторов Citra \- эмулятора портативной игровой консоли Nintendo 3DS, начал исследовать Nintendo Switch\. Из\-за сходства между Switch и 3ds, Yuzu очень похож на Citra\. В январе 2018 года команда Yuzu была сформирована из нескольких разработчиков Citra, и было принято решение выпустить проект публично\. Эмулятор написан на С и C\+\+, графический интерфейс реализован с помощью Qt5\. Размер проекта около 100000 строк кода\. Для нахождения багов я использовал PVS\-Studio \- статический анализатор кода для программ, написанных на языках С, C\+\+, C\# и Java\. Рассмотрим интересные ошибки в коде, найденные мною в процессе проверки этого проекта с целью познакомиться с PVS\-Studio\. ## Разыменование потенциально нулевого указателя ![0651_Yuzu_ru/image2.png](https://import.viva64.com/docx/blog/0651_Yuzu_ru/image2.png) [V595](https://pvs-studio.ru/ru/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) { .... } .... } ``` Указатель policy сначала разыменовывается, а потом проверяется на NULL\. Это может означать одно из двух очевидных вещей: возникнет неопределённое поведение, если указатель действительно нулевой, или указатель никогда не может иметь нулевое значение, и программа всегда работает корректно\. Если подразумевается первый вариант, то проверку стоит произвести до разыменования, во втором варианте можно опустить избыточную проверку\. Есть ещё один не такой очевидный сценарий: возможно, _policy_ не может быть нулевым указателем, если нулевым является указатель _id_\. Однако, такой взаимосвязанный код может сбивать с толку не только анализатор, но и программистов и так явно лучше не писать\. Аналогичные предупреждения: * [V595](https://pvs-studio.ru/ru/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.ru/ru/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.ru/ru/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 ## Подозрительное условие [V564](https://pvs-studio.ru/ru/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); .... } ``` Функция isExtIdx2\(\) возвращает значение типа bool, переменная Hi16Vidx тоже имеет тип bool\. Выражение выглядит очень подозрительно, как будто здесь была побитовая магия, а потом она превратилась волшебным образом в булеву логику\. Скорее всего код, который хотел написать автор: ```cpp bool Vp = !((v ? v->isExtIdx2() : 0) || Hi16Vidx); ``` На самом деле здесь нет ошибки\. Этот код будет работать одинаково как с оператором \|, так и с \|\|\. Однако, такой код заставляет задуматься и провести его рефакторинг\. ## Невозможное условие [V547](https://pvs-studio.ru/ru/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; } .... } ``` Константа 0x3FF \= 1023\. Посмотрим на следующую строчку, в данное условие мы не зайдём\. Значение module не может превысить 2000\. Возможно значение константы менялось в процессе разработки\. ![0651_Yuzu_ru/image3.png](https://import.viva64.com/docx/blog/0651_Yuzu_ru/image3.png) ## Ещё одно невозможное условие [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) \[[CWE\-570](https://cwe.mitre.org/data/definitions/570.html)\] Expression 'side \!\= MBEDTLS\_ECDH\_OURS' is always false\. ecdh\.c 192 int mbedtls\_ecdh\_get\_params\(\.\.\.\. , mbedtls\_ecdh\_side side \) ```cpp { .... if( side == MBEDTLS_ECDH_THEIRS ) return( mbedtls_ecp_copy( &ctx->Qp, &key->Q ) ); if( side != MBEDTLS_ECDH_OURS ) { .... } .... } ``` Функция обрабатывает ключи, значения которых хранятся в mbedtls\_ecdh\_side\. ```cpp typedef enum { MBEDTLS_ECDH_OURS, MBEDTLS_ECDH_THEIRS, } mbedtls_ecdh_side; ``` Как мы видим, мы никогда не сможем обработать значение, равное 'MBEDTLS\_ECDH\_OURS' из\-за того, что проверка осуществляется на отрицание равенства, и, так как значений всего 2, а в первый if мы не зашли, она никогда не даст результат true\. Скорее всего, правильным бы было добавить else к первому if\. Или производить проверку на равенство: ```cpp .... if( side == MBEDTLS_ECDH_OURS ) .... ``` ## Copy\-Paste for На каждый из этих операторов _for_ анализатор выдал предупреждение: [V621](https://pvs-studio.ru/ru/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); } .... } ``` Скорее всего это банальный copy\-paste и циклы должны были выполнять хотя бы одну итерацию\. ## Выравнивание данных [V802](https://pvs-studio.ru/ru/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; }; ``` В данном случае на 64 разрядной платформе \(например, 'WIN64, MSVC'\), где размер указателя равен 8 байтам, мы можем уменьшить размер структуры на 8 байт путём перестановки полей по убыванию\. Так как размер указателя составляет 8 байт, а переменной типа int 4 байта, структура, с полями, расположенными в такой последовательности будет занимать 24 байта, а не 32\. ```cpp struct option_w { const wchar_t* name; int *flag; int val; int has_arg; }; ``` Хочется дать общую рекомендацию: располагать данные в структурах по уменьшению размера, потому что при некоторых [моделях данных](https://pvs-studio.ru/ru/blog/terms/0012/) в системах, где будет использоваться приложение, это может дать серьёзное ускорение работы с памятью\. Было ещё 286 подобных предупреждений, вот некоторые из них: * [V802](https://pvs-studio.ru/ru/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.ru/ru/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.ru/ru/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 ## Боремся не только с ошибками, но и с мусором в коде В данном проекте много избыточного кода, что, по моему мнению, связано с тем, что разработчики невнимательно меняли его логику работы или допускали опечатки\. **Пример** **1\.** [V501](https://pvs-studio.ru/ru/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 заметил ненужное _\(c \=\= ' '\),_ которое дублируется через строчку\. **Пример 2\.** [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) \[[CWE\-571](https://cwe.mitre.org/data/definitions/571.html)\] Expression 'i \=\= 0' is always true\. bf\_buff\.c 187 buffer\_write\(BIO \*b, const char \*in, int inl\) ```cpp { .... 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); } } .... } ``` В данном фрагменте кода ненужная проверка i\=\=0, если мы дошли до этого блока кода, то уже произвели проверку _i<\=0_ результат которой _true,_ и проверку _i<0_, результат которой _false_, значит, _0_ может быть единственным значением i\. **Пример 3\.** [V547](https://pvs-studio.ru/ru/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); .... } } ``` Парадокс: если во многих случаях проверки ptr \!\= NULL очень не хватает, чтобы избежать неопределённого поведения из\-за разыменовывания нулевого указателя, то в данном случае вторая проверка оказалась лишней\. **Пример** **4\.** [V547](https://pvs-studio.ru/ru/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; } } ``` Функция check\_ca никогда не возвращает 2, в результате мы имеем большой фрагмент кода, который никогда не будет выполнен\. Возможно программист убрал блок кода с данным _return_ из _check\_ca_, но забыл убрать из другой части программы\. **Пример 5\.** [V1001](https://pvs-studio.ru/ru/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; } ``` В данном фрагменте анализатор сигнализирует, что копия переменной current\_value с которой мы работаем в функции UpdateTie не используется после присваивания ей значения new\_value, соответственно, мы можем спокойно удалить данную строчку кода\. Всего, в проекте было обнаружено ещё 19 предупреждений такого рода, вот сообщения PVS\-Studio о некоторых из них: * [V547](https://pvs-studio.ru/ru/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.ru/ru/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.ru/ru/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.ru/ru/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 ## Заключение С одной стороны, для оpen sоurсe проекта ошибок немного, особенно, учитывая, что над ним работает небольшая команда разработчиков, с другой же стороны, эмулятор является младшим братом Сitra, который может запускать практически все домашние и многие коммерческие 3ds игры, и в нём используются уже готовые фрагменты кода оттуда\. Я уверен, что в будущем пользователи смогут получить большую функциональность и меньшее количество багов\. ![0651_Yuzu_ru/image4.png](https://import.viva64.com/docx/blog/0651_Yuzu_ru/image4.png) Над рассмотренным эмулятором сейчас ведется активная работа, и существует сообщество модераторов, связаться с которыми можно через сайт\.