From 049ae93097c0c6f7cef6294f16c7f6de9138cd6d Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Thu, 13 Jan 2022 11:26:20 -0800 Subject: [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. Differential revision: https://reviews.llvm.org/D104886 --- .../Python/ScriptInterpreterPython.cpp | 68 ++++++++++++++++++++++ 1 file changed, 68 insertions(+) (limited to 'lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp') diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp index 37b2868..ed4ad27 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp @@ -70,6 +70,14 @@ extern "C" void init_lldb(void); #define LLDBSwigPyInit init_lldb #endif +#if defined(_WIN32) +// Don't mess with the signal handlers on Windows. +#define LLDB_USE_PYTHON_SET_INTERRUPT 0 +#else +// PyErr_SetInterrupt was introduced in 3.2. +#define LLDB_USE_PYTHON_SET_INTERRUPT \ + (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 2) || (PY_MAJOR_VERSION > 3) +#endif static ScriptInterpreterPythonImpl *GetPythonInterpreter(Debugger &debugger) { ScriptInterpreter *script_interpreter = @@ -920,6 +928,22 @@ void ScriptInterpreterPythonImpl::ExecuteInterpreterLoop() { } bool ScriptInterpreterPythonImpl::Interrupt() { +#if LLDB_USE_PYTHON_SET_INTERRUPT + // If the interpreter isn't evaluating any Python at the moment then return + // false to signal that this function didn't handle the interrupt and the + // next component should try handling it. + if (!IsExecutingPython()) + return false; + + // Tell Python that it should pretend to have received a SIGINT. + PyErr_SetInterrupt(); + // PyErr_SetInterrupt has no way to return an error so we can only pretend the + // signal got successfully handled and return true. + // Python 3.10 introduces PyErr_SetInterruptEx that could return an error, but + // the error handling is limited to checking the arguments which would be + // just our (hardcoded) input signal code SIGINT, so that's not useful at all. + return true; +#else Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_SCRIPT)); if (IsExecutingPython()) { @@ -941,6 +965,7 @@ bool ScriptInterpreterPythonImpl::Interrupt() { "ScriptInterpreterPythonImpl::Interrupt() python code not running, " "can't interrupt"); return false; +#endif } bool ScriptInterpreterPythonImpl::ExecuteOneLineWithReturn( @@ -3144,6 +3169,30 @@ ScriptInterpreterPythonImpl::AcquireInterpreterLock() { return py_lock; } +#if LLDB_USE_PYTHON_SET_INTERRUPT +namespace { +/// Saves the current signal handler for the specified signal and restores +/// it at the end of the current scope. +struct RestoreSignalHandlerScope { + /// The signal handler. + struct sigaction m_prev_handler; + int m_signal_code; + RestoreSignalHandlerScope(int signal_code) : m_signal_code(signal_code) { + // Initialize sigaction to their default state. + std::memset(&m_prev_handler, 0, sizeof(m_prev_handler)); + // Don't install a new handler, just read back the old one. + struct sigaction *new_handler = nullptr; + int signal_err = ::sigaction(m_signal_code, new_handler, &m_prev_handler); + lldbassert(signal_err == 0 && "sigaction failed to read handler"); + } + ~RestoreSignalHandlerScope() { + int signal_err = ::sigaction(m_signal_code, &m_prev_handler, nullptr); + lldbassert(signal_err == 0 && "sigaction failed to restore old handler"); + } +}; +} // namespace +#endif + void ScriptInterpreterPythonImpl::InitializePrivate() { if (g_initialized) return; @@ -3179,6 +3228,25 @@ void ScriptInterpreterPythonImpl::InitializePrivate() { "lldb.embedded_interpreter; from " "lldb.embedded_interpreter import run_python_interpreter; " "from lldb.embedded_interpreter import run_one_line"); + +#if LLDB_USE_PYTHON_SET_INTERRUPT + // Python will not just overwrite its internal SIGINT handler but also the + // one from the process. Backup the current SIGINT handler to prevent that + // Python deletes it. + RestoreSignalHandlerScope save_sigint(SIGINT); + + // Setup a default SIGINT signal handler that works the same way as the + // normal Python REPL signal handler which raises a KeyboardInterrupt. + // Also make sure to not pollute the user's REPL with the signal module nor + // our utility function. + PyRun_SimpleString("def lldb_setup_sigint_handler():\n" + " import signal;\n" + " def signal_handler(sig, frame):\n" + " raise KeyboardInterrupt()\n" + " signal.signal(signal.SIGINT, signal_handler);\n" + "lldb_setup_sigint_handler();\n" + "del lldb_setup_sigint_handler\n"); +#endif } void ScriptInterpreterPythonImpl::AddToSysPath(AddLocation location, -- cgit v1.1