diff --git a/Lib/test/test_readline.py b/Lib/test/test_readline.py index 91fd7dd13f9..7d07906df20 100644 --- a/Lib/test/test_readline.py +++ b/Lib/test/test_readline.py @@ -7,11 +7,12 @@ import sys 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 @@ readline.write_history_file(history_file) 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) 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 index 00000000000..b0547e0e588 --- /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 18256653548..6eab56ac52d 100644 --- a/Parser/myreadline.c +++ b/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;