[3.12] gh-123321: Fix Parser/myreadline.c to prevent a segfault during a multi-threaded race (GH-123323) (#123677)

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

Co-authored-by: Bar Harel <[email protected]>

* Remove @requires_gil_enabled for 3.12

---------

Co-authored-by: Bar Harel <[email protected]>
Co-authored-by: Sam Gross <[email protected]>

Author: 3 people

Lib/test/test_readline.py

@@ -12,6 +12,7 @@
 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 @@ def test_history_size(self):
             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)

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.

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;