classification
Title: Fix various crashes exposed through mro() customization
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.5, Python 3.4, Python 3.3, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: benjamin.peterson Nosy List: abusalimov, benjamin.peterson, lemburg, pitrou, python-dev, tim.peters
Priority: normal Keywords: patch

Created on 2014-10-26 21:06 by abusalimov, last changed 2015-02-14 17:32 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
mro-crashers-fix-v1.diff abusalimov, 2014-10-26 21:06 (whole patch) fix mro() issues, crasher tests included review
0001-minor-test_mro-test-case-stub-and-helpers.patch abusalimov, 2014-10-26 21:07 [PATCH 01/15] (minor) test_mro: test case stub and helpers
0002-test-crashers-NULL-old_mro-and-over-decref-on-reent.patch abusalimov, 2014-10-26 21:08 [PATCH 02/15] (test) (crashers) NULL old_mro and over-decref on reent
0003-minor-mro_internal-extract-mro_invoke-and-mro_check.patch abusalimov, 2014-10-26 21:08 [PATCH 03/15] (minor) mro_internal: extract mro_invoke and mro_check
0004-minor-type_set_bases-move-undoing-logic-to-the-end.patch abusalimov, 2014-10-26 21:09 [PATCH 04/15] (minor) type_set_bases: move undoing logic to the end
0005-minor-type_set_bases-extract-add_all_subclasses.patch abusalimov, 2014-10-26 21:10 [PATCH 05/15] (minor) type_set_bases: extract add_all_subclasses
0006-minor-mro_subclasses-loop-over-a-list-of-subclasses.patch abusalimov, 2014-10-26 21:10 [PATCH 06/15] (minor) mro_subclasses: loop over a list of subclasses
0007-fix-handle-tp_mro-overwritten-through-reentrancy.patch abusalimov, 2014-10-26 21:12 [PATCH 07/15] (fix) handle tp_mro overwritten through reentrancy
0008-test-crashers-tp_base-tp_subclasses-inherit-cycles.patch abusalimov, 2014-10-26 21:12 [PATCH 08/15] (test) (crashers) tp_base/tp_subclasses inherit-cycles
0009-minor-PyType_IsSubtype-extract-a-check-using-tp_base.patch abusalimov, 2014-10-26 21:12 [PATCH 09/15] (minor) PyType_IsSubtype: extract a check using tp_base
0010-fix-type_set_bases-inherit-cycles-and-reent-checks.patch abusalimov, 2014-10-26 21:13 [PATCH 10/15] (fix) type_set_bases: inherit-cycles and reent checks
0011-test-behavior-error-on-extending-an-incomplete-type.patch abusalimov, 2014-10-26 21:14 [PATCH 11/15] (test) (behavior) error on extending an incomplete type
0012-minor-mro_implementation-pmerge-refactory.patch abusalimov, 2014-10-26 21:14 [PATCH 12/15] (minor) mro_implementation, pmerge: refactory
0013-fix-mro_implementation-check-base-tp_mro-is-not-NULL.patch abusalimov, 2014-10-26 21:15 [PATCH 13/15] (fix) mro_implementation: check base->tp_mro is not NULL
0014-test-crasher-attr-lookup-on-super-with-uninitialized.patch abusalimov, 2014-10-26 21:15 [PATCH 14/15] (test) (crasher) attr lookup on super with uninitialized type
0015-fix-super_getattro-check-type-tp_mro-and-refactory.patch abusalimov, 2014-10-26 21:16 [PATCH 15/15] (fix) super_getattro: check type->tp_mro and refactory
Messages (12)
msg230044 - (view) Author: Eldar Abusalimov (abusalimov) * Date: 2014-10-26 21:06
The attached patch fixes several bugs revealed by providing a custom mro(). Most of these bugs are reentrancy issues (mainly, cls.__bases__ assignment within mro() which causes incorrect refcounting), but there are also some issues when using incomplete types with uninitialized MRO (dereferencing NULL when extending such types, attribute lookup on super, etc.). The patch is made against the default branch (py3k), however all these bugs exist in Python 2 as well.

I also tried to break the patch into smaller pieces (commits, in fact) to ease reviewing: a common pattern is test->minor->fix series. The patch set is an output of `git format-patch`, and most patches have a detailed commit message describing a change inside.

