Александр Куренев

16 Июн 2022

Теги:

#Cpp #GameDev

Проверяем эмулятор GPCS4, или сможем ли когда-нибудь поиграть в "Bloodborne" на PC

16 Июн 2022

Автор: Александр Куренев

О проекте
Потерянный break
Проверка указателя после использования
Повторное присвоение
Переполнение буфера
Учимся считать до семи, или ещё одно переполнение буфера
Неправильное использование флага
Лишняя проверка
Ещё одно всегда истинное условие
Ошибка копипасты
Почему лучше избегать сложных выражений?
Краевой случай в операторе перемещения
Повторное присвоение как повод для рефакторинга
Заключение

Эмулятор – это приложение, способное имитировать запуск программы, предназначенной для одной платформы, на другой. Примером эмулятора является GPCS4, предназначенный для запуска игр для PS4 на PC. Недавно состоялся первый релиз GPCS4, и мы решили проверить этот проект. Давайте посмотрим, какие ошибки удалось найти PVS-Studio в исходном коде этого эмулятора.

О проекте

GPCS4 – это эмулятор PlayStation 4, написанный на C и C++.

Изначально целью автора проекта было исследовать архитектуру PS4. Однако проект быстро развивался, и уже в начале 2020 года разработчикам GPCS4 удалось запустить на своём эмуляторе игру We are Doomed. Это был первый успешный стабильный запуск на PC игры для PS4. Впрочем, сам игровой процесс пока что был далёк от идеала: игра тормозила, картинка дёргалась. Тем не менее, основной разработчик проекта полон энтузиазма и продолжает совершенствовать эмулятор.

В конце апреля 2022 года состоялся первый релиз GPCS4. Я скачал и проверил версию 0.1.0 проекта. К слову, на момент публикации статьи уже вышел релиз 0.2.1 – проект развивается очень быстро. Давайте перейдём к ошибкам и недочётам, которые удалось найти анализатору PVS-Studio в первом релизе проекта GPCS4.

Потерянный break

V796 [CWE-484] It is possible that 'break' statement is missing in switch statement. AudioOut.cpp 137

static AudioProperties getAudioProperties(uint32_t param)
{
  uint32_t format       = param & 0x000000ff;
  AudioProperties props = {};

  switch (format)
  {
    // ....
    case SCE_AUDIO_OUT_PARAM_FORMAT_S16_8CH_STD:
    {
      props.nChannels   = 8;
      props.bytesPerSample  = 2;
      props.audioFormat = RTAUDIO_FORMAT_SINT16;
      break;
    }
    case SCE_AUDIO_OUT_PARAM_FORMAT_FLOAT_MONO:
    {
      props.nChannels   = 1;
      props.bytesPerSample  = 4;
      props.audioFormat = RTAUDIO_FORMAT_FLOAT32;         // <=
    }
    case SCE_AUDIO_OUT_PARAM_FORMAT_FLOAT_STEREO:
    {
      props.nChannels   = 2;
      props.bytesPerSample  = 4;
      props.audioFormat = RTAUDIO_FORMAT_FLOAT32;
      break;
    }
  }
  return props;
}

В данном фрагменте кода для случая SCE_AUDIO_OUT_PARAM_FORMAT_FLOAT_MONO потерян break. Из-за этого количество каналов будет выставлено неправильно.

Проверка указателя после использования

V595 The 'm_moduleData' pointer was utilized before it was verified against nullptr. Check lines: 49, 53. ELFMapper.cpp 49

struct NativeModule { /*....*/ };

class ELFMapper
{
  // ....
  NativeModule *m_moduleData;
};

bool ELFMapper::validateHeader()
{
  bool retVal      = false;
  auto &fileMemory = m_moduleData->m_fileMemory;
  do
  {
    if (m_moduleData == nullptr)
    {
      LOG_ERR("file has not been loaded");
      break;
    }
    // ....
  } while (false);
  
  return retVal;
}

В приведённом фрагменте указатель m_moduleData сначала разыменовывается, а затем в цикле do-while сравнивается с nullptr.

Внимательные читатели могут возразить: "Может быть, на вход функции всегда приходит валидный указатель? А потом в цикле do-while этот указатель модифицируется и может стать нулевым? Тогда и ошибки тут нет". В данном случае это не так. Во-первых, из-за условия while (false) выполнится ровно одна итерация цикла. Во-вторых, указатель m_moduleData не модифицируется.

