Neil Schemenauer <nas-pyt...@arctrix.com> added the comment:
> The problem of PyObject_GC_UnTrack() is just the most visible effect of the
> trashcan mecanism: tp_dealloc can be called twice, and this is *not* expected
> by the tp_dealloc API.
The fact that Py_TRASHCAN_BEGIN and Py_TRASHCAN_END can cause tp_dealloc to be
called more than once is not a huge issue, IMHO. The statements executed more
than once are the ones before or after the trashcan macros. If you use them
properly, the multiple calls doesn't cause issues. The big trap is
to use _PyObject_GC_UNTRACK before Py_TRASHCAN_BEGIN. It is not safe to call
that macro multiple times. Anywhere you see Py_TRASHCAN_BEGIN and
_PyObject_GC_UNTRACK before it, that's a bug.
The point of this PR is to make the macros harder to use incorrectly. If there
is no need to call untrack, Py_TRASHCAN_BEGIN and Py_TRASHCAN_END can normally
be the first and last lines of the tp_dealloc function (declarations can be
inside). Then, the fact that tp_dealloc is actually called more than once by
the
trashcan doesn't cause an issue.
tp_dealloc can get called more than once for another reason. If the object is
resurrected by tp_finalize or tp_del, then tp_dealloc can be called again.
It's pretty mind bending to try to understand all of implications of that. Most
of the ugliness is inside PyObject_CallFinalizerFromDealloc(). The type with
the finalizer has to be careful not to put the object in an invalid state
before it gets resurrected.
> Putting trashcan mecanism outside tp_dealloc can allow to make sure that
> tp_dealloc is called exactly once. _Py_Dealloc() sounds like a good
> candidate, but I didn't check if it's the only way to call tp_dealloc. Are
> there other places where tp_dealloc is called *directly*?
tp_dealloc is called from a few more places, as you found. As for providing
C-stack exhaustion protection via the trashcan, I think we are only interested
in places were tp_dealloc is called as a result of a DECREF. And I think that
only goes through _Py_Dealloc().
I suppose it would be possible to blow up the C stack via a call chain like:
subtype_dealloc -> basedealloc -> subtype_dealloc -> basedealloc -> ...
I think you would have to make a long chain of subclasses. Seems unlikely in
real code so I'm not worried about trying to fix that.
> Using military grade regex, I found the following functions calling
> tp_dealloc:
>
> grep 'dealloc[) ]*([a-zA-Z]\+)' */*.c
>
> * _Py_Dealloc() obviously
> * _PyTrash_thread_destroy_chain()
> * subtype_dealloc()
> * Modules/_testcapimodule.c: check_pyobject_freed_is_freed() <= unit test, it
> can be ignored
>
> So if we move the trashcan usage inside functions, 3 functions must be
> modified:
>
> * _Py_Dealloc()
> * _PyTrash_thread_destroy_chain()
> * subtype_dealloc()
Take a look at bpo-44897. It implements the _Py_Dealloc approach. You can see
what needs to be modified. I removed the Py_TRASHCAN_BEGIN/Py_TRASHCAN_END
macros as well, just because they are not needed anymore. _PyObject_GC_UNTRACK
calls inside tp_dealloc methods need to be removed because _Py_Dealloc already
does the untrack. For external extensions, they must be using
PyObject_GC_UnTrack() and so that's safe (object is already untracked and so it
does nothing).
I think the big change is calling tp_dealloc methods with the object already
untracked. If an extension did something like:
mytype_dealloc(PyObject *self)
{
...
assert(PyObject_GC_IsTracked(self));
...
}
That would break after PR 27738, at least as it's currently written.
The other issue I'm not quite sure about is that
PyObject_CallFinalizerFromDealloc insists that GC objects are tracked when that
function is called. I think it is okay to just call _PyObject_GC_TRACK on the
ones that are not tracked, to ensure that. Maybe Tim Peters will jump in and
correct the errors of my thinking.
----------
nosy: +tim.peters
_______________________________________
Python tracker <rep...@bugs.python.org>
<https://bugs.python.org/issue44881>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
