[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<lldb_private::IOHandler> const&) Debugger.cpp:1156 (liblldb.15.0.0git.dylib:arm64+0x3cb2a8)
    #3 lldb_private::Debugger::RemoveIOHandler(std::__1::shared_ptr<lldb_private::IOHandler> 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<lldb_private::Event> 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<lldb_private::Debugger::StartEventHandlerThread()::$_2, std::__1::allocator<lldb_private::Debugger::StartEventHandlerThread()::$_2>, 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

Author: JDevlieghere

lldb/source/Target/Process.cpp

@@ -4310,6 +4310,12 @@ class IOHandlerProcessSTDIO : public IOHandler {
 
   ~IOHandlerProcessSTDIO() override = default;
 
+  void SetIsRunning(bool running) {
+    std::lock_guard<std::mutex> 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<std::mutex> 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<std::mutex> 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<bool> m_is_running{false};
+  std::mutex m_mutex;
+  bool m_is_running = false;
 };
 
 void Process::SetSTDIOFileDescriptor(int fd) {

lldb/test/API/iohandler/stdio/Makefile

@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

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

lldb/test/API/iohandler/stdio/main.cpp

@@ -0,0 +1,8 @@
+#include <iostream>
+#include <string>
+
+int main(int argc, char **argv) {
+  for (std::string line; std::getline(std::cin, line);)
+    std::cout << "stdout: " << line << '\n';
+  return 0;
+}