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

bpo-22898: Singleton RecursionError may hold a traceback in _PyExc_Fi… #1981

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Include/pyerrors.h
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,6 @@ PyAPI_DATA(PyObject *) PyExc_IOError;
PyAPI_DATA(PyObject *) PyExc_WindowsError;
#endif

PyAPI_DATA(PyObject *) PyExc_RecursionErrorInst;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the public API, so ripping it out is a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this PR was removing PyExc_RecursionErrorInst, ripping it out sounds scary though and I am not sure it should be done then :-)
Why is it part of the public API ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For information, the PyExc_MemoryErrorInst singleton, also a part of the public API then, has been removed in 98e2b45 when fixing bpo-5437, and it was removed for the same reasons that this PR is removing the PyExc_RecursionErrorInst singleton.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it should at least be documented in What's New as having been removed since while it is an internal implementation detail, it's name wasn't given a leading _ to more significantly signal that it wasn't meant for use outside of Python itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have documented this removal in What's New just before the merge (to avoid merge conflicts) especially after you pointed out that it's part of the public API.


/* Predefined warning categories */
PyAPI_DATA(PyObject *) PyExc_Warning;
PyAPI_DATA(PyObject *) PyExc_UserWarning;
Expand Down
56 changes: 55 additions & 1 deletion Lib/test/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

from test.support import (TESTFN, captured_stderr, check_impl_detail,
check_warnings, cpython_only, gc_collect, run_unittest,
no_tracing, unlink, import_module)
no_tracing, unlink, import_module, script_helper)

class NaiveException(Exception):
def __init__(self, x):
Expand Down Expand Up @@ -908,6 +908,60 @@ def g():
self.assertIsInstance(v, RecursionError, type(v))
self.assertIn("maximum recursion depth exceeded", str(v))

@cpython_only
def test_recursion_normalizing_exception(self):
# Issue #22898.
# Test that an infinite recursion is not entered when recursion_depth
# is equal to recursion_limit in PyErr_NormalizeException(). Prior to
# #22898 a PyExc_RecursionErrorInst singleton was being used that held
# traceback data and locals indefinitely and would cause a segfault in
# _PyExc_Fini() upon finalization of these locals.
code = """if 1:
import sys
from _testcapi import get_recursion_depth

class MyException(Exception): pass

def setrecursionlimit(depth):
while 1:
try:
sys.setrecursionlimit(depth)
return depth
except RecursionError:
# sys.setrecursionlimit() raises a RecursionError if
# the new recursion limit is too low (issue #25274).
depth += 1

def recurse(cnt):
cnt -= 1
if cnt:
recurse(cnt)
else:
generator.throw(MyException)

def gen():
f = open(%a, mode='rb', buffering=0)
yield

generator = gen()
next(generator)
recursionlimit = sys.getrecursionlimit()
depth = get_recursion_depth()
try:
# Upon the last recursive invocation of recurse(),
# tstate->recursion_depth is equal to (recursion_limit - 1)
# and is equal to recursion_limit when _gen_throw() calls
# PyErr_NormalizeException().
recurse(setrecursionlimit(depth + 2) - depth - 1)
finally:
sys.setrecursionlimit(recursionlimit)
print('Done.')
""" % __file__
rc, out, err = script_helper.assert_python_failure("-c", code)
# Check that the program does not fail with SIGABRT.
self.assertEqual(rc, 1)
self.assertIn(b'RecursionError', err)
self.assertIn(b'Done.', out)