Другое возражение может заключаться в том, что взятие ссылки – это безопасно. Ведь использоваться эта ссылка будет, только если указатель валидный. Но нет, такой код содержит неопределённое поведение. Это ошибка. Скорее всего, нужно сделать проверку указателя до его первого разыменования:

bool ELFMapper::validateHeader()
{
  bool retVal      = false;
  
  do
  {
    if (m_moduleData == nullptr)
    {
      LOG_ERR("file has not been loaded");
      break;
    }

    auto &fileMemory = m_moduleData->m_fileMemory;
    // ....
  } while (false);

  return retVal;
}

Повторное присвоение

V519 [CWE-563] The '* memoryType' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 54, 55. sce_kernel_memory.cpp 55

int PS4API sceKernelGetDirectMemoryType(sce_off_t start, int *memoryType, 
    sce_off_t *regionStartOut, sce_off_t *regionEndOut)
{
  LOG_SCE_DUMMY_IMPL();
  *memoryType = SCE_KERNEL_WB_GARLIC;
  *memoryType = SCE_KERNEL_WC_GARLIC;
  return SCE_OK;
}

Как можно догадаться по названию LOG_SCE_DUMMY_IMPL, реализация метода sceKernelGetDirectMemoryType ещё будет меняться. И всё же, два присваивания по одному и тому же адресу memoryType выглядят странно. Возможно, так получилось в результате неудачного слияния кода.

Переполнение буфера

V512 [CWE-119] A call of the 'memset' function will lead to overflow of the buffer 'param->reserved'. sce_gnm_draw.cpp 420

V531 [CWE-131] It is odd that a sizeof() operator is multiplied by sizeof(). sce_gnm_draw.cpp 420

struct GnmCmdPSShader
{
  uint32_t              opcode;
  gcn::PsStageRegisters psRegs;
  uint32_t              reserved[27];
};

int PS4API sceGnmSetPsShader350(uint32_t* cmdBuffer, uint32_t numDwords, 
                                const gcn::PsStageRegisters *psRegs)
{
  // ....
  memset(param->reserved, 0, sizeof(param->reserved) * sizeof(uint32_t)); 
  return SCE_OK;
}

Иногда бывает, что на одну и ту же строчку кода указывают сразу несколько диагностик PVS-Studio. Так получилось и в этом примере. В данном фрагменте кода в функцию memset передаётся неправильное значение третьим аргументом. Выражение sizeof(param->reserved) уже вернёт размер массива param->reserved. Умножение на sizeof(uint32_t) увеличит это значение в 4 раза, и значение получится неправильным. Поэтому в результате вызова memset произойдёт выход за границу массива param->reserved. Нужно убрать лишнее умножение:

int PS4API sceGnmSetPsShader350( /*....*/ )
{
  // ....
  memset(param->reserved, 0, sizeof(param->reserved));
  return SCE_OK;
}

Всего анализатор обнаружил 20 подобных переполнений, приведу ещё один пример:

V512 [CWE-119] A call of the 'memset' function will lead to overflow of the buffer 'initParam->reserved'. sce_gnm_dispatch.cpp 16

uint32_t PS4API sceGnmDispatchInitDefaultHardwareState(uint32_t* cmdBuffer,
                                                       uint32_t numDwords)
{
  // ....
  memset(initParam->reserved, 0,
         sizeof(initParam->reserved) * sizeof(uint32_t));
  return initCmdSize;
}

В этом фрагменте кода происходит выход за границу массива initParam->reserved.

Учимся считать до семи, или ещё одно переполнение буфера

V557 [CWE-787] Array overrun is possible. The 'dynamicStateCount ++' index is pointing beyond array bound. VltGraphics.cpp 157

VkPipeline VltGraphicsPipeline::createPipeline(/* .... */) const
{
  // ....
  std::array<VkDynamicState, 6> dynamicStates;
  uint32_t                      dynamicStateCount = 0;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_VIEWPORT;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_SCISSOR;
  if (state.useDynamicDepthBias())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BIAS;
  if (state.useDynamicDepthBounds())
  {
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BOUNDS;
    dynamicStates[dynamicStateCount++] =
                             VK_DYNAMIC_STATE_DEPTH_BOUNDS_TEST_ENABLE;
  }
  if (state.useDynamicBlendConstants())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_BLEND_CONSTANTS;
  if (state.useDynamicStencilRef())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_STENCIL_REFERENCE;
  // ....
}

