A Collection of Little Fixes - Blog : Coconut Lizard

Intro

Here, thanks again largely to PVS-Studio, I present some little bug fixes, code tidy-ups and so on, ranging from the "how did this ever work?" to "if nobody's using this - is it still a bug?". Enjoy :-)

1. In MovieSceneSequenceInstance.h you'll find this interesting little function with a fairly obvious bug - which PVS Studio reports as "V1001: The variable is assigned but is not used by the end of the function":-

/**
 * Indicate that this sequence instance has finished evaluation and should remove its entities
 */
void SetFinished(bool bInFinished)
{
	bInFinished = bFinished;
}

As it happens, there's only one call to this function anyway - and bFinished was already false at this point. But, hey, if the function is there to be used, it should probably be correct. Maybe the function not being used was down to coders finding that it was unreliable ;-) (if this is the case - wrists should be slapped.. fix the function, don't work around it!) Anyway, of course, the correct code is:-

	bFinished = bInFinished;

2. The next is a little more interesting. In SlabAllocator.h, PVS Studio gives a "V1064: The '(16 - 1)' operand of integer division is less than the '16' one. The result will always be zero". Here's the code:-

virtual void* Allocate(uint64 Size) override
{
	uint64 AllocationSize = Size + (16 - 1) / 16;
	if (!CurrentSlab || CurrentSlabAllocatedSize + AllocationSize > SlabSize)

Of course the intention here is clearly to increase AllocationSize to the next multiple of 16. The correct math for this would be:-

	uint64 AllocationSize = (Size + (16 - 1)) / 16;

However... Unreal Engine has an Align() function already... so why not use it? In general, creating these functions/formula once and reusing across the codebase is a very good way to catch mistakes like this.

	uint64 AllocationSize = Align(Size, 16);

Probably we should also have the alignment value as a defined constant for good coding practice - but you get the point already...

3. SimpleHMD.cpp has a very tiny little copy-paste mistake in this block of code, showing in PVS Studio as "V512: A call of the 'memcpy' function will lead to the buffer becoming out of range":-

	static const uint16 Indices[] = { 0, 1, 2, 0, 2, 3 };

	FIndexBufferRHIRef IndexBufferRHI = RHICreateIndexBuffer(sizeof(uint16), sizeof(uint16) * 12, BUF_Volatile, CreateInfo);
	void* VoidPtr2 = RHILockIndexBuffer(IndexBufferRHI, 0, sizeof(uint16) * 12, RLM_WriteOnly);
	FPlatformMemory::Memcpy(VoidPtr2, Indices, sizeof(uint16) * 12);
	RHIUnlockIndexBuffer(IndexBufferRHI);

Basically, there are 6 indices here - but the create, lock and memcpy are all acting on 12. It won't cause any real harm and I don't believe this is ever likely to be performance critical code - but we should fix it anyway:-

	FIndexBufferRHIRef IndexBufferRHI = RHICreateIndexBuffer(sizeof(uint16), sizeof(uint16) * NumIndices, BUF_Volatile, CreateInfo);
	void* VoidPtr2 = RHILockIndexBuffer(IndexBufferRHI, 0, sizeof(uint16) * NumIndices, RLM_WriteOnly);
	FPlatformMemory::Memcpy(VoidPtr2, Indices, sizeof(uint16) * NumIndices);
	RHIUnlockIndexBuffer(IndexBufferRHI);

Earlier in the code you can then define NumIndices appropriately:-

static const uint32 NumIndices = NumTris * 3;

4. Within the new Chaos physics system, in EventDefaults.cpp I was surprised to see this copy-paste bug - highlighted by PVS Studio as "V778: Two similar code fragments were found. Perhaps, this is a typo". Note that this happens in two places in the same file (lines 174 and 259).

Data.ParticleProxy = Solver->GetProxies(Particle0->Handle()) && Solver->GetProxies(Particle0->Handle())->Array().Num() ? 
Solver->GetProxies(Particle0->Handle())->Array().operator[](0) : nullptr; // @todo(chaos) : Iterate all proxies
Data.LevelsetProxy = Solver->GetProxies(Particle1->Handle()) && Solver->GetProxies(Particle0->Handle())->Array().Num() ?
Solver->GetProxies(Particle1->Handle())->Array().operator[](0) : nullptr; // @todo(chaos) : Iterate all proxies

Ignoring the comment drawing attention to the fact that only the first proxy is being considered, there's another problem here: when lines 172-173 were (presumably) duplicated to 174-175 the coder missed changing one of the Particle0->Handle() parts to Particle1-Handle(). Line 174 should of course be:-

Data.LevelsetProxy = Solver->GetProxies(Particle1->Handle()) && Solver->GetProxies(Particle1->Handle())->Array().Num() ?

5. Now looking at something completely different, the Audio Editor, we find yet another copy-paste bug in SoundSubmixGraphSchema.cpp. PVS Studio notes "V656: Variables are initialized through the call to the same function. It's probably an erorr on unoptimized code":-

USoundSubmixWithParentBase* SubmixWithParentA = Cast<USoundSubmixWithParentBase>(SubmixA);
USoundSubmixWithParentBase* SubmixWithParentB = Cast<USoundSubmixWithParentBase>(SubmixA);

Hopefully it goes without saying that the second line should really be:-

USoundSubmixWithParentBase* SubmixWithParentB = Cast<USoundSubmixWithParentBase>(SubmixB);

6. Ten points for trying here but, well, I don't think this is quite as intended... the same code appears in both ImageReader.cpp and DatasmithTextureResize.cpp, both part of the Datasmith runtime. PVS Studio shows this up as "V614: Uninitialized variable used".

class NinePoints
{
public:
	NinePoints(){ aa = ab = ac = ba = bb = bc = ca = cb = cc; }
	float aa, ab, ac, ba, bb, bc, ca, cb, cc;
};

I'm going to assume that a good initial value here is zero..? It's certainly better than "who knows?":-

NinePoints(){ aa = ab = ac = ba = bb = bc = ca = cb = cc = 0; }

7. The next one is actually a problem in a third-party library that's linked into Unreal Engine.. the FBX SDK. PVS Studio gives two warnings for this one: "V501: There are identical sub-expressions to the left and to the right of the '==' operator" and "V547: Expression is always true". Here's the code in FBXPropertyTypes.h:-

inline bool FbxTypeCopy(FbxBlob& pDst, const FbxString& pSrc)
{
    bool lCastable = pSrc.GetLen() == pSrc.GetLen();
    FBX_ASSERT( lCastable );
    if( lCastable )
        pDst.Assign(pSrc.Buffer(), (int)pSrc.GetLen());
    return lCastable;
}

Again, the intent is fairly clear - this should be:-

    bool lCastable = pDst.GetLen() == pSrc.GetLen();

8. A different sort of problem is evident in the code below from NiagaraSystemInstance.cpp - which PVS-Studio shows as a "V571: Recurring check":-

for (const FNiagaraVariable& OverrideParameterVariable : OverrideParameterVariables)
{
	if (OverrideParameterVariable.IsDataInterface() && UserDINamesReadInSystemScripts.Contains(OverrideParameterVariable.GetName()))
	{
		if (UserDINamesReadInSystemScripts.Contains(OverrideParameterVariable.GetName()))
		{
			return true;
		}
	}
}

I did wonder whether the compiler would optimise the second Contains() call out... it doesn't, of course, as it really doesn't know what the first call to Contains() might've done, whether it's changed UserDINamesReadInSystemScripts or not. Anyway, we can of course change this to:-

for (const FNiagaraVariable& OverrideParameterVariable : OverrideParameterVariables)
{
	if (OverrideParameterVariable.IsDataInterface() && UserDINamesReadInSystemScripts.Contains(OverrideParameterVariable.GetName()))
	{
		return true;
	}
}

That's probably enough for today. None of the above are particularly groundbreaking - they do, however, highlight how useful static code analysis can be in sanitizing code. With some of the above, it's entirely possible that a bug could be introduced by a coder that is harder to track down due to an assumption that the newly added code is at fault (rather than existing functionality).

Until next time!

Credit(s): Robert Troughton (Coconut Lizard)
Help: Josef Gluyas, Gareth Martin (Coconut Lizard)
Status: Currently unimplemented in UE4.26
Github Pull: Not Yet