PVS-Studio для проверки эмулятора RPCS3

RPCS3 — интересный проект, эмулирующий консоль PS3. Он активно развивается. Недавно мы услышали новость о том, что эмулятор научился запускать все игры из каталога консоли. Хороший повод проанализировать проект. Посмотрим, какие ошибки остались после добавления в проект новых исправлений.

Введение

Проект достаточно весомый. Он содержит около 300 тысяч строк кода C++ и опирается на множество внешних зависимостей, включая следующие:

• llvm, набор инструментов для написания компиляторов и утилит. Кстати, недавно мы проверили LLVM 13;
• ffmpeg — библиотека для работы с медиафайлами;
• curl, полезный при сетевом взаимодействии и для работы с протоколом HTTP;
• zlib — библиотека сжатия данных, использующая алгоритм DEFLATE.

Для части с графическим интерфейсом в проекте используется Qt, однако он взят из системной библиотеки. На скриншоте ниже показан полный список зависимостей:

Обратите внимание, что используемый стандарт C++ является самым последним, C++20. PVS-Studio отлично справляется с проверкой такого современного кода. Это потому, что мы постоянно работаем над поддержкой инноваций. Да, есть еще кое-что, что нужно улучшить, и мы работаем над их исправлением. В целом проверка стала хорошей проверкой того, как анализатор поддерживает новые языковые конструкции.

В проекте RPCS3 используется система сборки CMake. К сожалению, при сборке у меня возникли некоторые проблемы — GCC 11.2 отказался компилировать некоторую конструкцию constexpr. Однако Clang отлично справился со сборкой. Я построил проект на версии Ubuntu для разработчиков, поэтому проблема, с которой я столкнулся, может быть связана с дистрибутивом.

Вся процедура сборки и проверки проекта на Linux в режиме межмодульного анализа выглядит следующим образом:

cmake -S. -Bbuild -DCMAKE_EXPORT_COMPILE_COMMANDS=On -DCMAKE_BUILD_TYPE=Debug \
          -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
cmake --build build -j$(nproc)
pvs-studio-analyzer analyze -f ./build/compile_commands.json -j`nproc` \
          -o pvs.log -e 3rdparty/ -e llvm/ --intermodular

Все, анализ сделан! Время смотреть на ошибки!

Не кодируй в стандартном, бро

V1061 Расширение пространства имен std может привести к неопределенному поведению. shared_ptr.hpp 1131

namespace std
{
  template <typename T>
  void swap(stx::single_ptr<T>& lhs, stx::single_ptr<T>& rhs) noexcept
  {
    lhs.swap(rhs);
  }
  template <typename T>
  void swap(stx::shared_ptr<T>& lhs, stx::shared_ptr<T>& rhs) noexcept
  {
    lhs.swap(rhs);
  }
}

Стандарт C++ явно запрещает определение шаблонов пользовательских функций в пространстве имен std. C++20 также запрещает определять специализации для шаблонов функций. Частой ошибкой такого рода является определение пользовательской функции swap. В этом случае вы можете сделать следующее:

• определите функцию swap в том же пространстве имен, где определен класс (stx);
• добавить директиву using std::swap в блок, требующий вызова функции swap;
• вызвать swap без указания пространства имен std, т. е. выполнить неквалифицированный вызов функции: swap(obj1, obj2);

Этот подход использует механизм поиска, зависящего от аргумента (ADL). В результате компилятор находит функцию swap, которую мы определили рядом с классом. Пространство имен std остается неизменным.

Удаленный мемсет

V597 Компилятор мог удалить вызов функции memset, который используется для сброса объекта cty. Функцию memset_s() следует использовать для удаления личных данных. aes.cpp 596

/*
 * AES key schedule (decryption)
 */
int aes_setkey_dec(....)
{
    aes_context cty;
    // ....
done:
    memset( &cty, 0, sizeof( aes_context ) );
    return( 0 );
}

Это частая ошибка. При оптимизации кода компилятор удаляет вызов memset, а приватные данные остаются в памяти. Да, в случае с эмулятором это вряд ли представляет угрозу утечки данных — но в любом случае ошибка присутствует.

PVS-Studio обнаружил больше локаций с ошибкой такого типа:

• V597 Компилятор мог удалить вызов функции memset, которая используется для очистки буфера tmpbuf. Функцию memset_s() следует использовать для удаления личных данных. sha1.cpp 371
• V597 Компилятор мог удалить вызов функции memset, который используется для сброса объекта ctx. Функцию memset_s() следует использовать для удаления личных данных. sha1.cpp 396

