[Lldb-commits] [lldb] 0bdbe7b - [lldb] Fix data race in ConnectionFileDescriptor

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-08-07T18:37:52-07:00
New Revision: 0bdbe7bd7f1589817495a60cc8422df49575b17b

URL: https://github.com/llvm/llvm-project/commit/0bdbe7bd7f1589817495a60cc8422df49575b17b
DIFF: https://github.com/llvm/llvm-project/commit/0bdbe7bd7f1589817495a60cc8422df49575b17b.diff

LOG: [lldb] Fix data race in ConnectionFileDescriptor

TSan reports the following data race:

  Write of size 4 at 0x000109e0b160 by thread T2 (mutexes: write M0, write M1):
    #0 NativeFile::Close() File.cpp:329
    #1 ConnectionFileDescriptor::Disconnect(lldb_private::Status*) ConnectionFileDescriptorPosix.cpp:232
    #2 Communication::Disconnect(lldb_private::Status*) Communication.cpp:61
    #3 process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1164
    #4 Process::SetExitStatus(int, char const*) Process.cpp:1097
    #5 process_gdb_remote::ProcessGDBRemote::MonitorDebugserverProcess(...) ProcessGDBRemote.cpp:3387

  Previous read of size 4 at 0x000109e0b160 by main thread (mutexes: write M2):
    #0 NativeFile::IsValid() const File.h:393
    #1 ConnectionFileDescriptor::IsConnected() const ConnectionFileDescriptorPosix.cpp:121
    #2 Communication::IsConnected() const Communication.cpp:79
    #3 process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) GDBRemoteCommunication.cpp:256
    #4 process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...l) GDBRemoteCommunication.cpp:244
    #5 process_gdb_remote::GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(llvm::StringRef, StringExtractorGDBRemote&) GDBRemoteClientBase.cpp:246

The problem is that in WaitForPacketNoLock's run loop, it checks that
the connection is still connected. This races with the
ConnectionFileDescriptor disconnecting. Most (but not all) access to the
IOObject in ConnectionFileDescriptorPosix is already gated by the mutex.
This patch just protects IsConnected in the same way.

Differential revision: https://reviews.llvm.org/D157347

Added: 
    

Modified: 
    lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
    lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
index 35773d5907e913..8c8424ed48154b 100644
--- a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -131,7 +131,7 @@ class ConnectionFileDescriptor : public Connection {
   lldb::IOObjectSP m_io_sp;
 
   Pipe m_pipe;
-  std::recursive_mutex m_mutex;
+  mutable std::recursive_mutex m_mutex;
   std::atomic<bool> m_shutting_down; // This marks that we are shutting down so
                                      // if we get woken up from
   // BytesAvailable to disconnect, we won't try to read again.

diff  --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 6a367a3307e543..9bb0268b2a704c 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -118,6 +118,7 @@ void ConnectionFileDescriptor::CloseCommandPipe() {
 }
 
 bool ConnectionFileDescriptor::IsConnected() const {
+  std::lock_guard<std::recursive_mutex> guard(m_mutex);
   return m_io_sp && m_io_sp->IsValid();
 }