Static Code Analysis With PVS-Studio (Part 3)

May 16, 2016

It’s been a while since our last blog entry – so we’re back with another batch of fixes, helped along by Viva64’s PVS-Studio again!

Let’s jump right in…

// Not expecting to have both a game and a program in the same project. These would alias because we share the project and solution configuration names (just because it makes sense to)
if (ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Game && ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Program ||
 ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Program && ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Game)
{
   throw new BuildException("Not expecting project {0} to already have a Game/Program target ({1}) associated with it while trying to add: {2}", ProjectFilePath, ExistingProjectTarget.TargetRules.ToString(), TargetRulesObject.ToString());
}

So… we’re saying something along the lines of “if the animal is a cat and a dog, or the animal is a dog and a cat”, warn the user. Two minor problems with this of course:-

If the animal is a dog, it’s a dog. It’s not a cat. Full stop;
If it were possible to be a dog AND a cat, it would obviously also be a cat AND a dog… (!?!)

The correct code, of course, should be:-

// Not expecting to have both a game and a program in the same project. These would alias because we share the project and solution configuration names (just because it makes sense to)
if ((ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Game && TargetRulesObject.Type == TargetRules.TargetType.Program)
   || (ExistingProjectTarget.TargetRules.Type == TargetRules.TargetType.Program && TargetRulesObject.Type == TargetRules.TargetType.Game))
{
   throw new BuildException("Not expecting project {0} to already have a Game/Program target ({1}) associated with it while trying to add: {2}", ProjectFilePath, ExistingProjectTarget.TargetRules.ToString(), TargetRulesObject.ToString());
}

This made PVS much happier – note:-

Congratulations! PVS-Studio has not found any issues in your source code!

While we’re looking at this, I wonder what the following line, just above the code quoted above, is trying to tell the user…?!?

