Verifica dell'emulatore GPCS4:potremo mai giocare a Bloodborne su PC?

Un emulatore è un'applicazione che consente a un computer con un sistema operativo di eseguire programmi progettati per un sistema operativo completamente diverso. Oggi parliamo di GPCS4, l'emulatore progettato per eseguire giochi PS4 su PC. Di recente, GPCS4 ha annunciato il loro primo rilascio, quindi abbiamo deciso di controllare il progetto. Vediamo quali errori è riuscito a trovare PVS-Studio nel codice sorgente dell'emulatore.

Informazioni sul progetto

GPCS4 è un emulatore PlayStation 4 scritto in C e C++.

Inizialmente, l'autore del progetto intendeva indagare sull'architettura PS4. Tuttavia, il progetto si è evoluto rapidamente e all'inizio del 2020 gli sviluppatori di GPCS4 sono riusciti a eseguire un gioco sul loro emulatore:We are Doomed. È stato il primo lancio di successo di un gioco per PS4 su PC. Il gioco è tutt'altro che perfetto, funziona a FPS molto bassi e presenta difetti grafici. Tuttavia, lo sviluppatore del progetto è pieno di entusiasmo e continua a migliorare l'emulatore.

La prima versione di GPCS4 è avvenuta alla fine di aprile 2022. Ho scaricato e verificato la v0.1.0 del progetto. In realtà, al momento della pubblicazione di questo articolo, la v0.2.1 è già stata rilasciata — il progetto si sta sviluppando rapidamente. Passiamo agli errori e ai difetti che l'analizzatore PVS-Studio è riuscito a trovare nella prima release del progetto GPCS4.

Pausa mancante

V796 [CWE-484] È possibile che nell'istruzione switch manchi l'istruzione 'break'. 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 questo frammento di codice, l'interruzione istruzione mancante in SCE_AUDIO_OUT_PARAM_FORMAT_FLOAT_MONO dichiarazione del caso. Di conseguenza, il numero di canali verrà impostato in modo errato.

Il puntatore viene controllato dopo il suo utilizzo

V595 Il puntatore 'm_moduleData' è stato utilizzato prima che fosse verificato rispetto a nullptr. Righe di controllo: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;
}

Nel frammento sopra, m_moduleData il puntatore viene prima dereferenziato e quindi confrontato con nullptr nel do-mentre ciclo.

I lettori attenti potrebbero obiettare:"Forse un puntatore valido viene passato alla funzione. E poi nel do-while loop, questo puntatore viene modificato e può diventare un puntatore nullo. Quindi non ci sono errori qui." Questo non è il caso. In primo luogo, a causa del while (falso) condizione, il ciclo viene ripetuto esattamente una volta. In secondo luogo, i m_moduleData il puntatore non viene modificato.

Un'altra obiezione potrebbe essere che l'utilizzo di un riferimento è sicuro. Dopotutto, questo riferimento verrà utilizzato solo se il puntatore è valido. Ma no, questo codice invoca un comportamento non definito. È un errore. Molto probabilmente è necessario eseguire un controllo del puntatore prima di dereferenziarlo:

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

Doppio incarico

V519 [CWE-563] Alla variabile '* memoryType' vengono assegnati valori due volte di seguito. Forse questo è un errore. Righe di controllo: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;
}

Come puoi intuire dal LOG_SCE_DUMMY_IMPL name, l'implementazione di sceKernelGetDirectMemoryType il metodo cambierà. Tuttavia, due assegnazioni allo stesso tipo di memoria l'indirizzo sembra strano. Questo potrebbe essere stato il risultato di un'unione di codice non riuscita.

Overflow del buffer

V512 [CWE-119] Un richiamo della funzione 'memset' provoca l'overflow del buffer 'param->reserved'. sce_gnm_draw.cpp 420

V531 [CWE-131] È strano che un operatore sizeof() venga moltiplicato per 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;
}

