FMallocStomp2 does not honor alignment values greater than page size

Development Platform & Builds
, ,
1

When `FMallocStomp2::TryMalloc()` is called with a high alignment value, such as the Task system’s 0x10000, well above the page size of 0x1000, the pointer alignment fails and will return pointer values that only obey the page size alignment. Systems that expect these larger alignments start going haywire when the allocator doesn’t honor those requirements, which will lead to crashes. This code doesn’t understand that the page allocation strategy might (and probably will) return a pointer that is less aligned than the request.

  if(bUseUnderrunMode)
  {
    const SIZE_T AlignedAllocationData = (Alignment > 0U) ? ((AllocationDataSize + Alignment - 1U) & -static_cast<int32>(Alignment)) : AllocationDataSize;
    ReturnedPointer = reinterpret_cast<void*>(reinterpret_cast<uint8*>(FullAllocationPointer) + PageSize + AlignedAllocationData);
  }
  else
  {
    ReturnedPointer = reinterpret_cast<void*>(reinterpret_cast<uint8*>(FullAllocationPointer) + AllocFullPageSize - AlignedSize);
  }
2

Steps to Reproduce

  1. Launch editor in “Debug editor” configuration with a command line to enable FMallocStomp2. Eg: `<gamename> -stomp2malloc -MallocStomp2MaxSize=262143`
  2. Game will assert (checkSlow) on the first or second use of a 0x10000 aligned block requested through the `TLinearAllocatorBase` for your Task system because the pointer that comes back is only page aligned (0x1000) rather than the higher requested alignment
  3. Game will crash even earlier if you use the `-MallocStomp2UnderrunMode` switch to enable whatever “underrun mode” is. It seems the pointer it wants to return isn’t valid memory and it crashes in the `memset(0)` call filling the allocated space
3

I believe the same is true of FMallocStomp

4

Hi Alexander, sorry for the delay on this. It’s been fixed by simply skipping stomp checks over the page size. The likelihood is that such allocations are going to come from some bulk allocator (like TConcurrentLinearAllocator in this case) and over/underruns are more likely to corrupt neighbouring chunks rather than wrap onto the next page.

It will be available in 5.8, or you can see the change here:

https://github.com/EpicGames/UnrealEngine/commit/7be5230cedd0a93972ef32d4f64534d1d4c8152a

FMallocStomp seems fine as it uses Alignment rather than PageSize. What problem did you see?

Steve

5

Hi Alexander, I’ve confirmed the problem, I’ll see about getting it fixed.

Steve

6

Looks good. I’ll try that out tomorrow. I went back and tried FMallocStomp and found it working. Not sure what got into my head there. I’d guess it was something, but maybe it was fixed in the 5.6 upgrade. If I hit something there again, I’ll give a shout.

Any chance I can get an explanation why those both exist?

7

Are you asking why we have two stomp implementations? Often when someone has a new idea for a system, and it can be done cleanly - like with our different Mallocs - it will be implemented as a parallel system to avoid disruption of existing users and to allow more choice or aid migration. If, over time, one of those implementations ends up never being used, it should ultimately get deprecated and removed. There’s no time scale on when this happens though - just when someone is doing a cleanup.

Steve

8

Thank you; that helps. I would suggest while there are choices facing developers unfamiliar with why there are choices like this (SavePackage2, AsyncLoading2), comments in the code can help people navigate them. One might note, for example, that FMallocStomp2 is not available on console targets. While you’re at it, documenting that if you don’t set `-MallocStomp2MaxSize=<something>` the allocator does not do anything would be helpful. Epic is big, but I would wager the majority of consumers of this code are not internal.

9

There are valid concerns, thanks. I will raise them with the owners of these systems.

Steve