# Tonight, ShadPS4 joins the hunt: uncovering bugs in the most popular PS4 emulator

August 20 marked the 136th anniversary of Howard Phillips Lovecraft's birth, the man who created his own genre, Lovecraftian horror\. Plenty of people have tried to copy or build on his ideas\. Only a few have actually pulled it off\.

![1405_ShadPS4/image1.png](https://import.viva64.com/docx/blog/1405_ShadPS4/image1.png)

Bloodborne is a good example\. Its world draws on Lovecraft's work but stands on its own\. For years, gamers could only dream of playing it on PC\. Now that's finally possible, thanks to today's hero, the ShadPS4 emulator\.

## Welcome to the PC, good hunter\. What bugs in ShadPS4 do you desire to hunt?

We checked ShadPS4's own code only, with no third\-party libraries involved\. The analysis was run on commit [24e68ba](https://github.com/shadps4-emu/shadPS4/commit/24e68bafdea0bb89a909a691903e1e204210ff93) from the main branch\. The findings in this article come from the High and Medium severity levels of the General Analysis group\. The comments starting with `//!` are the author's\.


> The commit links are there to make it easier to verify the findings and fix the issues\\\. They're not meant to discredit or criticize the people who wrote the code\\\. I'm a big fan of both Lovecraft and Miyazaki's games, so a personal thank\\\-you to everyone who contributes to the project\\\.

Unlike my [previous LLVM analysis](https://pvs-studio.com/en/blog/posts/cpp/1318/), there won't be any weird merge artifacts here, such as two implementations getting mixed together\. A hunter is never alone, and yet there are so few of them\. May the hunt begin\. Oh, I can't wait\.\.\. hee hee\.\.\.

## Results of the check

**Snippet 1\. Fear your blindness**

The PVS\-Studio warning: [V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The `3` index is pointing beyond array bound\. input\_handler\.h 397

```cpp
class InputBinding {
public:
  InputID keys[3];
  InputBinding(....) {
    ....
    if (k1 <= k2 && k1 <= k3) {
      ....
    } else if (k2 <= k1 && k2 <= k3) {
      ....
    } else {
      keys[0] = k3;
      if (k1 <= k2) {
        keys[1] = k1;
        keys[2] = k2;
      } else {
        keys[1] = k2;
        keys[3] = k1;
      }
    }
  }
}
```

A textbook buffer overflow\. The entire file was added in a massive [commit](https://github.com/shadps4-emu/shadPS4/commit/c4bfaa60312c6e224c26eca9a00b7b573253aa42) packed with all kinds of new features, so it's easy to see how this could have slipped through\.

As it turned out, while this article was being written, the bug was [found and fixed](https://github.com/shadps4-emu/shadPS4/commit/500f413735595a8c6d0c2dac5ad93568aeb04c1b) by changing `keys[3] = k1` to `keys[2] = k1`\. Still, it had managed to survive in the project for a year and a half\.

**Snippet 2\. No mercy for "Liar"**

The PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression `m_streams.size() >= 0` is always true\. Unsigned type value is always \>\= 0\. avplayer\_source\.cpp 132

```cpp
class AvPlayerSource{
  ....
  std::vector<Stream> m_streams;
}

bool AvPlayerSource::FindStreams() {
  ....
  return m_streams.size() >= 0;
}
```

Let's see if there's actually a bug here\. First of all, `m_streams` has always been a vector, so its `size` has always been unsigned\.

The code was changed in this [commit](https://github.com/shadps4-emu/shadPS4/commit/1ce5c2b5e6051c2706922d7e180e1f71c60d43f3): the `AvPlayerSource::FindStreams` function replaced `AvPlayerSource::HasStreams`, and also took over some code from `AvPlayerSource::Init`\.

```cpp
bool AvPlayerSource::HasStreams() {
  return m_streams.size() >= 0;
}
```

If we dig [even deeper](https://github.com/shadps4-emu/shadPS4/commit/0f9b6890581e6ac987d88162fbfb899cbc65ec5d), `AvPlayerSource::HasStreams` also had a predecessor:

```cpp
bool AvPlayerSource::FindStreamInfo() {
  if (m_avformat_context == nullptr) {
    LOG_ERROR(Lib_AvPlayer, "Could not find stream info. NULL context.");
    return false;
  }
  if (m_avformat_context->nb_streams > 0) {
    return true;

  }
  return avformat_find_stream_info(m_avformat_context.get(), nullptr) == 0;
}
```

`m_avformat_context` is a smart pointer to the `AVFormatContext` type, and the `nb_streams` field is declared as follows:

```cpp
unsigned int nb_streams;
```

As we can see, things were a little less tautological back then\.

If we look for where `AvPlayerSource::FindStreams` is used, we find it only in `AvPlayerState::ProcessEvent`:

```cpp
void AvPlayerState::ProcessEvent() {
  ....
  case AvEventType::AddSource: {
    std::shared_lock lock(m_source_mutex);
    if (m_up_source->FindStreams()) { //! Previously HasStreams(), 
                                      //! And before FindStreamInfo()
      SetState(AvState::Ready);
      OnPlaybackStateChanged(AvState::Ready);
    } else {
      OnWarning(ORBIS_AVPLAYER_ERROR_NOT_SUPPORTED);
      SetState(AvState::Error);
    }
    break;
  }
  ....
}
```

So, everything points to a simple typo\. The developer most likely used `>=` instead of `>` when creating `AvPlayerSource::HasStreams`, and then copied the same condition into the `AvPlayerSource::FindStreams`\.

**Snippet 3\. Let us cleanse these foul streets**

The PVS\-Studio warnings:

[V568](https://pvs-studio.com/en/docs/warnings/v568/) It's odd that the argument of sizeof\(\) operator is the `sizeof (int) * 512` expression\. aio\.cpp 318

[V1086](https://pvs-studio.com/en/docs/warnings/v1086/) A call of the `memset` function will lead to underflow of the buffer `id_state`\. aio\.cpp 318

```cpp
namespace Libraries::Kernel {
#define MAX_QUEUE 512
static s32* id_state;
static s32 id_index;
....
}

void RegisterAio(Core::Loader::SymbolsResolver* sym) {
  id_index = 1;
  id_state = (int*)malloc(sizeof(int) * MAX_QUEUE);
  memset(id_state, 0, sizeof(sizeof(int) * MAX_QUEUE));
  ....
}
```

The entire file was added in a [single commit](https://github.com/shadps4-emu/shadPS4/commit/4695aaa8306158e94df8292106bdc3889976dd1e)\.

The expression `sizeof(int) * 512` has type `size_t`, so `sizeof(sizeof(int) * MAX_QUEUE))` is the size of `size_t`\. Since `size_t` is usually 4 or 8 bytes, only part of the array will actually be zeroed out\.

It's pretty safe to assume that the outer `sizeof` was added by mistake and that the intention was to zero out the entire buffer\. It's hard to say for sure, though, because the array elements are only read by the `sceKernelAio***` family of functions, where they're stored and called as pointers\. 

Hopefully, the hunters out there will take a closer look and either clean this whole thing up or leave a clear explanation of why it's actually needed\.

**Snippet 4\. Beware of "Attack from behind"**

The PVS\-Studio warning: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The `chunkIds` pointer was utilized before it was verified against nullptr\. Check lines: 173, 179\.

```cpp
s32 PS4_SYSV_ABI scePlayGoGetProgress(...., 
                  const OrbisPlayGoChunkId* chunkIds, ....)
{
  LOG_DEBUG(Lib_PlayGo, "called handle = {}, chunkIds = {},
            numberOfEntries = {}", handle,
           *chunkIds, numberOfEntries);
  if (handle != PlaygoHandle) {
    return ORBIS_PLAYGO_ERROR_BAD_HANDLE;
  }
  if (chunkIds == nullptr || outProgress == nullptr) {
    return ORBIS_PLAYGO_ERROR_BAD_POINTER;
  }
  ....
}
```

The `LOG_DEBUG` macro is defined in the `log.h` file\. After the macro is expanded, the code looks like this:

```cpp
do {
  if (auto logger = Common::Log::ALL_LOGGERS[Common::Log::Class::Lib_PlayGo]){
    logger->log(
      spdlog::level::debug,
      "[{}] <{}> ({}) {}:{} {}: "
      "called handle = {}, chunkIds = {}, numberOfEntries = {}",
      Common::Log::Class::Lib_PlayGo, 
      Common::Log::to_string_view(spdlog::level::debug),
      Common::GetCurrentThreadName(),
      spdlog::source_loc::basename(
      "path to playgo.cpp"),
      174, std::string_view(__func__) == "operator()" ? "lambda" : __func__,
      handle,
      *chunkIds, numberOfEntries);
  }
} while (false);
```

As we can see, when `chunkIds == nullptr`, we get a classic undefined behavior\. The only thing that could save us here is having no logger for `Common::Log::Class::Lib_PlayGo` at that point\. Unfortunately, all loggers are created in the `main` function at startup and remain active for the rest of the application's lifetime\.

Even though this is only debug logging, the call to `logger::log`, and therefore the potential undefined behavior, still happens in a release build\. The unnecessary log messages are filtered out inside the logger\.

The macro has been there all along, with some variations in its form but with the dereference present in every version\. The code that follows it was added [later](https://github.com/shadps4-emu/shadPS4/commit/516a3e71049c5df38d052489b402e9012e6069fa)\.

Judging by the commit message, the developer was expanding PlayGo support, the system that lets you start hunting before the whole game has finished downloading\. The function was no longer just a placeholder and now had the necessary checks\. The unconditional dereference of `chunkIds` however, seems to have slipped through\.



There are several obvious ways to fix this, depending on the logging style used in the project\. One option would be:

```cpp

LOG_DEBUG(Lib_PlayGo, "called handle = {}, chunkIds = {}, 
                       numberOfEntries = {}", handle, 
                       chunkIds ? *chunkIds : static_cast<s32>(-1), 
                       numberOfEntries);
```

**Snippet 5\. Oh, Yourself, please, carry on in my stead**

The PVS\-Studio warning: [V570](https://pvs-studio.com/en/docs/warnings/v570/) The same value is assigned twice to the `inComment` variable\. text\_editor\.cpp 2109

```cpp
void TextEditor::ColorizeInternal() 
{
  ....
  if (mCheckComments) {
  ....
    while (currentLine < endLine || currentIndex < endIndex) {
      .... 
     //!~50 dense lines of code in which
     //!commentStartLine and commentStartIndex may change
      .... 
      bool inComment =
          (commentStartLine < currentLine ||
          (commentStartLine == currentLine && 
              commentStartIndex <= currentIndex));
       .....
         inComment = inComment =
            (commentStartLine < currentLine || 
            (commentStartLine == currentLine && 
               commentStartIndex <= currentIndex));
       ....
    }
  }
}
```

The entire thing was added in a single, massive [commit](https://github.com/shadps4-emu/shadPS4/commit/cf2e617f08df118494dde8845e00b0fe4efc5134)\.

Obviously, assigning `inComment` to itself makes no sense here\. But how did it get there?

My guess is that the author wrote the `inComment` declaration, then copied the whole thing, initializer and all, from the line above and pasted it below without noticing the mistake\.

Another possibility is that `=` was supposed to be `==` or `!=`, although neither really fits the logic of the algorithm\. Of course, the final word here belongs to the developer or one of their fellow hunters\.

**Snippet 6\. Treat "The unseen" with care**

The PVS\-Studio warning: [V634](https://pvs-studio.com/en/docs/warnings/v634/) The priority of the `*` operation is higher than that of the `<<` operation\. It's possible that parentheses should be used in the expression\. liverpool\_to\_vk\.cpp 759

```cpp
// Table 8.13 Data and Image Formats 
//[Sea Islands Series Instruction Set Architecture]
//All values are under 64
static const size_t amd_gpu_data_format_bit_size = 6;
//All values are under 16
static const size_t amd_gpu_number_format_bit_size = 4;

static auto surface_format_table = []() constexpr {
  std::array<vk::Format, 
      1 << amd_gpu_data_format_bit_size * 1 << amd_gpu_number_format_bit_size>
    result;
  for (auto& entry : result) {
    entry = vk::Format::eUndefined;
  }
  for (const auto& supported_format : SurfaceFormats()) {
    result[GetSurfaceFormatTableIndex(supported_format.data_format,
                                      supported_format.number_format)] =
        supported_format.vk_format;
  }
  return result;
}();
```

The line

```cpp
1 << amd_gpu_data_format_bit_size * 1 << amd_gpu_number_format_bit_size
```

is equivalent to

```cpp
1 << 6 * 1 << 4
```

Because of operator precedence, this is interpreted as 

```cpp
(1 << (6 * 1)) << 4
```

which gives us 1024\.

According to `GetSurfaceFormatTableIndex` function, the maximum index in `result` is expected to be just under 1024:

```cpp
static size_t GetSurfaceFormatTableIndex(AmdGpu::DataFormat data_format,
                                       AmdGpu::NumberFormat num_format) {
  DEBUG_ASSERT(u32(data_format) < 1 << amd_gpu_data_format_bit_size);
  DEBUG_ASSERT(u32(num_format) < 1 << amd_gpu_number_format_bit_size);
  size_t result = static_cast<size_t>(num_format) |
                (static_cast<size_t>(data_format) << 
                 amd_gpu_number_format_bit_size);
  return result;
}
```

This is consistent with the [AMD documentation](https://www.techpowerup.com/gpu-specs/docs/amd-sea-islands-isa.pdf) mentioned in the comment: 64 data formats and 14 ways for shaders to interpret them\.

But the `* 1` is puzzling\. It looks like the intended expression might have been:

```cpp
(1 << amd_gpu_data_format_bit_size) * ( 1 << amd_gpu_number_format_bit_size)
```

That also gives 1024\. So, the operator precedence may have been overlooked, but the expression still happens to produce the correct result\.

All the functions and constants were added in a single [commit](https://github.com/shadps4-emu/shadPS4/commit/6862c9aad77c2df28486d1ec86204a044a96f481), so we can rule out any coordination issues\.

In the end, this doesn't seem to be a bug\. Still, the code is easy to misread, and some unsuspecting developer may one day fall prey to the horrors of operator precedence\.

**Snippet 7\. "Strong foe" waits ahead but "don't give up"**

The PVS\-Studio warning: [V579](https://pvs-studio.com/en/docs/warnings/v579/) The ZydisDecoderDecodeFull function receives the pointer and its size as arguments\. It is possibly a mistake\. Inspect the third argument\. decoder\.cpp 29

Previously, the file was named `Disassembler.cpp`:

```cpp
void DecoderImpl::printInstruction(void* code, u64 address) {
  ZydisDecodedInstruction instruction;
  ZydisDecodedOperand operands[ZYDIS_MAX_OPERAND_COUNT_VISIBLE];
  ZyanStatus status =
    ZydisDecoderDecodeFull(&m_decoder, code, sizeof(code), 
                           &instruction, operands);
  if (!ZYAN_SUCCESS(status)) {
    fmt::print("decode instruction failed at {}\n", fmt::ptr(code));
  } else {
    printInst(instruction, operands, address);
  }
}
```

The `ZydisDecoderDecodeFull` function has an explanatory comment:

```cpp
/**
 * @param buffer  A pointer to the input buffer.
 * @param length  The length of the input buffer. 
 *                Note that this can be bigger than the
 *                actual size of the instruction -- you don`t have to know
 *                the size up front. This length is merely used to prevent
 *                 Zydis from doing out-of-bounds reads on your buffer.
**/
```

Passing a pointer to some buffer along with the size of the pointer as the buffer length looks rather suspicious\.

The `length` parameter takes quite a winding path through the code, but eventually ends up in a structure field: 

```cpp
  state.buffer_len = length;
```

That field is then used to limit instruction parsing, for example:

```cpp
static ZyanStatus ZydisInputPeek(ZydisDecoderState* state,
    ZydisDecodedInstruction* instruction, ZyanU8* value)
{
  ....
  if (state->buffer_len > 0)
  {
      *value = state->buffer[0];
      return ZYAN_STATUS_SUCCESS;
  }
  return ZYDIS_STATUS_NO_MORE_DATA;
}

static ZyanStatus ZydisInputNext(ZydisDecoderState* state,
    ZydisDecodedInstruction* instruction, ZyanU8* value)
{
  ....
  if (state->buffer_len > 0)
  {
    *value = state->buffer++[0];
    ++instruction->length;
    --state->buffer_len;
    return ZYAN_STATUS_SUCCESS;
  }
  return ZYDIS_STATUS_NO_MORE_DATA;
}
```

[Previously](https://github.com/shadps4-emu/shadPS4/commit/66ea5329d21e3b10a3f77ad8a08c602b20978cc4), a constant was used instead of `sizeof(code)`:

```cpp
#define ZYDIS_MAX_INSTRUCTION_LENGTH    15
```

That is the maximum instruction length in bytes on x86\.

For the current code to be correct, there would have to be some guarantee that no instruction can be longer than the size of a pointer\. I couldn't find one\.

There is also some indirect evidence that this is a mistake in an example from the [Zydis](https://github.com/zyantific/zydis) project, the disassembly framework used by the emulator:

```cpp
ZyanU8 data[] = 
  { 
     0x51, 0x8D, 0x45, 0xFF, 0x50, 0xFF, 0x75, 0x0C, 0xFF, 0x75, 
     0x08, 0xFF, 0x15, 0xA0, 0xA5, 0x48, 0x76, 0x85, 0xC0, 0x0F, 
     0x88, 0xFC, 0xDA, 0x02, 0x00 
  }; 
  
 // The runtime address (instruction pointer) was chosen
 // arbitrarily here in order to better
 // visualize relative addressing. In your actual program,
 // set this to e.g. the memory address 
 // that the code being disassembled was read from.
 ZyanU64 runtime_address = 0x007FFFFFFF400000;
  
 // Loop over the instructions in our buffer.
 ZyanUSize offset = 0; 
 ZydisDisassembledInstruction instruction;
 while (ZYAN_SUCCESS(ZydisDisassembleIntel(
     /* machine_mode:    */ ZYDIS_MACHINE_MODE_LONG_64,
     /* runtime_address: */ runtime_address,
     /* buffer:          */ data + offset,
     /* length:          */ sizeof(data) - offset,
     /* instruction:     */ &instruction
 ))) { 
     printf("%016" PRIX64 "  %s\n", runtime_address, instruction.text);
     offset += instruction.info.length; 
     runtime_address += instruction.info.length; 
 } 
 
output:
 
007FFFFFFF400000   push rcx
007FFFFFFF400001   lea eax, [rbp-0x01]
007FFFFFFF400004   push rax
007FFFFFFF400005   push qword ptr [rbp+0x0C]
007FFFFFFF400008   push qword ptr [rbp+0x08]
007FFFFFFF40000B   call [0x008000007588A5B1]
007FFFFFFF400011   test eax, eax
007FFFFFFF400013   js 0x007FFFFFFF42DB15
```

Here, `sizeof` is used is used too, but this time on an array whose bounds are known at compile time, so it correctly gives the size of the buffer in bytes\.

There is also almost identical code in the emulator's history\. It was [removed](https://github.com/shadps4-emu/shadPS4/commit/af1845395012050a6dad4363318beab98b0a66d3#diff-f23b01d99f3fa2c5f6b3aff25df831df47692464ae64fb58a6f421d9ca3ad93cL171) in 2023:

```cpp
void Linker::LoadModuleToMemory(Module* m){
  ....
  auto* rt1 = reinterpret_cast<uint8_t*>
                      (m->elf.GetElfEntry() + m->base_virtual_addr);
  ZyanU64 runtime_address = m->elf.GetElfEntry() + m->base_virtual_addr;

  // Loop over the instructions in our buffer.
  ZyanUSize offset = 0;
  ZydisDisassembledInstruction instruction;
  while (ZYAN_SUCCESS(ZydisDisassembleIntel(
    /* machine_mode:    */ ZYDIS_MACHINE_MODE_LONG_64,
    /* runtime_address: */ runtime_address,
    /* buffer:          */ rt1 + offset,
    /* length:          */ sizeof(rt1) - offset,
    /* instruction:     */ &instruction
  ))) {
    fmt::print("{:#x}" PRIX64 "  {}\n", runtime_address, instruction.text);
    offset += instruction.info.length;
    runtime_address += instruction.info.length;
  }
  ....
}
```

The first thing that stands out is the exact same `sizeof(***) - offset` pattern for `length` as in Zydis example\. The pointer's size is also obtained in exactly the same way as in the current code\. That makes the case for this being a real error even stronger\.

There is more\. Another, now\-defunct [PS4 emulator](https://github.com/Inori/GPCS4) also used Zydis and called `ZydisDecoderDecodeFull` with an instruction length of 15:

```cpp
  ZyanStatus status = 
      ZydisDecoderDecodeFull(&m_decoder, code,
                             ZYDIS_MAX_INSTRUCTION_LENGTH,
                             &instruction, operands, 
                             ZYDIS_MAX_OPERAND_COUNT_VISIBLE,
                             ZYDIS_DFLAG_VISIBLE_OPERANDS_ONLY);
```

Still, to say for certain, we'd need to hear from the project's experienced developers\.

**Snippet 8\. Despicable "Metamorphosis" therefore fear "Moon"**

The PVS\-Studio warning: [V610](https://pvs-studio.com/en/docs/warnings/v610/) Undefined behavior\. Check the shift operator `<<`\. The right operand \(`(64 - systemLang - 1)` \= \[16\.\.63\]\) is greater than or equal to the length in bits of the promoted left operand\. playgo\.cpp 233

```cpp
int scePlayGoConvertLanguage(int systemLang) {
    if (systemLang >= 0 && systemLang < 48) {  //! systemLang: [0..47]
        return (1 << (64 - systemLang - 1));   //! 1 << [16..63]
    } else {
        return 0;
    }
}
```

The function was added [in its entirety](https://github.com/shadps4-emu/shadPS4/commit/516a3e71049c5df38d052489b402e9012e6069fa#diff-0d3262217859760627215e55b69b54f3ab84a555bf33df61e1a5465ed83615a8R212), along with the `scePlayGoInitialize` code below it\.

Since the emulator is currently built for the x86\-64 architecture, where `int` is 4 bytes, any `systemLang` value below 32 will cause an overflow and undefined behavior\.

But does that actually happen? Let's trace where the `systemLang` parameter comes from\.

Our function is called only from `scePlayGoInitialize`:

```cpp
s32 PS4_SYSV_ABI scePlayGoInitialize(OrbisPlayGoInitParams* param) {
  ....
  s32 system_lang = 0;
  sceSystemServiceParamGetInt(OrbisSystemServiceParamId::Lang, &system_lang);
  playgo->langMask = scePlayGoConvertLanguage(system_lang);

  return ORBIS_OK;
}
```

And `system_lang` gets its value from `sceSystemServiceParamGetInt`:

```cpp
s32 PS4_SYSV_ABI sceSystemServiceParamGetInt(
  OrbisSystemServiceParamId param_id,
  int* value
)
{
  // TODO this probably should be stored in config for UI configuration
  LOG_DEBUG(Lib_SystemService, "called param_id {}", u32(param_id));
  if (value == nullptr) {
    LOG_ERROR(Lib_SystemService, "value is null");
    return ORBIS_SYSTEM_SERVICE_ERROR_PARAMETER;
  }
  switch (param_id) {
  case OrbisSystemServiceParamId::Lang: {
    s32 lang = EmulatorSettings.GetConsoleLanguage();
    if (lang == 0x15 && g_sdk_version < Common::ElfInfo::FW_200) {
      lang = 0x12;
    }
    if (lang == 0x16 && g_sdk_version < Common::ElfInfo::FW_250) {
      lang = 2;
    }
    if ((lang >= 0x17 && lang <= 0x1a) && 
         g_sdk_version < Common::ElfInfo::FW_500) {
      lang = 0x12;
    }
    if ((lang >= 0x1b && lang <= 0x1d) && 
        g_sdk_version < Common::ElfInfo::FW_500) {
      lang = 1;
    }
    if (lang == 0x1e && g_sdk_version < Common::ElfInfo::FW_1000) {
      lang = 0x12;
    }
    *value = lang;
    break;
  }
  ....
  }
  return ORBIS_OK;
}
```

As we can see, small `system_lang` values are entirely possible\. In fact, they are the ones we are most likely to get\. According to the documentation, each supported language has a number from 0 to 47\. Japanese, for example, is 0, English is 1, and Russian is 8\. So, any game using Japanese, for example, is guaranteed to trigger the overflow\.

The conditions mentioned above also make more sense now\. For example, when the SDK version is old enough, standard French is used instead of Canadian French:

```cpp

  if (lang == 0x16 && g_sdk_version < Common::ElfInfo::FW_250)
    lang = 2;
```

There's more evidence that this is a bug\. The same function in [another PlayGo implementation](https://github.com/drakmor/pgo_stub/blob/main/library.h#L134) is virtually identical, but avoids the overflow:

```cpp
static inline ScePlayGoLanguageMask scePlayGoConvertLanguage(int32_t systemLang)
{
  return (systemLang >= 0 && systemLang < 48) ? 
         (1ULL << (64 - systemLang - 1))      : 
         0ULL;
}
```

**Snippet 9\. Time for "Hidden path"**

The PVS\-Studio warning: [V796](https://pvs-studio.com/en/docs/warnings/v796/) It is possible that `break` statement is missing in switch statement\. hull\_shader\_transform\.cpp 247

```cpp
void WalkUsersOfTessConstantHelper(IR::Use use, u32 inc,bool propagateError){
  IR::Inst* inst = use.user;
  switch (use.user->GetOpcode()) {
  case IR::Opcode::LoadSharedU32:
  case IR::Opcode::LoadSharedU64:
  case IR::Opcode::WriteSharedU32:
  case IR::Opcode::WriteSharedU64: {
    bool is_addr_operand = use.operand == 0;
    if (is_addr_operand) {
      u32 counter = inst->Flags<u32>();
      inst->SetFlags<u32>(counter + inc);
      ASSERT_MSG(!propagateError, 
          "LDS instruction {} accesses ambiguous attribute type",
           fmt::ptr(use.user));
      // Stop here
      return;
    }
  }
  case IR::Opcode::Phi: {
    auto it = phi_infos.find(use.user);
    ....
  }
  ....
}
```

Earlier in this branch, there was an unconditional exit:

```cpp
  switch (use.user->GetOpcode()) {
  case IR::Opcode::LoadSharedU32:
  case IR::Opcode::LoadSharedU64:
  case IR::Opcode::WriteSharedU32:
  case IR::Opcode::WriteSharedU64: {
    u32 counter = inst->Flags<u32>();
    inst->SetFlags<u32>(counter + inc);
    // Stop here
    return;
  }
```

The [commit](https://github.com/shadps4-emu/shadPS4/commit/11ee79a33348de7a55ab8d7b688144d5b8a0ce55) message says:

> Ignore when a user contributes to the wrong operand of an LDS inst, for
>
> example the data operand of WriteShared\* instead of the address operand\.
>
> This can mistakenly happen due to phi nodes\.

The commit says to ignore cases where a user contributes to the wrong operand\. That means we shouldn't fall through when `use.operand != 0`, and a `break` is indeed missing here\. 

If that's not the case, a little hint like `[[fallthrough]]` would have helped guide those of us who weren't blessed with the same insight\.

**Snippet 10\. Treat "Lever" with care or you must accept "Ignoring"**

The PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression `compare < 0` is always false\. Unsigned type value is never < 0\. np\_common\.cpp 49

```cpp
s32 PS4_SYSV_ABI sceNpCmpNpIdInOrder(OrbisNpId* np_id1, OrbisNpId* np_id2,
                                     u32* out_result) {
  ....
  // Compare data
  u32 compare =
    std::strncmp(np_id1->handle.data, np_id2->handle.data, 
                 ORBIS_NP_ONLINEID_MAX_LENGTH);
  if (compare < 0) {
    *out_result = -1;
      return ORBIS_OK;
  } else if (compare > 0) {
    *out_result = 1;
    return ORBIS_OK;
  }
  ....
}
```

Once again, the entire file was added in a [single commit](https://github.com/shadps4-emu/shadPS4/commit/62bbad62fc9bf23c176b8dd69e16cfef47c5e1f8) \(it had previously lived in a different directory\)\.

Let me briefly recap how `strncmp` works: 

```cpp
int strncmp( const char* lhs, const char* rhs, std::size_t count );
```

> The sign of the result is the sign of the difference between the values of the first pair of characters \(both interpreted as unsigned char\) that differ in the arrays being compared\.

It looks like the author simply got the type wrong, using an unsigned type where a signed one was intended\.

With our co\-op in one world wrapped up, let's take a quick look at another\. Besides the main repository, the ShadPS4 project has a fairly popular fork specifically tailored to Bloodborne: [diegolix29/shadPS4](https://github.com/diegolix29/shadPS4)\.

It has drifted a couple thousand commits away from upstream and, alongside the usual problems, has a few of its own\. 

**Snippet 11\. The sky and the cosmos are one**

The PVS\-Studio warning: [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting the 'deltaTime <\= 0\.0f \|\| deltaTime < 0\.0001f' expression\. The expression is excessive or contains a misprint\. controller\.cpp 201

```cpp
void GameController::CalculateOrientation(/*....*/
                           float deltaTime,
                           Libraries::Pad::OrbisFQuaternion& lastOrientation,
                           Libraries::Pad::OrbisFQuaternion& orientation) {
  constexpr float MAX_DELTA_TIME = 0.1f;
  if (deltaTime > MAX_DELTA_TIME) {
    deltaTime = MAX_DELTA_TIME;
  }

  if (deltaTime <= 0.0f || deltaTime < 0.0001f) {
    orientation = lastOrientation;
    return;
  }
  ....
}
```

The `MAX_DELTA_TIME` block and the `orientation` assignment came in a separate [commit](https://github.com/diegolix29/shadPS4/commit/a49a1decdc6176c989469002645b5b6ebdaf18c0)\.

At first glance, it looks like `deltaTime < 0.0001f` should actually be `deltaTime > 0.0001f`\. This is indirectly supported by a similar snippet in the upstream repository:

```cpp
  if (delta_time > 1.0f) {
    orientation = last_orientation;
    return;
  }
```

But then the `MAX_DELTA_TIME` block would make little sense\. So I guess it's time to dig a little deeper into the lore of controller orientation calculations\.

First, let's look at what `delta_time` actually is\. Surprisingly, it's the time elapsed since the last update, for example:

```cpp
const float delta_time = static_cast<float>
            (timestamp - m_last_orientation_update) / 1'000'000.f;
```

After looking into it, I realized that the upstream version with the `1.0f` constant is there to guard against emulator slowdowns and pauses that could otherwise make things go haywire\.

The fork takes a slightly different approach: in this case, it simply updates the orientation using a fixed delta of `0.1f`\. This means the questionable part is:

```cpp
  if (deltaTime <= 0.0f || deltaTime < 0.0001f) {
    orientation = lastOrientation;
    return;
  }
```

It seems to serve a different purpose: a paranoid safeguard against invalid data, since both negative and extremely small deltas can result in incorrect values\.

So, with a fair degree of confidence, we can say that the left side of the check `deltaTime <= 0.0f` is unnecessary\.

**Snippet 12\. Time for "jump"**

The PVS\-Studio warning: [V1082](https://pvs-studio.com/en/docs/warnings/v1082/) Function marked as 'noreturn' may return control\. This will result in undefined behavior\. ir\_emitter\.cpp 15

```cpp
[[noreturn]] void ThrowInvalidType(Type type,
             std::source_location loc = std::source_location::current()) {
  const std::string functionName = loc.function_name();
  const int lineNumber = loc.line();
  // UNREACHABLE_MSG("Invalid type = {}, functionName = {}, line = {}",
  //                  u32(type), functionName,
  //                  lineNumber);
}
```

The `UNREACHABLE_MSG` macro did indeed make the function `noreturn`: 

```cpp
#define UNREACHABLE_MSG(...)                                 \
  do {                                                       \
      LOG_CRITICAL(Debug, "Unreachable code!\n" __VA_ARGS__);\
      unreachable_impl();                                    \
  } while (0)

[[noreturn]] void unreachable_impl() {
  Common::Log::Stop();
  std::fflush(stdout);
  Crash();
  throw std::runtime_error("Unreachable code");
}
```

These lines were [commented out](https://github.com/diegolix29/shadPS4/commit/cf5825b8bd5a56174a29ec9cd00d85503ee4c60f) in a commit with the message "SOTC hacks" \(SOTC presumably stands for Shadow of the Colossus\)\.

This change isn't present in the upstream version, so the analyzer had nothing to complain about there\.

There's not much mystery here: the attribute was simply left behind\. According to the [C\+\+ standard](https://timsong-cpp.github.io/cppwp/n4950/dcl.attr.noreturn#2), this is undefined behavior, so `[[noreturn]]` should be removed\.

**Snippet 13\. Treat hunter with care and don't be fooled**

The PVS\-Studio warning: [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'serial \=\= "CUSA03014"' to the left and to the right of the '\|\|' operator\. storage\_image\_sync\.cpp 42

```cpp
void StorageImageSync::Sync(VideoCore::ImageId image_id) {
  const auto& serial = Common::ElfInfo::Instance().GameSerial();
  if (serial == "CUSA11227" || serial == "CUSA12982" || serial == "CUSA00093" ||
   serial == "CUSA03173" || serial == "CUSA00900" || serial == "CUSA00208" ||
   serial == "CUSA01363" || serial == "CUSA01322" || serial == "CUSA003027" ||
   serial == "CUSA00299" || serial == "CUSA00207" || serial == "CUSA03014" ||
   serial == "CUSA03023" || serial == "CUSA03014" || serial == "CUSA00900" ||
   serial == "CUSA00003" || serial == "CUSA01627" || serial == "CUSA01778" ||
   serial == "CUSA03388" || serial == "CUSA01589" || serial == "CUSA01760" ||
   serial == "CUSA07439" || serial == "CUSA07339" || serial == "CUSA08692" ||
   serial == "CUSA08495" || serial == "CUSA50617" || serial == "CUSA18723" ||
   serial == "CUSA28863" || serial == "CUSA00093" || serial == "CUSA00003") {
     return;
  }
  ....
}
```

As we can see, the comparison with `CUSA03014` appears twice, and both copies were added [in the same commit](https://github.com/diegolix29/shadPS4/commit/c60cc0c56d1d892c3ac1ceb077925f41c68d819e)\.

This file doesn't exist in the upstream repository at all, so there is no corresponding warning there\.

It's somewhat fitting that a `CUSA ID` is a five\-digit serial number assigned to PS4 games, while our duplicate, `CUSA03014`, belongs to _Bloodborne: The Old Hunters Edition_\.

`CUSA003027` also stands out visually: it's longer than all the other IDs, breaking up the neat sequence, and being six digits long, it doesn't make sense\.

Whether those redundant comparisons can simply be removed, or some of the IDs should be replaced with others, is something only the developer can say\.

**Snippet 14\. Remember key but fear Malformed thing**

The PVS\-Studio warning: [V766](https://pvs-studio.com/en/docs/warnings/v766/) An item with the same key 'vk::Format::eR8G8B8A8Srgb' has already been added\. host\_compatibility\.cpp 202

```cpp
/**
 * @brief The format compatibility class according to the Vulkan specification
 * @url
 * https://registry.khronos.org/vulkan/specs/
.* 1.3-extensions/html/vkspec.html#formats-compatibility-classes
 * @url
 * https://github.com/KhronosGroup/VulkanValidationLayers/
 * blob/d37c676f/layers/generated/vk_format_utils.cpp#L70-L812
**/
static const std::unordered_map<vk::Format,CompatibilityClass> FORMAT_TABLE ={
    ....
    {vk::Format::eR64G64Uint, CompatibilityClass::_128BIT},
    {vk::Format::eR64Sfloat, CompatibilityClass::_64BIT},
    {vk::Format::eR64Sint, CompatibilityClass::_64BIT},
    {vk::Format::eR64Uint, CompatibilityClass::_64BIT},
    {vk::Format::eR8G8B8A8Sint, CompatibilityClass::_32BIT},
    {vk::Format::eR8G8B8A8Snorm, CompatibilityClass::_32BIT},
    {vk::Format::eR8G8B8A8Srgb, CompatibilityClass::_32BIT},   // <= 32BIT
    {vk::Format::eR8G8B8A8Sscaled, CompatibilityClass::_32BIT},
    {vk::Format::eR8G8B8A8Uint, CompatibilityClass::_32BIT},
    {vk::Format::eR8G8B8A8Unorm, CompatibilityClass::_32BIT},
    {vk::Format::eR8G8B8A8Uscaled, CompatibilityClass::_32BIT},
    ....
    {vk::Format::eR8G8Uscaled, CompatibilityClass::_16BIT},
    {vk::Format::eR8Sint, CompatibilityClass::_8BIT},
    {vk::Format::eR8Snorm, CompatibilityClass::_8BIT},
    {vk::Format::eR8Srgb, CompatibilityClass::_8BIT},
    {vk::Format::eR8G8B8A8Srgb, CompatibilityClass::_8BIT},    // <= 8BIT
    {vk::Format::eR8Sscaled, CompatibilityClass::_8BIT},
    {vk::Format::eR8Uint, CompatibilityClass::_8BIT},
    ....
};
```

The key `eR8G8B8A8Srgb` appears twice, with two different values: `CompatibilityClass::_8BIT` and `CompatibilityClass::_32BIT`\. The C\+\+ standard does not specify which entry will take precedence\.

The link in the comment leads to the following entry:

```cpp
  {VK_FORMAT_R8G8B8A8_SRGB,
       {FORMAT_COMPATIBILITY_CLASS::_32BIT, ....} }
```

In other words, the `8BIT` value doesn't match the pattern and also seems unnecessary\. There is no corresponding entry between `VK_FORMAT_R8_SRGB` and `VK_FORMAT_R8_SSCALED` values in the reference map\. The same map is present upstream, but, unsurprisingly, it doesn't contain the second 8BIT option, which was [added](https://github.com/diegolix29/shadPS4/commit/a5b4a0c9314b179fd2b5ded73a238541eeac8962) separately later in the fork\. 

Only the developers can say for certain which of the two entries should be kept\.

## Will the hunt ever end?

You will be freed\.\.\. from this terrible hunter's dream\. 

Even with the power of C\+\+ at their disposal, developers are still human\. They can make mistakes, miss typos, and lose focus\. Luckily, there are tools to help them survive the hunt, including those we craft in our _Abandoned Old Workshop_\. One of them is PVS\-Studio, [which you can use for free on open\-source projects](https://pvs-studio.com/en/order/open-source-license/)\.

Even if your project isn't open source, you can still [try the analyzer for free](https://pvs-studio.com/en/pvs-studio/try-free/)\. You know that, right?

As usual, we opened GitHub issues for what the check found: [upstream](https://github.com/shadps4-emu/shadPS4/issues?q=is%3Aissue%20%22Issues%20found%20PVS%20studio%22), [fork](https://github.com/diegolix29/shadPS4/issues?q=is%3Aissue%20Issues%20found%20PVS%20studio)\.

Farewell good hunter, may you find your worth in the waking world\.