Анализатор предупреждает, что может произойти переполнение массива dynamicStates. В данном фрагменте кода есть 4 проверки:

if (state.useDynamicDepthBias())
if (state.useDynamicDepthBounds())
if (state.useDynamicBlendConstants())
if (state.useDynamicStencilRef())

Каждая из них – это проверка одного из независимых флагов. Например, проверка if (state.useDynamicDepthBias()):

bool useDynamicDepthBias() const
{
  return rs.depthBiasEnable();
}

VkBool32 depthBiasEnable() const
{
  return VkBool32(m_depthBiasEnable);
}

Получается, все эти 4 проверки могут быть истинными одновременно. Тогда выполнится 7 строк вида 'dynamicStates[dynamicStateCount++] = ....'. На седьмой такой строке произойдёт обращение к dynamicStates[6]. Это выход за границу массива.

Для исправления нужно выделить память на 7 элементов:

VkPipeline VltGraphicsPipeline::createPipeline(/* .... */) const
{
  // ....
  std::array<VkDynamicState, 7> dynamicStates; // <=
  uint32_t                      dynamicStateCount = 0;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_VIEWPORT;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_SCISSOR;
  if (state.useDynamicDepthBias())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BIAS;
  if (state.useDynamicDepthBounds())
  {
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BOUNDS;
    dynamicStates[dynamicStateCount++] =
                             VK_DYNAMIC_STATE_DEPTH_BOUNDS_TEST_ENABLE;
  }
  if (state.useDynamicBlendConstants())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_BLEND_CONSTANTS;
  if (state.useDynamicStencilRef())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_STENCIL_REFERENCE;
  // ....
}

Неправильное использование флага

V547 [CWE-570] Expression 'nOldFlag & VMPF_NOACCESS' is always false. PlatMemory.cpp 22

#define PAGE_NOACCESS           0x01
#define PAGE_READONLY           0x02
#define PAGE_READWRITE          0x04
#define PAGE_EXECUTE            0x10
#define PAGE_EXECUTE_READ       0x20
#define PAGE_EXECUTE_READWRITE  0x40

enum VM_PROTECT_FLAG
{
  VMPF_NOACCESS  = 0x00000000,
  VMPF_CPU_READ  = 0x00000001,
  VMPF_CPU_WRITE = 0x00000002,
  VMPF_CPU_EXEC  = 0x00000004,
  VMPF_CPU_RW    = VMPF_CPU_READ | VMPF_CPU_WRITE,
  VMPF_CPU_RWX   = VMPF_CPU_READ | VMPF_CPU_WRITE | VMPF_CPU_EXEC,
};

inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  uint32_t nNewFlag = 0;
  do
  {
    if (nOldFlag & VMPF_NOACCESS)
    {
      nNewFlag = PAGE_NOACCESS;
      break;
    }

    if (nOldFlag & VMPF_CPU_READ)
    {
      nNewFlag = PAGE_READONLY;
    }

    if (nOldFlag & VMPF_CPU_WRITE)
    {
      nNewFlag = PAGE_READWRITE;
    }

    if (nOldFlag & VMPF_CPU_EXEC)
    {
      nNewFlag = PAGE_EXECUTE_READWRITE;
    }

  } while (false);
  return nNewFlag;
}

Функция GetProtectFlag конвертирует флаг с правами доступа к файлу из одного формата в другой. Однако делает это некорректно. Программист не учёл, что значение VMPF_NOACCESS равно нулю. Из-за этого условие if (nOldFlag & VMPF_NOACCESS) всегда ложное и функция никогда не вернёт значение PAGE_NOACCESS.

Кроме того, функция GetProtectFlag неправильно конвертирует не только флаг VMPF_NOACCESS, но и другие. Например, флаг VMPF_CPU_EXEC будет сконвертирован во флаг PAGE_EXECUTE_READWRITE.

Когда я думал, как это исправить, первой мыслью было написать что-то в таком роде:

inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  uint32_t nNewFlag = PAGE_NOACCESS;
  if (nOldFlag & VMPF_CPU_READ)
  {
    nNewFlag |= PAGE_READ;
  }

  if (nOldFlag & VMPF_CPU_WRITE)
  {
    nNewFlag |= PAGE_WRITE;
  }

  if (nOldFlag & VMPF_CPU_EXEC)
  {
    nNewFlag |= PAGE_EXECUTE;
  }

  return nNewFlag;
}

Однако в данном случае такой подход не работает. Дело в том, что PAGE_NOACCESS, PAGE_READONLY и остальные – это Windows-флаги со своей спецификой. Например, среди них нет флага PAGE_WRITE. Считается, что если есть права на запись, то как минимум есть права еще и на чтение. По тем же причинам нет флага PAGE_EXECUTE_WRITE.

Кроме того, побитовое "ИЛИ" двух Windows-флагов не даёт в результате флаг, соответствующий сумме прав: PAGE_READONLY | PAGE_EXECUTE != PAGE_EXECUTE_READ. Поэтому нужно перебирать все возможные комбинации флагов:

inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  switch (nOldFlag)
  {
    case VMPF_NOACCESS:
      return PAGE_NOACCESS;
    case VMPF_CPU_READ:
      return PAGE_READONLY;
    case VMPF_CPU_WRITE: // same as ReadWrite
    case VMPF_CPU_RW:
      return PAGE_READWRITE;
    case VMPF_CPU_EXEC:
      return PAGE_EXECUTE;
    case VMPF_CPU_READ | VMPF_CPU_EXEC:
      return PAGE_EXECUTE_READ:
    case VMPF_CPU_WRITE | VMPF_CPU_EXEC: // same as ExecuteReadWrite
    case VMPF_CPU_RWX:
      return PAGE_EXECUTE_READWRITE;
    default:
      LOG("unknown PS4 flag");
      return PAGE_NOACCESS;
  }
}

Лишняя проверка

V547 [CWE-571] Expression 'retAddress' is always true. Memory.cpp 373

void* MemoryAllocator::allocateInternal(void* addrIn, size_t len,
                                        size_t alignment, int prot)
{
  // ....
  while (searchAddr < SCE_KERNEL_APP_MAP_AREA_END_ADDR)
    {
      // ....
      void* retAddress = VMAllocate(reinterpret_cast<void*>(regionAddress), len,
                                    plat::VMAT_RESERVE_COMMIT, uprot);
      if (!retAddress)
      {
        searchAddr = reinterpret_cast<size_t>(mi.pRegionStart) + mi.nRegionSize;
        continue;
      }
      // ....
      if (retAddress)
      {
        // unlikely
        plat::VMFree(retAddress);
      }
    // ....
    }
  // ....
}

В этом фрагменте кода указатель retAddress проверяется дважды. Сначала делается проверка if (!retAddress). Если указатель нулевой, то выполнение перейдёт к следующей итерации цикла while. Иначе указатель retAddress ненулевой. Поэтому вторая проверка if (retAddress) всегда истинная, и её можно убрать.

Ещё одно всегда истинное условие

V547 [CWE-571] Expression 'pipeConfig == kPipeConfigP16' is always true. GnmDepthRenderTarget.h 170

uint8_t getZReadTileSwizzleMask(void) const
    {
      // From IDA
      auto pipeConfig = getPipeConfig();
      auto zfmt       = getZFormat();
      auto tileMode   = getTileMode();
      if (pipeConfig != kPipeConfigP16 ||     // <=
        zfmt == kZFormatInvalid ||
        !GpuAddress::isMacroTiled(tileMode))
      {
        return 0;
      }

      auto     dataFormat          = DataFormat::build(zfmt);
      auto     totalBitsPerElement = dataFormat.getTotalBitsPerElement();
      uint32_t numFragments          = 1 << getNumFragments();
      uint32_t shift               = 0;
      NumBanks numBanks            = {};
      if (pipeConfig == kPipeConfigP16)      // <=
      {
        GpuAddress::getAltNumBanks(&numBanks, tileMode,
                                   totalBitsPerElement, numFragments);
        shift = 4;
      }
      else
      {
        GpuAddress::getNumBanks(&numBanks, tileMode,
                                totalBitsPerElement, numFragments);
        shift = 3;
      }

      return (this->m_regs[2] & (((1 << (numBanks + 1)) - 1) << shift)) >> 4;
    }

