CPython hangs on error __context__ set to the error itself #69968
Comments
|
try: |
1st1
added
interpreter-core
|
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: 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"? |
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? |
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__ = exHowever, 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 |
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? |
|
|
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. |
Don't do that, a few hours (!) is not enough to test a fix. It's too The bug was here since at least Python 3.3, there is no urgency to fix it. |
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*? |
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. |
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:
It's a choice for the release manager. But IHMO there will always be |
Nothing. Or may be raise an exception (because C.__context__ can't be set to what you try).
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? |
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, 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). |
|
I think this issue needs deeper discussion to know how to proceed.
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 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: |
iritkatriel
added
3.11
|
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: The result would be simply 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?
|
|
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. |
|
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.) |
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? |
No, I meant C -> A -> B -> C -> A ....
Yes, exactly, see above.
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. |
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. |
I agree, but avoiding the hang is higher priority.
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. |
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: >>> class MyError(Exception):
... ...
...
>>> e = MyError('e')
>>> e.__context__ = e
>>>
>>> try:
... raise e
... except MyError:
... print('done')
...
done # hangs after this
^C^ZThe same code works with python3.8 |
|
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. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: