Duplicate pending FreeMem can create a self-referential pending-free list before validation

[janrysavy]

Context

This comes from a third GPT-5.5 Pro review pass over current upstream HEAD:

https://chatgpt.com/share/69f0f0fc-e53c-83eb-9388-64b161c31382

I reviewed the finding manually and reproduced it locally against current upstream HEAD:

ee42f790ab4e6a22e805f397af4ae19189182d68
#71 Prevent corruption of the freed block linked list when FastMM_BeginEraseFreedBlockContent is active and the application attempts to free the same block twice.

This is separate from #71. The #71 fix prevents the erase-before-free wrapper from overwriting an already freed small-block free-list link. This issue is in the normal pending-free enqueue path when an arena manager is locked.

Problem

When a small-block manager is locked, FreeMem cannot return the block to the span immediately, so FastMM links the block into TSmallBlockManager.PendingFreeList.

The current Win64 inline path does this:

@ManagerCurrentlyLocked:
  mov rax, TSmallBlockManager(rsi).PendingFreeList
  mov [rdx], rax
  lock cmpxchg TSmallBlockManager(rsi).PendingFreeList, rdx
  jne @ManagerCurrentlyLocked

The Win32 inline path and the PurePascal small/medium/large pending-free enqueue paths use the same pattern: write the old pending-list head into the first word of the block, then publish the block as the new head.

That is safe for the first pending free. It is not safe for a duplicate pending free of the same pointer before the first pending free has been processed.

The sequence is:

1. Thread A uses the public FastMM_WalkBlocks API. The callback is invoked while the small-block manager is locked.
2. Thread B calls FreeMem(P). Since the manager is locked, FastMM queues P on PendingFreeList and writes the previous head into P^.
3. Thread B calls FreeMem(P) again. The application call is invalid, but the block header still does not say "free", because the first free is only pending.
4. FastMM reaches the pending-free enqueue path again, reads PendingFreeList = P, writes P^ := P, and publishes P again.

The pending-free list is now a one-node self-cycle. Later pending-free processing can hang or repeatedly process the same block.

Reproducer

The test uses only public APIs. It intentionally performs an invalid duplicate FreeMem(P), but the observed bug is that FastMM writes allocator-owned pending-list metadata before rejecting or reporting the duplicate.

Current upstream Win64 output:

BUG: duplicate pending FreeMem created a self-referential pending-free list. P=0000026069373040 FirstWordAfterFirstFree=0000000000000000 FirstWordAfterSecondFree=0000026069373040

Current upstream Win32 output:

BUG: duplicate pending FreeMem created a self-referential pending-free list. P=037818E0 FirstWordAfterFirstFree=00000000 FirstWordAfterSecondFree=037818E0

Standalone DPR:

program PendingFreeDuplicateCycleRegression;

{$APPTYPE CONSOLE}

{Regression target:

  FastMM queues frees on a manager's PendingFreeList when that manager is
  currently locked.  The first pointer-sized word of the pending block becomes
  allocator-owned pending-list metadata at that point.

  This test forces that public pending-free path by holding a small-block
  manager lock through FastMM_WalkBlocks, then performs the same invalid
  duplicate FreeMem(P) twice while the first free is still pending.

  The duplicate FreeMem(P) is intentionally invalid application behavior.  The
  FastMM-side invariant being tested is narrower: even for an invalid duplicate
  public FreeMem call, FastMM must not corrupt its own PendingFreeList before it
  rejects or reports the invalid free.

  Expected current upstream failure:

    BUG: duplicate pending FreeMem created a self-referential pending-free list.

  Expected fixed behavior:

    OK: duplicate pending FreeMem was rejected before corrupting the
    pending-free list.

  or:

    OK: duplicate pending FreeMem did not create a pending-free self-cycle.
}

uses
  FastMM5,
  System.Classes,
  System.SyncObjs,
  System.SysUtils,
  Winapi.Windows;

const
  TestBlockSize = 24;

