Issue 2548: Undetected error in exception handling

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

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.

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.

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.

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

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.

This is a bug that can be fixed after beta, so I'm knocking it back to
critical for beta 1.

Ping

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.

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.

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.

The crashers which were deleted in rev58032 should at least have been
turned into unittests... well.

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.

Ouch, there were a couple of useless lines in ceval.h. New patch.

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.

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

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

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.