tp_dealloc trashcan shouldn't be called for subclasses

[jdemeyer]

BPO	35983
Nosy	@pitrou, @scoder, @ericsnowcurrently, @serhiy-storchaka, @jdemeyer, @matrixise
PRs	bpo-35983: skip trashcan for subclasses #11841 bpo-35983: improve and test old trashcan macros #12607 [2.7] bpo-35983: skip trashcan for subclasses (GH-11841) #12699 [3.7] bpo-35983: skip trashcan for subclasses (GH-11841) #12730

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2019-02-13.11:02:31.402>
labels = ['interpreter-core', 'type-bug', '3.8']
title = "tp_dealloc trashcan shouldn't be called for subclasses"
updated_at = <Date 2019-05-15.14:34:02.694>
user = 'https://github.com/jdemeyer'

bugs.python.org fields:

activity = <Date 2019-05-15.14:34:02.694>
actor = 'ned.deily'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2019-02-13.11:02:31.402>
creator = 'jdemeyer'
dependencies = []
files = []
hgrepos = []
issue_num = 35983
keywords = ['patch']
message_count = 19.0
messages = ['335405', '335406', '335440', '335453', '335455', '335463', '335476', '335513', '338976', '338981', '338982', '339017', '339021', '339022', '339023', '339119', '339519', '339609', '342080']
nosy_count = 6.0
nosy_names = ['pitrou', 'scoder', 'eric.snow', 'serhiy.storchaka', 'jdemeyer', 'matrixise']
pr_nums = ['11841', '12607', '12699', '12730']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35983'
versions = ['Python 3.8']

[jdemeyer]

When designing an extension type subclassing an existing type, it makes sense to call the tp_dealloc of the base class from the tp_dealloc of the subclass.

Now suppose that I'm subclassing "list" which uses the trashcan mechanism. Then it can happen that the tp_dealloc of list puts this object in the trashcan, even though the tp_dealloc of the subclass has already been called. Then the tp_dealloc of the subclass may be called multiple times, which is unsafe (tp_dealloc is expected to be called exactly once).

To solve this, the trashcan mechanism should be disabled when tp_dealloc is called from a subclass.

Interestingly, subtype_dealloc also solves this in a much more involved way (see the "Q. Why the bizarre (net-zero) manipulation of _PyRuntime.trash_delete_nesting around the trashcan macros?") in
Objects/typeobject.c

[matrixise]

Jeroen, could you share your example? I am learning the C-API of Python and this example could be interesting.

[jdemeyer]

The problem is easily reproduced with Cython:

cdef class List(list):
cdef int deallocated

def \_\_dealloc__(self):
    if self.deallocated:
        print("Deallocated twice!")
    self.deallocated = 1

L = None
for i in range(10**4):
L = List((L,))
del L

[ericsnowcurrently]

FWIW, subclassing builtin types is a relatively new thing. There are still a number of lingering (older) implementation details throughout CPython that were written assuming no subclassing. I'd guess that this is one of them.

[pitrou]

By "relatively new thing", you mean less than 20 years old? :-)

[ericsnowcurrently]

On Wed, Feb 13, 2019 at 8:42 AM Antoine Pitrou <report@bugs.python.org> wrote:

Antoine Pitrou <pitrou@free.fr> added the comment:

By "relatively new thing", you mean less than 20 years old? :-)

Yeah, looks like it was in the 2.2 release (Dec 2001) for PEP-253.
Anyway, I know of several core devs who are still opposed to
subclassing builtin types. :)

[jdemeyer]

NOTE: also OrderedDict currently uses trashcan hacking to work around this problem:

    /* Call the base tp_dealloc().  Since it too uses the trashcan mechanism,
     * temporarily decrement trash_delete_nesting to prevent triggering it
     * and putting the partially deallocated object on the trashcan's
     * to-be-deleted-later list.
     */
    --tstate->trash_delete_nesting;
    assert(_tstate->trash_delete_nesting < PyTrash_UNWIND_LEVEL);
    PyDict_Type.tp_dealloc((PyObject *)self);
    ++tstate->trash_delete_nesting;

So this seems to be a known problem which deserves to be fixed properly.

[jdemeyer]

Jeroen, could you share your example? I am learning the C-API of Python and this example could be interesting.

Either use the Cython code I posted above or run the testcase I added in PR 11841.

[serhiy-storchaka]

Disabling the trashcan mechanism returns the problem for solving which the trashcan mechanism was introduced.

>>> from _testcapi import MyList
>>> L = None
>>> for i in range(1000000):
...     L = MyList((L,))
... 
>>> del L
Segmentation fault (core dumped)

I think we need better way to resolve this issue.

For example, setting the object type to the parent type should solve this issue.

[jdemeyer]

Yes of course. When not using the trashcan, stuff crashes. I don't get your point...

[jdemeyer]

To clarify: the purpose of MyList is specifically to check that no double deallocations occur. For this test to make sense, MyList should not use the trashcan itself.

[serhiy-storchaka]

In your example you can add

    PyTypeObject *tp = Py_TYPE(self);
    Py_TYPE(self) = &PyList_Type;
    if (PyType_GetFlags(tp) & Py_TPFLAGS_HEAPTYPE) {
        Py_DECREF(tp);
    }

before calling PyList_Type.tp_dealloc().

It is possible to add such code directly in PyList_Type.tp_dealloc and other deallocators that use the trashcan mechanism.

[jdemeyer]

Changing types like that looks like an ugly hack and a recipe for breakage. For example, in list_dealloc(), the following needs the type to be correct:

    if (numfree < PyList_MAXFREELIST && PyList_CheckExact(op))
        free_list[numfree++] = op;
    else
        Py_TYPE(op)->tp_free((PyObject *)op);

Could you please clarify your opinion: do you think that there's something wrong with PR 11841? And if yes: what's wrong with it? Or are you just giving optional suggestions?

[serhiy-storchaka]

Yes, I think that there's something wrong with PR 11841. It disables the trashcan mechanism and does not provide other mechanism for solving that problem.

[jdemeyer]

It disables the trashcan mechanism

Yes, it disables the trashcan in some cases. But only when using the trashcan mechanism would probably crash CPython anyway due to a double deallocation. So at the very least, PR 11841 improves things from "crashing whenever the trashcan is used" to "crashing on stack overflows".

Do you have a real example where PR 11841 actually makes things *worse*?

and does not provide other mechanism for solving that problem.

The recommended thing to do is that the subclass also implements the trashcan. See OrderedDict for an example: both the base class "dict" and the subclass "OrderedDict" use the trashcan.

[jdemeyer]

I created an additional PR 12607 with some more changes to the, in particular to make the old backwards-compatibility trashcan macros safer. This should be seen as part of the same bugfix but I decided to make a new PR because PR 11841 had several positive reviews already.

[jdemeyer]

I realized that there is a nasty interaction between the trashcan and __del__: if you're very close to the trashcan limit and you're calling __del__, then objects that should have been deallocated in __del__ (in particular, an object involving self) might instead end up in the trashcan. So self might be resurrected when it shouldn't be.

This is causing test failures in the 2.7 backport due to a different implementation of __del__.

[jdemeyer]

In Python 3, the resurrection issue probably appears too. But it's not so much a problem since __del__ (mapped to tp_finalize) is only called once anyway. So there are no bad consequences if the object is resurrected incorrectly.

[pitrou]

New changeset 351c674 by Antoine Pitrou (Jeroen Demeyer) in branch 'master':
bpo-35983: skip trashcan for subclasses (GH-11841)
351c674

[kumaraditya303]

Closing as it seems fixed, feel free to reopen if required.