Failed memory-manager mode transitions poison nesting counters and later report success

[janrysavy]

Context

This comes from a GPT-5.6-SOL-ULTRA follow-up review of current FastMM5 master. I
reviewed the finding manually and reproduced it locally against the unmodified
revision:

13976edfc84f83c3c55e5731d30fd8686ad51f15
#81 Export FastMM_ProcessAllPendingFrees in BorlndMM.dll

The issue is related to, but distinct from, #72. It does not require concurrent
mode switching, FullDebugMode, or an application callback.

Problem

FastMM_SetMemoryManagerEntryPoints refuses to modify the installed memory
manager if it has been changed externally:

function FastMM_SetMemoryManagerEntryPoints: Boolean;
begin
  {Check that the memory manager has not been changed since the last time it was set.}
  if FastMM_InstalledMemoryManagerChangedExternally then
    Exit(False);

  {Select and install the configured entry points...}
end;

The public debug/erasure mode functions modify their nesting counter before this
operation and do not roll the modification back when it returns False.

For example:

function FastMM_BeginEraseFreedBlockContent: Boolean;
begin
  SetMemoryManagerLock.Lock;
  try
    if CurrentInstallationState = mmisInstalled then
    begin
      Inc(EraseFreedBlockContentCounter);
      if EraseFreedBlockContentCounter = 1 then
        Result := FastMM_SetMemoryManagerEntryPoints
      else
        Result := True;
    end
    else
      Result := False;
  finally
    SetMemoryManagerLock.Unlock;
  end;
end;

A single-threaded failure sequence is enough:

1. An external memory-manager record is installed after FastMM.
2. The first FastMM_BeginEraseFreedBlockContent increments the counter from 0
   to 1.
3. FastMM_SetMemoryManagerEntryPoints detects the external change and returns
   False.
4. The counter incorrectly remains 1.
5. A second begin call increments it to 2 and returns True without retrying the
   entry-point installation.
6. The external FreeMem entry point remains installed, so freed blocks are not
   erased even though the public call reported success.

FastMM_EraseFreedBlockContentActive also reports True, because it only tests
whether the poisoned counter is positive:

function FastMM_EraseFreedBlockContentActive: Boolean;
begin
  Result := EraseFreedBlockContentCounter > 0;
end;

The same counter-before-installation pattern appears in all three mode families:

• FastMM_EnterDebugMode / FastMM_ExitDebugMode;
• FastMM_BeginEraseAllocatedBlockContent /
  FastMM_EndEraseAllocatedBlockContent;
• FastMM_BeginEraseFreedBlockContent /
  FastMM_EndEraseFreedBlockContent.

Both begin/enter and end/exit transitions can leave the counter inconsistent if
the required entry-point update fails.

Reproducer

The reproducer copies FastMM's installed TMemoryManagerEx, changes only its
GetMem entry point to a pass-through wrapper, and installs that record through
the RTL. This safely makes FastMM_InstalledMemoryManagerChangedExternally
return True while preserving ordinary allocation behavior.

It then calls FastMM_BeginEraseFreedBlockContent twice and verifies that the
second call returns True even though the original external FreeMem entry point
is still installed.

Cleanup first restores FastMM's original memory-manager record and calls the end
routine only while the public active-state query remains true. Thus the buggy
implementation drains its two poisoned nesting levels, while a fixed
implementation whose failed begin calls roll back to zero makes no unmatched end
calls.

program ModeFailureCounterRollbackRegression;

{$APPTYPE CONSOLE}

uses
  FastMM5;

var
  GOriginalMemoryManager: TMemoryManagerEx;

function ForwardGetMem(ASize: NativeInt): Pointer;
begin
  Result := GOriginalMemoryManager.GetMem(ASize);
end;

function SameFreeMemEntryPoint(const ALeft, ARight: TMemoryManagerEx): Boolean;
begin
  Result := NativeUInt(@ALeft.FreeMem) = NativeUInt(@ARight.FreeMem);
end;

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

var
  LExternalMemoryManager: TMemoryManagerEx;
  LFirstResult: Boolean;
  LInstalledAfterSecondCall: TMemoryManagerEx;
  LSecondResult: Boolean;
begin
  GetMemoryManager(GOriginalMemoryManager);
  LExternalMemoryManager := GOriginalMemoryManager;
  LExternalMemoryManager.GetMem := ForwardGetMem;

  SetMemoryManager(LExternalMemoryManager);
  try
    LFirstResult := FastMM_BeginEraseFreedBlockContent;
    LSecondResult := FastMM_BeginEraseFreedBlockContent;
    GetMemoryManager(LInstalledAfterSecondCall);
  finally
    SetMemoryManager(GOriginalMemoryManager);
    while FastMM_EraseFreedBlockContentActive do
    begin
      if not FastMM_EndEraseFreedBlockContent then
        Fail('Cleanup could not restore the erase-before-free nesting state.');
    end;
  end;

  if LFirstResult then
    Fail('Unexpected: first begin call succeeded after an external memory-manager change.');

  if LSecondResult
    and SameFreeMemEntryPoint(LInstalledAfterSecondCall, LExternalMemoryManager) then
    Fail('BUG: the second begin call returned True although the erase-before-free entry point was never installed.');

  Writeln('OK: failed mode transition did not poison the nesting counter.');
end.

Local validation

Built and run with RAD Studio 37.0 against unmodified 13976ed:

Build	Result
Default Win32	Exit 1: false success reproduced
Default Win64	Exit 1: false success reproduced

Both runs print:

BUG: the second begin call returned True although the erase-before-free entry point was never installed.

Both results were reproduced again while preparing this report.

Expected fixed output:

OK: failed mode transition did not poison the nesting counter.

