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

CPython hangs on error __context__ set to the error itself #69968

Closed
1st1 opened this issue Dec 2, 2015 · 72 comments
Closed

CPython hangs on error __context__ set to the error itself #69968

1st1 opened this issue Dec 2, 2015 · 72 comments
Assignees
Labels
3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@1st1
Copy link
Member

1st1 commented Dec 2, 2015

BPO 25782
Nosy @birkenfeld, @terryjreedy, @ncoghlan, @cjerdonek, @ambv, @serhiy-storchaka, @1st1, @corona10, @miss-islington, @sweeneyde, @sobolevn, @iritkatriel
PRs
  • bpo-25782: Prevent cycles in the __context__ chain. #20543
  • bpo-25782: Do not hang when exception contexts form a cycle. #20539
  • bpo-25782: avoid hang in PyErr_SetObject when current exception has a… #27626
  • [3.10] bpo-25782: avoid hang in PyErr_SetObject when current exception has a cycle in its context chain (GH-27626) #27706
  • [3.9] bpo-25782: avoid hang in PyErr_SetObject when current exception has a cycle in its context chain (GH-27626) #27707
  • Files
  • Issue25782.patch
  • Issue25782_2.patch
  • Issue25782_3.patch
  • Issue25782_4.patch
  • set_context_reordering.patch
  • set_context_reordering2.patch
  • Issue25782_5.patch
  • issue27122_broken_cm.py: Explicit reproducer for python/issues-test-cpython#27122
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = None
    created_at = <Date 2015-12-02.17:14:01.351>
    labels = ['interpreter-core', 'type-bug', '3.9', '3.10', '3.11']
    title = 'CPython hangs on error __context__ set to the error itself'
    updated_at = <Date 2021-08-11.12:41:10.037>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2021-08-11.12:41:10.037>
    actor = 'corona10'
    assignee = 'serhiy.storchaka'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2015-12-02.17:14:01.351>
    creator = 'yselivanov'
    dependencies = []
    files = ['41212', '41214', '41219', '41341', '41354', '43380', '43381', '43382']
    hgrepos = []
    issue_num = 25782
    keywords = ['patch']
    message_count = 71.0
    messages = ['255731', '255739', '255740', '255744', '255747', '255752', '255753', '255754', '255758', '255759', '255760', '255766', '255767', '255768', '255770', '255771', '255772', '255773', '255775', '255776', '255780', '255781', '255788', '255995', '256618', '256644', '256681', '256687', '256688', '268309', '268310', '268465', '268467', '268472', '268473', '268485', '268509', '268563', '268565', '268567', '268578', '283459', '291825', '291826', '291828', '314242', '314430', '370401', '370404', '370420', '391637', '399080', '399091', '399092', '399093', '399094', '399099', '399100', '399218', '399219', '399220', '399227', '399228', '399230', '399243', '399281', '399310', '399314', '399325', '399332', '399387']
    nosy_count = 16.0
    nosy_names = ['georg.brandl', 'terry.reedy', 'ncoghlan', 'chris.jerdonek', 'lukasz.langa', 'Yury.Selivanov', 'python-dev', 'serhiy.storchaka', 'yselivanov', 'Rotem Yaari', 'larsonreever', 'corona10', 'miss-islington', 'Dennis Sweeney', 'sobolevn', 'iritkatriel']
    pr_nums = ['20543', '20539', '27626', '27706', '27707']
    priority = 'high'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25782'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    try:
    raise Exception
    except Exception as ex:
    ex.__context__ = ex
    hasattr(1, 'aa')

    @1st1 1st1 added interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker labels Dec 2, 2015
    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    The bug is in "PyErr_SetObject":

                while ((context = PyException_GetContext(o))) {
                    Py_DECREF(context);
                    if (context == value) {
                        PyException_SetContext(o, NULL);
                        break;
                    }
                    o = context;
                }

    The loop can be infinite.

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    Looks like this is the original code committed in CPython in 2ee09afee126. Patch by Antoine Pitrou.

    Antoine, how would you fix this?

    @serhiy-storchaka
    Copy link
    Member

    I would change __context__ setter to check if it creates a loop.

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    Serhiy, good idea, thanks! Please review the attached patch.

    Larry, I view this as a very serious bug. Can we ship 3.5.1 with it fixed?

    @oconnor663
    Copy link
    Mannequin

    oconnor663 mannequin commented Dec 2, 2015

    Yury, do we need to handle more complicated infinite loops, where "self" doesn't actually show up in the loop? Here's an example:

    try:
        raise Exception
    except Exception as ex:
        loop1 = Exception()
        loop2 = Exception()
        loop1.__context__ = loop2
        loop2.__context__ = loop1
        ex.__context__ = loop1
        hasattr(1, 'aa')
    

    I'm unfamiliar with CPython, so I don't know whether full-blown loop detection belongs here. Maybe we could add a hardcoded limit like "fail if we loop more than X times"?

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    Yury, do we need to handle more complicated infinite loops, where "self" doesn't actually show up in the loop? Here's an example:

    My patch works for your example too. Since it checks for loops in __context__ setter, you shouldn't be able to create complicated loops.

    @serhiy-storchaka
    Copy link
    Member

    Should we do the same for __cause__? Is it possible to create __context__ or __cause__ loop without assigning these attributes directly?

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    Should we do the same for __cause__? Is it possible to create __context__ or __cause__ loop without assigning these attributes directly?

    Yes, let's mirror the __context__ behaviour for __cause__. New patch attached.

    Serhiy, Guido,

    The new patch raises a TypeError in __cause__ and __context__ setters when a cycle was introduced, so in pure Python the following won't work:

       # will throw TypeError("cycle in exception context chain")
       ex.__context__ = ex chain")
    
       # will throw TypeError("cycle in exception cause chain")
       ex.__cause__ = ex

    However, since PyException_SetContext and PyException_SetCause are public APIs, and their return type is 'void', I can't raise an error when a C code introduces a cycle, in that case, the exc->cause/exc->context will be set to NULL.

    Thoughts?

    I think that this approach is the only sane one here. We can certainly fix the infinite loop in PyErr_SetObject, but it will only be a matter of time until we discover a similar loop somewhere else.

    @gvanrossum
    Copy link
    Member

    Ouch, it's unfortunate those APIs don't have an error return. :-(

    Setting it to NULL is one option -- silently ignoring the assignment
    (leaving whatever was there) might also be good? In 90% of the cases it
    would be the same thing right? (I'm not familiar with this part of the code
    TBH.)

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    Setting it to NULL is one option -- silently ignoring the assignment
    (leaving whatever was there) might also be good? In 90% of the cases it
    would be the same thing right?

    But leaving the old __context__ there will completely mask the bug...

    And as for pure python code -- do you agree we better raise a TypeError if a cycle is about to be introduced?

    @gvanrossum
    Copy link
    Member

    But leaving the old __context__ there will completely mask the bug...
    OK, NULL is fine then.

    we better raise a TypeError if a cycle is about to be introduced?
    Yes.

    @serhiy-storchaka
    Copy link
    Member

    Yet one option is the emersion of the exception. Affected exception is removed from the chain and added at it head.

    If there is a chain A -> B -> C -> D -> E, after assignment C.__context__ = A we will get a chain C -> A -> B -> D -> E. No exception is lost.

    @vstinner
    Copy link
    Member

    vstinner commented Dec 2, 2015

    Larry, I view this as a very serious bug. Can we ship 3.5.1 with it fixed?

    Don't do that, a few hours (!) is not enough to test a fix. It's too
    late after a RC1 for such critical change (exceptions).

    The bug was here since at least Python 3.3, there is no urgency to fix it.

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    Don't do that, a few hours (!) is not enough to test a fix. It's too
    late after a RC1 for such critical change (exceptions).

    Maybe we can add an RC2?

    @vstinner
    Copy link
    Member

    vstinner commented Dec 2, 2015

    Maybe we can add an RC2?

    Seriously? I'm waiting Python 3.5.1 since 3.5.0 was released. I'm amazed how much time it takes to release a first bugfix version, 3.5.0 was full a bugs (see the changelog).

    It's very easy to workaround this issue in pure Python. Why do you want the fix *RIGHT NOW*?

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    If there is a chain A -> B -> C -> D -> E, after assignment C.__context__ = A we will get a chain C -> A -> B -> D -> E. No exception is lost.

    What to do when you try to chain "C -> C"?

    I'm not sure I like this reordering idea -- it might introduce some *very* hard to find bugs -- you expected one type of exception, then you used some external library, and after that you have a completely different exception.

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    It's very easy to workaround this issue in pure Python. Why do you want the fix *RIGHT NOW*?

    Please look at http://bugs.python.org/issue25779. I think we either should fix this issue, or fix http://bugs.python.org/issue25786 in 3.5.1, since I can't find a workaround for it. The latter issue is probably easier to get into 3.5.1?

    @vstinner
    Copy link
    Member

    vstinner commented Dec 2, 2015

    Yury Selivanov added the comment:

    Please look at http://bugs.python.org/issue25779. I think we either should fix this issue, or fix http://bugs.python.org/issue25786 in 3.5.1, since I can't find a workaround for it. The latter issue is probably easier to get into 3.5.1?

    It's a choice for the release manager. But IHMO there will always be
    bugs :-) It looks like you found a lot of issues related to exceptions
    handling. Maybe it would be better to fix all the them "at once" in
    3.5.2? And then ask Larry to release it early?

    @serhiy-storchaka
    Copy link
    Member

    What to do when you try to chain "C -> C"?

    Nothing. Or may be raise an exception (because C.__context__ can't be set to what you try).

    you expected one type of exception, then you used some external library, and after that you have a completely different exception.

    I don't understand what new can add the reordering. It doesn't change original exception, it only changes __context__, but you change it in any case. If silently ignore the loop, you would get __context__ different from what you expected. If raise TypeError, you would get TypeError instead of expected exception.

    And if the decision will be to raise TypeError, may be RuntimeError is more appropriate?

    @larryhastings
    Copy link
    Contributor

    Please look at http://bugs.python.org/issue25779.

    You closed that one and marked it "not a bug"...?

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    You closed that one and marked it "not a bug"...?

    That particular issue was about asyncio. After reducing it, I created two new issues -- this one, and another one about another bug in contextlib.

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 2, 2015

    Serhiy, Victor, thank you for your reviews. Another version of the patch is attached.

    @larryhastings
    Copy link
    Contributor

    I'm not going to hold up 3.5.1 for this.

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 17, 2015

    A new patch is attached. Please review.

    I decided to remove the fix for recursive cause. Currently, raise e from e doesn't cause any problem, and if we fix the interpreter to raise an RuntimeError in such cases it will be a backwards incompatible change. I don't see any point in introducing such a change in a bugfix Python release. We can open a separate issue for 3.6 though.

    Fixing __context__ in 3.5.2 is way more important, since the interpreter can actually infinitely loop itself in some places. And since there is no syntax for setting __context__ manually (as opposed to __cause__ via raise .. from), I suspect that there will be much less people affected by this fix.

    @serhiy-storchaka
    Copy link
    Member

    The patch LGTM (but I prefer reordering solution).

    @serhiy-storchaka serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Dec 18, 2015
    @sweeneyde
    Copy link
    Member

    For clarification, the existing behavior on master:
    When trying to raise the exception H,
    F -> G -> H -> I -> NULL
    becomes
    H -> F -> G -> NULL

    But when trying to set the exception A on top of
        B -> C -> D -> E -> C -> ...,
        it gets stuck in an infinite loop from the existing cycle.
    

    My PR 20539 keeps the first behavior and resolves the infinite loop by making it
    A -> B -> C -> D -> E -> NULL,
    which seems consistent with the existing behavior.

    So it should be strictly a bugfix. It also only changes the PyErr_SetObject code and not the PyException_SetContext code.

    @cjerdonek
    Copy link
    Member

    I think this issue needs deeper discussion to know how to proceed.

    If there is a chain A -> B -> C -> D -> E, after assignment C.__context__ = A we will get a chain C -> A -> B -> D -> E. No exception is lost.

    I understand not wanting to lose exceptions here. However, if there were a separate exception F and the assignment were instead C.__context__ = F without raising C, the new chain would be A -> B -> C -> F. We would again lose D -> E. So why is that not a problem here, and yet it's a problem above? If we really didn't want to lose the exception, we could make it A -> B -> C -> F -> D -> E (and if raising C, it would become C -> F -> D -> E).

    Thus, I think we may want to consider separately the cases of explicitly setting the context (calling PyException_SetContext) and implicitly setting it (calling PyErr_SetObject). Maybe when setting explicitly, losing the previous value is okay.

    Also, there's another option for the top example in the implicit case of raising C. We could create a copy C' of C, so the new chain would be C -> A -> B -> C' -> D -> E. The code already has logic to create a new exception in certain cases: both _PyErr_SetObject and _PyErr_NormalizeException call _PyErr_CreateException. There are yet more options but I don't want to lengthen this comment further.

    Lastly, regarding Dennis's patch, I think the question of how to detect cycles should be discussed separately from what the behavior should be.

    @iritkatriel
    Copy link
    Member

    I agree with Chris that the issue of not hanging is independent of the question what to do about the cycle. The ExitStack issue that brought this issue about is now fixed in ExitStack (see bpo-25786, bpo-27122).

    If we take a step back from that, the situation is this: SetObject's cycle detection loop is intended to prevent the creation of new cycles when adding a new context link. It hangs when the exception chain has a pre-existing cycle. That pre-existing cycle is arguably a bug, and I'm not sure it's SetObject's job to do anything about it.

    So I would suggest to:
    (1) Make it not hang, ASAP because this is a bug in SetObject.
    (2) Leave the cycle alone.

    @iritkatriel iritkatriel added 3.11 bug and security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes labels Aug 6, 2021
    @iritkatriel
    Copy link
    Member

    I added a third patch to the discussion. (It overlaps Dennis's suggestion, and I'm happy to close it in favour of a tweaked version of Dennis' patch, but I thought this would be the simplest way to say what I mean, and hopefully push the discussion along).

    @cjerdonek
    Copy link
    Member

    Thanks, Irit. Can you show how your patch behaves using some representative examples (maybe using arrow examples from above)? And if it behaves differently from Dennis's patch, can you include an example showing that, too?

    @iritkatriel
    Copy link
    Member

    Like Dennis' patch, mine changes PyErr_SetObject. The difference is that Dennis' patch gets rid of the cycle while mine leaves it as it is, just avoids hanging on it.

    So in this case:

    when trying to set the exception A on top of
        B -> C -> D -> E -> C -> ...,
    

    The result would be simply

        A -> B -> C -> D -> E -> C -> ...,
    

    As I said in msg391637, a pre-existing cycle is due to a bug somewhere, and I don't think PyErr_SetObject can or should try to fix that bug. Nonsense in, nonsense out. If we change it we probably just make it more nonsensical.

    @cjerdonek
    Copy link
    Member

    Yes, that seems like a good approach. And how about with Serhiy's example from above?

    If there is a chain A -> B -> C -> D -> E, after assignment C.__context__ = A ...

    @iritkatriel
    Copy link
    Member

    Serhiy's patch is modifying a different part of this system - he changes the Exception object's SetContext to break cycles when they are first created. Dennis and I targeted the place where an exception is about to be raised and it gets a __context__ that may contain a cycle already.

    I think it's possible to take the more drastic step of preventing the cycles being created altogether, as Serhiy did. I would prefer that we raise an exception and refuse to create the cycle rather than try to fix it. I don't think we can come up with a meaningful fix to what is, really, a user bug.

    In any case, at the moment we have a situation where user bugs (like bpo-40696) can cause the interpreter to hang, and if we need more time to decide about the full strategy, we should at least prevent the hang.

    @cjerdonek
    Copy link
    Member

    That's okay. I didn't mean to suggest I thought your patch needed to handle that case or that we needed to decide it before moving forward. I was just wondering what would happen with your patch with it, even if it means a hang. Or were you saying that example can't arise in the code path you're modifying?

    @iritkatriel
    Copy link
    Member

    My patch doesn't change that at all, so it will be the same as it is currently: the result is C -> A -> B -> C. (But with my patch if you try to raise something in the context of C, it won't hang.)

    @cjerdonek
    Copy link
    Member

    the result is C -> A -> B -> C

    Did you mean C -> A -> B?

    By the way, if you applied to this example your reasoning that PyErr_SetObject shouldn't try to fix user bugs, should this example instead be C -> A -> B -> C -> ... (leaving the cycle as is but just not hanging)? Is it not being changed then because the reasoning doesn't apply, or because we're restricted in what we can do by backwards compatibility?

    @iritkatriel
    Copy link
    Member

    > the result is C -> A -> B -> C

    Did you mean C -> A -> B?

    No, I meant C -> A -> B -> C -> A ....
    the cycle remains unchanged.

    By the way, if you applied to this example your reasoning that PyErr_SetObject shouldn't try to fix user bugs, should this example instead be C -> A -> B -> C -> ... (leaving the cycle as is but just not hanging)?

    Yes, exactly, see above.

    Is it not being changed then because the reasoning doesn't apply, or because we're restricted in what we can do by backwards compatibility?

    The reason for leaving the cycle unchanged is not backwards compatibility, it's that this function cannot break the cycle in a meaningful way (this is why it's hard to agree on how it should do that).

    It can't be that A happened in the context of B at the same time that B happened in the context of A. So a cycle means there was a bug somewhere, and the exception's history is corrupt, so changing it will only make it more corrupt and harder to debug.

    Note that PyErr_SetObject avoids creating cycles, so the cycle was not created by someone catching an exception e and doing "raise e". It was caused by some other code tampering with the __context__ in an incorrect way.

    @cjerdonek
    Copy link
    Member

    No, I meant C -> A -> B -> C -> A ....

    Oh, good. I support your reasoning and approach, by the way.

    @serhiy-storchaka
    Copy link
    Member

    My argument is the loop creation should be prevented at first place. PyErr_SetObject() is not the only C code that can hang or overflow the stack when iterate a loop. Preventing creation of the loop will fix all other code that iterate the __context__ chain.

    @iritkatriel
    Copy link
    Member

    My argument is the loop creation should be prevented at first place.

    I agree, but avoiding the hang is higher priority.

    Preventing creation of the loop will fix all other code that iterate the __context__ chain.

    There could still be a cycles involving both __context__ and __cause__ links. This is why the traceback code uses a visited set to detect cycles.

    @cjerdonek
    Copy link
    Member

    Preventing creation of the loop will fix all other code that iterate the __context__ chain.

    We can still do / discuss that following Irit's proposed change, which is an improvement, IMO.

    @iritkatriel
    Copy link
    Member

    Note that my PR can (and should) be backported, while a change in the semantics of __context__ assignment, I'm not sure.

    @sobolevn
    Copy link
    Member

    sobolevn commented Aug 9, 2021

    There's also a similar case with python3.9:

    >>> class MyError(Exception):
    ...   ...
    ... 
    >>> e = MyError('e')
    >>> e.__context__ = e
    >>> 
    >>> try:
    ...   raise e
    ... except MyError:
    ...   print('done')
    ... 
    done  # hangs after this
    ^C^Z

    The same code works with python3.8
    We got hit by this in RustPython: RustPython/RustPython#2820

    @ambv
    Copy link
    Contributor

    ambv commented Aug 10, 2021

    New changeset d5c2174 by Irit Katriel in branch 'main':
    bpo-25782: avoid hang in PyErr_SetObject when current exception has a cycle in its context chain (GH-27626)
    d5c2174

    @ambv
    Copy link
    Contributor

    ambv commented Aug 10, 2021

    New changeset 6f4cded by Miss Islington (bot) in branch '3.9':
    bpo-25782: avoid hang in PyErr_SetObject when current exception has a cycle in its context chain (GH-27626) (GH-27707)
    6f4cded

    @miss-islington
    Copy link
    Contributor

    New changeset d86bbe3 by Miss Islington (bot) in branch '3.10':
    bpo-25782: avoid hang in PyErr_SetObject when current exception has a cycle in its context chain (GH-27626)
    d86bbe3

    @ambv
    Copy link
    Contributor

    ambv commented Aug 10, 2021

    Serhiy, I'm not closing this yet in case you'd like to finish implementing your PR. If not, feel free to close.

    @corona10
    Copy link
    Member

    Serhiy, I'm not closing this yet in case you'd like to finish implementing your PR. If not, feel free to close.

    I check that PR 20543 makes CPython works correctly with msg399281 code.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel
    Copy link
    Member

    Serhiy, I'm not closing this yet in case you'd like to finish implementing your PR. If not, feel free to close.

    I don't think we should do try to prevent cycles from being created. That means traversing the whole cause and context tree every time we add a new link in the chain, which is quadratic overall. We should only try to detect cycles when we have another reason to traverse the tree.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests