From bf05ff94b1e1bfc31b3f22f0d69e6298c0cbb70c Mon Sep 17 00:00:00 2001 From: Bar Harel Date: Wed, 4 Sep 2024 18:21:30 +0300 Subject: [PATCH 1/2] 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 --- Lib/test/test_readline.py | 28 ++++++++++++++++++- ...-08-26-00-58-26.gh-issue-123321.ApxcnE.rst | 2 ++ Parser/myreadline.c | 13 +++++++-- 3 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-08-26-00-58-26.gh-issue-123321.ApxcnE.rst diff --git a/Lib/test/test_readline.py b/Lib/test/test_readline.py index 58cc6b7e7f4803..935d46dc4259fb 100644 --- a/Lib/test/test_readline.py +++ b/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') @@ -346,6 +347,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) 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 00000000000000..b0547e0e588e3d --- /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 7074aba74b728c..2890ff83f3f64b 100644 --- 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; From 9aacbbe82d0ea62e77b75d5c5a5e530ef4c42fa2 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 4 Sep 2024 19:28:50 +0000 Subject: [PATCH 2/2] Remove @requires_gil_enabled for 3.12 --- Lib/test/test_readline.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_readline.py b/Lib/test/test_readline.py index 935d46dc4259fb..fab124ae4ad45c 100644 --- a/Lib/test/test_readline.py +++ b/Lib/test/test_readline.py @@ -7,7 +7,7 @@ import tempfile import textwrap import unittest -from test.support import requires_gil_enabled, verbose +from test.support import 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 @@ -348,7 +348,6 @@ def test_history_size(self): 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"""