В данном фрагменте кода анализатор нашёл всегда истинное условие if (pipeConfig == kPipeConfigP16). Давайте разберёмся, почему это так.

Если вызов функции getPipeConfig вернул значение, не равное kPipeConfigP16, то первое условие будет верным и выполнение программы не достигнет проверки if (pipeConfig == kPipeConfigP16).

Получается, что вторая проверка этой переменной либо не выполняется, либо всегда истинна. Но не стоит спешить и убирать её. Возможно, первое условие было добавлено временно и будет убрано в дальнейшем.

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

V517 [CWE-570] The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 469, 475. GnmGpuAddress.cpp 469

int32_t sce::GpuAddress::adjustTileMode(/* .... */)
{
switch(microTileMode)
{
  case Gnm::kMicroTileModeThin:
    if      (newArrayMode == Gnm::kArrayMode3dTiledThick)
      *outTileMode = Gnm::kTileModeThick_3dThick;
    else if      (newArrayMode == Gnm::kArrayMode2dTiledThick)
      *outTileMode = Gnm::kTileModeThick_2dThick;
    else if (newArrayMode == Gnm::kArrayMode1dTiledThick)
      *outTileMode = Gnm::kTileModeThick_1dThick;
    else if (newArrayMode == Gnm::kArrayMode3dTiledThin)
      *outTileMode = Gnm::kTileModeThin_3dThin; // ....
    else if (newArrayMode == Gnm::kArrayMode3dTiledThinPrt)
      *outTileMode = Gnm::kTileModeThin_3dThinPrt; // ....
    else if (newArrayMode == Gnm::kArrayMode2dTiledThin)                  // <=
      *outTileMode = Gnm::kTileModeThin_2dThin; // ....
    else if (newArrayMode == Gnm::kArrayMode2dTiledThinPrt)
      *outTileMode = Gnm::kTileModeThin_2dThinPrt; // ....
    else if (newArrayMode == Gnm::kArrayModeTiledThinPrt)
      *outTileMode = Gnm::kTileModeThin_ThinPrt; // ....
    else if (newArrayMode == Gnm::kArrayMode2dTiledThin)                  // <=
      *outTileMode = Gnm::kTileModeThin_2dThin;
    else if (newArrayMode == Gnm::kArrayMode1dTiledThin)
      *outTileMode = Gnm::kTileModeThin_1dThin;
    else
      break;
    return kStatusSuccess;
  // ....
}
}

Не обошлось и без ошибок копипасты. В данном фрагменте кода дважды написана одна и та же проверка newArrayMode == Gnm::kArrayMode2dTiledThin.

Сложно сказать, как именно нужно это поправить. Скорее всего, вторая проверка должна быть несколько другой. А может быть, она лишняя, и её можно убрать.

Почему лучше избегать сложных выражений?

V732 [CWE-480] Unary minus operator does not modify a bool type value. Consider using the '!' operator. GnmRenderTarget.h 237

typedef enum RenderTargetChannelType
{
  kRenderTargetChannelTypeUNorm            = 0x00000000,
  kRenderTargetChannelTypeSNorm            = 0x00000001,
  kRenderTargetChannelTypeUInt             = 0x00000004,
  kRenderTargetChannelTypeSInt             = 0x00000005,
  kRenderTargetChannelTypeSrgb             = 0x00000006,
  kRenderTargetChannelTypeFloat            = 0x00000007,
} RenderTargetChannelType;

void setDataFormat(DataFormat format)
{
  // ....
  int v3;
  RenderTargetChannelType  type;  // [rsp+4h] [rbp-3Ch]
  __int64                  v9;  // [rsp+10h] [rbp-30h]
  bool typeConvertable = format.getRenderTargetChannelType(&type);
  v2 = type | kRenderTargetChannelTypeSNorm;
  v3  = (uint8_t) - (type < 7) & (uint8_t)(0x43u >> type) & 1; // <=
  // ....
}

Похоже, что программист ожидал следующего поведения при вычислении выражения:

пусть переменная type меньше 7;
тогда выражение type < 7 равно true;
затем к true применяется унарный минус, получается -1;
значение -1 приводится к unsigned char, получается 0b1111'1111.

Однако на самом деле происходит следующее:

