To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (an extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Checking the GPCS4 emulator: will we ev…

Checking the GPCS4 emulator: will we ever be able to play "Bloodborne" on PC?

Jun 16 2022

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.

0955_GPCS4/image1.png

About the project

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.

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

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.

The pointer is checked after its use

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

Double assignment

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.

Buffer overflow

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.

Learning to count to seven, or another buffer overflow

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:

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

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;
  // ....
}

Incorrect flag usage

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

Extra check

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.

One more condition that is always true

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.

Copy paste error

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.

Why is it better to avoid complex expressions?

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:

  • let the type variable be less than 7;
  • then the type < 7 expression is true;
  • a unary minus is applied to true, the result is -1;
  • the -1 value is converted to an unsigned char, the result is 0b1111'1111.

However, that's what actually happens:

  • let the type variable be less than 7;
  • then the type < 7 expression is true;
  • a unary minus is applied to true, the result is 1;
  • the 1 value is converted to an unsigned char, the result is 0b0000'0001.

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.

  • Then the type < 7 expression is false;
  • A unary minus is applied to false, the result is false.
  • False is converted to unsigned char, the result is 0b0000'0000.
  • A bitwise "AND" with 0 always gives 0, so we get 0 as a result.

The second case: the type variable is less than 7.

  • As we found out earlier, the (uint8_t) is (type < 7) expression equals 1.
  • In this case, it makes sense to calculate the 0x43u >> type expression.
  • For convenience, let's write the binary representation of the number the following way: 0x43 = 0b0100'0011.
  • We are only interested in the least significant bit, because the bitwise "AND" with 1 will be applied to the result of the 0x43u >> type expression.
  • If type equals 0, 1, or 6, the least significant bit will be 1, and the result of the entire expression will be 1. In all other cases, the expression result will be 0.

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.

Corner case in move operator

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

Repeated assignment as a reason to refactor

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.

  • The retVal variable is initialized to false.
  • If the isEncodedSymbol function call returned false, the true value is assigned to retVal and the do-while loop is interrupted.
  • The result of the decodeSymbol function call is assigned to the retVal variable. After that, if retVal == false, the do-while loop is interrupted.
  • The same thing happens with two calls of the getModNameFromId function. If any of the calls returns false, the do-while loop is interrupted.

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:

  • removed the do-while loop and the extra retVal variable;
  • replaced each retVal variable check by a check of the result of the corresponding function call;
  • replaced each break of the do-while loop by the corresponding return statement — true / false. We know which value to return from the analysis of the retVal variable we did earlier.

In my opinion, such code is easier to read and maintain.

Conclusion

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!

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept