From 6f7346bb3983cd7a6aa97eeeafffb3cecd5292b8 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 3 Jun 2020 18:28:18 +0200 Subject: [3.9] bpo-40826: Fix GIL usage in PyOS_Readline() (GH-20613) (GH-20616) * bpo-40826: Fix GIL usage in PyOS_Readline() (GH-20579) Fix GIL usage in PyOS_Readline(): lock the GIL to set an exception. Pass tstate to my_fgets() and _PyOS_WindowsConsoleReadline(). Cleanup these functions. (cherry picked from commit c353764fd564e401cf47a5d9efab18c72c60014e) * bpo-40826: Add _PyOS_InterruptOccurred(tstate) function (GH-20599) my_fgets() now calls _PyOS_InterruptOccurred(tstate) to check for pending signals, rather calling PyOS_InterruptOccurred(). my_fgets() is called with the GIL released, whereas PyOS_InterruptOccurred() must be called with the GIL held. test_repl: use text=True and avoid SuppressCrashReport in test_multiline_string_parsing(). Fix my_fgets() on Windows: fgets(fp) does crash if fileno(fp) is closed. (cherry picked from commit fa7ab6aa0f9a4f695e5525db5a113cd21fa93787) --- Include/internal/pycore_pystate.h | 3 + Lib/test/test_repl.py | 22 ++-- .../2020-06-01-20-31-07.bpo-40826.XCI4M2.rst | 2 + Modules/signalmodule.c | 26 ++++- Parser/myreadline.c | 118 +++++++++++++++------ 5 files changed, 128 insertions(+), 43 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2020-06-01-20-31-07.bpo-40826.XCI4M2.rst diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index f90e7e1..96d5e31 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -317,6 +317,9 @@ PyAPI_FUNC(void) _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime); PyAPI_FUNC(void) _PyGILState_Reinit(_PyRuntimeState *runtime); + +PyAPI_FUNC(int) _PyOS_InterruptOccurred(PyThreadState *tstate); + #ifdef __cplusplus } #endif diff --git a/Lib/test/test_repl.py b/Lib/test/test_repl.py index 71f192f..563f188 100644 --- a/Lib/test/test_repl.py +++ b/Lib/test/test_repl.py @@ -29,7 +29,9 @@ def spawn_repl(*args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, **kw): # test.support.script_helper. env = kw.setdefault('env', dict(os.environ)) env['TERM'] = 'vt100' - return subprocess.Popen(cmd_line, executable=sys.executable, + return subprocess.Popen(cmd_line, + executable=sys.executable, + text=True, stdin=subprocess.PIPE, stdout=stdout, stderr=stderr, **kw) @@ -49,12 +51,11 @@ class TestInteractiveInterpreter(unittest.TestCase): sys.exit(0) """ user_input = dedent(user_input) - user_input = user_input.encode() p = spawn_repl() with SuppressCrashReport(): p.stdin.write(user_input) output = kill_python(p) - self.assertIn(b'After the exception.', output) + self.assertIn('After the exception.', output) # Exit code 120: Py_FinalizeEx() failed to flush stdout and stderr. self.assertIn(p.returncode, (1, 120)) @@ -86,13 +87,22 @@ class TestInteractiveInterpreter(unittest.TestCase): """ ''' user_input = dedent(user_input) - user_input = user_input.encode() p = spawn_repl() - with SuppressCrashReport(): - p.stdin.write(user_input) + p.stdin.write(user_input) output = kill_python(p) self.assertEqual(p.returncode, 0) + def test_close_stdin(self): + user_input = dedent(''' + import os + print("before close") + os.close(0) + ''') + process = spawn_repl() + output = process.communicate(user_input)[0] + self.assertEqual(process.returncode, 0) + self.assertIn('before close', output) + if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-06-01-20-31-07.bpo-40826.XCI4M2.rst b/Misc/NEWS.d/next/Core and Builtins/2020-06-01-20-31-07.bpo-40826.XCI4M2.rst new file mode 100644 index 0000000..a03ed18 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2020-06-01-20-31-07.bpo-40826.XCI4M2.rst @@ -0,0 +1,2 @@ +Fix GIL usage in :c:func:`PyOS_Readline`: lock the GIL to set an exception +and pass the Python thread state when checking if there is a pending signal. diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 0c9a267..119fc35 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -187,14 +187,20 @@ itimer_retval(struct itimerval *iv) #endif static int -is_main(_PyRuntimeState *runtime) +is_main_interp(_PyRuntimeState *runtime, PyInterpreterState *interp) { unsigned long thread = PyThread_get_thread_ident(); - PyInterpreterState *interp = _PyRuntimeState_GetThreadState(runtime)->interp; return (thread == runtime->main_thread && interp == runtime->interpreters.main); } +static int +is_main(_PyRuntimeState *runtime) +{ + PyInterpreterState *interp = _PyRuntimeState_GetThreadState(runtime)->interp; + return is_main_interp(runtime, interp); +} + static PyObject * signal_default_int_handler(PyObject *self, PyObject *args) { @@ -1726,12 +1732,14 @@ PyOS_FiniInterrupts(void) finisignal(); } + +// The caller doesn't have to hold the GIL int -PyOS_InterruptOccurred(void) +_PyOS_InterruptOccurred(PyThreadState *tstate) { if (_Py_atomic_load_relaxed(&Handlers[SIGINT].tripped)) { _PyRuntimeState *runtime = &_PyRuntime; - if (!is_main(runtime)) { + if (!is_main_interp(runtime, tstate->interp)) { return 0; } _Py_atomic_store_relaxed(&Handlers[SIGINT].tripped, 0); @@ -1740,6 +1748,16 @@ PyOS_InterruptOccurred(void) return 0; } + +// The caller must to hold the GIL +int +PyOS_InterruptOccurred(void) +{ + PyThreadState *tstate = _PyThreadState_GET(); + return _PyOS_InterruptOccurred(tstate); +} + + static void _clear_pending_signals(void) { diff --git a/Parser/myreadline.c b/Parser/myreadline.c index 43e5583..d7ed357 100644 --- a/Parser/myreadline.c +++ b/Parser/myreadline.c @@ -25,25 +25,36 @@ static PyThread_type_lock _PyOS_ReadlineLock = NULL; int (*PyOS_InputHook)(void) = NULL; /* This function restarts a fgets() after an EINTR error occurred - except if PyOS_InterruptOccurred() returns true. */ + except if _PyOS_InterruptOccurred() returns true. */ static int -my_fgets(char *buf, int len, FILE *fp) +my_fgets(PyThreadState* tstate, char *buf, int len, FILE *fp) { #ifdef MS_WINDOWS - HANDLE hInterruptEvent; + HANDLE handle; + _Py_BEGIN_SUPPRESS_IPH + handle = (HANDLE)_get_osfhandle(fileno(fp)); + _Py_END_SUPPRESS_IPH + + /* bpo-40826: fgets(fp) does crash if fileno(fp) is closed */ + if (handle == INVALID_HANDLE_VALUE) { + return -1; /* EOF */ + } #endif - char *p; - int err; + while (1) { - if (PyOS_InputHook != NULL) + if (PyOS_InputHook != NULL) { (void)(PyOS_InputHook)(); + } + errno = 0; clearerr(fp); - p = fgets(buf, len, fp); - if (p != NULL) + char *p = fgets(buf, len, fp); + if (p != NULL) { return 0; /* No error */ - err = errno; + } + int err = errno; + #ifdef MS_WINDOWS /* Ctrl-C anywhere on the line or Ctrl-Z if the only character on a line will set ERROR_OPERATION_ABORTED. Under normal @@ -59,7 +70,7 @@ my_fgets(char *buf, int len, FILE *fp) through to check for EOF. */ if (GetLastError()==ERROR_OPERATION_ABORTED) { - hInterruptEvent = _PyOS_SigintEvent(); + HANDLE hInterruptEvent = _PyOS_SigintEvent(); switch (WaitForSingleObjectEx(hInterruptEvent, 10, FALSE)) { case WAIT_OBJECT_0: ResetEvent(hInterruptEvent); @@ -69,23 +80,27 @@ my_fgets(char *buf, int len, FILE *fp) } } #endif /* MS_WINDOWS */ + if (feof(fp)) { clearerr(fp); return -1; /* EOF */ } + #ifdef EINTR if (err == EINTR) { - int s; - PyEval_RestoreThread(_PyOS_ReadlineTState); - s = PyErr_CheckSignals(); + PyEval_RestoreThread(tstate); + int s = PyErr_CheckSignals(); PyEval_SaveThread(); - if (s < 0) - return 1; - /* try again */ + + if (s < 0) { + return 1; + } + /* try again */ continue; } #endif - if (PyOS_InterruptOccurred()) { + + if (_PyOS_InterruptOccurred(tstate)) { return 1; /* Interrupt */ } return -2; /* Error */ @@ -99,7 +114,7 @@ my_fgets(char *buf, int len, FILE *fp) extern char _get_console_type(HANDLE handle); char * -_PyOS_WindowsConsoleReadline(HANDLE hStdIn) +_PyOS_WindowsConsoleReadline(PyThreadState *tstate, HANDLE hStdIn) { static wchar_t wbuf_local[1024 * 16]; const DWORD chunk_size = 1024; @@ -134,11 +149,12 @@ _PyOS_WindowsConsoleReadline(HANDLE hStdIn) if (WaitForSingleObjectEx(hInterruptEvent, 100, FALSE) == WAIT_OBJECT_0) { ResetEvent(hInterruptEvent); - PyEval_RestoreThread(_PyOS_ReadlineTState); + PyEval_RestoreThread(tstate); s = PyErr_CheckSignals(); PyEval_SaveThread(); - if (s < 0) + if (s < 0) { goto exit; + } } break; } @@ -151,17 +167,22 @@ _PyOS_WindowsConsoleReadline(HANDLE hStdIn) if (wbuf == wbuf_local) { wbuf[total_read] = '\0'; wbuf = (wchar_t*)PyMem_RawMalloc(wbuflen * sizeof(wchar_t)); - if (wbuf) + if (wbuf) { wcscpy_s(wbuf, wbuflen, wbuf_local); + } else { + PyEval_RestoreThread(tstate); PyErr_NoMemory(); + PyEval_SaveThread(); goto exit; } } else { wchar_t *tmp = PyMem_RawRealloc(wbuf, wbuflen * sizeof(wchar_t)); if (tmp == NULL) { + PyEval_RestoreThread(tstate); PyErr_NoMemory(); + PyEval_SaveThread(); goto exit; } wbuf = tmp; @@ -170,33 +191,45 @@ _PyOS_WindowsConsoleReadline(HANDLE hStdIn) if (wbuf[0] == '\x1a') { buf = PyMem_RawMalloc(1); - if (buf) + if (buf) { buf[0] = '\0'; + } else { + PyEval_RestoreThread(tstate); PyErr_NoMemory(); + PyEval_SaveThread(); } goto exit; } - u8len = WideCharToMultiByte(CP_UTF8, 0, wbuf, total_read, NULL, 0, NULL, NULL); + u8len = WideCharToMultiByte(CP_UTF8, 0, + wbuf, total_read, + NULL, 0, + NULL, NULL); buf = PyMem_RawMalloc(u8len + 1); if (buf == NULL) { + PyEval_RestoreThread(tstate); PyErr_NoMemory(); + PyEval_SaveThread(); goto exit; } - u8len = WideCharToMultiByte(CP_UTF8, 0, wbuf, total_read, buf, u8len, NULL, NULL); + + u8len = WideCharToMultiByte(CP_UTF8, 0, + wbuf, total_read, + buf, u8len, + NULL, NULL); buf[u8len] = '\0'; exit: - if (wbuf != wbuf_local) + if (wbuf != wbuf_local) { PyMem_RawFree(wbuf); + } if (err) { - PyEval_RestoreThread(_PyOS_ReadlineTState); + PyEval_RestoreThread(tstate); PyErr_SetFromWindowsErr(err); PyEval_SaveThread(); } - return buf; } @@ -210,6 +243,8 @@ PyOS_StdioReadline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) { size_t n; char *p, *pr; + PyThreadState *tstate = _PyOS_ReadlineTState; + assert(tstate != NULL); #ifdef MS_WINDOWS if (!Py_LegacyWindowsStdioFlag && sys_stdin == stdin) { @@ -231,7 +266,9 @@ PyOS_StdioReadline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) if (wlen) { wbuf = PyMem_RawMalloc(wlen * sizeof(wchar_t)); if (wbuf == NULL) { + PyEval_RestoreThread(tstate); PyErr_NoMemory(); + PyEval_SaveThread(); return NULL; } wlen = MultiByteToWideChar(CP_UTF8, 0, prompt, -1, @@ -250,7 +287,7 @@ PyOS_StdioReadline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) } } clearerr(sys_stdin); - return _PyOS_WindowsConsoleReadline(hStdIn); + return _PyOS_WindowsConsoleReadline(tstate, hStdIn); } } #endif @@ -258,16 +295,19 @@ PyOS_StdioReadline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) n = 100; p = (char *)PyMem_RawMalloc(n); if (p == NULL) { + PyEval_RestoreThread(tstate); PyErr_NoMemory(); + PyEval_SaveThread(); return NULL; } fflush(sys_stdout); - if (prompt) + if (prompt) { fprintf(stderr, "%s", prompt); + } fflush(stderr); - switch (my_fgets(p, (int)n, sys_stdin)) { + switch (my_fgets(tstate, p, (int)n, sys_stdin)) { case 0: /* Normal case */ break; case 1: /* Interrupt */ @@ -279,29 +319,40 @@ PyOS_StdioReadline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) *p = '\0'; break; } + n = strlen(p); while (n > 0 && p[n-1] != '\n') { size_t incr = n+2; if (incr > INT_MAX) { PyMem_RawFree(p); + PyEval_RestoreThread(tstate); PyErr_SetString(PyExc_OverflowError, "input line too long"); + PyEval_SaveThread(); return NULL; } + pr = (char *)PyMem_RawRealloc(p, n + incr); if (pr == NULL) { PyMem_RawFree(p); + PyEval_RestoreThread(tstate); PyErr_NoMemory(); + PyEval_SaveThread(); return NULL; } p = pr; - if (my_fgets(p+n, (int)incr, sys_stdin) != 0) + + if (my_fgets(tstate, p+n, (int)incr, sys_stdin) != 0) { break; + } n += strlen(p+n); } + pr = (char *)PyMem_RawRealloc(p, n+1); if (pr == NULL) { PyMem_RawFree(p); + PyEval_RestoreThread(tstate); PyErr_NoMemory(); + PyEval_SaveThread(); return NULL; } return pr; @@ -324,7 +375,8 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) char *rv, *res; size_t len; - if (_PyOS_ReadlineTState == _PyThreadState_GET()) { + PyThreadState *tstate = _PyThreadState_GET(); + if (_PyOS_ReadlineTState == tstate) { PyErr_SetString(PyExc_RuntimeError, "can't re-enter readline"); return NULL; @@ -343,7 +395,7 @@ PyOS_Readline(FILE *sys_stdin, FILE *sys_stdout, const char *prompt) } } - _PyOS_ReadlineTState = _PyThreadState_GET(); + _PyOS_ReadlineTState = tstate; Py_BEGIN_ALLOW_THREADS PyThread_acquire_lock(_PyOS_ReadlineLock, 1); -- cgit v0.12