Hello,
TInlineAllocator can corrupt memory when calling void ResizeAllocation(int32 PreviousNumElements,int32 NumElements,SIZE_T NumBytesPerElement).
If allocation is done in dynamic buffer and PreviousNumElements > NumInlineElements the RelocateConstructItems will corrupt memory, it will move PreviousNumElements instead of max NumInlineElements elements
Hello Astraya,
Is this a regression from a previous version? Also, can you provide a reproduction case of some sort? A list of steps or a project that reproduces the issue would be helpful.
Hello Matthew,
This is not a regression, this code exist this 324683c on 14 Mar 2014.
To reproduce the corruption just use TInlineAllocator with for exemple NumInlineElements == 8 and call ResizeAllocation(0, 16, 4), this will allocate the dynamic buffer. Then call ResizeAllocation(16, 8, 4), this will try to reallocate 16 item in a static buffer of 8 items (Using Memmove or placement new depending of the ElementType).
Buffer overflow possibly appears here.
The fix is simple:
Do not call RelocateItems((void*)InlineData, (ElementType*)SecondaryData.GetAllocation(), PreviousNumElements); but RelocateItems((void*)InlineData, (ElementType*)SecondaryData.GetAllocation(), NumInlineElements);. Here we only want to move memory of maximum NumInlineElements.
If you know what changes should be made to fix the issue, I would suggest making a pull request on Github. This way, you can submit your proposed change and the developers can take a look at it. They’ll respond to you once they’ve reviewed it either with questions or to let you know that it is being accepted. You can follow this guide if you do not know how to submit a Github pull request: A new, community-hosted Unreal Engine Wiki - Announcements - Epic Developer Community Forums
I’d go one step further and call this a non-issue.
As far as I understand the engine code, it’s wrong to call ResizeAllocation() with PreviousNumElements > NumElements. The only thing that might be missing there would be an assertion that tells the user that invalid parameters were supplied.
Also, as PreviousNumElements is in all cases smaller than NumElements, calling RelocateItems with NumInlineElements as last parameter instead of PreviousNumElements will lead to issues, as in case bitwise relocation is not possible the destructor of the stored type might be called multiple times (or on otherwise invalid data).