classification
Title: Undetected error in exception handling
Type: crash Stage:
Components: Interpreter Core Versions: Python 2.6
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, ajaksu2, amaury.forgeotdarc, barry, benjamin.peterson, brett.cannon, jafo, nnorwitz, pitrou, theller
Priority: critical Keywords: needs review, patch

Created on 2008-04-04 08:16 by theller, last changed 2010-08-16 21:38 by BreamoreBoy. This issue is now closed.

Files
File name Uploaded Description Edit
excrecurse.patch pitrou, 2008-08-30 00:15
Messages (18)
msg64920 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-04-04 08:16
[Found by Daniel Diniz (ajaksu2), see issue #2542]

The following code triggers an undetected error with a debug build:

"""
import sys
def g():
  try:
    return g()
  except:
    return sys.exc_info()
g()
print 42
"""

Running the code prints this:

C:\svn\trunk\PCbuild>python_d test2.py
42
XXX undetected error
Traceback (most recent call last):
  File "test2.py", line 8, in <module>
    print 42
RuntimeError: maximum recursion depth exceeded
[8826 refs]

C:\svn\trunk\PCbuild>
msg64954 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-04-05 00:34
Analysis: the primary recursion error is correctly raised, but then
there is a call to PyErr_NormalizeException, which calls
PyEval_CallObject, which increases the stack depth and hit the recursion
limit again...
python2.5 don't have the problem because PyEval_CallObject did not check
the recursion limit.

Different solutions I can think of:
- use a prebuilt exception: not possible if we still want an error
message containing the context.
- in PyErr_NormalizeException, "PyEval_CallObject" is too generic. We
could have a special path for exception types deriving from
BaseException: directly call the constructor.
msg64961 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2008-04-05 03:05
Brett, didn't you have a similar problem you fixed a while ago?  I
assigned to you for more input, feel free to reset to nobody.
msg64983 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-04-05 15:32
Neal has the memory of an elephant or something.

Yes, I dealt with a similar issue where the recursion limit was hit, but 
then normalizing the exception just caused it to hit it again. I thought 
I changed it such that normalizing an exception actually turned off the 
depth check or to raise a pre-defined exception for the recursion depth 
limit.

I don't have time to look at this right now (still on holiday in 
Brussels), but if I remember correctly the last time this came up I 
liked the pre-allocated recursion limit exception.
msg65053 - (view) Author: Daniel Diniz (ajaksu2) (Python triager) Date: 2008-04-06 21:22
I've identified rev58032 [1] as the one introducing this issue. It's
Brett's code, fixing a nasty crasher and adding a pre-built exception
(PyExc_RecursionErrorInst).


[1] http://svn.python.org/view?rev=58032&view=rev

P.S.: Thanks Thomas for correctly identifying an issue I mis-reported :)
msg66398 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2008-05-08 03:39
I'm not going to hold up the 2.6 alpha release for this, but will bump
it up for the first 2.6 beta.
msg68018 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2008-06-11 21:15
This is a bug that can be fixed after beta, so I'm knocking it back to
critical for beta 1.
msg70482 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-07-31 02:16
Ping
msg71095 - (view) Author: Daniel Diniz (ajaksu2) (Python triager) Date: 2008-08-13 19:49
FWIW, rev58032 introduced this:
    tstate = PyThreadState_GET();
    if (++tstate->recursion_depth > Py_GetRecursionLimit()) {
        --tstate->recursion_depth;
        PyErr_SetObject(PyExc_RuntimeError, PyExc_RecursionErrorInst);
        return;
    }
above this line: 
    PyErr_NormalizeException(exc, val, tb);

Contrary to (what I understand from) Amaury's analysis, ISTM that the
call to PyErr_SetObject is the problem, as after the recursion limit is
hit PyErr_NormalizeException isn't called again.

Commenting off the PyErr_SetObject line suppresses the "undetected
errors" and passes the unittests (including the infinite recursion
crashers removed in that rev). I have no idea about the problems it may
cause, though.
msg71107 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-08-14 02:36
On Wed, Aug 13, 2008 at 12:49 PM, Daniel Diniz <report@bugs.python.org> wrote:
>
> Daniel Diniz <ajaksu@gmail.com> added the comment:
>
> FWIW, rev58032 introduced this:
>    tstate = PyThreadState_GET();
>    if (++tstate->recursion_depth > Py_GetRecursionLimit()) {
>        --tstate->recursion_depth;
>        PyErr_SetObject(PyExc_RuntimeError, PyExc_RecursionErrorInst);
>        return;
>    }
> above this line:
>    PyErr_NormalizeException(exc, val, tb);
>
> Contrary to (what I understand from) Amaury's analysis, ISTM that the
> call to PyErr_SetObject is the problem, as after the recursion limit is
> hit PyErr_NormalizeException isn't called again.
>
> Commenting off the PyErr_SetObject line suppresses the "undetected
> errors" and passes the unittests (including the infinite recursion
> crashers removed in that rev). I have no idea about the problems it may
> cause, though.
>

