Memory protected allocations feature request

[hvassbotn]

Hi Pierre,

And then a feature request, that I think would be very useful. As you know, debugging and finding the actual cause of memory overwrites can be very tricky indeed (since any buggy code or DLL could cause it).

Earlier we had success using FastMM to detect reproducible memory overwrites, then using the madExcept debug feature "instantly crash on buffer...." "overrun" to find the culprit. That isn't always enough and for our application, not really feasible as we have way too many allocations for realistic workflows.

So after thinking about this some more, I think I would like two separate features, actually:

1. Global flag to turn on Memory overrun protected allocations
   In this mode, FastMM would revert to allocate each memory block from separate 4 Kb pages, and put the user accessible part of the allocation at the end of the page. It should then also virtual protect the next page in address space, so that any memory overrun after the size of the block would cause an access violation.

2. Global flag to turn on Memory overrun protected allocations and frees
   This mode would work like the one above, with the difference that when freeing the memory block (in debug mode), it would also change the virtual protection bits for the allocated user page to be no-read and no-write. Then any in-memory overwrite or even read in an already freed block would cause an access violation.

Both these flags would only be used in debug situations and only for specific short periods of time. For instance, the overwrite it detected now, happened on a block allocated by TSizeConstraints.Create (for some reason). To debug it, I would patch the Vcl.Controls unit to use FastMM5 and to turn on the new global setting FastMM_DebugMode_AllocateAndFreeProtection := True before the call to TSizeConstraints.Create and FastMM_DebugMode_AllocateAndFreeProtection := False after the call. FastMM would know to allocate in this mode, and the mode would be encoded into the block, so that it would know to keep and expand the protection when it is freed.

I don't know if I was able to convey the concept properly.
I might find the time to try to code this myself and create a pull request for you.

/Hallvard

[pleriche]

Hi Hallvard,

This is a feature that has been mooted before, but it never got off the ground. The concern was around the increase in address space usage and whether it would be possible to run in this mode for long enough to catch the buffer overrun before running out of address space (or entering disk swapping hell under 64-bit). Perhaps a surgical enable and disable around a small chunk of code would be viable.

For all practical purposes this would imply a new block type, but to prevent duplication of code it should probably share as much as possible with the "debug" block type. The format of debug blocks would then need to change in order to accommodate the scenario where there is no block footer. (Variable length entries like the stack traces currently go in the footer.) You would also need to maintain a separate list of all such blocks, since in the case of protected freed blocks you cannot store a linked list inside the block itself.

This is all doable of course, but it's not a quick task. Once you have the core up and running then there's all the peripheral stuff that also needs to work: Walking the memory pool, leak reporting, statistics gathering, etc.

There are some other debugging aids that are also fairly drastic, like FastMM_DebugMode_ScanForCorruptionBeforeEveryOperation. You can also inject liberal calls to FastMM_ScanDebugBlocksForCorruption. Have you had any success with these?

Best regards,
Pierre

[hvassbotn]

Thanks for the quick feedback, Pierre!

You raise good concerns, but I think even a much simpler solution would be highly useful.

Since this is a debug tool that (IMO) should only be used for a very few allocations / deallocations, there is no need to maintain free lists (don't care about leaks while debugging a specific, known memory overwrite only). Nor any need to keep variable length entries like the stack trace.

It's been a while since I dived into the FastMM code, but as far as I can see, the debug header record is allocated before the user block, and thus can be kept. We'd need to allocate another bit here:

  {Block status flags}
  CBlockIsFreeFlag = 1;
  CHasDebugInfoFlag = 2;
  CIsSmallBlockFlag = 4;
  CIsMediumBlockFlag = 8;
  CIsLargeBlockFlag = 16;
  CIsDebugBlockFlag = 32;

for instance:

CIsProtectedDebugBlockFlag = 64;

When freeing the block, we first set the CBlockIsFreeFlag bit as usual, an only then change the VirtualProtect bits of the pages of the allocation to make them (at least) write protected, maybe even read-protected. I don't think FastMM would need to ever look at the memory block again - we let it stay allocated at the OS level, and rely on the processor to break with an AV exception whenever the bad code does the memory overwrite (or even read).

So yes, it is a "quick" and dirty tool that causes minor leaks, does not clean up after itself, does not provide all the niceties you get with the normal debug blocks, but that is fine, as long as it makes it easier (or even possible) to find the cause of memory overwrites.

I appreciate it still will be a lot of work, and I fully understand if you cannot commit time to it. I'm not sure I will be able to either.

Maybe our discussion can cause inspiration for someone else to have a go... :).

[hvassbotn]

Just as a short follow-up, I was able to find the memory overwrite - using the same basic CPU memory protection technique as suggested above, just not by messing the the FastMM code, but rather by replacing the TObject NewInstance and FreeInstance methods at runtime, for this specific class that triggered the problem.

I patched the code in the initialization section of the standard Vcl.Controls unit:

{$DEFINE DEBUG_FREED_TSIZECONSTRAINTS_OVERWRITE}
{$IFDEF DEBUG_FREED_TSIZECONSTRAINTS_OVERWRITE}
function TVmtHack_NewInstance(Self: TClass): TObject;
begin
  Result := Winapi.Windows.VirtualAlloc(nil, Self.InstanceSize, MEM_COMMIT, PAGE_READWRITE);
  Result := Self.InitInstance(Result);
end;

procedure TVmtHack_FreeInstance(Self: TObject);
var
  Ignore: DWORD;
begin
  Self.CleanupInstance;
  Winapi.Windows.VirtualProtect(PPointer(Self)^, Self.InstanceSize, PAGE_NOACCESS, Ignore);
end;

procedure TurnoffCodeProtection(Vmt: PVmt);
var
  RestoreProtection: DWORD;