Избыточная проверка

V547 Выражение rawcode == CELL_KEYC_KPAD_NUMLOCK всегда ложно. cellKb.cpp 126

enum Keys
{
  // ....
  CELL_KEYC_KPAD_NUMLOCK          = 0x53,
  // ....
};
u16 cellKbCnvRawCode(u32 arrange, u32 mkey, u32 led, u16 rawcode)
{
  // ....
  // CELL_KB_RAWDAT
  if (rawcode <= 0x03
      || rawcode == 0x29
      || rawcode == 0x35
      || (rawcode >= 0x39 && rawcode <= 0x53)    // <=
      || rawcode == 0x65
      || rawcode == 0x88
      || rawcode == 0x8A
      || rawcode == 0x8B)
  {
    return rawcode | 0x8000;
  }
  const bool is_alt = mkey & (CELL_KB_MKEY_L_ALT | CELL_KB_MKEY_R_ALT);
  const bool is_shift = mkey & (CELL_KB_MKEY_L_SHIFT | CELL_KB_MKEY_R_SHIFT);
  const bool is_caps_lock = led & (CELL_KB_LED_CAPS_LOCK);
  const bool is_num_lock = led & (CELL_KB_LED_NUM_LOCK);
  // CELL_KB_NUMPAD
  if (is_num_lock)
  {
    if (rawcode == CELL_KEYC_KPAD_NUMLOCK)  return 0x00 | 0x4000; // <=
    if (rawcode == CELL_KEYC_KPAD_SLASH)    return 0x2F | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_ASTERISK) return 0x2A | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_MINUS)    return 0x2D | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_PLUS)     return 0x2B | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_ENTER)    return 0x0A | 0x4000;
    if (rawcode == CELL_KEYC_KPAD_0)        return 0x30 | 0x4000;
    if (rawcode >= CELL_KEYC_KPAD_1 && rawcode <= CELL_KEYC_KPAD_9)
      return (rawcode - 0x28) | 0x4000;
  }
}

Здесь ошибка скрыта в первом условии: это условие блокирует следующее условие, которое проверяет, равно ли значение переменной rawcode постоянному значению CELL_KEYC_KPAD_NUMLOCK. Значение CELL_KEYC_KPAD_NUMLOCK соответствует 0x53 — это число соответствует первому условию, поэтому функция завершается там. Следовательно, нижний блок if никогда не выполняется.

Ошибка могла быть вызвана одной из двух причин — либо первое условие не учитывает значение константы, либо константа определена неправильно.

Переполнение массива

V557 Возможен опустошение массива. Значение индекса месяц + — 1 может достигать -1. cellRtc.cpp 1470

error_code cellRtcGetDaysInMonth(s32 year, s32 month)
{
  cellRtc.todo("cellRtcGetDaysInMonth(year=%d, month=%d)", year, month);
  if ((year < 0) || (month < 0) || (month > 12))
  {
    return CELL_RTC_ERROR_INVALID_ARG;
  }
  if (is_leap_year(year))
  {
    return not_an_error(DAYS_IN_MONTH[month + 11]);
  }
  return not_an_error(DAYS_IN_MONTH[month + -1]); // <=
}

В приведенном выше коде значение аргумента month может быть равно 0. Следовательно, оператор возврата может попытаться получить доступ к элементу массива DAYS_IN_MONTH с индексом -1.

Скорее всего, ошибка в первом условии. Приведенный выше код считает месяцы с единицы, а условие гарантирует, что month не меньше нуля. Правильным условием будет месяц ‹ 1.

Эта ошибка напомнила мне интересный случай из проекта protobuf: 31 февраля.

Ошибка копирования-вставки

V519 Переменной evnt-›color.white_x два раза подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 51, 52. sys_uart.cpp 52

struct av_get_monitor_info_cmd : public ps3av_cmd
{
  bool execute(....) override
  {
    // ....
    evnt->color.blue_x = 0xFFFF;
    evnt->color.blue_y = 0xFFFF;
    evnt->color.green_x = 0xFFFF;
    evnt->color.green_y = 0xFFFF;
    evnt->color.red_x = 0xFFFF;
    evnt->color.red_y = 0xFFFF;
    evnt->color.white_x = 0xFFFF;
    evnt->color.white_x = 0xFFFF; // <=
    evnt->color.gamma = 100;
    // ....
  {
};

Распространенная ошибка: при написании функции разработчик скопировал строку и забыл изменить нужную переменную. И обнаружить эту ошибку, просто прочитав код, довольно сложно — в то время как статический анализатор в этих случаях отлично справляется.

Повторные проверки

V581 Условные выражения операторов if, расположенных рядом друг с другом, идентичны. Проверить строки: 4225, 4226. PPUTranslator.cpp 4226

void PPUTranslator::MTFSFI(ppu_opcode_t op)
{
  SetFPSCRBit(op.crfd * 4 + 0, m_ir->getInt1((op.i & 8) != 0), false);
  if (op.crfd != 0) SetFPSCRBit(op.crfd * 4 + 1,
                                m_ir->getInt1((op.i & 4) != 0), false);
  if (op.crfd != 0) SetFPSCRBit(op.crfd * 4 + 2,
                                m_ir->getInt1((op.i & 2) != 0), false);
  SetFPSCRBit(op.crfd * 4 + 3, m_ir->getInt1((op.i & 1) != 0), false);
  if (op.rc) SetCrFieldFPCC(1);
}

Это похоже на очередную ошибку копирования-вставки. Скорее всего, кто-то скопировал условие и забыл его изменить. Однако тогдашняя часть сейчас другая.

Интересно, что это не единственный случай такой ошибки. Анализатор обнаружил еще одну ошибку такого рода:

• V581 Условные выражения операторов if, расположенных рядом друг с другом, идентичны. Строки проверки: 758, 759. RSXThread.cpp 759

Ошибка цикла

V560 Часть условного выражения всегда истинна: ​​i != 1. PPUTranslator.cpp 4252

void PPUTranslator::MTFSF(ppu_opcode_t op)
{
  const auto value = GetFpr(op.frb, 32, true);
  for (u32 i = 16; i < 20; i++)
  {
    if (i != 1 && i != 2 && (op.flm & (128 >> (i / 4))) != 0)
    {
      SetFPSCRBit(i, Trunc(m_ir->CreateLShr(value, i ^ 31),
                  GetType<bool>()), false);
    }
  }
  if (op.rc) SetCrFieldFPCC(1);
}

Приведенный выше цикл for работает с числами от 16 до 20, что означает, что условие блока if внутри цикла никогда не выполняется, а значение переменной i никогда не сравнивается с 1 и 2. Возможно. кто-то рефакторил этот код и забыл поменять индексы на правильные.

Разыменование указателя перед проверкой

V595 Указатель cached_dest использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 3059, 3064. texture_cache.h 3059

template <typename surface_store_type, typename blitter_type, typename ...Args>
blit_op_result upload_scaled_image(....)
{
  // ....
  if (!use_null_region) [[likely]]
  {
    // Do preliminary analysis
    typeless_info.analyse();
    blitter.scale_image(cmd, vram_texture, dest_texture, src_area, dst_area,
                        interpolate, typeless_info);
  }
  else
  {
    cached_dest->dma_transfer(cmd, vram_texture, src_area, // <=
                              dst_range, dst.pitch);
  }
  blit_op_result result = true;
  if (cached_dest) // <=
  {
    result.real_dst_address = cached_dest->get_section_base();
    result.real_dst_size = cached_dest->get_section_size();
  }
  else
  {
    result.real_dst_address = dst_base_address;
    result.real_dst_size = dst.pitch * dst_dimensions.height;
  }
  return result;
}

Здесь мы видим еще один частый паттерн — сначала используется указатель, и только потом он проверяется. Опять же, кто-то мог неосознанно создать эту ошибку при изменении кода.

Проверка «нового» результата на нуль

V668 Проверять указатель movie на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. movie_item.h 56

void init_movie(const QString& path)
{
  if (path.isEmpty() || !m_icon_callback) return;
  if (QMovie* movie = new QMovie(path); movie && movie->isValid())
  {
    m_movie = movie;
  }
  else
  {
    delete movie;
    return;
  }
  QObject::connect(m_movie, &QMovie::frameChanged, m_movie, m_icon_callback);
}

Проверка на nullptr здесь бессмысленна: если вызов new вызывает ошибку, генерируется исключение std::bad_alloc. Если нет необходимости кидать исключение, можно использовать конструкцию std::nothrow — в этом случае будет возвращен нулевой указатель.

Вот еще несколько мест с этой ошибкой:

• V668 Проверять указатель m_render_creator на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. emu_settings.cpp 75
• V668 Проверять указатель trophy_slider_label на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. trophy_manager_dialog.cpp 216

Утечка памяти

V773 Выход из функции без отпускания указателя буфер. Возможна утечка памяти. rsx_debugger.cpp 380

u8* convert_to_QImage_buffer(rsx::surface_color_format format,
                             std::span<const std::byte> orig_buffer,
                             usz width, usz height) noexcept
{
  u8* buffer = static_cast<u8*>(std::malloc(width * height * 4));
  if (!buffer || width == 0 || height == 0)
  {
    return nullptr;
  }
  for (u32 i = 0; i < width * height; i++)
  {
    // depending on original buffer, the colors may need to be reversed
    const auto &colors = get_value(orig_buffer, format, i);
    buffer[0 + i * 4] = colors[0];
    buffer[1 + i * 4] = colors[1];
    buffer[2 + i * 4] = colors[2];
    buffer[3 + i * 4] = 255;
  }
  return buffer;
}

Вначале функция использует malloc для выделения памяти. Если возвращается nullptr, функция завершает работу. Все идет нормально. Затем проверяются параметры width и height — это происходит после выделения памяти. В случае успеха функция также возвращает nullptr. Да, если эти переменные равны нулю, malloc возвращает 0 байт. Однако в стандарте указано, что в этом случае функция может возвращать либо nullptr, либо действительный указатель, который нельзя разыменовать. Но в любом случае его нужно освободить. Кроме того, free также может принимать нулевой указатель. Таким образом, исправление может выглядеть так:

if (!buffer || width == 0 || height == 0)
{
  std::free(buffer)
  return nullptr;
}

Как вариант, можно вообще убрать проверки на 0 — в этом случае цикл выполняться не будет:

if (!buffer)
{
  return nullptr;
}
for (u32 i = 0; i < width * height; i++)
{
  // ....
}
return buffer;

Неправильная проверка размера

V557 Возможен переполнение массива. Индекс pad указывает за пределы массива. pad_thread.cpp 191

void pad_thread::SetRumble(const u32 pad, u8 largeMotor, bool smallMotor)
{
  if (pad > m_pads.size())
    return;
  if (m_pads[pad]->m_vibrateMotors.size() >= 2)
  {
    m_pads[pad]->m_vibrateMotors[0].m_value = largeMotor;
    m_pads[pad]->m_vibrateMotors[1].m_value = smallMotor ? 255 : 0;
  }
}

В приведенном выше коде используется оператор › вместо ›= для проверки входных данных. В результате значение pad может быть равно размеру контейнера m_pads. Это может привести к переполнению при следующем доступе к контейнеру.

Сдвиг в неправильном направлении

V547 Выражение текущая_версия ‹ пороговая_версия всегда ложно. Значение типа без знака никогда не равно ‹ 0. device.cpp 91

void physical_device::create(VkInstance context,
                             VkPhysicalDevice pdev,
                             bool allow_extensions)
{
  else if (get_driver_vendor() == driver_vendor::NVIDIA)
  {
#ifdef _WIN32
    // SPIRV bugs were fixed in 452.28 for windows
    const u32 threshold_version = (452u >> 22) | (28 >> 14);
#else
    // SPIRV bugs were fixed in 450.56 for linux/BSD
    const u32 threshold_version = (450u >> 22) | (56 >> 14);
#endif
    // Clear patch and revision fields
    const auto current_version = props.driverVersion & ~0x3fffu;
    if (current_version < threshold_version)
    {
      rsx_log.error(....);
    }
  }
}

Константа threshold_version всегда равна 0, так как используется сдвиг вправо, а не влево. Сдвиг вправо эквивалентен делению на степень двойки — в нашем случае на 2²² и 2¹⁴ соответственно. Очевидно, что значения из приведенных выше выражений меньше этих степеней. Это означает, что результат всегда равен нулю.

Похоже, кто-то скопировал этот фрагмент из кода, расшифровывающего значения версий, и забыл изменить операторы.

Заключение

Анализатор проверил проект и обнаружил различные ошибки: от традиционных — вроде опечаток — до более сложных, вроде логических ошибок, вызванных тем, что некоторые части кода не тестировались. Надеемся, что эта проверка поможет исправить пару ошибок. Мы также надеемся, что разработчики эмулятора продолжат свою работу по поддержке игр, и желаем им отличной работы эмулятора. Стало любопытно? Вы можете скачать пробную версию анализатора PVS-Studio и посмотреть, какие ошибки он найдет в вашем коде. А если вы разрабатываете игру или проект с открытым исходным кодом, мы предлагаем вам рассмотреть нашу бесплатную лицензию.