If I remember correctly, that is on purpose as normalizing the
exception could lead to the stack being blown again. But this totally
off of memory, so I could be wrong.
msg71651 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-21 15:25
For what it's worth, py3k has a subtler recursion checking algorithm
which would probably fix this problem if backported properly. See
_Py_CheckRecursiveCall() in ceval.c (lines 462+), and especially the
role played by the tstate->overflowed flag, which allows a moderate
overflow of the recursion count in order for error handling code to
execute properly.
msg72163 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-29 21:53
The crashers which were deleted in rev58032 should at least have been
turned into unittests... well.
msg72176 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-30 00:05
Ok, here is a patch which seems to cover all bases. It fixes the present
bug, adds tests for the aforementioned ex-crashers, backports the
somewhat smarter recursion checking from py3k, and improves the latter
by fixing a nasty recursion bug when exceptions are ignored by C code.

I have to explain the latter problem a bit. Some functions like
PyDict_GetItem() are designed to silence all exceptions. If a
RuntimeError is raised because of a recursion overflow, however, a flag
(named "overflowed") is additionally set in the thread state structure;
this flag temporarily bumps the recursion_limit by 50, so that the  code
which caught the exception can perform some cleanup without itself
getting into the recursion limit. However, if recursion_limit + 50 is in
turned reached, the interpreter aborts with a fatal error.

Now, if the RuntimeError is discarded by PyDict_GetItem(), the
"overflowed" flag must also be reset, because the caller won't know that
there was a recursion overflow and that it must clean up. Instead, it
will simply assume the requested value is not in the dict, and will
continue happily... until it overflows the recursion limit again, which
will trigger the aforementioned fatal error if "overflowed" has not been
reset.

Consequently, PyErr_Clear() now resets the "overflowed" flag if the
current recursion count has gone back below the recursion limit. This
improvement must also be merged back to py3k.

Of course, we may also decide that all those subtle changes are not
worth the risk, just to fix a few obscure glitches.
msg72179 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-30 00:15
Ouch, there were a couple of useless lines in ceval.h. New patch.
msg72181 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-30 00:40
After thinking about it a bit, I think the whole recursion checking
thing has gone a bit mad. It probably deserves proper discussion on the
mailing-list. In the meantime, I'm downgrading this bug to critical.
msg101353 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2010-03-20 02:11
The final word on this seems to be this:

   - Wait until when we aren't in a beta release.  (DONE)

   - Quoting:

   Well, For Py3K at least we might need to consider going through the C
   API and fixing it so that these incorrect assumptions that functions
   like PyDict_GetItem() make are no longer made by introducing some new
   functions that behave in a "better" way.

   And for the recursion issue, I think it stems from corners that are
   cut in the C API by us. We inline functions all over the place, assume
   that Python's implementation underneath the hood is going to make
   calls that stay in C, etc. But as time has gone on and we have added
   flexibility to Python, more and more places have a chance to call
   Python code and trigger issues.

from Brett's e-mail at:

   http://mail.python.org/pipermail/python-dev/2008-August/082107.html

Anyone up to trying to make the C API changes required to get this
resolved?

Sean
msg110185 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-13 13:14
This has been fixed in Python 2.7, I suppose that the code could be backported to 2.6, or is it now too late so this can be closed as "won't fix"?
msg110215 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-07-13 18:50
It theoretically could be backported so it can stay open for now if someone feels adventurous to backport before 2.6.6. Once that's out the door, though, this can get closed.
History
Date User Action Args
2010-08-16 21:38:15BreamoreBoysetstatus: open -> closed
resolution: wont fix
2010-07-13 18:50:56brett.cannonsetmessages: + msg110215
2010-07-13 13:14:12BreamoreBoysetnosy: + BreamoreBoy
messages: + msg110185
2010-03-20 02:11:48jafosetnosy: + jafo
messages: + msg101353
2008-08-30 00:40:02pitrousetpriority: release blocker -> critical
messages: + msg72181
2008-08-30 00:15:13pitrousetfiles: + excrecurse.patch
messages: + msg72179
2008-08-30 00:14:33pitrousetfiles: - excrecurse.patch
2008-08-30 00:05:58pitrousetkeywords: + needs review
2008-08-30 00:05:45pitrousetfiles: + excrecurse.patch
keywords: + patch
messages: + msg72176
2008-08-29 21:53:47pitrousetmessages: + msg72163
2008-08-21 15:25:21pitrousetnosy: + pitrou
messages: + msg71651
2008-08-21 14:24:49benjamin.petersonsetpriority: critical -> release blocker
2008-08-14 02:36:43brett.cannonsetmessages: + msg71107
2008-08-13 19:49:34ajaksu2setmessages: + msg71095
2008-07-31 02:16:26benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg70482
2008-06-11 21:15:44barrysetpriority: release blocker -> critical
messages: + msg68018
2008-05-13 08:38:44georg.brandlsetpriority: critical -> release blocker
2008-05-08 03:39:35barrysetpriority: release blocker -> critical
nosy: + barry
messages: + msg66398
2008-04-06 21:22:38ajaksu2setmessages: + msg65053
2008-04-05 15:32:02brett.cannonsetassignee: brett.cannon ->
messages: + msg64983
2008-04-05 04:02:48ajaksu2setnosy: + ajaksu2
2008-04-05 03:05:04nnorwitzsetpriority: release blocker
assignee: brett.cannon
messages: + msg64961
nosy: + brett.cannon, nnorwitz
2008-04-05 00:34:24amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg64954
2008-04-04 08:16:41thellercreate