FastMM_ScanDebugBlocksForCorruption no longer detects header/footer corruption of small debug blocks (regression in cad1f04)

[TetzkatLipHoka]

Since cad1f04 ("Fix a potential race condition in FastMM_WalkBlocks: The header for a small block cannot be assumed to be valid if it was recently split off from a sequential feed span") FastMM_ScanDebugBlocksForCorruption silently ignores small debug blocks whose header or footer checksum is wrong - which is exactly the state it exists to find. Medium and large blocks are unaffected, and so is the use-after-free (fill pattern) check for small blocks.

Why

In the small block branch of FastMM_WalkBlocks, the decision whether the block may be reported as a debug block ends with the two checksum comparisons:

LPDebugHeader := PFastMM_DebugBlockHeader(LBlockInfo.BlockAddress);
LMayCheckForDebugInfo := BlockHasDebugInfo(LPDebugHeader)
  and (GetSpanForSmallBlock(LPDebugHeader) = LPMediumBlock)
  and (LBlockInfo.UsableSize > CDebugBlockHeaderSize)
  and (LBlockInfo.UsableSize >= CDebugBlockHeaderSize + LPDebugHeader.UserSize
    + CalculateDebugBlockFooterSize(LPDebugHeader.StackTraceEntryCount))
  and (LPDebugHeader.CalculateHeaderCheckSum = LPDebugHeader.HeaderCheckSum)
  and (LPDebugHeader.CalculateFooterCheckSum = LPDebugHeader.DebugFooterPtr^);

A block whose checksums do not match is therefore handed to the callback with DebugInformation = nil, and FastMM_ScanDebugBlocksForCorruption_CallBack returns at its first line. So the one condition the scan is looking for is the one condition that hides the block from it. (The same applies to FastMM_DebugMode_ScanForCorruptionBeforeEveryOperation.)

The corruption is still caught when the block is eventually freed, via CheckDebugBlockHeaderAndFooterCheckSumsValid - what is lost is the early detection the scan provides, and the detection of blocks that are never freed.

Repro

program ScanBlindSpot;
{$APPTYPE CONSOLE}
uses
  FastMM5, SysUtils;

function ScanDetects: Boolean;
begin
  try
    FastMM_ScanDebugBlocksForCorruption(1000);
    Result := False;
  except
    Result := True;
  end;
end;

procedure Test(const AWhat: string; ASize: Integer);
var
  LP: Pointer;
  LHeader: PFastMM_DebugBlockHeader;
  LOriginal: Cardinal;
  LDetected: Boolean;
begin
  GetMem(LP, ASize);
  LHeader := PFastMM_DebugBlockHeader(PByte(LP) - SizeOf(TFastMM_DebugBlockHeader));
  LOriginal := LHeader.HeaderCheckSum;
  LHeader.HeaderCheckSum := LOriginal xor $DEADBEEF;   //simulate a corruption
  LDetected := ScanDetects;
  LHeader.HeaderCheckSum := LOriginal;                 //repair before freeing
  FreeMem(LP);
  WriteLn(AWhat, ' -> detected: ', LDetected);
end;

begin
  FastMM_MessageBoxEvents := [];
  FastMM_LogToFileEvents := [];
  FastMM_EnterDebugMode;
  Test('small  (100 bytes)', 100);
  Test('medium (50000 bytes)', 50000);
  Test('large  (300000 bytes)', 300000);
  FastMM_ExitDebugMode;
end.

Output on current master (fb46810), Delphi 13.1 Win32:

small  (100 bytes) -> detected: FALSE
medium (50000 bytes) -> detected: TRUE
large  (300000 bytes) -> detected: TRUE

I ran a slightly larger matrix (header checksum corrupted, buffer overrun into the debug footer, and a write into a freed block) across the three size classes:

Case	cad1f04^	fb46810
header checksum, small (100 and 2000 bytes)	detected	not detected
header checksum, medium / large	detected	detected
overrun into the footer, small	detected	not detected
overrun into the footer, medium / large	detected	detected
write after free (fill pattern), all sizes	detected	detected

So all ten cases pass on the commit before cad1f04, and three fail on master. The use-after-free case still works for small blocks because a freed debug block's checksums are recalculated when it is freed, so they are valid and the block stays visible to the scan.

Suggested fix

The first four conditions are what make the block safe to touch: the debug flag, the span identity check, and the two size checks that keep DebugFooterPtr inside the block. It is only the last two - the checksums themselves - that swallow real corruption. Dropping those two lines restores all ten cases:

LMayCheckForDebugInfo := BlockHasDebugInfo(LPDebugHeader)
  and (GetSpanForSmallBlock(LPDebugHeader) = LPMediumBlock)
  and (LBlockInfo.UsableSize > CDebugBlockHeaderSize)
  and (LBlockInfo.UsableSize >= CDebugBlockHeaderSize + LPDebugHeader.UserSize
    + CalculateDebugBlockFooterSize(LPDebugHeader.StackTraceEntryCount));

A half-written header that gets through the remaining checks is then handled by the retry loop you added in the scan callback: it re-checks BlockHasDebugInfo and the checksums up to 100 times before reporting anything, so a block that is still being initialised converges instead of being flagged.

To see whether that reintroduces the problem cad1f04 was about, I ran a stress test: 6 threads continuously allocating, filling and freeing small debug blocks (16..2016 bytes, 64 live blocks each, so spans are constantly fed, split off and recycled) while a seventh thread runs FastMM_ScanDebugBlocksForCorruption in a loop. Nothing corrupts anything, so any exception from the scan is a false positive. 25 seconds per variant, Delphi 13.1 Win32:

Variant	allocations	scans	false positives	A/Vs
cad1f04^ (no guard at all)	244 M	72,308	0	0
fb46810 (current)	254 M	56,427	0	0
fb46810 minus the two checksum conditions	259 M	69,791	0	0

I should be explicit about what this does and does not show: my workload does not reproduce the original race (the unguarded build is clean too), so it is evidence that the proposed change does not make things worse here, not evidence that the guard is unnecessary. If you have the repro from the original report, that is the one worth running against the change.

If you would rather keep the conservative behaviour for the other FastMM_WalkBlocks consumers (FastMM_LogStateToFile, the leak report), the alternative is to make the strictness a parameter of the walk so only the corruption scan opts into seeing blocks with mismatched checksums. That is a bigger change and your call - I am happy to send whichever you prefer as a PR, with the test programs above.

Found while merging your recent changes into my fork; the test programs are plain console Delphi and need nothing from the fork.

[pleriche]

Oh dear, that was not intended. Sorry. It's a bit of a catch-22 unfortunately: The header for a small block cannot be relied on to be valid during a block scan, so I either have to take the chance of a false positive or not check for corruptions at all.

The intial change was due to an AI report. I don't have a test case to actually reproduce it since the window for it to occur is infinitesimally small. I've modified the check to only look at the block header and not the purported debug content.

Assuming random in-memory data there's a 1 in 32768 chance that an invalid small block header will appear valid. Assuming another thread has just split off that same block and hasn't set its header yet and it ends up in the debug block checks in FastMM_ScanDebugBlocksForCorruption_CallBack, then there's still a loop that rechecks the block 100 times. I did consider adding a SwitchToThread call in the loop, but eventually decided it may be a bit excessive.

[TetzkatLipHoka]

Verified on 1a6fb17 with Delphi 13.1 Win32 - thank you for the quick turnaround.

• The full matrix is green again: header checksum corrupted, buffer overrun into the debug footer, and a write into a freed block, each for small (100 and 2000 bytes), medium and large blocks - 10 of 10 detected.
• Race stress (6 threads allocating, filling and freeing small debug blocks, one thread scanning in a loop, 25 s): 252 M allocations, 70,229 completed scans, 0 false positives, 0 A/Vs.

I also probed the thing I would have worried about in your simplification: the two size conditions you dropped were bounding where the scan reads, since DebugFooterPtr is derived from UserSize and the stack traces from StackTraceEntryCount. So I corrupted exactly those fields and nothing else (UserSize := $30000000, StackTraceEntryCount := 255), allocated and freed, across all three size classes. Every case comes out as a clean EInvalidPointer, never an access violation.

The reason is that any corruption of those fields also invalidates the header checksum, and the header checksum is compared first - with {$BoolEval Off} the footer read is short-circuited away, and CheckDebugBlockHeaderAndFooterCheckSumsValid likewise returns at the header check before touching the footer. So there is no wild-read path through the removed conditions; they were only ever filtering, and the filtering is what hid the corruption. Worth knowing if anyone ever reconsiders that expression: the ordering of those two comparisons is load-bearing.

On the 1 in 32768: with the retry loop and the new early Exit when the debug flag is no longer set, a transient look-alike header has to keep looking valid across all 100 attempts to produce a false positive, which seems a fair trade against a scan that could not see small block corruption at all. Agreed on leaving SwitchToThread out.

[pleriche]

I also probed the thing I would have worried about in your simplification: the two size conditions you dropped were bounding where the scan reads, since DebugFooterPtr is derived from UserSize and the stack traces from StackTraceEntryCount.

I was a bit ambivalent about this change. On the one hand checking the UserSize in the debug header against the actual block size will further reduce the likelihood of a false positive, but it may also cause a corruption of the UserSize field to go unnoticed. In the end I decided to only tentatively trust the two byte small block header and none of the debug info.

P.S. I have seen your other open issues. I will respond in due course.

[TetzkatLipHoka]

it may also cause a corruption of the UserSize field to go unnoticed

That part you can stop worrying about - with the code as it now stands it does not go unnoticed, and I measured it rather than reasoned about it. Corrupting UserSize (to $30000000) or StackTraceEntryCount (to 255) and nothing else, allocated and freed, across all three size classes: 8 of 8 cases come out as a clean EInvalidPointer.

The reason it works out is the one thing worth writing down: CalculateHeaderCheckSum covers the size fields, so corrupting one of them invalidates the header checksum, and the header checksum is compared before anything derived from UserSize is read. Complete boolean evaluation is off, so the footer read is short-circuited away, and CheckDebugBlockHeaderAndFooterCheckSumsValid returns at the header check for the same reason. The only way a corrupted UserSize could slip through is if the corruption also happened to leave the stored checksum matching, which is a one in 2^32 coincidence rather than a mode of failure.

So the ordering of those two comparisons is load-bearing now, not cosmetic. I have put that case into the test suite in #103 as FastMM5Test_ScanHeaderBounds - it corrupts exactly those fields and fails on an access violation, so if the comparisons are ever reordered, or the footer is read before the header is validated, it says so instead of turning into a rare crash in the field.

No rush on the rest - thanks for turning this one around so quickly.