Skip to content

Commit cef1e07

Browse files
committed
[lldb] Fix that the embedded Python REPL crashes if it receives SIGINT
When LLDB receives a SIGINT while running the embedded Python REPL it currently just crashes in `ScriptInterpreterPythonImpl::Interrupt` with an error such as the one below: ``` Fatal Python error: PyThreadState_Get: the function must be called with the GIL held, but the GIL is released (the current Python thread state is NULL) ``` The faulty code that causes this error is this part of `ScriptInterpreterPythonImpl::Interrupt`: ``` PyThreadState *state = PyThreadState_GET(); if (!state) state = GetThreadState(); if (state) { long tid = state->thread_id; PyThreadState_Swap(state); int num_threads = PyThreadState_SetAsyncExc(tid, PyExc_KeyboardInterrupt); ``` The obvious fix I tried is to just acquire the GIL before this code is running which fixes the crash but the `KeyboardInterrupt` we want to raise immediately is actually just queued and would only be raised once the next line of input has been parsed (which e.g. won't interrupt Python code that is currently waiting on a timer or IO from what I can see). Also none of the functions we call here is marked as safe to be called from a signal handler from what I can see, so we might still end up crashing here with some bad timing. Python 3.2 introduced `PyErr_SetInterrupt` to solve this and the function takes care of all the details and avoids doing anything that isn't safe to do inside a signal handler. The only thing we need to do is to manually setup our own fake SIGINT handler that behaves the same way as the standalone Python REPL signal handler (which raises a KeyboardInterrupt). From what I understand the old code used to work with Python 2 so I kept the old code around until we officially drop support for Python 2. There is a small gap here with Python 3.0->3.1 where we might still be crashing, but those versions have reached their EOL more than a decade ago so I think we don't need to bother about them. Reviewed By: JDevlieghere Differential Revision: https://reviews.llvm.org/D104886
1 parent bf5748a commit cef1e07

File tree

2 files changed

+132
-0
lines changed

2 files changed

+132
-0
lines changed

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1067,6 +1067,23 @@ void ScriptInterpreterPythonImpl::ExecuteInterpreterLoop() {
10671067
}
10681068

10691069
bool ScriptInterpreterPythonImpl::Interrupt() {
1070+
// PyErr_SetInterrupt was introduced in 3.2.
1071+
#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 2) || (PY_MAJOR_VERSION > 3)
1072+
// If the interpreter isn't evaluating any Python at the moment then return
1073+
// false to signal that this function didn't handle the interrupt and the
1074+
// next component should try handling it.
1075+
if (!IsExecutingPython())
1076+
return false;
1077+
1078+
// Tell Python that it should pretend to have received a SIGINT.
1079+
PyErr_SetInterrupt();
1080+
// PyErr_SetInterrupt has no way to return an error so we can only pretend the
1081+
// signal got successfully handled and return true.
1082+
// Python 3.10 introduces PyErr_SetInterruptEx that could return an error, but
1083+
// the error handling is limited to checking the arguments which would be
1084+
// just our (hardcoded) input signal code SIGINT, so that's not useful at all.
1085+
return true;
1086+
#else
10701087
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_SCRIPT));
10711088

10721089
if (IsExecutingPython()) {
@@ -1088,6 +1105,7 @@ bool ScriptInterpreterPythonImpl::Interrupt() {
10881105
"ScriptInterpreterPythonImpl::Interrupt() python code not running, "
10891106
"can't interrupt");
10901107
return false;
1108+
#endif
10911109
}
10921110

