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

A tp_dealloc of a subclassed class cannot resurrect an object #52459

Closed
kristjanvalur mannequin opened this issue Mar 23, 2010 · 14 comments
Closed

A tp_dealloc of a subclassed class cannot resurrect an object #52459

kristjanvalur mannequin opened this issue Mar 23, 2010 · 14 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@kristjanvalur
Copy link
Mannequin

kristjanvalur mannequin commented Mar 23, 2010

BPO 8212
Nosy @pitrou, @kristjanvalur, @vstinner, @benjaminp, @meadori
Files
  • basedealloc.patch
  • basedealloc.diff
  • resurrect.patch
  • 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 = <Date 2020-06-25.09:31:43.695>
    created_at = <Date 2010-03-23.13:28:30.742>
    labels = ['interpreter-core', 'type-feature']
    title = 'A tp_dealloc of a subclassed class cannot resurrect an object'
    updated_at = <Date 2020-06-25.09:31:43.690>
    user = 'https://github.com/kristjanvalur'

    bugs.python.org fields:

    activity = <Date 2020-06-25.09:31:43.690>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-06-25.09:31:43.695>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2010-03-23.13:28:30.742>
    creator = 'kristjan.jonsson'
    dependencies = []
    files = ['25222', '25236', '25247']
    hgrepos = []
    issue_num = 8212
    keywords = ['patch']
    message_count = 14.0
    messages = ['101580', '101585', '158324', '158328', '158407', '158444', '158449', '158452', '158504', '158505', '158508', '158512', '227608', '372323']
    nosy_count = 5.0
    nosy_names = ['pitrou', 'kristjan.jonsson', 'vstinner', 'benjamin.peterson', 'meador.inge']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue8212'
    versions = ['Python 3.3']

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Mar 23, 2010

    The tp_dealloc of a type can chose to resurrect an object. the subtype_dealloc() in typeobject.c does this when it calls the tp_del() member and it has increased the refcount.

    The problem is, that if you subclass a custom C object, and that C object's tp_dealloc() chooses to resurrect a dying object, that doesn't go well with subtype_dealloc().

    After calling basedealloc() (line 1002 in typeobject.c), the object's type is decrefed. But if the object was resurrected by basedealloc() this shouldn't have been done. The object will be alive, but the type will be missing a reference. This will cause a crash later.

    This could be fixable if we knew somehow after calling basedealloc() if the object were still alive. But we cannot check "self" because it may have died.

    The only way out of this conundrum that I can see is to change the signature of tp_dealloc() to return a flag, whether it did actually delete the object or not.

    Of course, I see no easy way around not clearing the slots, but the clearing of the dict could be postponed until after the call to basedealloc().

    Since tp_dealloc _can_ resurrect objects (subtype_dealloc does it), subclassing such objects should work, and not crash the interpreter if the base class' dp_dealloc() decides to do so.
    No suggested

    @kristjanvalur kristjanvalur mannequin added the type-crash A hard crash of the interpreter, possibly with a core dump label Mar 23, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Mar 23, 2010

    Indeed. The io module has had to circumvent this and uses the following snippet when resurrecting an instance of a subclass of one of its types (see iobase_dealloc() in Modules/_io/iobase.c):

            /* When called from a heap type's dealloc, the type will be
               decref'ed on return (see e.g. subtype_dealloc in typeobject.c). */
            if (PyType_HasFeature(Py_TYPE(self), Py_TPFLAGS_HEAPTYPE))
                Py_INCREF(Py_TYPE(self));
            return;

    I agree it would be nice to have an automatic way of handling this. I don't see an obvious solution, though.

    @pitrou pitrou added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Mar 23, 2010
    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Apr 15, 2012

    Here is a patch that rectifies this situation, albeit in a somewhat 'hacky' manner.
    It works by injecting a monitoring 'tp_free' call into the type during the basedealloc call, which sets a flag if it was called with the object, i.e. if actual deletion took place.
    This value is then returned, and a decision to decref the object's "type" is made on this result.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Apr 15, 2012

    Another, less hacky but more intrusive, way would be to change the signature of tp_dealloc in a backwards compatible way:
    typedef void (*destructor)(PyObject *, int *destroyed);

    The destructor can then set the flag pointed to by 'destroyed' to 1 or 0, depending on whether actual destruction took place. The caller will set the flag to '1' by default.

    We could then change all internal destructors to conform, and know that external destructors will continue to work in the old way.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Apr 16, 2012

    Updated the patch with better documentation, and recursion safety.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 16, 2012

    Urg, that's a horrible hack.
    How about instead having an API function to resurrect an object from a tp_dealloc?

    That way the iobase_dealloc code would be written:

        if (_PyIOBase_finalize((PyObject *) self) < 0) {
            _PyObject_ResurrectFromDealloc(self);
            return;
        }

    That API function could also perhaps take care of the _Py_NewReference stuff (see the end of _PyIOBase_finalize).

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Apr 16, 2012

    Ok, that sounds reasonable, particularly in light of that _NewReference stuff. I'll work out a different patch then. But I think the API must be public, since it would need to work from extension modules.

    So: if a c type decides that it wants to live after a tp_dealloc(), it must call this api ...

    @pitrou
    Copy link
    Member

    pitrou commented Apr 16, 2012

    Ok, that sounds reasonable, particularly in light of that
    _NewReference stuff. I'll work out a different patch then. But I
    think the API must be public, since it would need to work from
    extension modules.

    Needing to work from (stdlib) extension modules and being public are too
    different things. I don't think we want to encourage third-party types
    to resurrect their instances.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Apr 16, 2012

    Incidentally, Looking at this, I noticed code such as: (textio.c)

    static int
    _textiowrapper_clear(textio *self)
    {
        if (self->ok && _PyIOBase_finalize((PyObject *) self) < 0)
            return -1;
    
    This shows something scary:  During a GC run, it is possible to invoke the  "close()" method on a textio object.  This is dangerous, and probably not allowed at all.
    Returning -1 from a tp_clear() slot has no particular meaning, and the return value appears to be ignored by gcmodule.c (also, it won't ever ber resurrected through that pathway, since it is incref'd first:
        Py_INCREF(op);
        clear(op);
        Py_DECREF(op);
    )

    See bpo-9141 for more info.
    IMHO, this is another case where objects must tell GC that they cannot be collected but instead must be placed in gc.garbage.

    This probably requires a defect of its own.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 16, 2012

    static int
    _textiowrapper_clear(textio *self)
    {
    if (self->ok && _PyIOBase_finalize((PyObject *) self) < 0)
    return -1;

    This shows something scary: During a GC run, it is possible to invoke
    the "close()" method on a textio object. This is dangerous, and
    probably not allowed at all.

    How is it scary? This is not worse than invoking a __del__ method.

    Returning -1 from a tp_clear() slot has no particular meaning, and the
    return value appears to be ignored by gcmodule.c

    I don't think this is a problem.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Apr 16, 2012

    __del__ methods are never invoked from GC. All objects that have finalizers, and all objects reachable from them _must_ be put in gc.garbage.
    If I'm not mistaken, this is a fundamental rule of python's gc module and it is not because of the unknown order of finalizers as some people seem to think, but because the state of the interpreter during the collection phase is so fragile, with objects being linked up in various lists, and so on, that it isn't permissable to run any dynamic code.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Apr 16, 2012

    Here is a patch with the suggested change.
    I put _PyObject_ResurrectFromDealloc into typeobject.c since it seems more at home there than in object.c, but I'm not sure.
    Also note that other types, that were calling _PyIOBase_finalize() from their tp_dealloc slots (bufferedio, fileio) will now also get the correct behavior with the incref'd ob_type slot, if the object is resurrected.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 26, 2014

    I think PEP-442 makes this request obsolete: you can simply implement tp_finalize() and incref the object naturally from there. Kristjan, what do you think?

    @vstinner
    Copy link
    Member

    I think PEP-442 makes this request obsolete: you can simply implement tp_finalize() and incref the object naturally from there.

    Right. I close the issue.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants