[3.9] bpo-40826: Fix GIL usage in PyOS_Readline() (GH-20613) (GH-20616)

vstinner · web-flow · commit 6f7346bb3983 · 2020-06-03T18:28:18.000+02:00
* bpo-40826: Fix GIL usage in PyOS_Readline() (GH-20579) Fix GIL usage in PyOS_Readline(): lock the GIL to set an exception. Pass tstate to my_fgets() and _PyOS_WindowsConsoleReadline(). Cleanup these functions. (cherry picked from commit c353764) * bpo-40826: Add _PyOS_InterruptOccurred(tstate) function (GH-20599) my_fgets() now calls _PyOS_InterruptOccurred(tstate) to check for pending signals, rather calling PyOS_InterruptOccurred(). my_fgets() is called with the GIL released, whereas PyOS_InterruptOccurred() must be called with the GIL held. test_repl: use text=True and avoid SuppressCrashReport in test_multiline_string_parsing(). Fix my_fgets() on Windows: fgets(fp) does crash if fileno(fp) is closed. (cherry picked from commit fa7ab6a)
diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h
@@ -317,6 +317,9 @@ PyAPI_FUNC(void) _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime);
 
 PyAPI_FUNC(void) _PyGILState_Reinit(_PyRuntimeState *runtime);
 
+
+PyAPI_FUNC(int) _PyOS_InterruptOccurred(PyThreadState *tstate);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/Lib/test/test_repl.py b/Lib/test/test_repl.py
@@ -29,7 +29,9 @@ def spawn_repl(*args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, **kw):
     # test.support.script_helper.
     env = kw.setdefault('env', dict(os.environ))
     env['TERM'] = 'vt100'
-    return subprocess.Popen(cmd_line, executable=sys.executable,
+    return subprocess.Popen(cmd_line,
+                            executable=sys.executable,
+                            text=True,
                             stdin=subprocess.PIPE,
                             stdout=stdout, stderr=stderr,
                             **kw)
@@ -49,12 +51,11 @@ def test_no_memory(self):
             sys.exit(0)
         """
         user_input = dedent(user_input)
-        user_input = user_input.encode()
         p = spawn_repl()
         with SuppressCrashReport():
             p.stdin.write(user_input)
         output = kill_python(p)
-        self.assertIn(b'After the exception.', output)
+        self.assertIn('After the exception.', output)
         # Exit code 120: Py_FinalizeEx() failed to flush stdout and stderr.
         self.assertIn(p.returncode, (1, 120))
 
@@ -86,13 +87,22 @@ def test_multiline_string_parsing(self):
         </test>"""
         '''
         user_input = dedent(user_input)
-        user_input = user_input.encode()
         p = spawn_repl()
-        with SuppressCrashReport():
-            p.stdin.write(user_input)
+        p.stdin.write(user_input)
         output = kill_python(p)
         self.assertEqual(p.returncode, 0)
 
+    def test_close_stdin(self):
+        user_input = dedent('''
+            import os
+            print("before close")
+            os.close(0)
+        ''')
+        process = spawn_repl()
+        output = process.communicate(user_input)[0]
+        self.assertEqual(process.returncode, 0)
+        self.assertIn('before close', output)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-06-01-20-31-07.bpo-40826.XCI4M2.rst b/Misc/NEWS.d/next/Core and Builtins/2020-06-01-20-31-07.bpo-40826.XCI4M2.rst
@@ -0,0 +1,2 @@
+Fix GIL usage in :c:func:`PyOS_Readline`: lock the GIL to set an exception
+and pass the Python thread state when checking if there is a pending signal.
diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c
@@ -187,14 +187,20 @@ itimer_retval(struct itimerval *iv)
 #endif
 
 static int
-is_main(_PyRuntimeState *runtime)
+is_main_interp(_PyRuntimeState *runtime, PyInterpreterState *interp)
 {
     unsigned long thread = PyThread_get_thread_ident();
-    PyInterpreterState *interp = _PyRuntimeState_GetThreadState(runtime)->interp;
     return (thread == runtime->main_thread
             && interp == runtime->interpreters.main);
 }
 
+static int
+is_main(_PyRuntimeState *runtime)
+{
+    PyInterpreterState *interp = _PyRuntimeState_GetThreadState(runtime)->interp;
+    return is_main_interp(runtime, interp);
+}
+
 static PyObject *
 signal_default_int_handler(PyObject *self, PyObject *args)
 {
@@ -1726,12 +1732,14 @@ PyOS_FiniInterrupts(void)
     finisignal();
 }
 
+
+// The caller doesn't have to hold the GIL
 int
-PyOS_InterruptOccurred(void)
+_PyOS_InterruptOccurred(PyThreadState *tstate)
 {
     if (_Py_atomic_load_relaxed(&Handlers[SIGINT].tripped)) {
         _PyRuntimeState *runtime = &_PyRuntime;
-        if (!is_main(runtime)) {
+        if (!is_main_interp(runtime, tstate->interp)) {
             return 0;
         }
         _Py_atomic_store_relaxed(&Handlers[SIGINT].tripped, 0);
@@ -1740,6 +1748,16 @@ PyOS_InterruptOccurred(void)
     return 0;
 }
 
+
+// The caller must to hold the GIL
+int
+PyOS_InterruptOccurred(void)
+{
+    PyThreadState *tstate = _PyThreadState_GET();
+    return _PyOS_InterruptOccurred(tstate);
+}
+
+
 static void
 _clear_pending_signals(void)
 {
diff --git a/Parser/myreadline.c b/Parser/myreadline.c

Original file line number	Diff line number	Diff line change
`@@ -317,6 +317,9 @@ PyAPI_FUNC(void) _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime);`
`317`	`317`
`318`	`318`	`PyAPI_FUNC(void) _PyGILState_Reinit(_PyRuntimeState *runtime);`
`319`	`319`
	`320`	`+`
	`321`	`+PyAPI_FUNC(int) _PyOS_InterruptOccurred(PyThreadState *tstate);`
	`322`	`+`
`320`	`323`	`#ifdef __cplusplus`
`321`	`324`	`}`
`322`	`325`	`#endif`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+Fix GIL usage in :c:func:`PyOS_Readline`: lock the GIL to set an exception
	`2`	`+and pass the Python thread state when checking if there is a pending signal.`