Skip to content

gh-128130: Fix unhandled keyboard interrupt data race #129975

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
Feb 13, 2025
Merged

gh-128130: Fix unhandled keyboard interrupt data race #129975

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
2 changes: 1 addition & 1 deletion Include/internal/pycore_pylifecycle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ extern PyStatus _Py_PreInitializeFromConfig(

extern wchar_t * _Py_GetStdlibDir(void);

extern int _Py_HandleSystemExit(int *exitcode_p);
extern int _Py_HandleSystemExitAndKeyboardInterrupt(int *exitcode_p);

extern PyObject* _PyErr_WriteUnraisableDefaultHook(PyObject *unraisable);

Expand Down
12 changes: 3 additions & 9 deletions Modules/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ static int
pymain_err_print(int *exitcode_p)
{
int exitcode;
if (_Py_HandleSystemExit(&exitcode)) {
if (_Py_HandleSystemExitAndKeyboardInterrupt(&exitcode)) {
*exitcode_p = exitcode;
return 1;
}
Expand Down Expand Up @@ -292,11 +292,7 @@ pymain_start_pyrepl_no_main(void)
goto done;
}
if (!PyDict_SetItemString(kwargs, "pythonstartup", _PyLong_GetOne())) {
_PyRuntime.signals.unhandled_keyboard_interrupt = 0;
console_result = PyObject_Call(console, empty_tuple, kwargs);
if (!console_result && PyErr_Occurred() == PyExc_KeyboardInterrupt) {
_PyRuntime.signals.unhandled_keyboard_interrupt = 1;
}
if (console_result == NULL) {
res = pymain_exit_err_print();
}
Expand Down Expand Up @@ -338,11 +334,7 @@ pymain_run_module(const wchar_t *modname, int set_argv0)
Py_DECREF(module);
return pymain_exit_err_print();
}
_PyRuntime.signals.unhandled_keyboard_interrupt = 0;
result = PyObject_Call(runmodule, runargs, NULL);
if (!result && PyErr_Occurred() == PyExc_KeyboardInterrupt) {
_PyRuntime.signals.unhandled_keyboard_interrupt = 1;
}
Py_DECREF(runmodule);
Py_DECREF(module);
Py_DECREF(runargs);
Expand Down Expand Up @@ -763,6 +755,8 @@ Py_RunMain(void)
{
int exitcode = 0;

_PyRuntime.signals.unhandled_keyboard_interrupt = 0;

pymain_run_python(&exitcode);

if (Py_FinalizeEx() < 0) {
Expand Down
33 changes: 8 additions & 25 deletions Python/pythonrun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,8 +589,13 @@ parse_exit_code(PyObject *code, int *exitcode_p)
}

int
_Py_HandleSystemExit(int *exitcode_p)
_Py_HandleSystemExitAndKeyboardInterrupt(int *exitcode_p)
{
if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) {
_Py_atomic_store_int(&_PyRuntime.signals.unhandled_keyboard_interrupt, 1);
return 0;
}

int inspect = _Py_GetConfig()->inspect;
if (inspect) {
/* Don't exit if -i flag was given. This flag is set to 0
Expand Down Expand Up @@ -646,7 +651,7 @@ static void
handle_system_exit(void)
{
int exitcode;
if (_Py_HandleSystemExit(&exitcode)) {
if (_Py_HandleSystemExitAndKeyboardInterrupt(&exitcode)) {
Py_Exit(exitcode);
}
}
Expand Down Expand Up @@ -1105,8 +1110,6 @@ _PyErr_Display(PyObject *file, PyObject *unused, PyObject *value, PyObject *tb)
}
}

int unhandled_keyboard_interrupt = _PyRuntime.signals.unhandled_keyboard_interrupt;

// Try first with the stdlib traceback module
PyObject *print_exception_fn = PyImport_ImportModuleAttrString(
"traceback",
Expand All @@ -1120,11 +1123,9 @@ _PyErr_Display(PyObject *file, PyObject *unused, PyObject *value, PyObject *tb)
Py_XDECREF(print_exception_fn);
if (result) {
Py_DECREF(result);
_PyRuntime.signals.unhandled_keyboard_interrupt = unhandled_keyboard_interrupt;
Copy link
Member

@gpshead gpshead Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this being removed from the _PyErr_Display code paths? It was added by @pablogsal 's #110702 work & @iritkatriel 's review.

While we're sadly lacking comments in the code as to why... I believe this was for the scenario where we're heading back into Python code within the traceback printing and could thus receive a KeyboardInterrupt during that which we either wind up swallowing (bad - the "multiple Ctrl-C required to trigger the handler raising from a point the KeyboardInterrupt exception can propagate upwards to interrupt actual execution" problem that Python has had at times in the past? OR that none of the python code in the traceback printing path can swallow the KeyboardInterrupt state and prevent an ultimate exit_sigint() from terminating the process? (did I just describe the same thing twice?)

I'd be conservative and leave these here in _PyErr_Display, just using its now required atomic read and stores. It shouldn't be a speed-path, we're already rendering big strings and sending them out some file handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm generally sympathetic to being conservative when making changes, the unhandled_keyboard_interrupt logic can't be fixed by using atomic reads and writes. The saving and restoration done here could still end up erroneously clearing it in a different thread (even in the GIL-enabled build).

It also is not useful here anymore. We previously cleared unhandled_keyboard_interrupt in run_eval_code_obj, which is called from many places, including _PyErr_Display potentially. Now we are clearing it in Py_RunMain, which is the same place we read it.

The most important thing I'd like to convince you of is that we should be handling KeyboardInterrupt the same way as SystemError. They are both exceptions that when unhandled should lead to Python exiting with specific error code and/or behavior.

I believe this was for the scenario where we're heading back into Python code within the traceback printing and could thus receive a KeyboardInterrupt during that which we either wind up swallowing...

That's true before and after this PR -- the code did not handle that case. See this demo, for example: https://gist.github.com/colesbury/6871cc3d412c5d4fc51c7fd70e33e8a2

I think we should better handle that case, but probably not as part of this PR. This PR largely keeps the same behavior and handles the same cases as before, but avoids the race conditions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! This code is one of those places that I go "eew" every time I dive in on it; challenging to reason about and hard to craft reliable specific regression tests for.

I like the after state of this change better. Definitely cleaner to keep the clearing and use in one place.

return;
}
fallback:
_PyRuntime.signals.unhandled_keyboard_interrupt = unhandled_keyboard_interrupt;
#ifdef Py_DEBUG
if (PyErr_Occurred()) {
PyErr_FormatUnraisable(
Expand Down Expand Up @@ -1297,20 +1298,6 @@ flush_io(void)
static PyObject *
run_eval_code_obj(PyThreadState *tstate, PyCodeObject *co, PyObject *globals, PyObject *locals)
{
PyObject *v;
/*
* We explicitly re-initialize _Py_UnhandledKeyboardInterrupt every eval
* _just in case_ someone is calling into an embedded Python where they
* don't care about an uncaught KeyboardInterrupt exception (why didn't they
* leave config.install_signal_handlers set to 0?!?) but then later call
* Py_Main() itself (which _checks_ this flag and dies with a signal after
* its interpreter exits). We don't want a previous embedded interpreter's
* uncaught exception to trigger an unexplained signal exit from a future
* Py_Main() based one.
*/
// XXX Isn't this dealt with by the move to _PyRuntimeState?
_PyRuntime.signals.unhandled_keyboard_interrupt = 0;

/* Set globals['__builtins__'] if it doesn't exist */
if (!globals || !PyDict_Check(globals)) {
PyErr_SetString(PyExc_SystemError, "globals must be a real dict");
Expand All @@ -1328,11 +1315,7 @@ run_eval_code_obj(PyThreadState *tstate, PyCodeObject *co, PyObject *globals, Py
}
}

v = PyEval_EvalCode((PyObject*)co, globals, locals);
if (!v && _PyErr_Occurred(tstate) == PyExc_KeyboardInterrupt) {
_PyRuntime.signals.unhandled_keyboard_interrupt = 1;
}
return v;
return PyEval_EvalCode((PyObject*)co, globals, locals);
}

static PyObject *
Expand Down
2 changes: 1 addition & 1 deletion Tools/c-analyzer/TODO
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ Python/pythonrun.c:PyId_stdin _Py_IDENTIFIER(
Python/pythonrun.c:PyId_stdout _Py_IDENTIFIER(stdout)
Python/pythonrun.c:PyRun_InteractiveOneObjectEx():PyId___main__ _Py_IDENTIFIER(__main__)
Python/pythonrun.c:PyRun_InteractiveOneObjectEx():PyId_encoding _Py_IDENTIFIER(encoding)
Python/pythonrun.c:_Py_HandleSystemExit():PyId_code _Py_IDENTIFIER(code)
Python/pythonrun.c:_Py_HandleSystemExitAndKeyboardInterrupt():PyId_code _Py_IDENTIFIER(code)
Python/pythonrun.c:parse_syntax_error():PyId_filename _Py_IDENTIFIER(filename)
Python/pythonrun.c:parse_syntax_error():PyId_lineno _Py_IDENTIFIER(lineno)
Python/pythonrun.c:parse_syntax_error():PyId_msg _Py_IDENTIFIER(msg)
Expand Down
Loading