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

Author: bharel

Lib/test/test_readline.py

@@ -7,11 +7,12 @@
 import tempfile
 import textwrap
 import unittest
-from test.support import verbose
+from test.support import requires_gil_enabled, verbose
 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')
@@ -349,6 +350,31 @@ def test_history_size(self):
             self.assertEqual(len(lines), history_size)
             self.assertEqual(lines[-1].strip(), b"last input")
 
+    @requires_working_threading()
+    @requires_gil_enabled()
+    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

@@ -392,9 +392,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
@@ -418,11 +423,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;