Static Code Analysis With PVS-Studio (Part 5)

It’s been a while now since we last ran PVS-Studio across the Unreal codebase… so, we decided to fire it up once more and to see what it came up with…

First up:-

V767 Suspicious access to element of ‘SelectedObjects’ array by a constant index inside a loop.

Let’s take a look… this lies in /Engine/Source/Editor/DetailCustomizations/Private/SkeletonNotifyDetails.cpp here:-

void FSkeletonNotifyDetails::CustomizeDetails( IDetailLayoutBuilder& DetailBuilder )
{
...
  UEditorSkeletonNotifyObj* EdObj = NULL;
  for(int i = 0; i < SelectedObjects.Num(); ++i)
  {
    UObject* Obj = SelectedObjects[0].Get();
    EdObj = Cast<UEditorSkeletonNotifyObj>(Obj);
    if(EdObj)
    {
      break;
    }
  }
  if(EdObj)

The warning is pointing us at line 7 above… where, as you’ll see, regardless of where we’re at in the loop across the selected objects, we’re always accessing the first element. So, yeah, let’s go ahead and fix that pretty ridiculous mistake:-

void FSkeletonNotifyDetails::CustomizeDetails( IDetailLayoutBuilder& DetailBuilder )
{
...
  UEditorSkeletonNotifyObj* EdObj = NULL;
  for(int32 i = 0; i < SelectedObjects.Num(); ++i)
  {
    UObject* Obj = SelectedObjects[i].Get();
    EdObj = Cast<UEditorSkeletonNotifyObj>(Obj);
    if(EdObj)
    {
      break;
    }
  }
  if(EdObj)

Next up, let’s take a look at:-

V607 Ownerless expression.

This can be seen below in /Engine/Source/Runtime/D3D12RHI/Public/D3D12Resources.h:-

const D3D12_RESOURCE_STATES GetOptimalInitialState() const
{
  if (bSRVOnly)
  {
    return D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE | D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE;
  }
  else if (bBuffer && !bUAV)
  {
    (bReadBackResource) ? D3D12_RESOURCE_STATE_COPY_DEST : D3D12_RESOURCE_STATE_GENERIC_READ;
  }
  else if (bWritable) // This things require tracking anyway
  {
    return D3D12_RESOURCE_STATE_COMMON;
  }

The error should stand out pretty clearly if you look closely at line 9… which is neglecting to return the correct value – or to anything with it at all, actually. You can feel free to fix this with:-

const D3D12_RESOURCE_STATES GetOptimalInitialState() const
{
  if (bSRVOnly)
  {
    return D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE | D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE;
  }
  else if (bBuffer && !bUAV)
  {
    return bReadBackResource ? D3D12_RESOURCE_STATE_COPY_DEST : D3D12_RESOURCE_STATE_GENERIC_READ;
  }
  else if (bWritable) // This things require tracking anyway
  {
    return D3D12_RESOURCE_STATE_COMMON;
  }

A common mistake is when checking for NULL pointers… such as this one:-

V595 The ‘RenderTarget’ pointer was utilized before it was verified against nullptr.

You can find the problem in /Engine/Source/Runtime/Windows/D3D11RHI/Private/D3D11Commands.cpp here:-

void FD3D11DynamicRHI::RHITransitionResources(EResourceTransitionAccess TransitionType, FTextureRHIParamRef* InTextures, int32 NumTextures)
{
...
  for (int32 i = 0; i < NumTextures; ++i)
  {        
    FTextureRHIParamRef RenderTarget = InTextures[i];
    SCOPED_RHI_CONDITIONAL_DRAW_EVENTF(*this, RHITransitionResourcesLoop, bShowTransitionEvents, TEXT("To:%i - %s"), i, *RenderTarget->GetName().ToString());
    if (RenderTarget)
    {

Basically, if we need to check whether RenderTarget is NULL or not on line 8 above, that suggests that line 7 could be accessing invalid memory. So we tidy this up and reduce the chance of getting a crash sometime with:-

void FD3D11DynamicRHI::RHITransitionResources(EResourceTransitionAccess TransitionType, FTextureRHIParamRef* InTextures, int32 NumTextures)
{
...
  for (int32 i = 0; i < NumTextures; ++i)
  {        
    FTextureRHIParamRef RenderTarget = InTextures[i];
    if (RenderTarget)
    {
      SCOPED_RHI_CONDITIONAL_DRAW_EVENTF(*this, RHITransitionResourcesLoop, bShowTransitionEvents, TEXT("To:%i - %s"), i, *RenderTarget->GetName().ToString());

And another look at a bug near-identical to the one above, and in the same file:-

V595 The ‘NewRenderTarget’ pointer was utilized before it was verified against nullptr.

The problem this time is in the following function:-

void FD3D11DynamicRHI::RHISetRenderTargets(
  uint32 NewNumSimultaneousRenderTargets,
  const FRHIRenderTargetView* NewRenderTargetsRHI,
  const FRHIDepthRenderTargetView* NewDepthStencilTargetRHI,
  uint32 NewNumUAVs,
  const FUnorderedAccessViewRHIParamRef* UAVs
  )
{
...
      FD3D11TextureBase* NewRenderTarget = GetD3D11TextureFromRHITexture(NewRenderTargetsRHI[RenderTargetIndex].Texture);
      RenderTargetView = NewRenderTarget->GetRenderTargetView(RTMipIndex, RTSliceIndex);
      if (NewRenderTarget)
      {

And, like the earlier error, we simply need to move the code that’s relying on NewRenderTarget being a valid pointer to within the not-NULL check:-

void FD3D11DynamicRHI::RHISetRenderTargets(
  uint32 NewNumSimultaneousRenderTargets,
  const FRHIRenderTargetView* NewRenderTargetsRHI,
  const FRHIDepthRenderTargetView* NewDepthStencilTargetRHI,
  uint32 NewNumUAVs,
  const FUnorderedAccessViewRHIParamRef* UAVs
  )
{
...
      FD3D11TextureBase* NewRenderTarget = GetD3D11TextureFromRHITexture(NewRenderTargetsRHI[RenderTargetIndex].Texture);
      if (NewRenderTarget)
      {
        RenderTargetView = NewRenderTarget->GetRenderTargetView(RTMipIndex, RTSliceIndex);

Delving further, let’s look at the following warning:-

V547 Expression ‘* p && * p < L’0’ && * p > L’9” is always false.

This one can be found in /Engine/Source/Developer/ShaderFormatOpenGL/Private/OpenGLShaderCompiler.cpp here:-

// Skip to a number, take that to be the line number.
while (*p && *p < TEXT('0') && *p > TEXT('9')) { p++; }
while (*p && *p >= TEXT('0') && *p <= TEXT('9'))
{
  LineNumber = 10 * LineNumber + (*p++ - TEXT('0'));
}

The errant code is on the second line. This line is intended to skip over all the non-numeric entries… we can fix that by changing the code to :-

// Skip to a number, take that to be the line number.
while (*p && (*p < TEXT('0') || *p > TEXT('9'))) { p++; }
while (*p && *p >= TEXT('0') && *p <= TEXT('9'))
{
  LineNumber = 10 * LineNumber + (*p++ - TEXT('0'));
}

And… that’s it for today. We’ll be back soon with some more interesting blogs, some very cool optimizations that you can add to the engine – and we’ll be delving into some nice additions to the engine that we’ve been working on.

Credit(s): Robert Troughton (Coconut Lizard),
Evgeniy Ryzhkov (Viva64)
Status: Currently unfixed in 4.14