Меня зовут Владислав, и в данный момент я прохожу стажировку в компании PVS-Studio. Как известно, лучший способ ознакомится с продуктом - попробовать его, а заодно оформить свои наблюдения в виде статьи. Меня всегда очень интересовали эмуляторы игровых платформ, потребность в которых всё больше и больше ощущается с выходом новых игровых консолей. Yuzu - это первый Nintendo Switch эмулятор. На примере данного проекта, мы можем убедиться, что PVS-Studio помогает не только найти баги в коде, но и сделать его гораздо читабельнее и дружелюбнее, а также, при постоянном использовании, поможет избежать появления ситуаций, в которых могут возникать ошибки.
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.
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)
{
....
}
....
}Указатель policy сначала разыменовывается, а потом проверяется на NULL. Это может означать одно из двух очевидных вещей: возникнет неопределённое поведение, если указатель действительно нулевой, или указатель никогда не может иметь нулевое значение, и программа всегда работает корректно. Если подразумевается первый вариант, то проверку стоит произвести до разыменования, во втором варианте можно опустить избыточную проверку. Есть ещё один не такой очевидный сценарий: возможно, policy не может быть нулевым указателем, если нулевым является указатель id. Однако, такой взаимосвязанный код может сбивать с толку не только анализатор, но и программистов и так явно лучше не писать.
Аналогичные предупреждения:
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);
....
}Функция isExtIdx2() возвращает значение типа bool, переменная Hi16Vidx тоже имеет тип bool. Выражение выглядит очень подозрительно, как будто здесь была побитовая магия, а потом она превратилась волшебным образом в булеву логику. Скорее всего код, который хотел написать автор:
bool Vp = !((v ? v->isExtIdx2() : 0) || Hi16Vidx);На самом деле здесь нет ошибки. Этот код будет работать одинаково как с оператором |, так и с ||. Однако, такой код заставляет задуматься и провести его рефакторинг.
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;
}
....
}Константа 0x3FF = 1023. Посмотрим на следующую строчку, в данное условие мы не зайдём. Значение module не может превысить 2000. Возможно значение константы менялось в процессе разработки.
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 )
{
....
}
....
}Функция обрабатывает ключи, значения которых хранятся в mbedtls_ecdh_side.
typedef enum
{
MBEDTLS_ECDH_OURS,
MBEDTLS_ECDH_THEIRS,
} mbedtls_ecdh_side;Как мы видим, мы никогда не сможем обработать значение, равное 'MBEDTLS_ECDH_OURS' из-за того, что проверка осуществляется на отрицание равенства, и, так как значений всего 2, а в первый if мы не зашли, она никогда не даст результат true. Скорее всего, правильным бы было добавить else к первому if. Или производить проверку на равенство:
....
if( side == MBEDTLS_ECDH_OURS )
....На каждый из этих операторов for анализатор выдал предупреждение:
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);
}
....
}Скорее всего это банальный copy-paste и циклы должны были выполнять хотя бы одну итерацию.
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;
};В данном случае на 64 разрядной платформе (например, 'WIN64, MSVC'), где размер указателя равен 8 байтам, мы можем уменьшить размер структуры на 8 байт путём перестановки полей по убыванию. Так как размер указателя составляет 8 байт, а переменной типа int 4 байта, структура, с полями, расположенными в такой последовательности будет занимать 24 байта, а не 32.
struct option_w
{
const wchar_t* name;
int *flag;
int val;
int has_arg;
};Хочется дать общую рекомендацию: располагать данные в структурах по уменьшению размера, потому что при некоторых моделях данных в системах, где будет использоваться приложение, это может дать серьёзное ускорение работы с памятью.
Было ещё 286 подобных предупреждений, вот некоторые из них:
В данном проекте много избыточного кода, что, по моему мнению, связано с тем, что разработчики невнимательно меняли его логику работы или допускали опечатки.
Пример 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 заметил ненужное (c == ' '), которое дублируется через строчку.
Пример 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);
}
}
....
}В данном фрагменте кода ненужная проверка i==0, если мы дошли до этого блока кода, то уже произвели проверку i<=0 результат которой true, и проверку i<0, результат которой false, значит, 0 может быть единственным значением i.
Пример 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);
....
}
}Парадокс: если во многих случаях проверки ptr != NULL очень не хватает, чтобы избежать неопределённого поведения из-за разыменовывания нулевого указателя, то в данном случае вторая проверка оказалась лишней.
Пример 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;
}
}Функция check_ca никогда не возвращает 2, в результате мы имеем большой фрагмент кода, который никогда не будет выполнен. Возможно программист убрал блок кода с данным return из check_ca, но забыл убрать из другой части программы.
Пример 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;
}В данном фрагменте анализатор сигнализирует, что копия переменной current_value с которой мы работаем в функции UpdateTie не используется после присваивания ей значения new_value, соответственно, мы можем спокойно удалить данную строчку кода.
Всего, в проекте было обнаружено ещё 19 предупреждений такого рода, вот сообщения PVS-Studio о некоторых из них:
С одной стороны, для оpen sоurсe проекта ошибок немного, особенно, учитывая, что над ним работает небольшая команда разработчиков, с другой же стороны, эмулятор является младшим братом Сitra, который может запускать практически все домашние и многие коммерческие 3ds игры, и в нём используются уже готовые фрагменты кода оттуда. Я уверен, что в будущем пользователи смогут получить большую функциональность и меньшее количество багов.
Над рассмотренным эмулятором сейчас ведется активная работа, и существует сообщество модераторов, связаться с которыми можно через сайт.