A volte una riga di codice attiva diversi sistemi diagnostici PVS-Studio. L'esempio seguente è uno di questi casi. In questo frammento di codice, viene passato un valore errato al memset funzione come terzo argomento. Il sizeof(param->riservato) expression restituirà la dimensione del param->riservato Vettore. Moltiplicazione per sizeof(uint32_t) aumenterà questo valore di 4 volte e il valore non sarà corretto. Quindi il memset call comporterà un superamento del param->riservato Vettore. Devi rimuovere la moltiplicazione extra:

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

In totale, l'analizzatore ha rilevato 20 di tali overflow. Lascia che ti mostri un altro esempio:

V512 [CWE-119] Un richiamo della funzione 'memset' provocherà un overflow del 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 questo frammento di codice, initParam->riservato l'array va fuori limite.

Imparare a contare fino a sette o un altro overflow del buffer

V557 [CWE-787] È possibile il sovraccarico dell'array. L'indice 'dynamicStateCount ++' punta oltre il limite dell'array. 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;
  // ....
}

L'analizzatore avverte che un overflow di dynamicStates potrebbe verificarsi una matrice. Ci sono 4 controlli in questo frammento di codice:

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

Ciascuno di questi controlli è un controllo di uno dei flag indipendenti. Ad esempio, il controllo di if (state.useDynamicDepthBias()) :

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

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

Si scopre che tutti questi 4 controlli possono essere veri contemporaneamente. Quindi 7 righe di 'dynamicStates[dynamicStateCount++] =....' genere sarà eseguito. Sulla settima riga, ci sarà una chiamata a dynamicStates[6] . È un indice di matrice fuori limite.

Per risolverlo, devi allocare memoria per 7 elementi:

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

Uso errato del flag

V547 [CWE-570] L'espressione 'nOldFlag &VMPF_NOACCESS' è sempre falsa. 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;
}

Il GetProtectFlag la funzione converte un flag con autorizzazione di accesso ai file da un formato all'altro. Tuttavia, la funzione esegue questa operazione in modo errato. Lo sviluppatore non ha tenuto conto del valore di VMPF_NOACCESS è zero. Per questo motivo, if (nOldFlag &VMPF_NOACCESS) la condizione è sempre falsa e la funzione non restituirà mai PAGE_NOACCESS valore.

Inoltre, il GetProtectFlag la funzione converte in modo errato non solo il VMPF_NOACCESS bandiera, ma anche altre bandiere. Ad esempio, il VMPF_CPU_EXEC flag verrà convertito in PAGE_EXECUTE_READWRITE bandiera.

Quando stavo pensando a come risolvere questo problema, il mio primo pensiero è stato scrivere qualcosa del genere:

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

Tuttavia, in questo caso, questo approccio non funziona. Il fatto è, PAGE_NOACCESS , PAGE_READONLY e altri flag sono flag di Windows e hanno le loro specifiche. Ad esempio, non esiste PAGE_WRITE bandiera tra di loro. Si presume che se ci sono autorizzazioni di scrittura, almeno ci siano anche autorizzazioni di lettura. Per gli stessi motivi, non esiste PAGE_EXECUTE_WRITE bandiera.

Inoltre, la "OR" bit per bit con due flag di Windows non genera un flag che corrisponda alla somma delle autorizzazioni:PAGE_READONLY | PAGE_EXECUTE !=PAGE_EXECUTE_READ . Pertanto, è necessario scorrere tutte le possibili combinazioni di flag:

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

Assegno extra

V547 [CWE-571] L'espressione 'retAddress' è sempre vera. Memoria.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);
      }
    // ....
    }
  // ....
}

L'retAddress il puntatore viene controllato due volte nel frammento di codice sopra. Innanzitutto, if (!retAddress) è controllato. Se il puntatore è null, l'esecuzione procede all'iterazione successiva del while ciclo continuo. Altrimenti, retAddress il puntatore non è nullo. Quindi il secondo if (retAddress) check è sempre vero e può essere rimosso.

Un'altra condizione che è sempre vera

V547 [CWE-571] L'espressione 'pipeConfig ==kPipeConfigP16' è sempre vera. 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 questo frammento di codice, l'analizzatore ha trovato if (pipeConfig ==kPipeConfigP16) condizione che è sempre vera. Scopriamo perché è così.

