How should we deal with weak refs to finalizable objects? (was: Re: [PATCH] Bugfix and drop weak references to finalizable objects (was: Re: [PATCH] thread-safe handling of asynchronous events))

[Peter Bex]

On Fri, Jul 07, 2023 at 11:23:17PM +0200, felix.winkelmann@bevuta.com wrote:
> I'm not very comfortable with this change. This feels like trading in
> one inconsistency (weak refs being cleared for a potentially non-dead
> object) for another (potentially inconsistent ties of GC-controlled
> memory to non-GC'd resources).

It depends on how you view finalizers.  Personally, I would think a
finalizer should get run on what's *essentially* "already GCed" data.
Therefore, it makes no sense to pass a finalizer data that still holds
onto other cleared data.

But like I said, I can get behind your POV - you could also argue that
if an object holds onto other things and it's "already GCed", all the
things it (and nobody else) holds onto (even strongly) should be
cleared, and that absolutely makes no sense whatsoever.

(come to think of it, the ideal solution would probably be to clear
"outside" weak references to finalizable data but keep the object
itself internally intact.  But that's extremely hard to track in the GC)

However, there's one more concern:

> The potential use-after-free scenario can still happen if the object is
> kept alive, regardless of how we handle weak refs, this is unavoidable
> if we allow finalizers and keep the possibility of resurrection.

I have thought about this a bit more but I came to the conclusion that
from an abstraction point of view it's better to clear weak refs to
finalized data.  The reason is that when a module exposes an object, the
*user* should not need to know or care exactly how that object is
implemented and that it happens to use a finalizer.

So let's say an egg exposes an object, and I'm using it, but I want to
reference it weakly.  Then, all things will work fine most of the time.
However, there's a nasty race condition lurking: if the finalizer
happens to run before my code extracts the object from the weak ref,
but I extract it before the next GC, my code may crash.  Or it may not,
use-after-free is tricky like that.

This should not be the user's concern, and it's a breach of abstraction.
It's also a global issue, as it can affect *any* code that holds onto
an object from a "3rd party" (other module).

Note that this would be problematic *even* if the 3rd party wrote the
code so carefully that it clears the pointer from the object such that
there's no use-after-free bug.  Because the code will still raise an
exception when passed this invalidated object.  And again, that will
be a race condition for the person who uses this module.  This means
that the problem is spread to every single user who decides to use a
weak reference to a 3rd party object that involves resources that must
be freed.

On the other hand, if I'm writing an egg that exposes a foreign object
that needs to be collected, it *is* of my concern (and not a breach of
abstraction) that the data gets freed properly.  Let's say I decide to
store weak refs inside the object (which is not even *that* likely),
and those weak refs point to other things inside the same object, which
I know to be GCed at the time the finalizer is called.  Then that's
something I know and must take care of when writing the finalizer.
Finalizers of objects not seeing weak refs into that same object is
very much a localized concernn.

Also, perhaps more importantly, this will break *immediately* and
*consistently*.  The finalizer will simply see broken weak pointers, all
the time, every time it runs on the "collected" object.  Since this is
also documented in my patch, I think this is the least impactful way of
doing things.

I'm not even all that concerned about finalizers "reviving" dead
objects, but in that case it's *definitely* the responsibility of the
finalizer's author to make sure it doesn't cause any trouble.

So overall, IMHO the problem is a lot more self-contained/localized,
and more deterministic if we clear the references before running the
finalizer.  That has to count for something, I think!

Cheers,
Peter

signature.asc
Description: PGP signature