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

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.

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

Antoine, how would you fix this?

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

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?

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"?

> 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.

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

> 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.

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.)

> 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?

> 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.

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.

> 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.

>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?

> 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*?

> 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.

> 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?

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?

> 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?

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

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

> 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.

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

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

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.

The patch LGTM (but I prefer reordering solution).

> Serhiy Storchaka added the comment:
> 
> The patch LGTM (but I prefer reordering solution).

Serhiy, could you please describe your solution in more detail? An elaborate example would help. Perhaps I just misunderstand your idea. And thanks for the review!

Here is a patch that illustrates my idea (the documentation could be better).

And added more comments to your patch.

Patch review: I see two competing solutions with differing behaviors. 

Yury's raises a RuntimeError in the loop situation.

Serhiy's simply reorders the exception context to put the referred to one at the front of the chain in the event of a loop.

The 3.5 Release manager or someone familiar with the contextlib ExitStack code where this is triggered should make the decision as to the best way to fix this.

(I came to this bug via the ExitStack + subprocess issue27122 which merely has a suggested workaround patch as a band aid that might help until this is fixed)

I'm not the right person to decide this.

As RM, all I can do is decide whether or not to hold up a release based on the bug.  The answer: no.  Since this isn't fixed yet in the 3.5 branch, 3.5.2 will go out without it being fixed.  Sorry folks.

> Yury's raises a RuntimeError in the loop situation.

> Serhiy's simply reorders the exception context to put the referred to one at the front of the chain in the event of a loop.

Right, and I believe that my solution is more Pythonic.  Reordering feels highly unintuitive to me.

For both of the proposed patches, could we add a test case based on contextlib.ExitStack and a variant of Victor's #27122 reproducer script at http://bugs.python.org/file42999/hang_bug2.py that uses a deliberately broken __exit__ implementation that always re-raises the exception thrown into the underlying generator?

As discussed on that issue, I'm wondering if we need some additional defensive coding in ExitStack itself, or if resolving this underlying problem will be enough to also categorically prevent the hang in ExitStack.

As far as possible resolutions go, I tend to prefer lying about the shape of the exception state over changing the type of the raised exception - we already know the true exception state is a tree rather than a chain (see #18861 for some related discussions), and we should be able to ensure that all active exceptions remain somewhere in the chain, even if their order is a bit surprising.

With reordering the issue27122 test is passed without changes. No changes in ExitStack is needed. Here is updated patch.

With Yury's path the issue27122 test need to be changed (RuntimeError is now raised instead of original exception). Here is updated patch.

Thanks Serhiy. I've attached a new file (issue27122_broken_cm.py) with a context manager that is deliberately buggy in the same way as contextlib._GeneratorContextManager is currently, so the new test can be made independent of #27122 being fixed (Greg Smith has a pending patch to make that CM well behaved again, which would currently lead to your new test passing for the wrong reasons)

issue27122_broken_cm.py also shows why I think "make it work" is the right answer here - the odd context chaining is tolerated in the case where the interpreter is implicitly setting the exception context, so I believe it should also be tolerated when ExitStack sets the context explicitly. If we "do the right thing" (where "right" = "the same as what the interpreter does") in the setter, then not only ExitStack, but anyone else setting __context__ should automatically benefit from the adjustment.

> issue27122_broken_cm.py also shows why I think "make it work" is the right answer here [..]

But fixing this issue by reordering the exception chain will only mask bugs that just better to be fixed.  And sometimes, this will cause weird exceptions chains, that will make *some* people spend a lot of time debugging/googling to understand what's going on.

> But fixing this issue by reordering the exception chain will only mask bugs that just better to be fixed.  And sometimes, this will cause weird exceptions chains, that will make *some* people spend a lot of time debugging/googling to understand what's going on.

Right, but the way to fix that is to figure out a more coherent linearisation strategy (and/or move away from linearisation by allowing __context__ to be a tuple). Both #18861 and the frame annotation idea it spawned in #19585 have some ideas on how we might be able to improve that situation.

Adding *yet another* exception to an already confused exception chain isn't likely to make it easier to debug, and may mask the original exception that triggered the misbehaving error handler (entirely defeating the point of adding exception chaining in the first place).

I'm not saying Serhiy's patch is the right long term solution - I'm saying I think it's a better short term mitigation measure, since it doesn't lose information the way nulling out the __context__ may do.

I think I'm with Nick on this, the re-ordering fix is the closest to the right thing to do for 3.5 as it at least leaves all exceptions present in the chain.

As a more accurate long term fix, it would be great if we could express the tree via tuples in exception context in 3.6 or later.

New changeset 9ee36b74b432 by Gregory P. Smith in branch '3.5':
Issue #27123: When an exception is raised within the context being
https://hg.python.org/cpython/rev/9ee36b74b432

New changeset 9fadeee05880 by Gregory P. Smith in branch 'default':
Issue #27123: When an exception is raised within the context being
https://hg.python.org/cpython/rev/9fadeee05880

For reference: the correct issue number is #27122.

Are there examples of creating a loop not involving ExitStack or explicit setting the __context__ attribute?

Issue #28962 appears to be an example of this issue.  It uses 'raise e from e' to create the loop.

My patch works for your example too.  Since it checks for loops in __context__ setter, you shouldn't be able to create complicated loops. 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.(I came to this bug via the ExitStack + subprocess issue27122 which merely has a suggested workaround patch as a band aid that might help until this is fixed) 
Thanks: http://driverwhiz.com/device-drivers

Why is this still open?  GPS: didn't your checkin last June fix this?

Because we have two alternate solutions, issue25782_5.patch and set_context_reordering2.patch.

What should we do with this issue? Does it need opening a topic on Python-Dev?

I believe the original issue is fixed, the original problem and the one Jack followed up with no longer work in modern Python 3.

But there is discussion about possibly doing something better, if anyone is interested in doing that I suggest opening a new Enhancement issue and turning the patches floating around into PRs for discussion.

i'm closing this as the worst reported behavior has been fixed.

Issue40696 is other example of creating a cycle.

I think we should solve general problem preventing loops by merging one of proposed patches.

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.

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.

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 issue25786, issue27122).

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.

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).

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?

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.

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 ...

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 Issue40696) 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.

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?

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.)

> 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?

> > 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.

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

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

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.

> 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.

> 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.

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

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

```python
>>> 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: https://github.com/RustPython/RustPython/pull/2820

New changeset d5c217475c4957a8084ac3f92ae012ece5edc7cb 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)
https://github.com/python/cpython/commit/d5c217475c4957a8084ac3f92ae012ece5edc7cb

New changeset 6f4cdeddb97532144f93ca37b8b21451f445c7bf 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)
https://github.com/python/cpython/commit/6f4cdeddb97532144f93ca37b8b21451f445c7bf

New changeset d86bbe3cff0abefc13e5462cca1fb3344d4a5b52 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)
https://github.com/python/cpython/commit/d86bbe3cff0abefc13e5462cca1fb3344d4a5b52

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

> 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.