begin
  if Winapi.Windows.VirtualProtect(Vmt, SizeOf(Vmt^), PAGE_EXECUTE_READWRITE, RestoreProtection) then
    FlushInstructionCache(GetCurrentProcess, Vmt, SizeOf(Vmt^));
end;

procedure DoDebugFreedTSizeConstraintsOverwrite;
begin
  var Vmt := GetVmt(TSizeConstraints);
  TurnoffCodeProtection(Vmt);
  Vmt.NewInstance := TVmtHack_NewInstance;
  Vmt.FreeInstance := TVmtHack_FreeInstance;

end;

{$ENDIF}

initialization
{$IFDEF DEBUG_FREED_TSIZECONSTRAINTS_OVERWRITE}
  DoDebugFreedTSizeConstraintsOverwrite;
{$ENDIF}

With this in place I got a hardware AV exception when the memory overwrite occurred - caused by a double-free operation - and it was then easy to fix the problem :). Once fixed I could undefine DEBUG_FREED_TSIZECONSTRAINTS_OVERWRITE, or just remove the hack altogether. Could be nice to reuse the technique for another rainy day, though :).

This hack relies on my old HVVMT unit from here:
https://hallvards.blogspot.com/2006/03/hack-8-explicit-vmt-calls.html

Note that there have been some updates since then - my current version of the structures:

// Virtual method table
  PVmt = ^TVmt;
  TVmt = packed record
    SelfPtr           : TClass;
    IntfTable         : Pointer;
    AutoTable         : Pointer;
    InitTable         : Pointer;
    TypeInfo          : Pointer;
    FieldTable        : PPft;
    MethodTable       : PPmt;
    DynamicTable      : PDmt;
    ClassName         : PShortString;
    InstanceSize      : IntPtr;
    ClassParent       : PClass;
{$IF CompilerVersion >= 20}
    Equals            : PEquals;
    GetHashCode       : PGetHashCode;
    ToString          : PToString;
{$IFEND}
    SafeCallException : PSafeCallException;
    AfterConstruction : PAfterConstruction;
    BeforeDestruction : PBeforeDestruction;
    Dispatch          : PDispatch;
    DefaultHandler    : PDefaultHandler;
    NewInstance       : PNewInstance;
    FreeInstance      : PFreeInstance;
    Destroy           : PDestroy;
{$IFDEF CPP_ABI_SUPPORT}
    CPP_ABI_1         : Pointer;
    CPP_ABI_2         : Pointer;
    CPP_ABI_3         : Pointer;
{$IFEND}
   {UserDefinedVirtuals: array[0..999] of procedure;}
  end;

// Virtual method table

function GetVmt(AClass: TClass): PVmt;
begin
  Result := PVmt(AClass);
  Dec(Result);
end;

[hvassbotn]

And yes, using VirtualAlloc for each allocation is overkill (64 kB per instance), but I was too lazy to do it properly with HeapCreate/HeapAlloc etc.

[pleriche]

Hi Hallvard,

I'm glad you found the issue in your software. That was quite a neat trick you used!

I'm wondering though: Why would FastMM's debug mode not have caught the double free? Even outside of debug mode a double free should raise an immediate exception. Are there any additional checks I could add to FastMM that would have caught it?

Back to the feature request:
The implementation you propose is indeed a lot less work, but I know it will not be long before I cave in to pressure to implement the "full" solution. If I were to undertake this I might as well plan to go all in from the start.

I'm thinking of maintaining separate lists of all the "protected allocations" and "freed protected allocations". Once you switch out of buffer overrun protection mode then all the address space reserved for the latter ("freed protected allocations") that are still in existence should be released to the OS immediately. The memory for "protected allocations" can obviously only be released once the block is freed, but there's no need to let ones that have been freed by the application linger any longer.

I would not want to waste up to 64K address space per protected allocation so I'm also going to have to do some manual housekeeping and subdivide the 64K chunks returned by VirtualAlloc myself. This way I will be able to bring down the address space usage of each small block "protected allocation" from 64K to 8K. Still terrible, but less terrible :).

I would also want to include all protected allocations when walking the heap for leak reports, etc.

I'm putting it on my to-do list, but I can't promise anything at this stage.

Best regards,
Pierre

[hvassbotn]

Yes, a neat trick, but I suspect I haven't really found the original memory overwrite.

It is a little like a Heisenbug - when you try to look / catch the overwrite you change the environment so that it is not deterministic anymore. The issue with memory overwrites, of course, is that you have some unknown code site A that is overwriting the memory allocated (and sometimes freed) of code B. I changed the way the memory was allocated (VirtualAlloc instead of GetMem), that changed the address, and now code A is most likely overwriting some other memory block, not the protected one I allocated.

Yes, I was also surprised to see the double free generate an AV instead of being caught by the FastMM logic, but I think it can happen if the two frees are "far" apart in space/time and there can be allocations in between that re-use the initially freed memory. Then there can be a "dangling" pointer to this memory block (possibly in the middle of it) that crashes when you try to free it. I'm not sure...

[davidberneda]

PFJMI, maybe a brute force slow log with allocation/deallocation addresses and sizes can be post-mortem analyzed?

[hvassbotn]

Hi David,

Thanks for jumping in - long time! :)

Yes, that is a good idea. I think I also will follow Pierre's original suggestion and sprinkle FastMM_ScanDebugBlocksForCorruption calls at suspected sites, maybe doing a kind of binary search to narrow down on the possible bad code sites.

I did try to call FastMM_ScanDebugBlocksForCorruption inside the application WndProc, but that was too coarse.

I know the problem occurs when I have a number of forms open, then close the app (after a specific dialog box has been opened, that performs the alloc/free of the overwritten block). So the FormClose/FormDestroy methods are the main suspects, I think.