From a4562fedadb73fe1e978dece65c3bcefb4606678 Mon Sep 17 00:00:00 2001
From: Bar Harel <bharel@barharel.com>
Date: Wed, 4 Sep 2024 18:21:30 +0300
Subject: gh-123321: Fix Parser/myreadline.c to prevent a segfault during a
multi-threaded race (#123323)
---
Lib/test/test_readline.py | 28 +++++++++++++++++++++-
.../2024-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 91fd7dd..7d07906 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 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 1825665..6eab56a 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;
--
cgit v0.12