From 3976267ad10c170f8c1b993bf2a2eb804c20354b Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 4 Sep 2024 17:03:23 +0000 Subject: [PATCH 1/2] gh-123321: Make Parser/myreadline.c locking safe in free-threaded build Use a `PyMutex` to avoid the race in mutex initialization. Use relaxed atomics to avoid the data race on reading _PyOS_ReadlineTState when checking for re-entrant calls. --- Lib/test/test_readline.py | 3 +-- Parser/myreadline.c | 33 +++++++++------------------------ 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/Lib/test/test_readline.py b/Lib/test/test_readline.py index 7d07906df20cc1..50e77cbbb6be13 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 @@ -351,7 +351,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""" diff --git a/Parser/myreadline.c b/Parser/myreadline.c index 6eab56ac52dc08..2c11f316e3551d 100644 --- a/Parser/myreadline.c +++ b/Parser/myreadline.c @@ -28,7 +28,7 @@ PyAPI_DATA(PyThreadState*) _PyOS_ReadlineTState; PyThreadState *_PyOS_ReadlineTState = NULL; -static PyThread_type_lock _PyOS_ReadlineLock = NULL; +static PyMutex _PyOS_ReadlineLock; int (*PyOS_InputHook)(void) = NULL; @@ -373,34 +373,21 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) size_t len; PyThreadState *tstate = _PyThreadState_GET(); - if (_PyOS_ReadlineTState == tstate) { + if (_Py_atomic_load_ptr_relaxed(&_PyOS_ReadlineTState) == tstate) { PyErr_SetString(PyExc_RuntimeError, "can't re-enter readline"); return NULL; } - + // GH-123321: We need to acquire the lock before setting + // _PyOS_ReadlineTState, otherwise the variable may be nullified by a + // different thread. + PyMutex_Lock(&_PyOS_ReadlineLock); + _Py_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, tstate); if (PyOS_ReadlineFunctionPointer == NULL) { PyOS_ReadlineFunctionPointer = PyOS_StdioReadline; } - if (_PyOS_ReadlineLock == NULL) { - _PyOS_ReadlineLock = PyThread_allocate_lock(); - if (_PyOS_ReadlineLock == NULL) { - PyErr_SetString(PyExc_MemoryError, "can't allocate lock"); - return NULL; - } - } - - 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 * a tty. This can happen, for example if python is run like @@ -426,10 +413,8 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) // 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); - - Py_END_ALLOW_THREADS + _Py_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, NULL); + PyMutex_Unlock(&_PyOS_ReadlineLock); if (rv == NULL) return NULL; From ebbdf97be79d9537482f644001cd11eac8cb3033 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 5 Sep 2024 16:16:26 +0000 Subject: [PATCH 2/2] Restore removed `Py_BEGIN_ALLOW_THREADS` macro call. The PyOS_ReadlineFunctionPointer and PyOS_StdioReadline expect to be called with the GIL released. --- Parser/myreadline.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Parser/myreadline.c b/Parser/myreadline.c index 2c11f316e3551d..74c44ff77717f5 100644 --- a/Parser/myreadline.c +++ b/Parser/myreadline.c @@ -382,6 +382,7 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) // GH-123321: We need to acquire the lock before setting // _PyOS_ReadlineTState, otherwise the variable may be nullified by a // different thread. + Py_BEGIN_ALLOW_THREADS PyMutex_Lock(&_PyOS_ReadlineLock); _Py_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, tstate); if (PyOS_ReadlineFunctionPointer == NULL) { @@ -415,6 +416,7 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) // taking the GIL. Otherwise a deadlock or segfault may occur. _Py_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, NULL); PyMutex_Unlock(&_PyOS_ReadlineLock); + Py_END_ALLOW_THREADS if (rv == NULL) return NULL;