Sign Of The Times

February 23, 2017

It’s now been a ridiculously long time since we posted anything to this blog … so I wanted to start doing something a little bit different – by introducing more regular, smaller blogs (we’ll still do the larger blogs of course – time-willing).

I’ll start with an interesting bit of code that I was looking at today. Running PVS-Studio across UE 4.15, I spotted an interesting warning:-

V555 The expression ‘MaxDWORDs – PreviousNumDWORDs > 0’ will work as ‘MaxDWORDs != PreviousNumDWORDs’.

Here’s the code:-

FORCENOINLINE void Realloc(int32 PreviousNumBits)
{
  const uint32 MaxDWORDs = AllocatorInstance.CalculateSlackReserve(
    FMath::DivideAndRoundUp(MaxBits, NumBitsPerDWORD),
    sizeof(uint32)
    );
  MaxBits = MaxDWORDs * NumBitsPerDWORD;
  const int32 PreviousNumDWORDs = FMath::DivideAndRoundUp(PreviousNumBits, NumBitsPerDWORD);

  AllocatorInstance.ResizeAllocation(PreviousNumDWORDs, MaxDWORDs, sizeof(uint32));

  if (MaxDWORDs && MaxDWORDs - PreviousNumDWORDs > 0)
  {
    // Reset the newly allocated slack DWORDs.
    FMemory::Memzero((uint32*)AllocatorInstance.GetAllocation() + PreviousNumDWORDs, (MaxDWORDs - PreviousNumDWORDs) * sizeof(uint32));
  }
}

The warning is pointing us at line 12. The problem comes that MaxDWORDs is unsigned while PreviousNumDWORDs is signed… the result of “MaxDWORDs – PreviousNumDWORDs” ends up unsigned too – meaning that the test for greater than zero will be true for everything apart from equality.

That could actually be pretty dangerous – if PreviousNumDWORDs is greater than MaxDWORDs, the Memzero() would be called – and could end up with nearly 16gb of memory being cleared. As it happens, it currently looks like the calling code can’t cause this to happen – but that of course doesn’t excuse the mistake and doesn’t mean that it shouldn’t be fixed… another programmer could add to the functionality surrounding this in the future and then wonder why there’s an odd memory crash. Anyway…

Two changes that we should make here:-

MaxDWORDs should just be signed. This might seem crazy for a counter – but everything else in this area of code is signed so, without a major rewrite, this is the right thing to do;
Get rid of the horrible (A – B > 0) test and replace with a simpler, less error-prone ( A > B ).. if the above had been written in this way, the programmer would’ve been warned about the problem earlier by the compiler with a signed/unsigned mismatch message.

So the new code becomes:-

FORCENOINLINE void Realloc(int32 PreviousNumBits)
{
  const uint32 MaxDWORDs = AllocatorInstance.CalculateSlackReserve(
    FMath::DivideAndRoundUp(MaxBits, NumBitsPerDWORD),
    sizeof(uint32)
    );
  MaxBits = MaxDWORDs * NumBitsPerDWORD;
  const uint32 PreviousNumDWORDs = FMath::DivideAndRoundUp(PreviousNumBits, NumBitsPerDWORD);

  AllocatorInstance.ResizeAllocation(PreviousNumDWORDs, MaxDWORDs, sizeof(uint32));

  if (MaxDWORDs && MaxDWORDs > PreviousNumDWORDs)
  {
    // Reset the newly allocated slack DWORDs.
    FMemory::Memzero((uint32*)AllocatorInstance.GetAllocation() + PreviousNumDWORDs, (MaxDWORDs - PreviousNumDWORDs) * sizeof(uint32));
  }
}

The same changes should be made in ReallocGrow() as well. To be fair, if anyone from Epic should look at this, Realloc() and ReallocGrow() should probably be consolidated into one function as well – there’s only a tiny difference (a single line) between the two functions so it just seems crazy to have so much code duplicated here…

And that’s your lot

Credit(s): Robert Troughton and Gareth Martin (Coconut Lizard),
Evgeniy Ryzhkov (Viva64)
Status: Currently unfixed in 4.15