@cpython_only
def test_MemoryError(self):
Expand Down
32 changes: 0 additions & 32 deletions Objects/exceptions.c
Original file line number Diff line number Diff line change
Expand Up @@ -2408,12 +2408,6 @@ SimpleExtendsException(PyExc_Warning, ResourceWarning,



/* Pre-computed RecursionError instance for when recursion depth is reached.
Meant to be used when normalizing the exception for exceeding the recursion
depth will cause its own infinite recursion.
*/
PyObject *PyExc_RecursionErrorInst = NULL;

#define PRE_INIT(TYPE) \
if (!(_PyExc_ ## TYPE.tp_flags & Py_TPFLAGS_READY)) { \
if (PyType_Ready(&_PyExc_ ## TYPE) < 0) \
Expand Down Expand Up @@ -2673,37 +2667,11 @@ _PyExc_Init(PyObject *bltinmod)
ADD_ERRNO(TimeoutError, ETIMEDOUT)

preallocate_memerrors();

if (!PyExc_RecursionErrorInst) {
PyExc_RecursionErrorInst = BaseException_new(&_PyExc_RecursionError, NULL, NULL);
if (!PyExc_RecursionErrorInst)
Py_FatalError("Cannot pre-allocate RecursionError instance for "
"recursion errors");
else {
PyBaseExceptionObject *err_inst =
(PyBaseExceptionObject *)PyExc_RecursionErrorInst;
PyObject *args_tuple;
PyObject *exc_message;
exc_message = PyUnicode_FromString("maximum recursion depth exceeded");
if (!exc_message)
Py_FatalError("cannot allocate argument for RecursionError "
"pre-allocation");
args_tuple = PyTuple_Pack(1, exc_message);
if (!args_tuple)
Py_FatalError("cannot allocate tuple for RecursionError "
"pre-allocation");
Py_DECREF(exc_message);
if (BaseException_init(err_inst, args_tuple, NULL))
Py_FatalError("init of pre-allocated RecursionError failed");
Py_DECREF(args_tuple);
}
}
}

void
_PyExc_Fini(void)
{
Py_CLEAR(PyExc_RecursionErrorInst);
free_preallocated_memerrors();
Py_CLEAR(errnomap);
}
Expand Down
1 change: 0 additions & 1 deletion PC/python3.def
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,6 @@ EXPORTS
PyExc_PermissionError=python37.PyExc_PermissionError DATA
PyExc_ProcessLookupError=python37.PyExc_ProcessLookupError DATA
PyExc_RecursionError=python37.PyExc_RecursionError DATA
PyExc_RecursionErrorInst=python37.PyExc_RecursionErrorInst DATA
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line means PyExc_RecursionErrorInst is part of the stable ABI and shouldn't be removed at all. See PEP 384 for more information about the stable ABI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least I think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the point is that using PyExc_RecursionErrorInst is an implementation oversight that may break Python, surely it is a case where its removal from the stable API is allowed no ?
IMO not removing it while not using it anymore in the CPython implementation looks worse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitrou you can also look at my comments above about this exact topic. Apparently we have done this in the past when we removed exception singletons.

If we need final closure on this we can always ask on python-dev. And while you're right, @pitrou , that it might be part of the stable ABI (have to go through the header file to verify), apparently the stable ABI has been broken for some time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently we have done this in the past when we removed exception singletons.

Hmm, thank you, I stand corrected. I guess the dynamic linker doesn't mind a symbol isn't there if the symbol isn't actually used in the program linking with libpython.

apparently the stable ABI has been broken for some time.

This is unfortunate but you're probably right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have other concern. This singleton may be needed. Otherwise it wouldn't be added at first case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the singleton was necessary on Python 2, which didn't have the two-level recursion threshold. It may not be necessary anymore on Python 3.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, the reason the singleton was added was to allow for a deeper recursion limit so that constructing the instance itself didn't eat into the recursion depth (IOW it could fail to construct the instance). @pitrou may be right about it not being necessary in Python 3, but I'm personally not sure.

PyExc_ReferenceError=python37.PyExc_ReferenceError DATA
PyExc_ResourceWarning=python37.PyExc_ResourceWarning DATA
PyExc_RuntimeError=python37.PyExc_RuntimeError DATA
Expand Down
11 changes: 1 addition & 10 deletions Python/errors.c
Original file line number Diff line number Diff line change
Expand Up @@ -306,16 +306,7 @@ PyErr_NormalizeException(PyObject **exc, PyObject **val, PyObject **tb)
}
/* normalize recursively */
tstate = PyThreadState_GET();
if (++tstate->recursion_depth > Py_GetRecursionLimit()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't removing this guard lead to stack overflow? See also #2035.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because the failed invocations of the functions that are followed by a 'goto finally' (i.e. PyObject_IsSubclass() and _PyErr_CreateException()) are all either protected themselves by a Py_EnterRecursiveCall() or they set a built-in exception that won't cause PyErr_NormalizeException() to recurse indefinitely when being normalized.

#2035 does not fix bpo-22898 which is about the PyExc_RecursionErrorInst singleton keeping a reference to a traceback whose frames have locals that are finalized when interp->sysdict is NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be doubly sure one could add the following guard here:

    if (++tstate->recursion_depth > Py_GetRecursionLimit() + 50) {
        Py_FatalError("Cannot recover from stack overflow.");
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a dead code this issue couldn't exist. Adding Py_FatalError() here will made the interpreter crashing earlier in the original reported example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your statement is not correct: test_recursion_normalizing_exception in this PR is a reproducer of the original reported example (check the gdb backtrace when run on the current code in master) and it does not abort or segfaults after adding the above Py_FatalError() guard to the PR and still raises the same RecursionError at the same recursion level and prints the ResourceWarning.

Are you aware that when tstate->overflowed is true a RecursionError is never raised ?

--tstate->recursion_depth;
/* throw away the old exception and use the recursion error instead */
Py_INCREF(PyExc_RecursionError);
Py_SETREF(*exc, PyExc_RecursionError);
Py_INCREF(PyExc_RecursionErrorInst);
Py_SETREF(*val, PyExc_RecursionErrorInst);
/* just keeping the old traceback */
return;
}
++tstate->recursion_depth;
PyErr_NormalizeException(exc, val, tb);
--tstate->recursion_depth;
}
Expand Down