From 0c080d7c77d826c1afab7bd6b73f61e714cffcb7 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 6 Sep 2024 15:07:08 -0400 Subject: [PATCH] gh-123321: Make Parser/myreadline.c locking safe in free-threaded build (#123690) 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, 11 insertions(+), 25 deletions(-) diff --git a/Lib/test/test_readline.py b/Lib/test/test_readline.py index 7d07906df20..50e77cbbb6b 100644 --- a/Lib/test/test_readline.py +++ b/Lib/test/test_readline.py @@ -7,7 +7,7 @@ import sys 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 @@ readline.write_history_file(history_file) 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 6eab56ac52d..74c44ff7771 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,22 @@ 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. + Py_BEGIN_ALLOW_THREADS + 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,9 +414,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_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, NULL); + PyMutex_Unlock(&_PyOS_ReadlineLock); Py_END_ALLOW_THREADS if (rv == NULL)