[lldb] Fix data race in PipePosix

[augusto2112]

Summary

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.

[bulbazord]

lldb/source/Host/posix/PipePosix.cpp

72–73

I think this lock needs to be at the beginning. Scenario:

Thread 1 calls the move assign operator and performs the base move assign. Gets unscheduled before acquiring lock.
Thread 2 calls the move assign operator and performs the base move assign. Acquires the lock, fills in m_fds, and returns.
Thread 1 is scheduled again, grabs the lock, and fills in m_fds.
this will have its PipeBase members filled in by the Thread 2 call while m_fds will be filled in by the Thread 1 call. Granted, one thread may be surprised that suddenly the thing didn't get moved in as expected, but at least this will be in a consistent state.

I think you also need to lock the mutex of pipe_posix at the same time. Imagine Thread 1 is trying to access something from pipe_posix while Thread 2 is trying to move it into another PipePosix object. (Not sure how likely this scenario is but I'd rather be safe than sorry).

[augusto2112]

lldb/source/Host/posix/PipePosix.cpp

72–73

I think you also need to lock the mutex of pipe_posix at the same time. Imagine Thread 1 is trying to access something from pipe_posix while Thread 2 is trying to move it into another PipePosix object. (Not sure how likely this scenario is but I'd rather be safe than sorry).

Yeah I thought about this too, since pipe_posix is an rvalue reference I was thinking this shouldn't be necessary, but you're right, I'll lock it anyway.

[augusto2112]

Lock earlier in operator=

[JDevlieghere]

Shouldn't we have two murexes, one for the read FD and one for the write FD? The current approach is overly strict: it preventing concurrent reads and writes because ReadWithTimeout and Write are locking the same mutex.

[augusto2112]

Update to two shared mutexes

[augusto2112]

@JDevlieghere I updated this to two shared mutexes because I'm assuming you can have more than one concurrent read and more than one concurrent write. If this is wrong I can go back to two regular mutexes.

[JDevlieghere]

In D157654#4581578, @augusto2112 wrote:

@JDevlieghere I updated this to two shared mutexes because I'm assuming you can have more than one concurrent read and more than one concurrent write. If this is wrong I can go back to two regular mutexes.

According to POSIX [1] that would be undefined:

The behavior of multiple concurrent reads on the same pipe, FIFO, or terminal device is unspecified.

A regular mutex should be fine.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html

[augusto2112]

Changed to regular mutex

[JDevlieghere]

LGTM!

lldb/include/lldb/Host/posix/PipePosix.h

 //===-- PipePosix.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_HOST_POSIX_PIPEPOSIX_H
 #define LLDB_HOST_POSIX_PIPEPOSIX_H
-
 #include "lldb/Host/PipeBase.h"
+#include <mutex>
 
 namespace lldb_private {
 
 /// \class PipePosix PipePosix.h "lldb/Host/posix/PipePosix.h"
 /// A posix-based implementation of Pipe, a class that abtracts
 ///        unix style pipes.
 ///
 /// A class that abstracts the LLDB core from host pipe functionality.
public:
   Status Delete(llvm::StringRef name) override;
 
   Status Write(const void *buf, size_t size, size_t &bytes_written) override;
   Status ReadWithTimeout(void *buf, size_t size,
                          const std::chrono::microseconds &timeout,
                          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
 
 #endif // LLDB_HOST_POSIX_PIPEPOSIX_H

lldb/source/Host/posix/PipePosix.cpp

PipePosix::PipePosix(lldb::pipe_t read, lldb::pipe_t write)
     : m_fds{read, write} {}
 
 PipePosix::PipePosix(PipePosix &&pipe_posix)
     : PipeBase{std::move(pipe_posix)},
       m_fds{pipe_posix.ReleaseReadFileDescriptor(),
             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;
 #if PIPE2_SUPPORTED
   if (::pipe2(m_fds, (child_processes_inherit) ? 0 : O_CLOEXEC) == 0)
     return error;
 #else
   if (::pipe(m_fds) == 0) {
 #ifdef FD_CLOEXEC
     if (!child_processes_inherit) {
       if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
         error.SetErrorToErrno();
-        Close();
+        CloseUnlocked();
         return error;
       }
     }
 #endif
     return error;
   }
 #endif
 
   error.SetErrorToErrno();
   m_fds[READ] = PipePosix::kInvalidDescriptor;
   m_fds[WRITE] = PipePosix::kInvalidDescriptor;
   return error;
 }
 
 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;
   if (::mkfifo(name.str().c_str(), 0660) != 0)
     error.SetErrorToErrno();
 
   return error;
 }
Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
 
   if (error.Success())
     name = named_pipe_path;
   return error;
 }
 
 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;
   if (!child_process_inherit)
     flags |= O_CLOEXEC;
 
   Status error;
   int fd = FileSystem::Instance().Open(name.str().c_str(), flags);
   if (fd != -1)
     m_fds[READ] = fd;
   else
     error.SetErrorToErrno();
 
   return error;
 }
 
 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;
   if (!child_process_inherit)
     flags |= O_CLOEXEC;
 
   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)
         return Status("timeout exceeded - reader hasn't opened so far");
     }
 
     errno = 0;
     int fd = ::open(name.str().c_str(), flags);
     if (fd == -1) {
       const auto errno_copy = errno;
       // We may get ENXIO if a reader side of the pipe hasn't opened yet.
       if (errno_copy != ENXIO && errno_copy != EINTR)
         return Status(errno_copy, eErrorTypePOSIX);
 
       std::this_thread::sleep_for(
           milliseconds(OPEN_WRITER_SLEEP_TIMEOUT_MSECS));
     } else {
       m_fds[WRITE] = fd;
     }
   }
 
   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;
   }
 }
 
 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);
   select_helper.FDSetRead(fd);
 
   Status error;
   while (error.Success()) {
     error = select_helper.Select();
if (error.Success()) {
         break;
       }
     }
   }
   return error;
 }
 
 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);
 
   Status error;
   while (error.Success()) {
     error = select_helper.Select();
     if (error.Success()) {