Skip to content

Commit

Permalink
[lldb] Fix data race in PipePosix
Browse files Browse the repository at this point in the history 
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)
    rust-lang#2 lldb_private::ConnectionFileDescriptor::Disconnect(lldb_private::Status*) ConnectionFileDescriptorPosix.cpp:239 (liblldb.18.0.0git.dylib:arm64+0x40a620) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00)
    rust-lang#3 lldb_private::Communication::Disconnect(lldb_private::Status*) Communication.cpp:61 (liblldb.18.0.0git.dylib:arm64+0x2a9318) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00)
    rust-lang#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)
    rust-lang#2 lldb_private::Communication::Disconnect(lldb_private::Status*) Communication.cpp:61 (liblldb.18.0.0git.dylib:arm64+0x2a9318) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00)
    rust-lang#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)
    rust-lang#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.

Differential Revision: https://reviews.llvm.org/D157654
  • Loading branch information
@augusto2112
augusto2112 committed Aug 15, 2023
1 parent 7897a94 commit 2241364
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 19 deletions.
17 changes: 16 additions & 1 deletion lldb/include/lldb/Host/posix/PipePosix.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

#ifndef LLDB_HOST_POSIX_PIPEPOSIX_H
#define LLDB_HOST_POSIX_PIPEPOSIX_H

#include "lldb/Host/PipeBase.h"
#include <mutex>

namespace lldb_private {

Expand Down Expand Up @@ -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
Expand Down
94 changes: 76 additions & 18 deletions lldb/source/Host/posix/PipePosix.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}
}
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<microseconds>(finish_time - Now()).count();
if (dur <= 0)
Expand All @@ -196,48 +204,96 @@ 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) {
return llvm::sys::fs::remove(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;
}
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 2241364

Please sign in to comment.