Adding memory mgmt and object model guys to nosy list.
msg230045 - (view) Author: Eldar Abusalimov (abusalimov) * Date: 2014-10-26 21:12
This is a patch with most significant changes, please review it carefully.
msg230046 - (view) Author: Eldar Abusalimov (abusalimov) * Date: 2014-10-26 21:29
Just in case, the previous message about reviewing is about [PATCH 07/15] (fix) handle tp_mro overwritten through reentrancy
msg230560 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-11-03 20:36
Thank you for the patchset. I wonder if we should just forbid most reentrancy i.e. set a flag on the type when it's being constructed and not let type_set_bases be called then. Setting __bases__ from within mro() doesn't seem very useful outside of producing pathologies.
msg230729 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-06 08:56
I think forbidding reentrancy would indeed be a good idea.
msg230823 - (view) Author: Eldar Abusalimov (abusalimov) * Date: 2014-11-07 19:35
Thank you for your replies.

I don't think forbidding reentrancy is a good idea, and I'm against it for the following reasons.

First of all, naive prohibition of type_set_bases within mro() on a class doesn't save from mro() reentrancy:

    def mro(cls):
        # cls is B,
        # B extends A

        A.__bases__ = ...  # What to do?

Changing bases of a class provokes MRO of all its subclasses to be updated as well. Therefore, one would also need to forbid changing __bases__ of any direct or indirect base of the class up to the 'object', and this doesn't feel natural at all. Why should mro() of some specialized class B prevent changing bases of its general parent class A?

Otherwise, there must be a check in mro_subclasses (mro_hierarchy in the patch), which will not recurse into a class flagged as being inside mro(). But in this case a running mro(B) will be unable to notice a change of MRO of the parent class A, in case if mro() involves some non-trivial logic, and A.__bases__ assignment occurs somewhere deep in the call stack and is done by a code from some unknown black-box module. Deferring mro() recalculation upon exiting from the outermost mro() in such case doesn't seem to be a good solution either, especially concerning its (most likely) tricky implementation.

    def mro(cls):
        # some complicated calculation based of MROs of bases:
        parent_mros = [base.__mro__ for base in cls.__bases__]

        # The line below does this:
        #   parent.__bases__ = ...
        # (deep-deep-deep in the call stack)
        third_party.do_mro(cls)

        # Either the line above raises an error,
        # or parent_mros becomes invalid.


Another example. Setting __bases__ from inside mro() may be useful for changing class behavior on the fly using a proxy base class (e.g. for debugging, logging, testing frameworks). The only place to do this except mro() is __new__, but in that case it is only possible to fix up bases at the moment of class creation. That is, a tricky class that change its __bases__ would break such framework by overriding a proxy base, which is necessary for this framework to function properly.

    def mro(cls):
        if cls or one of cls.__bases__ is not a proxy class:
            class Proxy(*cls.__bases__):
                ...
            cls.__bases__ = (Proxy,)  # reenter

        return type.mro(cls)

In other words, there should be a loophole to alter __bases__ upon assignment, and I suppose mro() is a good option.

