Проверяем эмулятор RPCS3 с помощью PVS-Studio15.11.2021 19:16

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

0886_rpcs3_ru/image1.png


Введение

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


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

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

0886_rpcs3_ru/image2.png

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

Проект использует сборочную систему CMake. К сожалению, во время сборки у меня возникли проблемы — GCC 11.2 отказывался компилировать какую-то constexpr-конструкцию, однако Clang 12 справился со сборкой отлично. Сборку я производил под devel-версией 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

Что же, проект проверен — время перейти к ошибкам!


Не пиши в std, брат

V1061 Extending the 'std' namespace may result in undefined behavior. shared_ptr.hpp 1131

namespace std
{
  template 
  void swap(stx::single_ptr& lhs, stx::single_ptr& rhs) noexcept
  {
    lhs.swap(rhs);
  }

  template 
  void swap(stx::shared_ptr& lhs, stx::shared_ptr& rhs) noexcept
  {
    lhs.swap(rhs);
  }
}

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


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

При таком подходе работает механизм Argument-Dependent Lookup (ADL), в результате чего находится функция swap, которую мы определили рядом с классом. А пространство имён std не изменено.


Удалённый memset

V597 The compiler could delete the 'memset' function call, which is used to flush 'cty' object. The memset_s () function should be used to erase the private data. aes.cpp 596

/*
 * AES key schedule (decryption)
 */
int aes_setkey_dec(....)
{
    aes_context cty;

    // ....

done:
    memset( &cty, 0, sizeof( aes_context ) );

    return( 0 );
}

Частая ошибка — вызов memset будет удалён компилятором во время оптимизаций, а приватные данные останутся лежать в памяти. Да, в случае кода эмулятора вряд ли это представляет какую-либо угрозу утечки данных, но тем не менее ошибка присутствует.

Также были найдены ещё случаи такой ошибки:


  • V597 The compiler could delete the 'memset' function call, which is used to flush 'tmpbuf' buffer. The memset_s () function should be used to erase the private data. sha1.cpp 371
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The memset_s () function should be used to erase the private data. sha1.cpp 396


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

V547 Expression 'rawcode == CELL_KEYC_KPAD_NUMLOCK' is always false. 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 соответствует числу 0×53 — это число попадает под первое условие, которое в результате выполнения выходит из функции, так что нижний if никогда не выполнится.

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


Выход за границу массива

V557 Array underrun is possible. The value of 'month + — 1' index could reach -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]); // <=
}

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

Скорее всего, ошибка находится в первом условии — судя по коду, отсчет месяцев ведётся с единицы, а условие проверяет на то, что month меньше 0, когда надо month < 1.

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


Ошибка копипасты

V519 The 'evnt→color.white_x' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 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);
}

Похоже на ещё одну ошибку копипасты — видимо, скопировали условие и забыли его поменять, однако then-часть теперь другая.

Что интересно, это не единственный случай такой ошибки. В коде было ещё одно срабатывание:


  • V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 758, 759. RSXThread.cpp 759


Ошибка в цикле

V560 A part of conditional expression is always true: 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()), false);
    }
  }

  if (op.rc) SetCrFieldFPCC(1);
}

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


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

V595 The 'cached_dest' pointer was utilized before it was verified against nullptr. Check lines: 3059, 3064. texture_cache.h 3059

template 
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;
}

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


Проверка на null результата new

V668 There is no sense in testing the 'movie' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. 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 There is no sense in testing the 'm_render_creator' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. emu_settings.cpp 75
  • V668 There is no sense in testing the 'trophy_slider_label' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. trophy_manager_dialog.cpp 216


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

V773 The function was exited without releasing the 'buffer' pointer. A memory leak is possible. rsx_debugger.cpp 380

u8* convert_to_QImage_buffer(rsx::surface_color_format format,
                             std::span orig_buffer,
                             usz width, usz height) noexcept
{
  u8* buffer = static_cast(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 Array overrun is possible. The 'pad' index is pointing beyond array bound. 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 Expression 'current_version < threshold_version' is always false. Unsigned type value is never < 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^22 и 2^14 соответственно. Очевидно, что числа из выражений меньше этих степеней, поэтому результат будет равен 0.

Похоже, что это место было скопировано из кода декодирования значения версии, но при копировании поменять операторы просто забыли.


Заключение

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Larin. PVS-Studio to check the RPCS3 emulator.

© Habrahabr.ru