From fa7ab6aa0f9a4f695e5525db5a113cd21fa93787 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 3 Jun 2020 14:39:59 +0200 Subject: [PATCH] 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. --- Include/internal/pycore_pystate.h | 3 +++ Lib/test/test_repl.py | 22 ++++++++++++++----- .../2020-06-01-20-31-07.bpo-40826.XCI4M2.rst | 3 ++- Modules/signalmodule.c | 14 ++++++++++-- Parser/myreadline.c | 17 ++++++++++---- 5 files changed, 46 insertions(+), 13 deletions(-) diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 423c8113d7a..0cd5550cfda 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -144,6 +144,9 @@ PyAPI_FUNC(int) _PyState_AddModule( PyObject* module, struct PyModuleDef* def); + +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 71f192f90d9..563f188706b 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 @@ def test_no_memory(self): 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 @@ def test_multiline_string_parsing(self): """ ''' 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 index f79f20d21d4..a03ed180eb9 100644 --- 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 @@ -1 +1,2 @@ -Fix GIL usage in :c:func:`PyOS_Readline`: lock the GIL to set an exception. +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 24dbd4255a6..ef3536a210b 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -1779,10 +1779,11 @@ PyOS_FiniInterrupts(void) finisignal(); } + +// The caller doesn't have to hold the GIL int -PyOS_InterruptOccurred(void) +_PyOS_InterruptOccurred(PyThreadState *tstate) { - PyThreadState *tstate = _PyThreadState_GET(); _Py_EnsureTstateNotNULL(tstate); if (!_Py_ThreadCanHandleSignals(tstate->interp)) { return 0; @@ -1797,6 +1798,15 @@ PyOS_InterruptOccurred(void) } +// The caller must to hold the GIL +int +PyOS_InterruptOccurred(void) +{ + PyThreadState *tstate = _PyThreadState_GET(); + return _PyOS_InterruptOccurred(tstate); +} + + #ifdef HAVE_FORK static void _clear_pending_signals(void) diff --git a/Parser/myreadline.c b/Parser/myreadline.c index d2787f0d345..2dd362321aa 100644 --- a/Parser/myreadline.c +++ b/Parser/myreadline.c @@ -24,14 +24,23 @@ 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(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 + while (1) { if (PyOS_InputHook != NULL) { (void)(PyOS_InputHook)(); @@ -60,7 +69,7 @@ my_fgets(PyThreadState* tstate, 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); @@ -90,7 +99,7 @@ my_fgets(PyThreadState* tstate, char *buf, int len, FILE *fp) } #endif - if (PyOS_InterruptOccurred()) { + if (_PyOS_InterruptOccurred(tstate)) { return 1; /* Interrupt */ } return -2; /* Error */