10931111
bool ScriptInterpreterPythonImpl::ExecuteOneLineWithReturn(
@@ -3293,6 +3311,28 @@ ScriptInterpreterPythonImpl::AcquireInterpreterLock() {
32933311
return py_lock;
32943312
}
32953313

3314+
namespace {
3315+
/// Saves the current signal handler for the specified signal and restores
3316+
/// it at the end of the current scope.
3317+
struct RestoreSignalHandlerScope {
3318+
/// The signal handler.
3319+
struct sigaction m_prev_handler;
3320+
int m_signal_code;
3321+
RestoreSignalHandlerScope(int signal_code) : m_signal_code(signal_code) {
3322+
// Initialize sigaction to their default state.
3323+
std::memset(&m_prev_handler, 0, sizeof(m_prev_handler));
3324+
// Don't install a new handler, just read back the old one.
3325+
struct sigaction *new_handler = nullptr;
3326+
int signal_err = ::sigaction(m_signal_code, new_handler, &m_prev_handler);
3327+
lldbassert(signal_err == 0 && "sigaction failed to read handler");
3328+
}
3329+
~RestoreSignalHandlerScope() {
3330+
int signal_err = ::sigaction(m_signal_code, &m_prev_handler, nullptr);
3331+
lldbassert(signal_err == 0 && "sigaction failed to restore old handler");
3332+
}
3333+
};
3334+
} // namespace
3335+
32963336
void ScriptInterpreterPythonImpl::InitializePrivate() {
32973337
if (g_initialized)
32983338
return;
@@ -3328,6 +3368,25 @@ void ScriptInterpreterPythonImpl::InitializePrivate() {
33283368
"lldb.embedded_interpreter; from "
33293369
"lldb.embedded_interpreter import run_python_interpreter; "
33303370
"from lldb.embedded_interpreter import run_one_line");
3371+
3372+
#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 2) || (PY_MAJOR_VERSION > 3)
3373+
// Python will not just overwrite its internal SIGINT handler but also the
3374+
// one from the process. Backup the current SIGINT handler to prevent that
3375+
// Python deletes it.
3376+
RestoreSignalHandlerScope save_sigint(SIGINT);
3377+
3378+
// Setup a default SIGINT signal handler that works the same way as the
3379+
// normal Python REPL signal handler which raises a KeyboardInterrupt.
3380+
// Also make sure to not pollute the user's REPL with the signal module nor
3381+
// our utility function.
3382+
PyRun_SimpleString("def lldb_setup_sigint_handler():\n"
3383+
" import signal;\n"
3384+
" def signal_handler(sig, frame):\n"
3385+
" raise KeyboardInterrupt()\n"
3386+
" signal.signal(signal.SIGINT, signal_handler);\n"
3387+
"lldb_setup_sigint_handler();\n"
3388+
"del lldb_setup_sigint_handler\n");
3389+
#endif
33313390
}
33323391

33333392
void ScriptInterpreterPythonImpl::AddToSysPath(AddLocation location,
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
"""
2+
Test sending SIGINT to the embedded Python REPL.
3+
"""
4+
5+
import os
6+
7+
import lldb
8+
from lldbsuite.test.decorators import *
9+
from lldbsuite.test.lldbtest import *
10+
from lldbsuite.test.lldbpexpect import PExpectTest
11+
12+
class TestCase(PExpectTest):
13+
14+
mydir = TestBase.compute_mydir(__file__)
15+
16+
def start_python_repl(self):
17+
""" Starts up the embedded Python REPL."""
18+
self.launch()
19+
# Start the embedded Python REPL via the 'script' command.
20+
self.child.send("script -l python --\n")
21+
# Wait for the Python REPL prompt.
22+
self.child.expect(">>>")
23+
24+
# PExpect uses many timeouts internally and doesn't play well
25+
# under ASAN on a loaded machine..
26+
@skipIfAsan
27+
def test_while_evaluating_code(self):
28+
""" Tests SIGINT handling while Python code is being evaluated."""
29+
self.start_python_repl()
30+
31+
# Start a long-running command that we try to abort with SIGINT.
32+
# Note that we dont actually wait 10000s in this code as pexpect or
33+
# lit will kill the test way before that.
34+
self.child.send("import time; print('running' + 'now'); time.sleep(10000);\n")
35+
36+
# Make sure the command is actually being evaluated at the moment by
37+
# looking at the string that the command is printing.
38+
# Don't check for a needle that also occurs in the program itself to
39+
# prevent that echoing will make this check pass unintentionally.
40+
self.child.expect("runningnow")
41+
42+
# Send SIGINT to the LLDB process.
43+
self.child.sendintr()
44+
45+
# This should get transformed to a KeyboardInterrupt which is the same
46+
# behaviour as the standalone Python REPL. It should also interrupt
47+
# the evaluation of our sleep statement.
48+
self.child.expect("KeyboardInterrupt")
49+
# Send EOF to quit the Python REPL.
50+
self.child.sendeof()
51+
52+
self.quit()
53+
54+
# PExpect uses many timeouts internally and doesn't play well
55+
# under ASAN on a loaded machine..
56+
@skipIfAsan
57+
# FIXME: On Linux the Python code that reads from stdin seems to block until
58+
# it has finished reading a line before handling any queued signals.
59+
@skipIfLinux
60+
def test_while_waiting_on_input(self):
61+
""" Tests SIGINT handling while the REPL is waiting on input from
62+
stdin."""
63+
self.start_python_repl()
64+
65+
# Send SIGINT to the LLDB process.
66+
self.child.sendintr()
67+
# This should get transformed to a KeyboardInterrupt which is the same
68+
# behaviour as the standalone Python REPL.
69+
self.child.expect("KeyboardInterrupt")
70+
# Send EOF to quit the Python REPL.
71+
self.child.sendeof()
72+
73+
self.quit()

0 commit comments

Comments
 (0)