An emulator is an application that enables a computer with one operating system to run programs designed for a completely different operating system. Today we talk about GPCS4 — the emulator designed to run PS4 games on PC. Recently, GPCS4 announced their first release, so we decided to check the project. Let's see what errors PVS-Studio managed to find in the source code of the emulator.
GPCS4 is a PlayStation 4 emulator written in C and C++.
Initially, the author of the project intended to investigate the PS4 architecture. However, the project has evolved rapidly, and in early 2020, the developers of GPCS4 managed to run a game on their emulator — We are Doomed. It was the first successful launch of a PS4 game on PC. The game is far from perfect though, it runs at very low FPS and has graphical glitches. Nevertheless, the developer of the project is full of enthusiasm and continues to enhance the emulator.
The first release of GPCS4 took place at the end of April 2022. I downloaded and checked the project's v0.1.0. Actually, at the time of publication of this article, v0.2.1 has already been released — the project is developing rapidly. Let's move on to the errors and defects that the PVS-Studio analyzer managed to find in the first release of the GPCS4 project.
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;
}In this code fragment, the break statement is missing in the SCE_AUDIO_OUT_PARAM_FORMAT_FLOAT_MONO case statement. As a result, the number of channels will be set incorrectly.
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;
}In the fragment above, the m_moduleData pointer is first dereferenced, and then compared with nullptr in the do-while loop.
Attentive readers might object: "It maybe that a valid pointer is passed to function. And then in the do-while loop, this pointer is modified and can become a null pointer. So there is no mistake here." This is not the case. Firstly, due to the while (false) condition, the loop is iterated exactly once. Secondly, the m_moduleData pointer is not modified.
Another objection may be that using a reference is safe. After all, this reference will be used only if the pointer is valid. But no, this code invokes undefined behavior. It's an error. Most likely you need to do a pointer check before dereferencing it:
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;
}As you can guess from the LOG_SCE_DUMMY_IMPL name, the implementation of the sceKernelGetDirectMemoryType method will be changing. Still, two assignments to the same memoryType address looks strange. This may have been the result of a failed code merge.
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;
}Sometimes one code line triggers several PVS-Studio diagnostics. The following example is one of those cases. In this code fragment, an incorrect value is passed to the memset function as the third argument. The sizeof(param->reserved) expression will return the size of the param->reserved array. Multiplication by sizeof(uint32_t) will increase this value by 4 times, and the value will be incorrect. So the memset call will result in an overrun of the param->reserved array. You need to remove the extra multiplication:
int PS4API sceGnmSetPsShader350( /*....*/ )
{
// ....
memset(param->reserved, 0, sizeof(param->reserved));
return SCE_OK;
}In total, the analyzer detected 20 such overflows. Let me show another example:
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;
}In this code fragment, the initParam->reserved array goes out of bounds.
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;
// ....
}The analyzer warns that an overflow of the dynamicStates array may occur. There are 4 checks in this code fragment:
Each of these checks is a check of one of the independent flags. For example, the check of if (state.useDynamicDepthBias()):
bool useDynamicDepthBias() const
{
return rs.depthBiasEnable();
}
VkBool32 depthBiasEnable() const
{
return VkBool32(m_depthBiasEnable);
}It turns out that all these 4 checks can be true at the same time. Then 7 lines of the 'dynamicStates[dynamicStateCount++] =....' kind will be executed. On the seventh such line, there will be a call to dynamicStates[6]. It's an array index out of bounds.
To fix it, you need to allocate memory for 7 elements:
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;
}The GetProtectFlag function converts a flag with file access permission from one format to another. However, the function does this incorrectly. The developer did not take into account that the value of VMPF_NOACCESS is zero. Because of this, the if (nOldFlag & VMPF_NOACCESS) condition is always false and the function will never return the PAGE_NOACCESS value.
In addition, the GetProtectFlag function incorrectly converts not only the VMPF_NOACCESS flag, but also other flags. For example, the VMPF_CPU_EXEC flag will be converted to the PAGE_EXECUTE_READWRITE flag.
When I was thinking how to fix this issue, my first thought was to write something like this:
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;
}However, in this case, this approach does not work. The thing is, PAGE_NOACCESS, PAGE_READONLY and other flags are Windows flags and they have their own specifics. For example, there is no PAGE_WRITE flag among them. It is assumed that if there are write permissions, then at least there are also read permissions. For the same reasons, there is no PAGE_EXECUTE_WRITE flag.
In addition, the bitwise "OR" with two Windows flags does not result in a flag that corresponds to the sum of the permissions: PAGE_READONLY | PAGE_EXECUTE != PAGE_EXECUTE_READ. Therefore, you need to iterate through all possible flag combinations:
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);
}
// ....
}
// ....
}The retAddress pointer is checked twice in the code fragment above. First, if (!retAddress) is checked. If the pointer is null, execution proceeds to the next iteration of the while loop. Otherwise, the retAddress pointer is not null. So the second if (retAddress) check is always true, and it can be removed.
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;
}In this code fragment, the analyzer found the if (pipeConfig == kPipeConfigP16) condition that is always true. Let's figure out why this is so.
If the getPipeConfig function call returns a value that doesn't equal kPipeConfigP16, the first condition will be true and the program execution will not proceed to the check of if (pipeConfig == kPipeConfigP16).
It turns out that the second check of this variable is either not performed, or is always true. But do not rush and remove it. Maybe the first condition was added temporarily and will be removed in the future.
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;
// ....
}
}Here come the copy-paste errors. In this code snippet, the same newArrayMode == Gnm::kArrayMode2dTiledThin check is written twice.
It's hard to say exactly how to fix this. Most likely, the second check should be somewhat different. Or maybe it is redundant and can be removed.
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; // <=
// ....
}It looks like the programmer was expecting the following behaviour during the expression calculation:
However, that's what actually happens:
Although, the following & 1 operation leads to the same result. By this happy coincidence, the whole code works as the developer intends. However, it's better to correct this code. Depending on the type value, let's guess what value is assigned to the v3 variable.
The first case: the type variable is greater than or equal to 7.
The second case: the type variable is less than 7.
To conclude, if type is 0, 1 or 6, the value 1 is written to the v3 variable. In all other cases, the value 0 is written to the v3 variable. It is worth replacing a complex expression with a simpler and more understandable one — (type == 0) || (type == 1) || (type == 6). Let me suggest the following code:
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);
// ....
}I also replaced the 0, 1 and 6 numbers with the corresponding named enumeration values and wrote the subexpressions in a table form.
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;
}If this operator is called and 'this == &other', all fields of the current object will be cleared and data will be lost. This behavior is incorrect, the check should be added. Fixed code:
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;
}In this code fragment, the true value is assigned twice to the retVal variable. Let's figure out why this is happening. First, let's view all possible modifications to the variable retVal prior to the assignment indicated by the analyzer.
Note that if the do-while loop was prematurely interrupted, the assignment indicated by the analyzer won't be executed. This means that the suspicious retVal == true assignment will only be executed if all the function calls discussed above have returned true. Therefore, the retVal variable is already true, and the assignment does not make sense.
And why use the 'do ... while(false)' construct at all? The thing is, this construct allows to make an early exit from the function with a single return. For functions with a single return, in turn, named return value optimization — NRVO – is more likely to be applied. This compiler optimization avoids unnecessary copying or moving of the return object. This is done by constructing the object directly at the function call location. In this case, the function returns the lightweight bool type, so the gain from NRVO is minor. In addition, modern compilers are able to apply NRVO to functions with multiple return statements, if the same object is returned in all return statements.
The GetSymbolInfo method does not contain errors and works as the programmer intended. However, it's better to refactor the GetSymbolInfo method and remove the do-while loop with the retVal variable. Let me suggest the following code:
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;
}I did the following:
In my opinion, such code is easier to read and maintain.
Of course, these are not all the errors and defects that we found in GPCS4. Some cases were quite difficult to describe, so I did not include them in the article.
We wish GPCS4 developers success in their further development of the emulator, and recommend checking the latest version of the project with PVS-Studio analyzer. You can just download the analyzer distribution and request a free license for Open Source projects. If you are interested in static analysis in general and PVS-Studio in particular, it's time to try it. You can also check GPCS4, or you can check your own project :) Thank you for your attention!