Also, __bases__ assignment is known as the only reliable way to invoke MRO recalculation (http://stackoverflow.com/a/20832588/545027). Forbidding it from the inside mro() makes it not so obvious and reliable.

Actually, I encountered one of these bugs while working on a real metaclass providing an auto-inheriting attributes feature needed for a custom DSL built on top of Python. In a nutshell, if B extends A, then make B.V extend A.V automatically (https://github.com/abusalimov/mybuild/blob/6c7c89521b856c798b46732501adb5e06dec7e03/util/inherit.py, still work in progress).

I know, some of these use cases may sound a bit unreal (or even crazy), but neither me nor you can imagine all scenarios, that Python programmers could ever invent. Actually, there could already exist some: it is possible to workaround most of reentrancy issues through holding a reference to old_mro from inside a Python code. That is, backward compatibility comes into play too.

Finally, why do forbid something that was not prohibited before? I think of it as a feature with some issues to fix, not to remove at all. After all, there is a fix provided, it is more or less straightforward (I hope so), with tests showing a desired behavior. The desired behavior is also more or less obvious: take a __mro__ calculated by mro() invoked due to the very last __bases__ assignment, regardless reentrancy (the innermost one in such case).

Summarizing, that is why I believe that reentrancy should not be forbidden. Furthermore, I considered that way, and I'm pretty sure it is a wrong one. It implies too many corner cases, it has a non-obvious behavior from the point of view of a Python code, and a possible implementation doesn't seem to be more simple or robust than it is now.

I would be happy to hear opposite arguments, and if I convinced you, get a feedback on the patch, of course. I could also prepare a backport patch fixing 2.7 line as well.
msg232573 - (view) Author: Eldar Abusalimov (abusalimov) * Date: 2014-12-12 20:32
ping?
msg232628 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-12-14 03:09
I will try to look eventually.
msg235445 - (view) Author: Eldar Abusalimov (abusalimov) * Date: 2015-02-05 19:01
I feel a bit uneasy, but... any news on this?
msg235461 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-06 03:31
New changeset 6384c0cd3b2d by Benjamin Peterson in branch '3.4':
fix many custom mro() edge cases and improve code quality (#22735)
https://hg.python.org/cpython/rev/6384c0cd3b2d

New changeset 75fd0bd89eef by Benjamin Peterson in branch 'default':
merge 3.4 (#22735)
https://hg.python.org/cpython/rev/75fd0bd89eef
msg235484 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-02-06 13:52
Thanks for the very high quality patch. The way it was split made it much easier to review.
msg235486 - (view) Author: Eldar Abusalimov (abusalimov) * Date: 2015-02-06 15:55
You're welcome, and thank you too for reviewing and merging it.
History
Date User Action Args
2015-02-14 17:32:53berker.peksagsetstatus: open -> closed
resolution: fixed
stage: resolved
2015-02-06 15:55:03abusalimovsetmessages: + msg235486
2015-02-06 13:52:32benjamin.petersonsetmessages: + msg235484
2015-02-06 03:31:03python-devsetnosy: + python-dev
messages: + msg235461
2015-02-05 19:01:02abusalimovsetmessages: + msg235445
2014-12-14 03:09:18benjamin.petersonsetassignee: benjamin.peterson
messages: + msg232628
2014-12-12 20:32:17abusalimovsetmessages: + msg232573
2014-11-07 19:35:07abusalimovsetmessages: + msg230823
2014-11-06 08:56:16pitrousetnosy: + pitrou
messages: + msg230729
2014-11-03 20:36:30benjamin.petersonsetmessages: + msg230560
2014-10-26 21:29:43abusalimovsetmessages: + msg230046
2014-10-26 21:16:39abusalimovsetfiles: + 0015-fix-super_getattro-check-type-tp_mro-and-refactory.patch
2014-10-26 21:15:59abusalimovsetfiles: + 0014-test-crasher-attr-lookup-on-super-with-uninitialized.patch
2014-10-26 21:15:20abusalimovsetfiles: + 0013-fix-mro_implementation-check-base-tp_mro-is-not-NULL.patch
2014-10-26 21:14:42abusalimovsetfiles: + 0012-minor-mro_implementation-pmerge-refactory.patch
2014-10-26 21:14:19abusalimovsetfiles: + 0011-test-behavior-error-on-extending-an-incomplete-type.patch
2014-10-26 21:13:50abusalimovsetfiles: + 0010-fix-type_set_bases-inherit-cycles-and-reent-checks.patch
2014-10-26 21:12:55abusalimovsetfiles: + 0009-minor-PyType_IsSubtype-extract-a-check-using-tp_base.patch
2014-10-26 21:12:33abusalimovsetfiles: + 0008-test-crashers-tp_base-tp_subclasses-inherit-cycles.patch
2014-10-26 21:12:14abusalimovsetfiles: + 0007-fix-handle-tp_mro-overwritten-through-reentrancy.patch

messages: + msg230045
2014-10-26 21:10:52abusalimovsetfiles: + 0006-minor-mro_subclasses-loop-over-a-list-of-subclasses.patch
2014-10-26 21:10:17abusalimovsetfiles: + 0005-minor-type_set_bases-extract-add_all_subclasses.patch
2014-10-26 21:09:36abusalimovsetfiles: + 0004-minor-type_set_bases-move-undoing-logic-to-the-end.patch
2014-10-26 21:08:42abusalimovsetfiles: + 0003-minor-mro_internal-extract-mro_invoke-and-mro_check.patch
2014-10-26 21:08:15abusalimovsetfiles: + 0002-test-crashers-NULL-old_mro-and-over-decref-on-reent.patch
2014-10-26 21:07:25abusalimovsetfiles: + 0001-minor-test_mro-test-case-stub-and-helpers.patch
2014-10-26 21:06:21abusalimovcreate