FastMM_FreeMem_EraseBeforeFree can overwrite small-block free-list metadata before validation

[janrysavy]

Context

This comes from a second GPT-5.5 Pro review pass over the current upstream HEAD after your recent fixes:

https://chatgpt.com/s/t_69f0c62ed38c8191a9e89e24da484b03

I reviewed this finding manually and reproduced it locally against:

fd1d4899478b10b6fa113d47fc900112563523ef
Add thread safety to the event log text file.

Problem

When FastMM_BeginEraseFreedBlockContent is active, FreeMem is routed through FastMM_FreeMem_EraseBeforeFree.

In current HEAD, that wrapper fills the block before the normal FastMM_FreeMem validation path runs:

function FastMM_FreeMem_EraseBeforeFree(APointer: Pointer): Integer;
begin
  {Fill the user area of the block with the debug fill pattern before passing the block to the regular FreeMem handler.}
  FillChar(APointer^, FastMM_BlockMaximumUserBytes(APointer), CDebugFillByteFreedBlock);

  Result := FastMM_FreeMem(APointer);
end;

That is safe only while APointer still points to a live allocated block.

For a small block that has already been freed, the first pointer-sized word of the former user area is allocator-owned metadata: the small-block free-list link. On a second invalid FreeMemory(P), the erase-before-free wrapper writes $80 bytes into that already-freed block before FastMM_FreeMem can reject the invalid free.

So the diagnostic erase mode can poison the allocator free-list metadata before invalid-free handling runs.

Reproducer

This is intentionally a narrow test. It reads the first word of a freed small block as instrumentation, because that word is the allocator free-list link after the first free.

Expected behavior on a fixed build:

OK: invalid second free did not overwrite the freed small-block free-list link.

Current behavior on fd1d489:

BUG: second FreeMemory overwrote the freed small-block free-list link. Before=0000000000000000 After=8080808080808080

Standalone test:

program EraseBeforeFreeDoubleFreeRegression;

{$APPTYPE CONSOLE}

uses
  FastMM5,
  System.SysUtils;

const
  TestSize = 64;

function FreedFillPatternNativeUInt: NativeUInt;
begin
  {FastMM's freed-block debug fill byte is $80. Build the native pointer-sized
  value that FillChar writes into the first word of the user area.}
  if SizeOf(NativeUInt) = 8 then
    Result := NativeUInt($8080808080808080)
  else
    Result := NativeUInt($80808080);
end;

procedure ExitWithFailure(const AMessage: string);
begin
  Writeln(AMessage);
  Halt(1);
end;

procedure CheckDoubleFreeDoesNotOverwriteSmallBlockFreeListLink;
var
  P: Pointer;
  LFreeListLinkAfterFirstFree: NativeUInt;
  LFirstWordAfterSecondFree: NativeUInt;
begin
  {Step 1: Enable FastMM's diagnostic mode that erases user data before freeing
  a block. The bug is in this mode's FreeMem wrapper.}
  if not FastMM_BeginEraseFreedBlockContent then
    ExitWithFailure('FastMM_BeginEraseFreedBlockContent failed.');

  try
    {Step 2: Allocate a small block. Small freed blocks store allocator free-list
    metadata in the first pointer-sized word of the former user area.}
    P := GetMemory(TestSize);
    if P = nil then
      ExitWithFailure('Initial allocation failed.');

    {Step 3: Free the block once. After this point P is intentionally not user
    memory anymore; the allocator owns the block and P^ is the small-block
    free-list link. Reading it here is test instrumentation.}
    FreeMemory(P);
    LFreeListLinkAfterFirstFree := PNativeUInt(P)^;

    {Step 4: Trigger an invalid second free. Correct behavior is to reject or
    report the invalid free without modifying the already freed block. The buggy
    FastMM_FreeMem_EraseBeforeFree fills P^ with $80 bytes before FastMM_FreeMem
    can validate that P is already free.}
    FreeMemory(P);
    LFirstWordAfterSecondFree := PNativeUInt(P)^;

    {Step 5: If the first word changed to the freed fill pattern, the diagnostic
    wrapper has overwritten allocator metadata before validation. Do not perform
    further same-size allocations after detecting this, because the process heap
    may now have a poisoned free-list link.}
    if LFirstWordAfterSecondFree = FreedFillPatternNativeUInt then
      ExitWithFailure(Format(
        'BUG: second FreeMemory overwrote the freed small-block free-list link. Before=%p After=%p',
        [Pointer(LFreeListLinkAfterFirstFree), Pointer(LFirstWordAfterSecondFree)]));

    Writeln('OK: invalid second free did not overwrite the freed small-block free-list link.');
  finally
    {If the bug was detected we Halt(1) before this point to avoid running more
    allocator cleanup on a corrupted free list. On a fixed build, balancing the
    mode counter is safe.}
    FastMM_EndEraseFreedBlockContent;
  end;
end;

begin
  CheckDoubleFreeDoesNotOverwriteSmallBlockFreeListLink;
end.

Local validation

I built and ran the test with RAD Studio 37.0.

Against unmodified upstream fd1d489:

• Win64 dcc64: build passed, test failed with After=8080808080808080
• Win32 DCC32: build passed, test failed with After=80808080

I also tested a minimal local fix that only performs the erase fill when the block header matches the same "currently allocated block" classifications used by FastMM_FreeMem before dispatching to the small/medium/large/debug free paths.

With that local fix:

• Win64 dcc64: build passed, test passed
• Win32 DCC32: build passed, test passed

Possible fix direction

The important invariant seems to be: erase-before-free should only write into a block while the block is still user-owned.

One minimal approach is to check the block flags first and skip the fill if the pointer is already marked free or does not look like a currently allocated small/medium/large/debug block. Then delegate to FastMM_FreeMem unchanged so invalid-free handling still decides the result.

The minimal local fix I used for validation was:

function FastMM_FreeMem_EraseBeforeFree(APointer: Pointer): Integer;
var
  LBlockHeader: Integer;
begin
  LBlockHeader := PBlockStatusFlags(APointer)[-1];
  if (LBlockHeader and (CBlockIsFreeFlag or CIsSmallBlockFlag) = CIsSmallBlockFlag)
    or (LBlockHeader and (not CHasDebugInfoFlag) = CIsMediumBlockFlag)
    or (LBlockHeader and (not CHasDebugInfoFlag) = CIsLargeBlockFlag)
    or (LBlockHeader = CIsDebugBlockFlag) then
  begin
    {Fill the user area of the block with the debug fill pattern before passing the block to the regular FreeMem handler.}
    FillChar(APointer^, FastMM_BlockMaximumUserBytes(APointer), CDebugFillByteFreedBlock);
  end;

  Result := FastMM_FreeMem(APointer);
end;

This keeps the existing FastMM_FreeMem dispatch and invalid-free handling in charge of the final result. The only behavioral change is that the erase wrapper no longer writes into blocks that the normal free path would classify as already free or invalid.

An alternative, more integrated approach would be to move the erase fill into the validated per-type free paths, after the pointer has been classified as a live block and before the block transitions to freed state.

[pleriche]

Thanks Jan, I have pushed a fix for it. A double-free should normally bring the app down anyway, but it's better not to do any additional damage.