Se getPipeConfig la chiamata di funzione restituisce un valore che non è uguale a kPipeConfigP16 , la prima condizione sarà vera e l'esecuzione del programma non procederà al controllo di if (pipeConfig ==kPipeConfigP16) .

Si scopre che il secondo controllo di questa variabile non viene eseguito o è sempre vero. Ma non avere fretta e rimuoverlo. Forse la prima condizione è stata aggiunta temporaneamente e verrà rimossa in futuro.

Errore copia incolla

V517 [CWE-570] È stato rilevato l'uso del pattern 'if (A) {...} else if (A) {...}'. C'è una probabilità di presenza di un errore logico. Righe di controllo: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;
  // ....
}
}

Ecco che arrivano gli errori di copia-incolla. In questo frammento di codice, lo stesso newArrayMode ==Gnm::kArrayMode2dTiledThin l'assegno viene scritto due volte.

È difficile dire esattamente come risolvere questo problema. Molto probabilmente, il secondo controllo dovrebbe essere leggermente diverso. O forse è ridondante e può essere rimosso.

Perché è meglio evitare espressioni complesse?

V732 [CWE-480] L'operatore meno unario non modifica un valore di tipo bool. Prendi in considerazione l'utilizzo di '!' operatore. 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; // <=
  // ....
}

Sembra che il programmatore si aspettasse il seguente comportamento durante il calcolo dell'espressione:

• lascia digitare la variabile deve essere inferiore a 7;
• quindi digita <7 l'espressione è vera;
• a true viene applicato un meno unario , il risultato è -1;
• il -1 il valore viene convertito in un carattere senza segno , il risultato è 0b1111'1111 .

Tuttavia, questo è ciò che effettivamente accade:

• lascia digitare la variabile deve essere inferiore a 7;
• quindi digita <7 l'espressione è vera;
• a true viene applicato un meno unario , il risultato è 1;
• il 1 il valore viene convertito in un carattere senza segno , il risultato è 0b0000'0001 .

Sebbene, i seguenti &1 operazione porta allo stesso risultato. Per questa felice coincidenza, l'intero codice funziona come previsto dallo sviluppatore. Tuttavia, è meglio correggere questo codice. A seconda del tipo value, indovina quale valore viene assegnato a v3 variabile.

Il primo caso:il tipo variabile è maggiore o uguale a 7.

• Quindi digita <7 l'espressione è falsa;
• A false viene applicato un meno unario , il risultato è falso .
• False viene convertito in unsigned char, il risultato è 0b0000'0000 .
• Un "AND" bit per bit con 0 dà sempre 0, quindi otteniamo 0 come risultato.

Il secondo caso:il tipo la variabile è minore di 7.

• Come abbiamo scoperto in precedenza, (uint8_t) è (tipo <7) espressione è uguale a 1.
• In questo caso, ha senso calcolare il tipo 0x43u>> espressione.
• Per comodità, scriviamo la rappresentazione binaria del numero nel modo seguente:0x43 =0b0100'0011 .
• Ci interessa solo il bit meno significativo, perché il bit per bit "AND" con 1 verrà applicato al risultato del tipo 0x43u>> espressione.
• Se digita è uguale a 0, 1 o 6, il bit meno significativo sarà 1 e il risultato dell'intera espressione sarà 1. In tutti gli altri casi, il risultato dell'espressione sarà 0.

Per concludere, se il tipo è 0, 1 o 6, il valore 1 viene scritto nella variabile v3. In tutti gli altri casi, il valore 0 viene scritto nella variabile v3. Vale la pena sostituire un'espressione complessa con una più semplice e comprensibile — (tipo ==0) || (tipo ==1) || (digitare ==6) . Mi permetto di suggerire il seguente codice:

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

Ho anche sostituito i numeri 0, 1 e 6 con i corrispondenti valori di enumerazione denominati e ho scritto le sottoespressioni in una tabella.

