Skip to content

Commit

Permalink
[lldb] Fix data race in ConnectionFileDescriptor
Browse files Browse the repository at this point in the history
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
    rust-lang#2 Communication::Disconnect(lldb_private::Status*) Communication.cpp:61
    rust-lang#3 process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1164
    rust-lang#4 Process::SetExitStatus(int, char const*) Process.cpp:1097
    rust-lang#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
    rust-lang#2 Communication::IsConnected() const Communication.cpp:79
    rust-lang#3 process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) GDBRemoteCommunication.cpp:256
    rust-lang#4 process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...l) GDBRemoteCommunication.cpp:244
    rust-lang#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
  • Loading branch information
JDevlieghere committed Aug 8, 2023
1 parent 0664db5 commit 0bdbe7b
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down

0 comments on commit 0bdbe7b

Please sign in to comment.