From 10222764a9a3b09678013d3d13b66790ea3298d9 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Wed, 2 Mar 2022 11:59:55 -0800 Subject: [PATCH] [lldb] Avoid data race in IOHandlerProcessSTDIO This patch fixes a data race in IOHandlerProcessSTDIO. The race is happens between the main thread and the event handling thread. The main thread is running the IOHandler (IOHandlerProcessSTDIO::Run()) when an event comes in that makes us pop the process IO handler which involves cancelling the IOHandler (IOHandlerProcessSTDIO::Cancel). The latter calls SetIsDone(true) which modifies m_is_done. At the same time, we have the main thread reading the variable through GetIsDone(). This patch avoids the race by using a mutex to synchronize the two threads. On the event thread, in IOHandlerProcessSTDIO ::Cancel method, we obtain the lock before changing the value of m_is_done. On the main thread, in IOHandlerProcessSTDIO::Run(), we obtain the lock before reading the value of m_is_done. Additionally, we delay calling SetIsDone until after the loop exists, to avoid a potential race between the two writes. Write of size 1 at 0x00010b66bb68 by thread T7 (mutexes: write M2862, write M718324145051843688): #0 lldb_private::IOHandler::SetIsDone(bool) IOHandler.h:90 (liblldb.15.0.0git.dylib:arm64+0x971d84) #1 IOHandlerProcessSTDIO::Cancel() Process.cpp:4382 (liblldb.15.0.0git.dylib:arm64+0x5ddfec) #2 lldb_private::Debugger::PopIOHandler(std::__1::shared_ptr const&) Debugger.cpp:1156 (liblldb.15.0.0git.dylib:arm64+0x3cb2a8) #3 lldb_private::Debugger::RemoveIOHandler(std::__1::shared_ptr const&) Debugger.cpp:1063 (liblldb.15.0.0git.dylib:arm64+0x3cbd2c) #4 lldb_private::Process::PopProcessIOHandler() Process.cpp:4487 (liblldb.15.0.0git.dylib:arm64+0x5c583c) #5 lldb_private::Debugger::HandleProcessEvent(std::__1::shared_ptr const&) Debugger.cpp:1549 (liblldb.15.0.0git.dylib:arm64+0x3ceabc) #6 lldb_private::Debugger::DefaultEventHandler() Debugger.cpp:1622 (liblldb.15.0.0git.dylib:arm64+0x3cf2c0) #7 std::__1::__function::__func, void* ()>::operator()() function.h:352 (liblldb.15.0.0git.dylib:arm64+0x3d1bd8) #8 lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(void*) HostNativeThreadBase.cpp:62 (liblldb.15.0.0git.dylib:arm64+0x4c71ac) #9 lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(void*) HostThreadMacOSX.mm:18 (liblldb.15.0.0git.dylib:arm64+0x29ef544) Previous read of size 1 at 0x00010b66bb68 by main thread: #0 lldb_private::IOHandler::GetIsDone() IOHandler.h:92 (liblldb.15.0.0git.dylib:arm64+0x971db8) #1 IOHandlerProcessSTDIO::Run() Process.cpp:4339 (liblldb.15.0.0git.dylib:arm64+0x5ddc7c) #2 lldb_private::Debugger::RunIOHandlers() Debugger.cpp:982 (liblldb.15.0.0git.dylib:arm64+0x3cb48c) #3 lldb_private::CommandInterpreter::RunCommandInterpreter(lldb_private::CommandInterpreterRunOptions&) CommandInterpreter.cpp:3298 (liblldb.15.0.0git.dylib:arm64+0x506478) #4 lldb::SBDebugger::RunCommandInterpreter(bool, bool) SBDebugger.cpp:1166 (liblldb.15.0.0git.dylib:arm64+0x53604) #5 Driver::MainLoop() Driver.cpp:634 (lldb:arm64+0x100006294) #6 main Driver.cpp:853 (lldb:arm64+0x100007344) Differential revision: https://reviews.llvm.org/D120762 --- lldb/source/Target/Process.cpp | 56 +++++++++++-------- lldb/test/API/iohandler/stdio/Makefile | 3 + .../stdio/TestIOHandlerProcessSTDIO.py | 30 ++++++++++ lldb/test/API/iohandler/stdio/main.cpp | 8 +++ 4 files changed, 74 insertions(+), 23 deletions(-) create mode 100644 lldb/test/API/iohandler/stdio/Makefile create mode 100644 lldb/test/API/iohandler/stdio/TestIOHandlerProcessSTDIO.py create mode 100644 lldb/test/API/iohandler/stdio/main.cpp diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 70717b332b7555..3cfda7129e6248 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -4310,6 +4310,12 @@ class IOHandlerProcessSTDIO : public IOHandler { ~IOHandlerProcessSTDIO() override = default; + void SetIsRunning(bool running) { + std::lock_guard guard(m_mutex); + SetIsDone(!running); + m_is_running = running; + } + // Each IOHandler gets to run until it is done. It should read data from the // "in" and place output into "out" and "err and return when done. void Run() override { @@ -4329,49 +4335,52 @@ class IOHandlerProcessSTDIO : public IOHandler { // FD_ZERO, FD_SET are not supported on windows #ifndef _WIN32 const int pipe_read_fd = m_pipe.GetReadFileDescriptor(); - m_is_running = true; - while (!GetIsDone()) { + SetIsRunning(true); + while (true) { + { + std::lock_guard guard(m_mutex); + if (GetIsDone()) + break; + } + SelectHelper select_helper; select_helper.FDSetRead(read_fd); select_helper.FDSetRead(pipe_read_fd); Status error = select_helper.Select(); - if (error.Fail()) { - SetIsDone(true); - } else { - char ch = 0; - size_t n; - if (select_helper.FDIsSetRead(read_fd)) { - n = 1; - if (m_read_file.Read(&ch, n).Success() && n == 1) { - if (m_write_file.Write(&ch, n).Fail() || n != 1) - SetIsDone(true); - } else - SetIsDone(true); - } + if (error.Fail()) + break; + + char ch = 0; + size_t n; + if (select_helper.FDIsSetRead(read_fd)) { + n = 1; + if (m_read_file.Read(&ch, n).Success() && n == 1) { + if (m_write_file.Write(&ch, n).Fail() || n != 1) + break; + } else + break; + if (select_helper.FDIsSetRead(pipe_read_fd)) { size_t bytes_read; // Consume the interrupt byte Status error = m_pipe.Read(&ch, 1, bytes_read); if (error.Success()) { - switch (ch) { - case 'q': - SetIsDone(true); + if (ch == 'q') break; - case 'i': + if (ch == 'i') if (StateIsRunningState(m_process->GetState())) m_process->SendAsyncInterrupt(); - break; - } } } } } - m_is_running = false; + SetIsRunning(false); #endif } void Cancel() override { + std::lock_guard guard(m_mutex); SetIsDone(true); // Only write to our pipe to cancel if we are in // IOHandlerProcessSTDIO::Run(). We can end up with a python command that @@ -4428,7 +4437,8 @@ class IOHandlerProcessSTDIO : public IOHandler { NativeFile m_write_file; // Write to this file (usually the primary pty for // getting io to debuggee) Pipe m_pipe; - std::atomic m_is_running{false}; + std::mutex m_mutex; + bool m_is_running = false; }; void Process::SetSTDIOFileDescriptor(int fd) { diff --git a/lldb/test/API/iohandler/stdio/Makefile b/lldb/test/API/iohandler/stdio/Makefile new file mode 100644 index 00000000000000..99998b20bcb050 --- /dev/null +++ b/lldb/test/API/iohandler/stdio/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp + +include Makefile.rules diff --git a/lldb/test/API/iohandler/stdio/TestIOHandlerProcessSTDIO.py b/lldb/test/API/iohandler/stdio/TestIOHandlerProcessSTDIO.py new file mode 100644 index 00000000000000..5a77c0cb2b7c62 --- /dev/null +++ b/lldb/test/API/iohandler/stdio/TestIOHandlerProcessSTDIO.py @@ -0,0 +1,30 @@ +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test.lldbpexpect import PExpectTest + +class TestIOHandlerProcessSTDIO(PExpectTest): + + mydir = TestBase.compute_mydir(__file__) + NO_DEBUG_INFO_TESTCASE = True + + # PExpect uses many timeouts internally and doesn't play well + # under ASAN on a loaded machine.. + @skipIfAsan + def test(self): + self.build() + self.launch(executable=self.getBuildArtifact("a.out")) + self.child.sendline("run") + + self.child.send("foo\n") + self.child.expect_exact("stdout: foo") + + self.child.send("bar\n") + self.child.expect_exact("stdout: bar") + + self.child.send("baz\n") + self.child.expect_exact("stdout: baz") + + self.child.sendcontrol('d') + self.expect_prompt() + self.quit() diff --git a/lldb/test/API/iohandler/stdio/main.cpp b/lldb/test/API/iohandler/stdio/main.cpp new file mode 100644 index 00000000000000..d6da7d58165a3b --- /dev/null +++ b/lldb/test/API/iohandler/stdio/main.cpp @@ -0,0 +1,8 @@ +#include +#include + +int main(int argc, char **argv) { + for (std::string line; std::getline(std::cin, line);) + std::cout << "stdout: " << line << '\n'; + return 0; +}