Custodia angolare nell'operatore di trasloco

V794 L'operatore di assegnazione dovrebbe essere protetto dal caso di '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;
}

Se viene chiamato questo operatore e 'this ==&other' , tutti i campi dell'oggetto corrente verranno cancellati e i dati andranno persi. Questo comportamento non è corretto, è necessario aggiungere il controllo. Codice fisso:

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

Assegnazione ripetuta come motivo per il refactoring

V1048 [CWE-1164] Alla variabile 'retVal' è stato assegnato lo stesso valore. Modulo.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 questo frammento di codice, il vero il valore viene assegnato due volte a retVal variabile. Scopriamo perché questo sta accadendo. Per prima cosa, vediamo tutte le possibili modifiche alla variabile retVal prima dell'assegnazione indicata dall'analizzatore.

• Il retVal la variabile viene inizializzata su false .
• Se il isEncodedSymbol la chiamata di funzione ha restituito false , il vero il valore è assegnato a retVal e il do-mentre il ciclo è interrotto.
• Il risultato del decodeSymbol la chiamata di funzione è assegnata a retVal variabile. Dopodiché, se retVal ==false , il do-mentre il ciclo è interrotto.
• La stessa cosa accade con due chiamate di getModNameFromId funzione. Se una qualsiasi delle chiamate restituisce false , il do-mentre il ciclo è interrotto.

Nota che se il fare-mentre il loop è stato interrotto prematuramente, l'assegnazione indicata dall'analizzatore non verrà eseguita. Ciò significa che il sospetto retVal ==true l'assegnazione verrà eseguita solo se tutte le chiamate di funzione discusse sopra hanno restituito true . Pertanto, il retVal la variabile è già vera , e l'assegnazione non ha senso.

E perché usare 'do ... while(false)' costruire affatto? Il fatto è che questo costrutto permette di uscire anticipatamente dalla funzione con un singolo ritorno . Per le funzioni con un unico ritorno , a sua volta, è più probabile che venga applicata l'ottimizzazione del valore di ritorno denominata - NRVO. Questa ottimizzazione del compilatore evita la copia o lo spostamento non necessari dell'oggetto restituito. Questo viene fatto costruendo l'oggetto direttamente nella posizione della chiamata di funzione. In questo caso, la funzione restituisce il valore leggero bool tipo, quindi il guadagno da NRVO è minore. Inoltre, i moderni compilatori sono in grado di applicare NRVO a funzioni con più ritorno istruzioni, se lo stesso oggetto viene restituito in tutti i return dichiarazioni.

GetSymbolInfo non contiene errori e funziona come previsto dal programmatore. Tuttavia, è meglio rifattorizzare GetSymbolInfo metodo e rimuovere il do-while ciclo con retVal variabile. Mi permetto di suggerire il seguente codice:

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

Ho fatto quanto segue:

• rimosso il do-while loop e l'extra retVal variabile;
• sostituito ogni retVal controllo delle variabili mediante un controllo del risultato della chiamata di funzione corrispondente;
• sostituito ogni interruzione del do-mentre loop dalla corrispondente dichiarazione di ritorno — true / falso . Sappiamo quale valore restituire dall'analisi del retVal variabile che abbiamo fatto in precedenza.

A mio parere, tale codice è più facile da leggere e mantenere.

Conclusione

Naturalmente, questi non sono tutti gli errori e i difetti che abbiamo riscontrato in GPCS4. Alcuni casi erano piuttosto difficili da descrivere, quindi non li ho inclusi nell'articolo.

Auguriamo agli sviluppatori GPCS4 il successo nel loro ulteriore sviluppo dell'emulatore e consigliamo di controllare l'ultima versione del progetto con l'analizzatore PVS-Studio. Puoi semplicemente scaricare la distribuzione dell'analizzatore e richiedere una licenza gratuita per i progetti Open Source. Se sei interessato all'analisi statica in generale e a PVS-Studio in particolare, è il momento di provarlo. Puoi anche controllare GPCS4, oppure puoi controllare il tuo progetto :) Grazie per l'attenzione!