[3.12] gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-thread...

* gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race (GH-123323)
(cherry picked from commit a4562fedadb73fe1e978dece65c3bcefb4606678)

Co-authored-by: Bar Harel <bharel@barharel.com>
* Remove @requires_gil_enabled for 3.12

---------

Co-authored-by: Bar Harel <bharel@barharel.com>
Co-authored-by: Sam Gross <colesbury@gmail.com>

diff --git a/Lib/test/test_readline.py b/Lib/test/test_readline.py
index 58cc6b7e7f480328b78f9678ea4bbc33e71d9832..fab124ae4ad45c5a4d5e1d6e0f115df4cbb3d371 100644 (file)
--- a/Lib/test/test_readline.py
+++ b/Lib/test/test_readline.py
@@ -12,6 +12,7 @@ from test.support.import_helper import import_module
 from test.support.os_helper import unlink, temp_dir, TESTFN
 from test.support.pty_helper import run_pty
 from test.support.script_helper import assert_python_ok
+from test.support.threading_helper import requires_working_threading
 
 # Skip tests if there is no readline module
 readline = import_module('readline')
@@ -346,6 +347,30 @@ readline.write_history_file(history_file)
             self.assertEqual(len(lines), history_size)
             self.assertEqual(lines[-1].strip(), b"last input")
 
+    @requires_working_threading()
+    def test_gh123321_threadsafe(self):
+        """gh-123321: readline should be thread-safe and not crash"""
+        script = textwrap.dedent(r"""
+            import threading
+            from test.support.threading_helper import join_thread
+
+            def func():
+                input()
+
+            thread1 = threading.Thread(target=func)
+            thread2 = threading.Thread(target=func)
+            thread1.start()
+            thread2.start()
+            join_thread(thread1)
+            join_thread(thread2)
+            print("done")
+        """)
+
+        output = run_pty(script, input=b"input1\rinput2\r")
+
+        self.assertIn(b"done", output)
+
+
     def test_write_read_limited_history(self):
         previous_length = readline.get_history_length()
         self.addCleanup(readline.set_history_length, previous_length)

diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-08-26-00-58-26.gh-issue-123321.ApxcnE.rst b/Misc/NEWS.d/next/Core and Builtins/2024-08-26-00-58-26.gh-issue-123321.ApxcnE.rst
new file mode 100644 (file)
index 0000000..b0547e0
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2024-08-26-00-58-26.gh-issue-123321.ApxcnE.rst
@@ -0,0 +1,2 @@
+Prevent Parser/myreadline race condition from segfaulting on multi-threaded
+use. Patch by Bar Harel and Amit Wienner.

diff --git a/Parser/myreadline.c b/Parser/myreadline.c
index 7074aba74b728c818dc94f23fea1e076a7c1cc52..2890ff83f3f64b3dd4aedb224527c937d1b38df9 100644 (file)
--- a/Parser/myreadline.c
+++ b/Parser/myreadline.c
@@ -386,9 +386,14 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt)
         }
     }
 
-    _PyOS_ReadlineTState = tstate;
     Py_BEGIN_ALLOW_THREADS
+
+    // GH-123321: We need to acquire the lock before setting
+    // _PyOS_ReadlineTState and after the release of the GIL, otherwise
+    // the variable may be nullified by a different thread or a deadlock
+    // may occur if the GIL is taken in any sub-function.
     PyThread_acquire_lock(_PyOS_ReadlineLock, 1);
+    _PyOS_ReadlineTState = tstate;
 
     /* This is needed to handle the unlikely case that the
      * interpreter is in interactive mode *and* stdin/out are not
@@ -412,11 +417,13 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt)
     else {
         rv = (*PyOS_ReadlineFunctionPointer)(sys_stdin, sys_stdout, prompt);
     }
-    Py_END_ALLOW_THREADS
 
+    // gh-123321: Must set the variable and then release the lock before
+    // taking the GIL. Otherwise a deadlock or segfault may occur.
+    _PyOS_ReadlineTState = NULL;
     PyThread_release_lock(_PyOS_ReadlineLock);
 
-    _PyOS_ReadlineTState = NULL;
+    Py_END_ALLOW_THREADS
 
     if (rv == NULL)
         return NULL;