Skip to content

Commit 74973c3

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 (cherry picked from commit cef1e07)
1 parent 93bcb83 commit 74973c3

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
@@ -1050,6 +1050,23 @@ void ScriptInterpreterPythonImpl::ExecuteInterpreterLoop() {
10501050
}
10511051

10521052
bool ScriptInterpreterPythonImpl::Interrupt() {
1053+
// PyErr_SetInterrupt was introduced in 3.2.
1054+
#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 2) || (PY_MAJOR_VERSION > 3)
1055+
// If the interpreter isn't evaluating any Python at the moment then return
1056+
// false to signal that this function didn't handle the interrupt and the
1057+
// next component should try handling it.
1058+
if (!IsExecutingPython())
1059+
return false;
1060+
1061+
// Tell Python that it should pretend to have received a SIGINT.
1062+
PyErr_SetInterrupt();
1063+
// PyErr_SetInterrupt has no way to return an error so we can only pretend the
1064+
// signal got successfully handled and return true.
1065+
// Python 3.10 introduces PyErr_SetInterruptEx that could return an error, but
1066+
// the error handling is limited to checking the arguments which would be
1067+
// just our (hardcoded) input signal code SIGINT, so that's not useful at all.
1068+
return true;
1069+
#else
10531070
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_SCRIPT));
10541071

10551072
if (IsExecutingPython()) {
@@ -1071,6 +1088,7 @@ bool ScriptInterpreterPythonImpl::Interrupt() {
10711088
"ScriptInterpreterPythonImpl::Interrupt() python code not running, "
10721089
"can't interrupt");
10731090
return false;
1091+
#endif
10741092
}
10751093

10761094
bool ScriptInterpreterPythonImpl::ExecuteOneLineWithReturn(
@@ -3276,6 +3294,28 @@ ScriptInterpreterPythonImpl::AcquireInterpreterLock() {
32763294
return py_lock;
32773295
}
32783296

3297+
namespace {
3298+
/// Saves the current signal handler for the specified signal and restores
3299+
/// it at the end of the current scope.
3300+
struct RestoreSignalHandlerScope {
3301+
/// The signal handler.
3302+
struct sigaction m_prev_handler;
3303+
int m_signal_code;
3304+
RestoreSignalHandlerScope(int signal_code) : m_signal_code(signal_code) {
3305+
// Initialize sigaction to their default state.
3306+
std::memset(&m_prev_handler, 0, sizeof(m_prev_handler));
3307+
// Don't install a new handler, just read back the old one.
3308+
struct sigaction *new_handler = nullptr;
3309+
int signal_err = ::sigaction(m_signal_code, new_handler, &m_prev_handler);
3310+
lldbassert(signal_err == 0 && "sigaction failed to read handler");
3311+
}
3312+
~RestoreSignalHandlerScope() {
3313+
int signal_err = ::sigaction(m_signal_code, &m_prev_handler, nullptr);
3314+
lldbassert(signal_err == 0 && "sigaction failed to restore old handler");
3315+
}
3316+
};
3317+
} // namespace
3318+
32793319
void ScriptInterpreterPythonImpl::InitializePrivate() {
32803320
if (g_initialized)
32813321
return;
@@ -3311,6 +3351,25 @@ void ScriptInterpreterPythonImpl::InitializePrivate() {
33113351
"lldb.embedded_interpreter; from "
33123352
"lldb.embedded_interpreter import run_python_interpreter; "
33133353
"from lldb.embedded_interpreter import run_one_line");
3354+
3355+
#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 2) || (PY_MAJOR_VERSION > 3)
3356+
// Python will not just overwrite its internal SIGINT handler but also the
3357+
// one from the process. Backup the current SIGINT handler to prevent that
3358+
// Python deletes it.
3359+
RestoreSignalHandlerScope save_sigint(SIGINT);
3360+
3361+
// Setup a default SIGINT signal handler that works the same way as the
3362+
// normal Python REPL signal handler which raises a KeyboardInterrupt.
3363+
// Also make sure to not pollute the user's REPL with the signal module nor
3364+
// our utility function.
3365+
PyRun_SimpleString("def lldb_setup_sigint_handler():\n"
3366+
" import signal;\n"
3367+
" def signal_handler(sig, frame):\n"
3368+
" raise KeyboardInterrupt()\n"
3369+
" signal.signal(signal.SIGINT, signal_handler);\n"
3370+
"lldb_setup_sigint_handler();\n"
3371+
"del lldb_setup_sigint_handler\n");
3372+
#endif
33143373
}
33153374

33163375
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)