Skip to content

Commit 0c080d7

Browse files
authored
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.
1 parent 8a46a2e commit 0c080d7

File tree

2 files changed

+11
-25
lines changed

2 files changed

+11
-25
lines changed

Lib/test/test_readline.py

+1-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import tempfile
88
import textwrap
99
import unittest
10-
from test.support import requires_gil_enabled, verbose
10+
from test.support import verbose
1111
from test.support.import_helper import import_module
1212
from test.support.os_helper import unlink, temp_dir, TESTFN
1313
from test.support.pty_helper import run_pty
@@ -351,7 +351,6 @@ def test_history_size(self):
351351
self.assertEqual(lines[-1].strip(), b"last input")
352352

353353
@requires_working_threading()
354-
@requires_gil_enabled()
355354
def test_gh123321_threadsafe(self):
356355
"""gh-123321: readline should be thread-safe and not crash"""
357356
script = textwrap.dedent(r"""

Parser/myreadline.c

+10-23
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
PyAPI_DATA(PyThreadState*) _PyOS_ReadlineTState;
2929
PyThreadState *_PyOS_ReadlineTState = NULL;
3030

31-
static PyThread_type_lock _PyOS_ReadlineLock = NULL;
31+
static PyMutex _PyOS_ReadlineLock;
3232

3333
int (*PyOS_InputHook)(void) = NULL;
3434

@@ -373,34 +373,22 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt)
373373
size_t len;
374374

375375
PyThreadState *tstate = _PyThreadState_GET();
376-
if (_PyOS_ReadlineTState == tstate) {
376+
if (_Py_atomic_load_ptr_relaxed(&_PyOS_ReadlineTState) == tstate) {
377377
PyErr_SetString(PyExc_RuntimeError,
378378
"can't re-enter readline");
379379
return NULL;
380380
}
381381

382-
382+
// GH-123321: We need to acquire the lock before setting
383+
// _PyOS_ReadlineTState, otherwise the variable may be nullified by a
384+
// different thread.
385+
Py_BEGIN_ALLOW_THREADS
386+
PyMutex_Lock(&_PyOS_ReadlineLock);
387+
_Py_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, tstate);
383388
if (PyOS_ReadlineFunctionPointer == NULL) {
384389
PyOS_ReadlineFunctionPointer = PyOS_StdioReadline;
385390
}
386391

387-
if (_PyOS_ReadlineLock == NULL) {
388-
_PyOS_ReadlineLock = PyThread_allocate_lock();
389-
if (_PyOS_ReadlineLock == NULL) {
390-
PyErr_SetString(PyExc_MemoryError, "can't allocate lock");
391-
return NULL;
392-
}
393-
}
394-
395-
Py_BEGIN_ALLOW_THREADS
396-
397-
// GH-123321: We need to acquire the lock before setting
398-
// _PyOS_ReadlineTState and after the release of the GIL, otherwise
399-
// the variable may be nullified by a different thread or a deadlock
400-
// may occur if the GIL is taken in any sub-function.
401-
PyThread_acquire_lock(_PyOS_ReadlineLock, 1);
402-
_PyOS_ReadlineTState = tstate;
403-
404392
/* This is needed to handle the unlikely case that the
405393
* interpreter is in interactive mode *and* stdin/out are not
406394
* 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)
426414

427415
// gh-123321: Must set the variable and then release the lock before
428416
// taking the GIL. Otherwise a deadlock or segfault may occur.
429-
_PyOS_ReadlineTState = NULL;
430-
PyThread_release_lock(_PyOS_ReadlineLock);
431-
417+
_Py_atomic_store_ptr_relaxed(&_PyOS_ReadlineTState, NULL);
418+
PyMutex_Unlock(&_PyOS_ReadlineLock);
432419
Py_END_ALLOW_THREADS
433420

434421
if (rv == NULL)

0 commit comments

Comments
 (0)