augusto2112 created this revision.
augusto2112 added reviewers: JDevlieghere, bulbazord.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
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<std::__1::ratio<1l, 1000000l>>, 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<std::__1::ratio<1l, 1000000l>>, bool)
GDBRemoteCommunication.cpp:243 (liblldb.18.0.0git.dylib:arm64+0x8b9904)
(BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00)
Fix this by adding a mutex to PipePosix.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D157654
Files:
lldb/include/lldb/Host/posix/PipePosix.h
lldb/source/Host/posix/PipePosix.cpp
Index: lldb/source/Host/posix/PipePosix.cpp
===================================================================
--- lldb/source/Host/posix/PipePosix.cpp
+++ lldb/source/Host/posix/PipePosix.cpp
@@ -66,15 +66,17 @@
PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) {
PipeBase::operator=(std::move(pipe_posix));
- m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor();
- m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor();
+ std::lock_guard<std::mutex> guard(m_lock);
+ 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::lock_guard<std::mutex> guard(m_lock);
+ if (CanReadUnlocked() || CanWriteUnlocked())
return Status(EINVAL, eErrorTypePOSIX);
Status error;
@@ -87,7 +89,7 @@
if (!child_processes_inherit) {
if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
error.SetErrorToErrno();
- Close();
+ CloseUnlocked();
return error;
}
}
@@ -103,7 +105,8 @@
}
Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
- if (CanRead() || CanWrite())
+ std::lock_guard<std::mutex> guard(m_lock);
+ if (CanReadUnlocked() || CanWriteUnlocked())
return Status("Pipe is already opened");
Status error;
@@ -140,7 +143,8 @@
Status PipePosix::OpenAsReader(llvm::StringRef name,
bool child_process_inherit) {
- if (CanRead() || CanWrite())
+ std::lock_guard<std::mutex> guard(m_lock);
+ if (CanReadUnlocked() || CanWriteUnlocked())
return Status("Pipe is already opened");
int flags = O_RDONLY | O_NONBLOCK;
@@ -161,7 +165,8 @@
PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name,
bool child_process_inherit,
const std::chrono::microseconds &timeout) {
- if (CanRead() || CanWrite())
+ std::lock_guard<std::mutex> guard(m_lock);
+ if (CanReadUnlocked() || CanWriteUnlocked())
return Status("Pipe is already opened");
int flags = O_WRONLY | O_NONBLOCK;
@@ -171,7 +176,7 @@
using namespace std::chrono;
const auto finish_time = Now() + timeout;
- while (!CanWrite()) {
+ while (!CanWriteUnlocked()) {
if (timeout != microseconds::zero()) {
const auto dur = duration_cast<microseconds>(finish_time - Now()).count();
if (dur <= 0)
@@ -196,25 +201,54 @@
return Status();
}
-int PipePosix::GetReadFileDescriptor() const { return m_fds[READ]; }
+int PipePosix::GetReadFileDescriptor() const {
+ std::lock_guard<std::mutex> guard(m_lock);
+ return GetReadFileDescriptorUnlocked();
+}
+
+int PipePosix::GetReadFileDescriptorUnlocked() const {
+ return m_fds[READ];
+}
+
+int PipePosix::GetWriteFileDescriptor() const {
+ std::lock_guard<std::mutex> guard(m_lock);
+ return GetWriteFileDescriptorUnlocked();
+}
-int PipePosix::GetWriteFileDescriptor() const { return m_fds[WRITE]; }
+int PipePosix::GetWriteFileDescriptorUnlocked() const {
+ return m_fds[WRITE];
+}
int PipePosix::ReleaseReadFileDescriptor() {
+ std::lock_guard<std::mutex> guard(m_lock);
+ return ReleaseReadFileDescriptorUnlocked();
+}
+
+int PipePosix::ReleaseReadFileDescriptorUnlocked() {
const int fd = m_fds[READ];
m_fds[READ] = PipePosix::kInvalidDescriptor;
return fd;
}
int PipePosix::ReleaseWriteFileDescriptor() {
+ std::lock_guard<std::mutex> guard(m_lock);
+ return ReleaseWriteFileDescriptorUnlocked();
+}
+
+int PipePosix::ReleaseWriteFileDescriptorUnlocked() {
const int fd = m_fds[WRITE];
m_fds[WRITE] = PipePosix::kInvalidDescriptor;
return fd;
}
void PipePosix::Close() {
- CloseReadFileDescriptor();
- CloseWriteFileDescriptor();
+ std::lock_guard<std::mutex> guard(m_lock);
+ CloseUnlocked();
+}
+
+void PipePosix::CloseUnlocked() {
+ CloseReadFileDescriptorUnlocked();
+ CloseWriteFileDescriptorUnlocked();
}
Status PipePosix::Delete(llvm::StringRef name) {
@@ -222,22 +256,41 @@
}
bool PipePosix::CanRead() const {
+ std::lock_guard<std::mutex> guard(m_lock);
+ return CanReadUnlocked();
+}
+
+bool PipePosix::CanReadUnlocked() const {
return m_fds[READ] != PipePosix::kInvalidDescriptor;
}
bool PipePosix::CanWrite() const {
+ std::lock_guard<std::mutex> guard(m_lock);
+ return CanWriteUnlocked();
+}
+
+bool PipePosix::CanWriteUnlocked() const {
return m_fds[WRITE] != PipePosix::kInvalidDescriptor;
}
void PipePosix::CloseReadFileDescriptor() {
- if (CanRead()) {
+ std::lock_guard<std::mutex> guard(m_lock);
+ CloseReadFileDescriptorUnlocked();
+}
+void PipePosix::CloseReadFileDescriptorUnlocked() {
+ if (CanReadUnlocked()) {
close(m_fds[READ]);
m_fds[READ] = PipePosix::kInvalidDescriptor;
}
}
void PipePosix::CloseWriteFileDescriptor() {
- if (CanWrite()) {
+ std::lock_guard<std::mutex> guard(m_lock);
+ CloseWriteFileDescriptorUnlocked();
+}
+
+void PipePosix::CloseWriteFileDescriptorUnlocked() {
+ if (CanWriteUnlocked()) {
close(m_fds[WRITE]);
m_fds[WRITE] = PipePosix::kInvalidDescriptor;
}
@@ -246,11 +299,12 @@
Status PipePosix::ReadWithTimeout(void *buf, size_t size,
const std::chrono::microseconds &timeout,
size_t &bytes_read) {
+ std::lock_guard<std::mutex> guard(m_lock);
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 +332,12 @@
}
Status PipePosix::Write(const void *buf, size_t size, size_t &bytes_written) {
+ std::lock_guard<std::mutex> guard(m_lock);
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);
Index: lldb/include/lldb/Host/posix/PipePosix.h
===================================================================
--- lldb/include/lldb/Host/posix/PipePosix.h
+++ lldb/include/lldb/Host/posix/PipePosix.h
@@ -9,8 +9,8 @@
#ifndef LLDB_HOST_POSIX_PIPEPOSIX_H
#define LLDB_HOST_POSIX_PIPEPOSIX_H
#if defined(__cplusplus)
-
#include "lldb/Host/PipeBase.h"
+#include <mutex>
namespace lldb_private {
@@ -71,7 +71,21 @@
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];
+
+ /// Lock for m_fds;
+ mutable std::mutex m_lock;
};
} // namespace lldb_private
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
