Владислав Столяров

05 Авг 2019

Теги:

#Cpp #GameDev

Анализ исходного кода проекта Yuzu c помощью статического анализатора PVS-Studio

05 Авг 2019

Автор: Владислав Столяров

О проекте
Разыменование потенциально нулевого указателя
Подозрительное условие
Невозможное условие
Ещё одно невозможное условие
Copy-Paste for
Выравнивание данных
Боремся не только с ошибками, но и с мусором в коде
Заключение

Меня зовут Владислав, и в данный момент я прохожу стажировку в компании 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. Однако, такой взаимосвязанный код может сбивать с толку не только анализатор, но и программистов и так явно лучше не писать.

Аналогичные предупреждения:

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

Подозрительное условие

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

Copy-Paste for

На каждый из этих операторов 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 подобных предупреждений, вот некоторые из них:

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

Боремся не только с ошибками, но и с мусором в коде

В данном проекте много избыточного кода, что, по моему мнению, связано с тем, что разработчики невнимательно меняли его логику работы или допускали опечатки.

Пример 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 о некоторых из них:

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

Заключение

С одной стороны, для оpen sоurсe проекта ошибок немного, особенно, учитывая, что над ним работает небольшая команда разработчиков, с другой же стороны, эмулятор является младшим братом Сitra, который может запускать практически все домашние и многие коммерческие 3ds игры, и в нём используются уже готовые фрагменты кода оттуда. Я уверен, что в будущем пользователи смогут получить большую функциональность и меньшее количество багов.