type
  TWalkContext = record
    {The live small block that the walker must find and hold locked.}
    TargetBlock: Pointer;

    {Signaled by the walker when TargetBlock is being reported from inside
    FastMM_WalkBlocks while the corresponding small-block manager is locked.}
    InTargetCallback: TEvent;

    {The main thread releases this event after it has performed the duplicate
    FreeMem calls and inspected the pending-list link field.}
    ReleaseTargetCallback: TEvent;
  end;

  TWalkThread = class(TThread)
  protected
    procedure Execute; override;
  public
    FailureMessage: string;
  end;

var
  GContext: TWalkContext;

function HexPointer(AValue: NativeUInt): string;
begin
  Result := IntToHex(AValue, SizeOf(Pointer) * 2);
end;

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

procedure WalkBlocksCallback(const ABlockInfo: TFastMM_WalkAllocatedBlocks_BlockInfo);
var
  LContext: ^TWalkContext;
begin
  LContext := ABlockInfo.UserData;

  {FastMM_WalkBlocks invokes the callback while the corresponding allocator
  manager lock is still held.  Matching our live small block here gives the
  main thread a deterministic window where FreeMem must use the pending-free
  path for exactly this block's small-block manager.}
  if (ABlockInfo.BlockType = btSmallBlock)
    and (ABlockInfo.BlockAddress = LContext.TargetBlock) then
  begin
    LContext.InTargetCallback.SetEvent;
    LContext.ReleaseTargetCallback.WaitFor(INFINITE);
  end;
end;

procedure TWalkThread.Execute;
begin
  try
    if not FastMM_WalkBlocks(WalkBlocksCallback, [btSmallBlock], True, @GContext, 5000) then
      FailureMessage := 'FastMM_WalkBlocks timed out while acquiring allocator locks.';
  except
    on E: Exception do
      FailureMessage := E.ClassName + ': ' + E.Message;
  end;
end;

procedure CheckDuplicatePendingFreeDoesNotCreateSelfCycle;
var
  LWalkThread: TWalkThread;
  LFirstWordAfterFirstFree: NativeUInt;
  LFirstWordAfterSecondFree: NativeUInt;
  LSecondFreeRaised: Boolean;
begin
  GContext.InTargetCallback := TEvent.Create(nil, True, False, '');
  GContext.ReleaseTargetCallback := TEvent.Create(nil, True, False, '');
  try
    {Allocate a live small block.  The callback will stop while FastMM is
    walking this exact block under the small-block manager lock.}
    GetMem(GContext.TargetBlock, TestBlockSize);
    if GContext.TargetBlock = nil then
      ExitWithFailure('Initial small-block allocation failed.');

    LWalkThread := TWalkThread.Create(True);
    try
      LWalkThread.Start;

      if GContext.InTargetCallback.WaitFor(5000) <> wrSignaled then
        ExitWithFailure('FastMM_WalkBlocks did not reach the target small block.');

      {First public FreeMem call.  Because the small-block manager is locked by
      FastMM_WalkBlocks, FastMM cannot free the block immediately and instead
      links it into the manager's PendingFreeList.  Reading P^ here is test
      instrumentation: after this call the allocator owns the block contents,
      and P^ should contain the previous pending-list head.  In this controlled
      single-block case that previous head is normally nil.}
      FreeMem(GContext.TargetBlock);
      LFirstWordAfterFirstFree := PNativeUInt(GContext.TargetBlock)^;

      {Second public FreeMem call for the same pointer.  The application call is
      invalid, but FastMM should reject/report it without corrupting allocator
      metadata.  Current upstream queues the same block again and writes the
      block's own address into P^, creating a one-node pending-list cycle.}
      LSecondFreeRaised := False;
      try
        FreeMem(GContext.TargetBlock);
      except
        on E: Exception do
          LSecondFreeRaised := True;
      end;
      LFirstWordAfterSecondFree := PNativeUInt(GContext.TargetBlock)^;

      {The specific corruption is easy to detect before allowing pending-free
      processing to run: if FastMM queued the same pointer again while it was
      already the pending-list head, it wrote P^ := P and created a one-node
      cycle.  Releasing the walker after this would let a later pending-free
      pass hang or repeatedly process the same block, so fail immediately.}
      if LFirstWordAfterSecondFree = NativeUInt(GContext.TargetBlock) then
        ExitWithFailure(
          'BUG: duplicate pending FreeMem created a self-referential pending-free list. ' +
          'P=' + HexPointer(NativeUInt(GContext.TargetBlock)) +
          ' FirstWordAfterFirstFree=' + HexPointer(LFirstWordAfterFirstFree) +
          ' FirstWordAfterSecondFree=' + HexPointer(LFirstWordAfterSecondFree));

      {A fixed build may either raise/report the second invalid free or leave the
      pending list unchanged.  Release the walker and ask FastMM to process the
      still-pending first free so a passing run exits cleanly.}
      GContext.ReleaseTargetCallback.SetEvent;
      LWalkThread.WaitFor;
      if LWalkThread.FailureMessage <> '' then
        ExitWithFailure(LWalkThread.FailureMessage);

      if not FastMM_ProcessAllPendingFrees then
        ExitWithFailure('FastMM_ProcessAllPendingFrees reported a pending lock conflict.');

      if LSecondFreeRaised then
        Writeln('OK: duplicate pending FreeMem was rejected before corrupting the pending-free list.')
      else
        Writeln('OK: duplicate pending FreeMem did not create a pending-free self-cycle.');
    finally
      LWalkThread.Free;
    end;
  finally
    GContext.ReleaseTargetCallback.Free;
    GContext.InTargetCallback.Free;
  end;
end;

begin
  try
    CheckDuplicatePendingFreeDoesNotCreateSelfCycle;
  except
    on E: Exception do
      ExitWithFailure(E.ClassName + ': ' + E.Message);
  end;
end.

Local validation

Built and run with RAD Studio 37.0:

• dcc64 against unmodified ee42f79: reproduces the self-cycle.
• DCC32 against unmodified ee42f79: reproduces the self-cycle.
• dcc64 with a local duplicate-pending-list check: passes.
• DCC32 with a local duplicate-pending-list check: passes.

The fixed output was:

OK: duplicate pending FreeMem was rejected before corrupting the pending-free list.

Suggested fix direction

Before writing PPointer(Block)^ := OldPendingFreeList, scan the current pending-list snapshot for the same block. If the block is already pending, return the invalid-free result instead of relinking it.

The check needs to be inside the CAS retry loop, because a failed CAS means the list head changed and the duplicate check must be repeated against the new snapshot.

Pseudo-code:

while True do
begin
  LOldPendingFreeList := Manager.PendingFreeList;

  if PendingFreeListContains(LOldPendingFreeList, Block) then
  begin
    Result := HandleInvalidFreeMemOrReallocMem(Block, False);
    Break;
  end;

  PPointer(Block)^ := LOldPendingFreeList;
  if AtomicCmpExchange(Manager.PendingFreeList, Block, LOldPendingFreeList) = LOldPendingFreeList then
  begin
    Result := 0;
    Break;
  end;
end;

In my local validation I applied this to:

• the PurePascal small-block pending enqueue path,
• the medium-block pending enqueue path,
• the large-block pending enqueue path,
• the Win32 inline small-block pending enqueue path,
• the Win64 inline small-block pending enqueue path.

The extra scan is only on the locked-manager pending-free path, and only before enqueuing a block, so it should not affect the normal unlocked free fast path.

[pleriche]

Freeing the same block twice is not something that a memory manager is required to be able to deal with gracefully. ChatGPT's assertion in this regard is false. FastMM does try to catch such instances, but it is not guaranteed to be successful and particularly in the case of large blocks the second attempt to free it will likely cause an A/V.

While I am all for trying to make detection and handling of double frees more robust, in this case the cure is worse than the disease: The proposed solution for detecting a doubly freed block in the pending free block list is so expensive that it would slow performance to a crawl.

I'll look into another way to detect such double free attempts, but ChatGPT's proposal is not viable.