Title: Py_ReprEnter potentially misbehaves during malformed thread states
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.1, Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 2.7
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, itaibn, pitrou, python-dev
Priority: normal Keywords:

Created on 2014-03-26 22:57 by itaibn, last changed 2014-03-31 20:05 by pitrou. This issue is now closed.

Messages (6)
msg214920 - (view) Author: Itai Bar-Natan (itaibn) Date: 2014-03-26 22:57
While browsing the Python source code, I found this suspicious snippet in Py_ReprEnter:

    dict = PyThreadState_GetDict();
    if (dict == NULL)
        return 0;

It seems to me like the last line should be "return -1;". The way the program currently behaves, if PyThreadState_GetDict() fails and returns NULL, Py_ReprEnter will fail silently and always report that the input isn't in a recursive loop. The correct behavior is to report an error.

It would be difficult to explicitly exhibit this error since it relies on another component of Python failing first. One possible way would be to call PyObject_Repr on a recursive structure before fully initializing Python. I haven't tested this.

Alternately, it's possible that this behavior is intentional because we want PyObject_Repr to work for non-self-referential structures even before Python is fully initialized (perhaps it could be called during initialization), in exchange for a small risk of failure if it is called with a self-referential structure before initialization. In that case I suggest that this should be pointed out explicitly in the comments to this function.
msg214975 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-27 18:17
"hg annotate" shows it dates back to 4f0b7acffc7d by Guido, with the following diff:

diff --git a/Misc/NEWS b/Misc/NEWS
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -150,6 +150,10 @@
+- PyThreadState_GetDict() was changed not to raise an exception or
+  issue a fatal error when no current thread state is available.  This
+  makes it possible to print dictionaries when no thread is active.
 - LONG_LONG was renamed to PY_LONG_LONG.
 - Added PyObject_SelfIter() to fill the tp_iter slot for the
diff --git a/Objects/object.c b/Objects/object.c
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -2119,7 +2119,7 @@
        dict = PyThreadState_GetDict();
        if (dict == NULL)
-               return -1;
+               return 0;
        list = PyDict_GetItemString(dict, KEY);
        if (list == NULL) {
                list = PyList_New(0);

Unless Guido has changed his mind about it, I'd close this issue as rejected ;-)
msg214976 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-27 18:19
(but you're right, we could add a comment explaining this)
msg214977 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-03-27 18:25
No, I haven't changed my mind. Feel free to add a comment explaining this. :-)
msg215261 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-03-31 20:05
New changeset c42cce290d50 by Antoine Pitrou in branch '3.4':
Issue #21073: explain why Py_ReprEnter() allows for a missing thread state.

New changeset fb217fc457ca by Antoine Pitrou in branch 'default':
Issue #21073: explain why Py_ReprEnter() allows for a missing thread state.
msg215262 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-31 20:05
Ok, I've now added a comment.
Date User Action Args
2014-03-31 20:05:58pitrousetstatus: open -> closed
resolution: rejected
messages: + msg215262

stage: resolved
2014-03-31 20:05:11python-devsetnosy: + python-dev
messages: + msg215261
2014-03-27 18:25:36gvanrossumsetnosy: gvanrossum, pitrou, itaibn
messages: + msg214977
2014-03-27 18:19:19pitrousetmessages: + msg214976
2014-03-27 18:17:43pitrousetnosy: + gvanrossum, pitrou
messages: + msg214975
2014-03-26 22:57:40itaibncreate