Impact and scope

This violates the Boolean success contract of the public mode functions and makes
the corresponding ...Active state queries unreliable after a failed transition.
The freed-content mode is documented as useful for security purposes, so false
success can also cause an application to believe freed memory is being erased
when its installed FreeMem entry point performs no erasure.

The reproducer deliberately uses an external memory-manager change—the exact
failure condition FastMM detects and reports through False. It does not rely on
invalid allocation/free behavior or a rapid cross-thread transition.

I would rate this as Medium severity for configuration integrity and false
security assurance.

Suggested fix direction

Make each counter transition transactional while SetMemoryManagerLock is held:

• if a 0-to-1 enter/begin transition cannot install its entry points, decrement
  the counter back to 0;
• if a 1-to-0 exit/end transition cannot install its entry points, increment the
  counter back to 1;
• apply the same rule consistently to debug mode, allocated-content erasure, and
  freed-content erasure.

For example, the begin side can retain the existing shape and add rollback only
on the transition that calls FastMM_SetMemoryManagerEntryPoints:

Inc(EraseFreedBlockContentCounter);
if EraseFreedBlockContentCounter = 1 then
begin
  Result := FastMM_SetMemoryManagerEntryPoints;
  if not Result then
    Dec(EraseFreedBlockContentCounter);
end
else
  Result := True;

This does not add work to the allocation/free fast paths. The affected public
configuration calls already hold SetMemoryManagerLock.

Prior issue check

Issue #72 reported a concurrent
nested FastMM_EnterDebugMode returning before the first thread completed the
entry-point transition. It noted that the erase routines had a similar transition
shape, and the resulting fix serialized transitions.

This finding is different: it is a failure-path rollback defect after
FastMM_SetMemoryManagerEntryPoints explicitly returns False, it reproduces with
one thread, and it causes a later call to return a false success without retrying.

[janrysavy]

Reopening because a6af62d fixes the immediate false-success result, but failed transitions are still retained in the nesting counter. This is the same FastMM_BeginEraseFreedBlockContent path and external-memory-manager setup as the original report, followed by one balanced recovery begin/end pair after FastMM's manager is restored.

On current master ade11ffad4f50646b34f7258a0c5793011b29f6f, the failed calls leave the counter at 2. The recovery pair changes it to 3 and then 2, so FastMM_EraseFreedBlockContentActive incorrectly remains True. I reproduced this with RAD Studio 37.0 on both Win32 and Win64; both runs print the BUG line and exit 1.

program ModeFailureCounterRecoveryRegression;

{$APPTYPE CONSOLE}

uses
  FastMM5;

var
  GOriginalMemoryManager: TMemoryManagerEx;

function ForwardGetMem(ASize: NativeInt): Pointer;
begin
  Result := GOriginalMemoryManager.GetMem(ASize);
end;

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

var
  LExternalMemoryManager: TMemoryManagerEx;
  LFirstFailedBegin: Boolean;
  LSecondFailedBegin: Boolean;
  LActiveAfterBalancedPair: Boolean;
begin
  GetMemoryManager(GOriginalMemoryManager);
  LExternalMemoryManager := GOriginalMemoryManager;
  LExternalMemoryManager.GetMem := ForwardGetMem;

  SetMemoryManager(LExternalMemoryManager);
  try
    LFirstFailedBegin := FastMM_BeginEraseFreedBlockContent;
    LSecondFailedBegin := FastMM_BeginEraseFreedBlockContent;
  finally
    SetMemoryManager(GOriginalMemoryManager);
  end;

  if LFirstFailedBegin or LSecondFailedBegin then
    Fail('A transition unexpectedly succeeded with an external manager installed.');

  if not FastMM_BeginEraseFreedBlockContent then
    Fail('The recovery begin unexpectedly failed.');
  if not FastMM_EndEraseFreedBlockContent then
    Fail('The recovery end unexpectedly failed.');

  LActiveAfterBalancedPair := FastMM_EraseFreedBlockContentActive;
  while FastMM_EraseFreedBlockContentActive do
    if not FastMM_EndEraseFreedBlockContent then
      Fail('Cleanup failed.');

  if LActiveAfterBalancedPair then
    Fail('BUG: failed transitions remained in the nesting count after recovery.');

  Writeln('OK: failed transitions were rolled back before recovery.');
end.

The original requested behavior still seems appropriate: if a transition returns False, its counter adjustment should be rolled back while SetMemoryManagerLock is held.

[TetzkatLipHoka]

Confirmed on ade11ff: two failed FastMM_BeginEraseFreedBlockContent calls against an external manager leave the counter at 2, and the balanced recovery pair then leaves FastMM_EraseFreedBlockContentActive stuck True, reproduced on Win32 and Win64. Worth noting the contamination also crosses modes - because FastMM_SetMemoryManagerEntryPoints reads all three counters, a poisoned erase-freed counter makes a later successful FastMM_EnterDebugMode install the erase-freed entry point too.

I've opened #94 with the rollback-under-lock fix you suggested, applied to all three Adjust*Counter helpers (both the not-installed early-out and the FastMM_SetMemoryManagerEntryPoints failure path). Verified with 10 Seattle and 13.1 (Win32+Win64): all three modes roll back, the cross-mode case is gone, and normal nesting is unchanged. Same fix is in our fork with a regression test covering the three modes plus the cross-mode scenario.

[pleriche]

The way these calls always worked is that the number of Begin/Enter calls must match the number of End/Exit calls. If Begin/Enter returns False you should still call End/Exit. The reason for this is so you can call these routines without checking the result, as many applications probably do. Best would perhaps be to remove the Boolean result altogether, but it's probably too late for that.

I will update the documentation to make this clear.