пусть переменная type меньше 7;
тогда выражение type < 7 равно true;
затем к true применяется унарный минус, получается 1;
значение 1 приводится к unsigned char, получается 0b0000'0001.

Впрочем, следующая операция & 1 приводит к одному и тому же результату. По этой счастливой случайности, код целиком работает так, как ожидает программист. Тем не менее, стоит поправить этот код. Попробуем понять, какое значение присваивается переменной v3 в зависимости от значения type.

Первый случай: переменная type больше или равна 7.

Тогда выражение type < 7 равно false.
К false применяется унарный минус, получается false.
False приводится к unsigned char, получается 0b0000'0000.
Побитовое "И" с 0 всегда даёт 0, поэтому в результате получаем 0.

Второй случай: переменная type меньше 7.

Как уже выяснили раньше, выражение (uint8_t) - (type < 7) равно 1.
В данном случае есть смысл вычислять выражение 0x43u >> type.
Для удобства запишем бинарное представление числа 0x43 = 0b0100'0011.
Нас интересует только младший бит, потому что к результату выражения 0x43u >> type применится побитовое "И" с 1.
Если type равен 0, 1 или 6, то младший бит будет равен 1, и результат всего выражения будет 1. Во всех других случаях получится 0.

Итого, если type равен 0, 1 или 6, то в переменную v3 будет записано значение 1, а во всех остальных случаях – значение 0. Стоит заменить сложное выражение на более простое и понятное (type == 0) || (type == 1) || (type == 6). Предлагаю следующий код:

typedef enum RenderTargetChannelType
    {
      kRenderTargetChannelTypeUNorm            = 0x00000000,
      kRenderTargetChannelTypeSNorm            = 0x00000001,
      kRenderTargetChannelTypeUInt             = 0x00000004,
      kRenderTargetChannelTypeSInt             = 0x00000005,
      kRenderTargetChannelTypeSrgb             = 0x00000006,
      kRenderTargetChannelTypeFloat            = 0x00000007,
    } RenderTargetChannelType;

void setDataFormat(DataFormat format)
{
  // ....
  int v3;
  RenderTargetChannelType  type;  // [rsp+4h] [rbp-3Ch]
  __int64                  v9;  // [rsp+10h] [rbp-30h]
  bool typeConvertable = format.getRenderTargetChannelType(&type);
  v2                   = type | kRenderTargetChannelTypeSNorm;
  v3                   = (type == kRenderTargetChannelTypeUNorm)
                      || (type == kRenderTargetChannelTypeSNorm)
                      || (type == kRenderTargetChannelTypeSrgb);
  // ....
}

Здесь я также заменил числа 0, 1 и 6 на соответствующие именованные значения перечисления и записал подвыражения в табличном виде.

Краевой случай в операторе перемещения

V794 The assignment operator should be protected from the case of 'this == &other'. VltShader.cpp 39

VltShaderConstData& VltShaderConstData::operator=(VltShaderConstData&& other)
{
  delete[] m_data;
  this->m_size = other.m_size;
  this->m_data = other.m_data;
  other.m_size = 0;
  other.m_data = nullptr;
  return *this;
}

Если при вызове такого оператора окажется, что 'this == &other', то все поля текущего объекта будут очищены и данные потеряются. Такое поведение некорректно, нужно добавить проверку. Исправленный код:

VltShaderConstData& VltShaderConstData::operator=(VltShaderConstData&& other)
{
  if (this == std::addressof(other))
  {
    return *this;
  }

  delete[] m_data;
  this->m_size = other.m_size;
  this->m_data = other.m_data;
  other.m_size = 0;
  other.m_data = nullptr;
  return *this;
}

Повторное присвоение как повод для рефакторинга

V1048 [CWE-1164] The 'retVal' variable was assigned the same value. Module.cpp 129

