From 22413641e236ebee7485ce8bc5880e8d1e41573f Mon Sep 17 00:00:00 2001 From: Augusto Noronha Date: Thu, 10 Aug 2023 14:25:31 -0700 Subject: [PATCH] [lldb] Fix data race in PipePosix Thread sanitizer reports the following data race: ``` WARNING: ThreadSanitizer: data race (pid=43201) Write of size 4 at 0x00010520c474 by thread T1 (mutexes: write M0, write M1): #0 lldb_private::PipePosix::CloseWriteFileDescriptor() PipePosix.cpp:242 (liblldb.18.0.0git.dylib:arm64+0x414700) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #1 lldb_private::PipePosix::Close() PipePosix.cpp:217 (liblldb.18.0.0git.dylib:arm64+0x4144e8) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #2 lldb_private::ConnectionFileDescriptor::Disconnect(lldb_private::Status*) ConnectionFileDescriptorPosix.cpp:239 (liblldb.18.0.0git.dylib:arm64+0x40a620) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #3 lldb_private::Communication::Disconnect(lldb_private::Status*) Communication.cpp:61 (liblldb.18.0.0git.dylib:arm64+0x2a9318) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #4 lldb_private::process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1167 (liblldb.18.0.0git.dylib:arm64+0x8ed984) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) Previous read of size 4 at 0x00010520c474 by main thread (mutexes: write M2, write M3): #0 lldb_private::PipePosix::CanWrite() const PipePosix.cpp:229 (liblldb.18.0.0git.dylib:arm64+0x4145e4) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #1 lldb_private::ConnectionFileDescriptor::Disconnect(lldb_private::Status*) ConnectionFileDescriptorPosix.cpp:212 (liblldb.18.0.0git.dylib:arm64+0x40a4a8) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #2 lldb_private::Communication::Disconnect(lldb_private::Status*) Communication.cpp:61 (liblldb.18.0.0git.dylib:arm64+0x2a9318) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #3 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote&, lldb_private::Timeout>, bool) GDBRemoteCommunication.cpp:373 (liblldb.18.0.0git.dylib:arm64+0x8b9c48) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #4 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote&, lldb_private::Timeout>, bool) GDBRemoteCommunication.cpp:243 (liblldb.18.0.0git.dylib:arm64+0x8b9904) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) ``` Fix this by adding a mutex to PipePosix. Differential Revision: https://reviews.llvm.org/D157654 --- lldb/include/lldb/Host/posix/PipePosix.h | 17 ++++- lldb/source/Host/posix/PipePosix.cpp | 94 +++++++++++++++++++----- 2 files changed, 92 insertions(+), 19 deletions(-) diff --git a/lldb/include/lldb/Host/posix/PipePosix.h b/lldb/include/lldb/Host/posix/PipePosix.h index 3bdb7431d8c770..ec4c752a24e94c 100644 --- a/lldb/include/lldb/Host/posix/PipePosix.h +++ b/lldb/include/lldb/Host/posix/PipePosix.h @@ -8,8 +8,8 @@ #ifndef LLDB_HOST_POSIX_PIPEPOSIX_H #define LLDB_HOST_POSIX_PIPEPOSIX_H - #include "lldb/Host/PipeBase.h" +#include namespace lldb_private { @@ -70,7 +70,22 @@ class PipePosix : public PipeBase { size_t &bytes_read) override; private: + bool CanReadUnlocked() const; + bool CanWriteUnlocked() const; + + int GetReadFileDescriptorUnlocked() const; + int GetWriteFileDescriptorUnlocked() const; + int ReleaseReadFileDescriptorUnlocked(); + int ReleaseWriteFileDescriptorUnlocked(); + void CloseReadFileDescriptorUnlocked(); + void CloseWriteFileDescriptorUnlocked(); + void CloseUnlocked(); + int m_fds[2]; + + /// Mutexes for m_fds; + mutable std::mutex m_read_mutex; + mutable std::mutex m_write_mutex; }; } // namespace lldb_private diff --git a/lldb/source/Host/posix/PipePosix.cpp b/lldb/source/Host/posix/PipePosix.cpp index 5e4e8618fa4f14..0050731acb3fcb 100644 --- a/lldb/source/Host/posix/PipePosix.cpp +++ b/lldb/source/Host/posix/PipePosix.cpp @@ -65,16 +65,20 @@ PipePosix::PipePosix(PipePosix &&pipe_posix) pipe_posix.ReleaseWriteFileDescriptor()} {} PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) { + std::scoped_lock guard(m_read_mutex, m_write_mutex, pipe_posix.m_read_mutex, + pipe_posix.m_write_mutex); + PipeBase::operator=(std::move(pipe_posix)); - m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor(); - m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor(); + m_fds[READ] = pipe_posix.ReleaseReadFileDescriptorUnlocked(); + m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptorUnlocked(); return *this; } PipePosix::~PipePosix() { Close(); } Status PipePosix::CreateNew(bool child_processes_inherit) { - if (CanRead() || CanWrite()) + std::scoped_lock guard(m_read_mutex, m_write_mutex); + if (CanReadUnlocked() || CanWriteUnlocked()) return Status(EINVAL, eErrorTypePOSIX); Status error; @@ -87,7 +91,7 @@ Status PipePosix::CreateNew(bool child_processes_inherit) { if (!child_processes_inherit) { if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) { error.SetErrorToErrno(); - Close(); + CloseUnlocked(); return error; } } @@ -103,7 +107,8 @@ Status PipePosix::CreateNew(bool child_processes_inherit) { } Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) { - if (CanRead() || CanWrite()) + std::scoped_lock (m_read_mutex, m_write_mutex); + if (CanReadUnlocked() || CanWriteUnlocked()) return Status("Pipe is already opened"); Status error; @@ -140,7 +145,9 @@ Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix, Status PipePosix::OpenAsReader(llvm::StringRef name, bool child_process_inherit) { - if (CanRead() || CanWrite()) + std::scoped_lock (m_read_mutex, m_write_mutex); + + if (CanReadUnlocked() || CanWriteUnlocked()) return Status("Pipe is already opened"); int flags = O_RDONLY | O_NONBLOCK; @@ -161,7 +168,8 @@ Status PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name, bool child_process_inherit, const std::chrono::microseconds &timeout) { - if (CanRead() || CanWrite()) + std::lock_guard guard(m_write_mutex); + if (CanReadUnlocked() || CanWriteUnlocked()) return Status("Pipe is already opened"); int flags = O_WRONLY | O_NONBLOCK; @@ -171,7 +179,7 @@ PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name, using namespace std::chrono; const auto finish_time = Now() + timeout; - while (!CanWrite()) { + while (!CanWriteUnlocked()) { if (timeout != microseconds::zero()) { const auto dur = duration_cast(finish_time - Now()).count(); if (dur <= 0) @@ -196,25 +204,54 @@ PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name, return Status(); } -int PipePosix::GetReadFileDescriptor() const { return m_fds[READ]; } +int PipePosix::GetReadFileDescriptor() const { + std::lock_guard guard(m_read_mutex); + return GetReadFileDescriptorUnlocked(); +} + +int PipePosix::GetReadFileDescriptorUnlocked() const { + return m_fds[READ]; +} -int PipePosix::GetWriteFileDescriptor() const { return m_fds[WRITE]; } +int PipePosix::GetWriteFileDescriptor() const { + std::lock_guard guard(m_write_mutex); + return GetWriteFileDescriptorUnlocked(); +} + +int PipePosix::GetWriteFileDescriptorUnlocked() const { + return m_fds[WRITE]; +} int PipePosix::ReleaseReadFileDescriptor() { + std::lock_guard guard(m_read_mutex); + return ReleaseReadFileDescriptorUnlocked(); +} + +int PipePosix::ReleaseReadFileDescriptorUnlocked() { const int fd = m_fds[READ]; m_fds[READ] = PipePosix::kInvalidDescriptor; return fd; } int PipePosix::ReleaseWriteFileDescriptor() { + std::lock_guard guard(m_write_mutex); + return ReleaseWriteFileDescriptorUnlocked(); +} + +int PipePosix::ReleaseWriteFileDescriptorUnlocked() { const int fd = m_fds[WRITE]; m_fds[WRITE] = PipePosix::kInvalidDescriptor; return fd; } void PipePosix::Close() { - CloseReadFileDescriptor(); - CloseWriteFileDescriptor(); + std::scoped_lock guard(m_read_mutex, m_write_mutex); + CloseUnlocked(); +} + +void PipePosix::CloseUnlocked() { + CloseReadFileDescriptorUnlocked(); + CloseWriteFileDescriptorUnlocked(); } Status PipePosix::Delete(llvm::StringRef name) { @@ -222,22 +259,41 @@ Status PipePosix::Delete(llvm::StringRef name) { } bool PipePosix::CanRead() const { + std::lock_guard guard(m_read_mutex); + return CanReadUnlocked(); +} + +bool PipePosix::CanReadUnlocked() const { return m_fds[READ] != PipePosix::kInvalidDescriptor; } bool PipePosix::CanWrite() const { + std::lock_guard guard(m_write_mutex); + return CanWriteUnlocked(); +} + +bool PipePosix::CanWriteUnlocked() const { return m_fds[WRITE] != PipePosix::kInvalidDescriptor; } void PipePosix::CloseReadFileDescriptor() { - if (CanRead()) { + std::lock_guard guard(m_read_mutex); + CloseReadFileDescriptorUnlocked(); +} +void PipePosix::CloseReadFileDescriptorUnlocked() { + if (CanReadUnlocked()) { close(m_fds[READ]); m_fds[READ] = PipePosix::kInvalidDescriptor; } } void PipePosix::CloseWriteFileDescriptor() { - if (CanWrite()) { + std::lock_guard guard(m_write_mutex); + CloseWriteFileDescriptorUnlocked(); +} + +void PipePosix::CloseWriteFileDescriptorUnlocked() { + if (CanWriteUnlocked()) { close(m_fds[WRITE]); m_fds[WRITE] = PipePosix::kInvalidDescriptor; } @@ -246,11 +302,12 @@ void PipePosix::CloseWriteFileDescriptor() { Status PipePosix::ReadWithTimeout(void *buf, size_t size, const std::chrono::microseconds &timeout, size_t &bytes_read) { + std::lock_guard guard(m_read_mutex); bytes_read = 0; - if (!CanRead()) + if (!CanReadUnlocked()) return Status(EINVAL, eErrorTypePOSIX); - const int fd = GetReadFileDescriptor(); + const int fd = GetReadFileDescriptorUnlocked(); SelectHelper select_helper; select_helper.SetTimeout(timeout); @@ -278,11 +335,12 @@ Status PipePosix::ReadWithTimeout(void *buf, size_t size, } Status PipePosix::Write(const void *buf, size_t size, size_t &bytes_written) { + std::lock_guard guard(m_write_mutex); bytes_written = 0; - if (!CanWrite()) + if (!CanWriteUnlocked()) return Status(EINVAL, eErrorTypePOSIX); - const int fd = GetWriteFileDescriptor(); + const int fd = GetWriteFileDescriptorUnlocked(); SelectHelper select_helper; select_helper.SetTimeout(std::chrono::seconds(0)); select_helper.FDSetWrite(fd);