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

Conversation

Projects
None yet
5 participants
@xdegaye
Copy link
Contributor

commented Jun 7, 2017

…ni()

@pitrou

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

Did you verify the removed code here wasn't needed anymore? For the record, it was added in changeset 1e534b5 by @brettcannon.

@@ -219,8 +219,6 @@ PyAPI_DATA(PyObject *) PyExc_IOError;
PyAPI_DATA(PyObject *) PyExc_WindowsError;
#endif

PyAPI_DATA(PyObject *) PyExc_RecursionErrorInst;

This comment has been minimized.

Copy link
@brettcannon

brettcannon Jun 9, 2017

Member

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

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 10, 2017

Author Contributor

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 ?

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 11, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@brettcannon

brettcannon Jun 12, 2017

Member

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.

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 13, 2017

Author Contributor

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.

@@ -306,16 +306,7 @@ PyErr_NormalizeException(PyObject **exc, PyObject **val, PyObject **tb)
}
/* normalize recursively */
tstate = PyThreadState_GET();
if (++tstate->recursion_depth > Py_GetRecursionLimit()) {

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 10, 2017

Member

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

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 10, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 11, 2017

Author Contributor

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.");
    }

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 11, 2017

Member

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.

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 13, 2017

Author Contributor

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 ?

@xdegaye

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2017

Did you verify the removed code here wasn't needed anymore?

Answering Antoine question on bpo-22898.

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

This comment has been minimized.

Copy link
@pitrou

pitrou Jun 13, 2017

Member

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.

This comment has been minimized.

Copy link
@pitrou

pitrou Jun 13, 2017

Member

Or at least I think so.

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 13, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@brettcannon

brettcannon Jun 13, 2017

Member

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

This comment has been minimized.

Copy link
@pitrou

pitrou Jun 14, 2017

Member

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.

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 14, 2017

Member

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

This comment has been minimized.

Copy link
@pitrou

pitrou Jun 14, 2017

Member

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.

This comment has been minimized.

Copy link
@brettcannon

brettcannon Jun 14, 2017

Member

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.

@xdegaye

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2017

Superseded by GH-2327.

@xdegaye xdegaye closed this Oct 26, 2017

@xdegaye xdegaye deleted the xdegaye:bpo-22898 branch Oct 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.