bool NativeModule::getSymbolInfo( /* .... */) const
{
  bool retVal = false;
  do
  {
    uint32_t modId = 0, libId = 0;
    if (modName == nullptr || libName == nullptr || nid == nullptr)
      {
        break;
      }
      if (!isEncodedSymbol(encSymbol))
      {
        *modName = "";
        *libName = "";
        *nid     = 0;
        retVal   = true;
        break;
      }
      retVal = decodeSymbol(encSymbol, &modId, &libId, nid);
      if (!retVal)
      {
        LOG_ERR("fail to decode encoded symbol");
        break;
      }
      retVal = getModNameFromId(modId, mods, modName);
      if (!retVal)
      {
        LOG_ERR("fail to get module name for symbol: %s in %s",
        encSymbol.c_str(), fileName.c_str());
        break;
      }
      retVal = getLibNameFromId(libId, libs, libName);
      if (!retVal)
      {
        LOG_ERR("fail to get library name");
        break;
      }
      retVal = true;                                                      // <=
    } while (false);
  return retVal;
}

В данном фрагменте кода есть повторное присваивание значения true в переменную retVal. Давайте разберемся, почему так происходит. Для этого рассмотрим все возможные модификации переменной retVal до присваивания, указанного анализатором.

Переменная retVal инициализируется значением false.
Если вызов функции isEncodedSymbol вернул false, то переменной retVal присваивается значение true и прерывается цикл do-while.
Переменной retVal присваивается результат вызова функции decodeSymbol. После этого если retVal == false, то цикл do-while прерывается.
То же самое происходит и с двумя вызовами функции getModNameFromId. Если любой из вызовов вернёт false, то цикл do-while прерывается.

Заметим, что если цикл do-while был досрочно прерван, то и указанное анализатором присваивание не выполнится. Это значит, подозрительное присваивание retVal == true будет выполнено, только если все рассмотренные выше вызовы функций вернули true. Поэтому переменная retVal уже равна true, и присваивание не имеет смысла.

А зачем вообще использовать конструкцию 'do ... while(false)'? Дело в том, что такая конструкция позволяет сделать ранний выход из функции с одним return. К функциям с одним return, в свою очередь, с большей вероятностью будет применена NRVO – named return value optimization. Эта оптимизация компилятора позволяет избежать лишнего копирования или перемещения возвращаемого объекта, конструируя его сразу на месте вызова функции. В данном случае функция возвращает легковесный тип bool, поэтому выигрыш от NRVO будет незначительным. Кроме того, современные компиляторы умеют применять NRVO и к функциям с несколькими return, если во всех return возвращается один и тот же объект.

Метод getSymbolInfo не содержит ошибки и работает так, как задумал программист. Однако стоит провести рефакторинг метода getSymbolInfo и убрать цикл do-while вместе с переменной retVal. Предлагаю следующий код:

bool NativeModule::getSymbolInfo( /* .... */) const
{
  uint32_t modId = 0, libId = 0;
  if (modName == nullptr || libName == nullptr || nid == nullptr)
  {
    return false;
  }

  if (!isEncodedSymbol(encSymbol))
  {
    *modName = "";
    *libName = "";
    *nid     = 0;
    return true;
  }

  if (!decodeSymbol(encSymbol, &modId, &libId, nid))
  {
    LOG_ERR("fail to decode encoded symbol");
    return false;
  }

  if (!getModNameFromId(modId, mods, modName))
  {
    LOG_ERR("fail to get module name for symbol: %s in %s",
    encSymbol.c_str(), fileName.c_str());
    return false;
  }

  if (!getLibNameFromId(libId, libs, libName))
  {
    LOG_ERR("fail to get library name");
    return false;
  }

  return true;
}

Здесь я сделал следующее:

убрал цикл do-while и лишнюю переменную retVal;
каждую проверку переменной retVal заменил на проверку результата вызова соответствующей функции;
каждый break цикла do-while заменил на возврат нужного значения true / false. Какое именно значение вернуть, знаем из анализа переменной retVal, который провели ранее.

На мой взгляд, такой код проще читать и поддерживать.

Заключение

Конечно, это не все ошибки и недочёты, которые мы нашли в GPCS4. Некоторые случаи было довольно сложно описать, поэтому я не включил их в статью.

Мы желаем разработчикам GPCS4 успехов в дальнейшем развитии эмулятора и рекомендуем проверить текущую версию проекта анализатором PVS-Studio. Для этого достаточно скачать дистрибутив анализатора и запросить бесплатную лицензию для Open Source проектов. Если вас заинтересовал статический анализ в целом и PVS-Studio в частности – самое время попробовать. Вы можете проверить GPCS4 вслед за нами, а можете проверить свой собственный проект:) Спасибо за внимание!

#Cpp #GameDev