throw new BuildException("Not expecting project {0} to already have a target rules of with configuration name {1} ...

The programmer clearly gained a 100% score in the “Baffling Warning Messages 101” class.

Another copy-paste bug next … I’ll just give you the code and the fix in this case – I doubt this one would actually affect anyone … but, then, if it doesn’t, it begs the question of what the code’s there for at all I guess … so it might as well be patched up.

In MovieScene3DAttachSection.cpp, note the following lines:-

if (!bConstrainRx)
{
   OutRotation.Roll = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Roll;
}

if (!bConstrainRx)
{
   OutRotation.Pitch = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Pitch;
}

if (!bConstrainRx)
{
   OutRotation.Yaw = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Yaw;
}

You can just go ahead and change these to the following:-

if (!bConstrainRx)
{
   OutRotation.Roll = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Roll;
}

if (!bConstrainRy)
{
   OutRotation.Pitch = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Pitch;
}

if (!bConstrainRz)
{
   OutRotation.Yaw = SceneComponent->GetRelativeTransform().GetRotation().Rotator().Yaw;
}

That being said, a better fix to avoid confusion might be to use better naming – so bConstrainRz, for example, would become bConstrainYaw (that’s just good coding practice IMO).

There’s a possible array overrun in SteamVRController.cpp (from the code, I guess the overrun would only happen if you had more than 4 SteamVR controllers in use?)

V557 Array overrun is possible. The value of ‘DeviceIndex’ index could reach 15.

on the 3rd line of this block of code:-

DeviceToControllerMap[DeviceIndex] = FMath::FloorToInt(NumControllersMapped / CONTROLLERS_PER_PLAYER);
ControllerToDeviceMap[NumControllersMapped] = DeviceIndex;
ControllerStates[DeviceIndex].Hand = (EControllerHand)(NumControllersMapped % CONTROLLERS_PER_PLAYER);
++NumControllersMapped;

DeviceIndex is within the range of [0, vr::k_unMaxTrackedDeviceCount) with k_unMaxTrackedDeviceCount defined to be 16.

NumControllersMapped, on the other hand, is within the range [0, MaxControllers) with MaxControllers set to be 8 by way of MaxUnrealControllers and MAX_STEAMVR_CONTROLLER_PAIRS.

I don’t have any SteamVR controllers to test with, and certainly don’t have 5+, so can’t easily test a fix for this… but an educated guess tells me that the correct code should be:-

ControllerStates[NumControllersMapped].Hand = (EControllerHand)(NumControllersMapped % CONTROLLERS_PER_PLAYER);

We can also go ahead and fix a bug later in the sourcefile before it happens by changing the following:-

 int32 ControllerIndex = DeviceToControllerMap[DeviceIndex];
 FControllerState& ControllerState = ControllerStates[ DeviceIndex ];
 EControllerHand HandToUse = ControllerState.Hand;

to:-

int32 ControllerIndex = DeviceToControllerMap[DeviceIndex];
FControllerState& ControllerState = ControllerStates[ControllerIndex];
EControllerHand HandToUse = ControllerState.Hand;

As I say, I can’t test this … but, regardless, the code as it was would never have worked.

There’s a small problem in RHIDefinitions.h, identified by the warning:-

V501 There are identical sub-expressions ‘Platform == SP_OPENGL_ES2_ANDROID’ to the left and to the right of the ‘||’ operator.

The code as it currently stands:-

inline bool RHIHasTiledGPU(const EShaderPlatform Platform)
{
   return (Platform == SP_METAL_MRT) || Platform == SP_METAL || Platform == SP_OPENGL_ES2_IOS || Platform == SP_OPENGL_ES2_ANDROID || Platform == SP_OPENGL_ES2_ANDROID;
}

This problem came about with the following change:-

Rename shader platform SP_OPENGL_ES2 to SP_OPENGL_ES2_ANDROID
[CL 2705741 by Jack Porter in Main branch]

So it looks like the correct code would be just:-

inline bool RHIHasTiledGPU(const EShaderPlatform Platform)
{
   return (Platform == SP_METAL_MRT) || Platform == SP_METAL || Platform == SP_OPENGL_ES2_IOS || Platform == SP_OPENGL_ES2_ANDROID;
}

UE4’s CrossCompiler has a small snafu found by PVS with the warning:-

V547 Expression is always false.

Note the offending code in ShaderCompilerCommon.cpp:-

CCTCmdLine += ((CCFlags & HLSLCC_PackUniformsIntoUniformBuffers) == HLSLCC_PackUniformsIntoUniformBuffers) ? TEXT(" -packintoubs") : TEXT("");
CCTCmdLine += ((CCFlags & HLSLCC_FixAtomicReferences) == HLSLCC_PackUniformsIntoUniformBuffers) ? TEXT(" -fixatomics") : TEXT("");

The correct code being, of course:-

CCTCmdLine += ((CCFlags & HLSLCC_PackUniformsIntoUniformBuffers) == HLSLCC_PackUniformsIntoUniformBuffers) ? TEXT(" -packintoubs") : TEXT("");
CCTCmdLine += ((CCFlags & HLSLCC_FixAtomicReferences) == HLSLCC_FixAtomicReferences) ? TEXT(" -fixatomics") : TEXT("");

An interesting warning shows up for ScriptCore.cpp:-

V523 The ‘then’ statement is equivalent to the ‘else’ statement. scriptcore.cpp 1210

Here’s the code (from the current UE4 codebase):-

if (Function->FunctionFlags & FUNC_Native)
{
   Function->Invoke(this, NewStack, ReturnValueAdress);
}
else
{
   Function->Invoke(this, NewStack, ReturnValueAdress);
}

I can’t say for sure what’s intended here – though I can see through looking back at the file’s history that it’s been operating this way for a while… so perhaps this should just be:-

Function->Invoke(this, NewStack, ReturnValueAdress);

I doubt this change would make any difference – but it’s always best to try to keep the code clean…

you never know for sure that the compiler will optimize the code
if another programmer sees this code, they might waste time looking into whether or not this is actually a bug

Several of these warnings came up in BoostrapPackagedGame.cpp:-

V611 The memory was allocated using ‘new T[]’ operator but was released using the ‘delete’ operator. Consider inspecting this code. It’s probably better to use ‘delete [] Buffer;’.
V611 The memory was allocated using ‘new T[]’ operator but was released using the ‘delete’ operator. Consider inspecting this code. It’s probably better to use ‘delete [] ChildCmdLine;’.

The problem here being that Buffer and ChildCmdLine are allocated like this:-

WCHAR* Buffer = new WCHAR[wcslen(CmdLine) + 50];

WCHAR* ChildCmdLine = new WCHAR[wcslen(BaseDirectory) + wcslen(ExecFile) + wcslen(BaseArgs) + wcslen(CmdLine) + 20];

But are then cleaned up with:-

delete Buffer;

delete ChildCmdLine;

These can just be changed to:-

delete [] Buffer;

delete [] ChildCmdLine;

(note that there are 3 occurrences of the latter)

And that wraps up another analysis of UE4 sourcecode. Hope you enjoyed it!

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