Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identifying use after recreate issues #32

Open
obones opened this issue Feb 21, 2023 · 4 comments
Open

Identifying use after recreate issues #32

obones opened this issue Feb 21, 2023 · 4 comments

Comments

@obones
Copy link

obones commented Feb 21, 2023

When activating full debug mode, a freed memory area sees its content filled with the $80808080 pattern.
This means that a reference to an object will see all the fields pointing to invalid memory location triggering an access violation on access.
This is fine as it allows quite easy identification of Use after Free scenarios.

What I'm having a hard time identifying, even in full debug mode, is the following situation:

procedure TSomeObject.DoSomething;
begin
  if Assigned(FOnSomeEvent) do
    FOnSomeEvent();

  if FFirstField.SomeProperty then
    SomeOtherMethod;
end;

Due to some poorly constructed code, the FOnSomeEvent callback will destroy the current TSomeObject instance and immediately create an instance of another class that happens to have the same instance size and very similar fields.
This leads to the following scenario:

TSomeObject.DoSomething begin -> Self = $123456, Self.FFirstField = $789012
FOnSomeEvent, destroys TSomeObject -> FFirstField = $80808080
FOnSomeEvent, creates TOtherClass -> Instance = $123456, Instance.FFirstField = $456789
TSomeObject.DoSomething tries to access FFirstField

Here, I would have expected FFirstField to be $80808080 but because of the immediate allocation of a same sized object, its value is somewhat valid and no crash occurs, despite accessing a destroyed object.

Well, I say "no crash", that's only valid for some time, under some conditions but I have seen a crash from times to times, especially in the 64 bit builds of our applications, while the 32 bits one rarely if ever crash and exhibit the above behavior.
The difference could come from different background threads and that makes it hard to track down.

This is where I think FastMM could help, by not giving back the same memory area immediately. I tried to look in the source code for such a feature but I could not find it. Note that I would only need it in full debug mode as this means memory would only be reused after a while, leading to large memory usage in production cases.

Is there already an option that I overlooked?

@pleriche
Copy link
Owner

pleriche commented Feb 21, 2023

Hi Olivier,

Unfortunately there is no such option currently. Preference is given to the reuse blocks that are "hot" in the cache - also in debug mode.

When FreeMem is called on a debug block then FastMM_FreeMem_FreeDebugBlock is called and from there the actual freeing operation is routed back to FastMM_FreeMem. I cannot change FastMM_FreeMem without affecting the performance outside of debug mode, so the approriate place to delay the reuse of debug blocks would be FastMM_FreeMem_FreeDebugBlock.

I was thinking that perhaps I could add a "free queue" in FastMM_FreeMem_FreeDebugBlock whereby the last few free operations are held back. It could take the form of a FIFO queue of the last 50, 100 or more frees. This would at least prevent a freed debug block from being reused immediately. In order to avoid too much address space pressure I could limit this delayed freeing to blocks under a certain size.

Of course, if there are more frees than the capacity of the FIFO queue in-between the TSomeObject.Destroy and TOtherClass.Create calls then the same block could still be reused, so there would be no guarantees, but the likelihood would be lower.

Will this work for you, or do you have a different solution in mind?

Best regards,
Pierre

@obones
Copy link
Author

obones commented Feb 21, 2023

Well, that was my assessment as well when I saw that FastMM_FreeMem_FreeDebugBlock ultimately calls FastMM_FreeMem so at least I have the comfort of knowing I understood the situation a little bit.

As to a solution, I did not have one specifically in mind but a free queue is definitely something that makes sense to me. I totally understand the limits that you pointed out but as you do, I believe them to be quite acceptable and manageable.
May I suggest 100 items of 1024 bytes maximum each? That would take 100k at most which is definitely nothing to be afraid off, especially considering this would only come out in debug mode.

@pleriche
Copy link
Owner

I'll see what I can do.

@obones
Copy link
Author

obones commented Feb 24, 2023

Thanks.
No special hurry here, as I have found and fixed the offending code already.
On further thoughts, having a configurable number of items and size limit would be ideal, I have multiple scenarios in mind where those limits would be different. A compile time setting would be enough for me, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants