Skip to content

[3.13] gh-123321: Make Parser/myreadline.c locking safe in free-threaded build (GH-123690) #123798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 6, 2024
Merged

[3.13] gh-123321: Make Parser/myreadline.c locking safe in free-threaded build (GH-123690) #123798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Lib/test/test_readline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"""
Expand Down
33 changes: 10 additions & 23 deletions Parser/myreadline.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
Loading