[lldb] Fix data race in NativeFile

[JDevlieghere]

Summary

TSan reports the following data race:

Write of size 4 at 0x000109e0b160 by thread T2 (mutexes: write M0, write M1):
  #0 lldb_private::NativeFile::Close() File.cpp:329 (liblldb.18.0.0git.dylib:arm64+0x58dac0)
  #1 lldb_private::ConnectionFileDescriptor::Disconnect(lldb_private::Status*) ConnectionFileDescriptorPosix.cpp:232 (liblldb.18.0.0git.dylib:arm64+0x5bad6c)
  #2 lldb_private::Communication::Disconnect(lldb_private::Status*) Communication.cpp:61 (liblldb.18.0.0git.dylib:arm64+0x451f04)
  #3 lldb_private::process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1164 (liblldb.18.0.0git.dylib:arm64+0xadfd80)
  #4 lldb_private::Process::SetExitStatus(int, char const*) Process.cpp:1097 (liblldb.18.0.0git.dylib:arm64+0x6ed338)
  #5 lldb_private::process_gdb_remote::ProcessGDBRemote::MonitorDebugserverProcess(std::__1::weak_ptr<lldb_private::process_gdb_remote::ProcessGDBRemote>, unsigned long long, int, int) ProcessGDBRemote.cpp:3387 (liblldb.18.0.0git.dylib:arm64+0xae9464)

Previous read of size 4 at 0x000109e0b160 by main thread (mutexes: write M2):
  #0 lldb_private::NativeFile::IsValid() const File.h:393 (liblldb.18.0.0git.dylib:arm64+0x590830)
  #1 lldb_private::ConnectionFileDescriptor::IsConnected() const ConnectionFileDescriptorPosix.cpp:121 (liblldb.18.0.0git.dylib:arm64+0x5b8ab4)
  #2 lldb_private::Communication::IsConnected() const Communication.cpp:79 (liblldb.18.0.0git.dylib:arm64+0x451fcc)
  #3 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote&, lldb_private::Timeout<std::__1::ratio<1l, 1000000l>>, bool) GDBRemoteCommunication.cpp:256 (liblldb.18.0.0git.dylib:arm64+0xaac060)
  #4 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote&, lldb_private::Timeout<std::__1::ratio<1l, 1000000l>>, bool) GDBRemoteCommunication.cpp:244 (liblldb.18.0.0git.dylib:arm64+0xaabe78)
  #5 lldb_private::process_gdb_remote::GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(llvm::StringRef, StringExtractorGDBRemote&) GDBRemoteClientBase.cpp:246 (liblldb.18.0.0git.dylib:arm64+0xaaa184)

I tried fixing the problem at the ConnectionFileDescriptor level, but the underlying problem is that NativeFile isn't thread safe. NativeFile can hold a file descriptor and/or a file stream. Throughout its implementation, it would check if the descriptor or stream is valid and than do something based on that. While that works in a single threaded environment, nothing prevents another thread from modifying the descriptor or stream between the IsValid check and when it's actually being used. This patch prevents such issues by returning a ValueGuard RAII object. As long as the object is in scope, the value is guaranteed by a lock.

[augusto2112]

LGTM

[bulbazord]

Makes sense to me.

[bulbazord]

lldb/include/lldb/Host/File.h

434–442

Hmm, this should be okay since URVO is mandatory as of C++17 (meaning this will not perform a copy and potentially screw up the locking mechanism).

lldb/source/Host/common/File.cpp

223

nit: delete this stray semicolon.

328–329

I think you need to grab both mutexes before you can modify either one of them in Close. As an example:
Thread 1 calls Close.
Thread 2 calls GetStream.
Thread 1 grabs the stream_guard and does all the relevant work with it.
Thread 2 grabs the stream_guard and subsequently grabs the descriptor_guard. It opens the stream again and sets the descriptor to unowned.
Thread 1 resumes, grabs the descriptor_guard, but does nothing with it because it's unowned. However, it does reset the state of the other member variables.

In this case, the work that Thread 1 did to close the stream was undone by Thread 2. The File is not closed and is left is a strange half-initialized state.

Reading through this and thinking about it more, I wonder if Close is the right abstraction for a File that can be touched by multiple threads. If one thread closes it and the other is still trying to work with it, you could end up in a weird state no matter what else is going on. Maybe it would make more sense to have a File have a reference count? Users wouldn't call File::Close directly but just let it go out of scope and allow the destructor to handle resource management. That might be out of the scope of this patch though.

[JDevlieghere]

Address @bulbazord's code review feedback

[bulbazord]

Looks right to me now. Thanks!

[augusto2112]

I have one nit that I'll leave up to you whether it should be addressed now or not.

lldb/source/Host/common/File.cpp

560–563

I believe descriptor_guard will still be in scope in the else branch (weird scoping rules), but

624–627

One thing that's not super obvious for people reading this is that descriptor_guard is still in scope in the else branch (due to C++'s weird scoping rules when it comes to declaring variables inside if statements), and will keep the descriptor mutex locked which is probably not what you intended. I don't think this affects the correctness of this implementation at the moment, but could be dangerous with further changes on top of this one.

[bulbazord]

lldb/source/Host/common/File.cpp

624–627

If that's true, then this change needs further adjustment. GetStream acquires the ValueGuards in the opposite order, meaning Thread 1 can call this function and acquire descriptor_guard while Thread 2 can call GetStream and acquire stream_guard at the same time. Then they're both waiting on the other lock (deadlock).

[augusto2112]

lldb/source/Host/common/File.cpp

624–627

You're right, nice catch.

[JDevlieghere]

Limit scope of ValueGuard by avoiding else if.

lldb/source/Host/common/File.cpp

624–627

Thanks, I didn't know about that and that was certainly not the intention.

[bulbazord]

LGTM!

lldb/include/lldb/Host/File.h

NativeFile(FILE *fh, bool transfer_ownership)
         m_options(), m_own_stream(transfer_ownership) {}
 
   NativeFile(int fd, OpenOptions options, bool transfer_ownership)
       : m_descriptor(fd), m_own_descriptor(transfer_ownership),
         m_stream(kInvalidStream), m_options(options), m_own_stream(false) {}
 
   ~NativeFile() override { Close(); }
 
-  bool IsValid() const override {
-    return DescriptorIsValid() || StreamIsValid();
-  }
+  bool IsValid() const override;
 
   Status Read(void *buf, size_t &num_bytes) override;
   Status Write(const void *buf, size_t &num_bytes) override;
   Status Close() override;
   WaitableHandle GetWaitableHandle() override;
   Status GetFileSpec(FileSpec &file_spec) const override;
   int GetDescriptor() const override;
   FILE *GetStream() override;
public:
 
   static char ID;
   bool isA(const void *classID) const override {
     return classID == &ID || File::isA(classID);
   }
   static bool classof(const File *file) { return file->isA(&ID); }
 
 protected:
-  bool DescriptorIsValid() const {
+  struct ValueGuard {
+    ValueGuard(std::mutex &m, bool b) : guard(m, std::adopt_lock), value(b) {}
+    std::lock_guard<std::mutex> guard;
+    bool value;
+    operator bool() { return value; }
+  };
+
+  bool DescriptorIsValidUnlocked() const {
+
     return File::DescriptorIsValid(m_descriptor);
   }
-  bool StreamIsValid() const { return m_stream != kInvalidStream; }
 
-  // Member variables
+  bool StreamIsValidUnlocked() const { return m_stream != kInvalidStream; }
+
+  ValueGuard DescriptorIsValid() const {
+    m_descriptor_mutex.lock();
+    return ValueGuard(m_descriptor_mutex, DescriptorIsValidUnlocked());
+  }
+
+  ValueGuard StreamIsValid() const {
+    m_stream_mutex.lock();
+    return ValueGuard(m_stream_mutex, StreamIsValidUnlocked());
+  }
+
   int m_descriptor;
   bool m_own_descriptor = false;
+  mutable std::mutex m_descriptor_mutex;
+
   FILE *m_stream;
+  mutable std::mutex m_stream_mutex;
+
   OpenOptions m_options{};
   bool m_own_stream = false;
   std::mutex offset_access_mutex;
 
 private:
   NativeFile(const NativeFile &) = delete;
   const NativeFile &operator=(const NativeFile &) = delete;
 };

lldb/source/Host/common/File.cpp

size_t File::Printf(const char *format, ...) {
   size_t result = PrintfVarArg(format, args);
   va_end(args);
   return result;
 }
 
 size_t File::PrintfVarArg(const char *format, va_list args) {
   llvm::SmallString<0> s;
   if (VASprintf(s, format, args)) {
-    size_t written = s.size();;
+    size_t written = s.size();
     Write(s.data(), written);
     return written;
   }
   return 0;
 }
 
 Expected<File::OpenOptions> File::GetOptions() const {
   return llvm::createStringError(
       llvm::inconvertibleErrorCode(),
uint32_t File::GetPermissions(Status &error) const {
   if (::fstat(fd, &file_stats) == -1) {
     error.SetErrorToErrno();
     return 0;
   }
   error.Clear();
   return file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
 }
 
+bool NativeFile::IsValid() const {
+  std::scoped_lock lock(m_descriptor_mutex, m_stream_mutex);
+  return DescriptorIsValidUnlocked() || StreamIsValidUnlocked();
+}
+
 Expected<File::OpenOptions> NativeFile::GetOptions() const { return m_options; }
 
 int NativeFile::GetDescriptor() const {
-  if (DescriptorIsValid())
+  if (ValueGuard descriptor_guard = DescriptorIsValid()) {
     return m_descriptor;
+  }
 
   // Don't open the file descriptor if we don't need to, just get it from the
   // stream if we have one.
-  if (StreamIsValid()) {
+  if (ValueGuard stream_guard = StreamIsValid()) {
 #if defined(_WIN32)
     return _fileno(m_stream);
 #else
     return fileno(m_stream);
 #endif
   }
 
   // Invalid descriptor and invalid stream, return invalid descriptor.
   return kInvalidDescriptor;
 }
 
 IOObject::WaitableHandle NativeFile::GetWaitableHandle() {
   return GetDescriptor();
 }
 
 FILE *NativeFile::GetStream() {
-  if (!StreamIsValid()) {
-    if (DescriptorIsValid()) {
+  ValueGuard stream_guard = StreamIsValid();
+  if (!stream_guard) {
+    if (ValueGuard descriptor_guard = DescriptorIsValid()) {
       auto mode = GetStreamOpenModeFromOptions(m_options);
       if (!mode)
         llvm::consumeError(mode.takeError());
       else {
         if (!m_own_descriptor) {
 // We must duplicate the file descriptor if we don't own it because when you
 // call fdopen, the stream will own the fd
 #ifdef _WIN32
-          m_descriptor = ::_dup(GetDescriptor());
+          m_descriptor = ::_dup(m_descriptor);
 #else
-          m_descriptor = dup(GetDescriptor());
+          m_descriptor = dup(m_descriptor);
 #endif
           m_own_descriptor = true;
         }
 
         m_stream = llvm::sys::RetryAfterSignal(nullptr, ::fdopen, m_descriptor,
                                                mode.get());
 
         // If we got a stream, then we own the stream and should no longer own
         // the descriptor because fclose() will close it for us
 
         if (m_stream) {
           m_own_stream = true;
           m_own_descriptor = false;
         }
       }
     }
   }
   return m_stream;
 }
 
 Status NativeFile::Close() {
+  std::scoped_lock lock(m_descriptor_mutex, m_stream_mutex);
+
   Status error;
-  if (StreamIsValid()) {
+
+  if (StreamIsValidUnlocked()) {
     if (m_own_stream) {
       if (::fclose(m_stream) == EOF)
         error.SetErrorToErrno();
     } else {
       File::OpenOptions rw =
           m_options & (File::eOpenOptionReadOnly | File::eOpenOptionWriteOnly |
                        File::eOpenOptionReadWrite);
 
       if (rw == eOpenOptionWriteOnly || rw == eOpenOptionReadWrite) {
         if (::fflush(m_stream) == EOF)
           error.SetErrorToErrno();
       }
     }
   }
-  if (DescriptorIsValid() && m_own_descriptor) {
+
+  if (DescriptorIsValidUnlocked() && m_own_descriptor) {
     if (::close(m_descriptor) != 0)
       error.SetErrorToErrno();
   }
-  m_descriptor = kInvalidDescriptor;
+
   m_stream = kInvalidStream;
-  m_options = OpenOptions(0);
   m_own_stream = false;
+  m_descriptor = kInvalidDescriptor;
   m_own_descriptor = false;
+  m_options = OpenOptions(0);
   m_is_interactive = eLazyBoolCalculate;
   m_is_real_terminal = eLazyBoolCalculate;
   return error;
 }
 
 Status NativeFile::GetFileSpec(FileSpec &file_spec) const {
   Status error;
 #ifdef F_GETPATH
#endif
 
   if (error.Fail())
     file_spec.Clear();
   return error;
 }
 
 off_t NativeFile::SeekFromStart(off_t offset, Status *error_ptr) {
   off_t result = 0;
-  if (DescriptorIsValid()) {
+  if (ValueGuard descriptor_guard = DescriptorIsValid()) {
     result = ::lseek(m_descriptor, offset, SEEK_SET);
 
     if (error_ptr) {
       if (result == -1)
         error_ptr->SetErrorToErrno();
       else
         error_ptr->Clear();
     }
-  } else if (StreamIsValid()) {
+    return result;
+  }
+
+  if (ValueGuard stream_guard = StreamIsValid()) {
     result = ::fseek(m_stream, offset, SEEK_SET);
 
     if (error_ptr) {
       if (result == -1)
         error_ptr->SetErrorToErrno();
       else
         error_ptr->Clear();
     }
-  } else if (error_ptr) {
-    error_ptr->SetErrorString("invalid file handle");
+    return result;
   }
+
+  if (error_ptr)
+    error_ptr->SetErrorString("invalid file handle");
   return result;
 }
 
 off_t NativeFile::SeekFromCurrent(off_t offset, Status *error_ptr) {
   off_t result = -1;
-  if (DescriptorIsValid()) {
+  if (ValueGuard descriptor_guard = DescriptorIsValid()) {
     result = ::lseek(m_descriptor, offset, SEEK_CUR);
 
     if (error_ptr) {
       if (result == -1)
         error_ptr->SetErrorToErrno();
       else
         error_ptr->Clear();
     }
-  } else if (StreamIsValid()) {
+    return result;
+  }
+
+  if (ValueGuard stream_guard = StreamIsValid()) {
     result = ::fseek(m_stream, offset, SEEK_CUR);
 
     if (error_ptr) {
       if (result == -1)
         error_ptr->SetErrorToErrno();
       else
         error_ptr->Clear();
     }
-  } else if (error_ptr) {
-    error_ptr->SetErrorString("invalid file handle");
+    return result;
   }
+
+  if (error_ptr)
+    error_ptr->SetErrorString("invalid file handle");
   return result;
 }
 
 off_t NativeFile::SeekFromEnd(off_t offset, Status *error_ptr) {
   off_t result = -1;
-  if (DescriptorIsValid()) {
+  if (ValueGuard descriptor_guard = DescriptorIsValid()) {
     result = ::lseek(m_descriptor, offset, SEEK_END);
 
     if (error_ptr) {
       if (result == -1)
         error_ptr->SetErrorToErrno();
       else
         error_ptr->Clear();
     }
-  } else if (StreamIsValid()) {
+    return result;
+  }
+
+  if (ValueGuard stream_guard = StreamIsValid()) {
     result = ::fseek(m_stream, offset, SEEK_END);
 
     if (error_ptr) {
       if (result == -1)
         error_ptr->SetErrorToErrno();
       else
         error_ptr->Clear();
     }
-  } else if (error_ptr) {
-    error_ptr->SetErrorString("invalid file handle");
   }
+
+  if (error_ptr)
+    error_ptr->SetErrorString("invalid file handle");
   return result;
 }
 
 Status NativeFile::Flush() {
   Status error;
-  if (StreamIsValid()) {
+  if (ValueGuard stream_guard = StreamIsValid()) {
     if (llvm::sys::RetryAfterSignal(EOF, ::fflush, m_stream) == EOF)
       error.SetErrorToErrno();
-  } else if (!DescriptorIsValid()) {
+    return error;
+  }
+
+  {
+    ValueGuard descriptor_guard = DescriptorIsValid();
+    if (!descriptor_guard)
       error.SetErrorString("invalid file handle");
   }
   return error;
 }
 
 Status NativeFile::Sync() {
   Status error;
-  if (DescriptorIsValid()) {
+  if (ValueGuard descriptor_guard = DescriptorIsValid()) {
 #ifdef _WIN32
     int err = FlushFileBuffers((HANDLE)_get_osfhandle(m_descriptor));
     if (err == 0)
       error.SetErrorToGenericError();
 #else
     if (llvm::sys::RetryAfterSignal(-1, ::fsync, m_descriptor) == -1)
       error.SetErrorToErrno();
 #endif
while (bytes_left > 0) {
       if (error.Fail())
         break;
     }
     return error;
   }
 #endif
 
   ssize_t bytes_read = -1;
-  if (DescriptorIsValid()) {
-    bytes_read = llvm::sys::RetryAfterSignal(-1, ::read, m_descriptor, buf, num_bytes);
+  if (ValueGuard descriptor_guard = DescriptorIsValid()) {
+    bytes_read =
+        llvm::sys::RetryAfterSignal(-1, ::read, m_descriptor, buf, num_bytes);
     if (bytes_read == -1) {
       error.SetErrorToErrno();
       num_bytes = 0;
     } else
       num_bytes = bytes_read;
-  } else if (StreamIsValid()) {
+    return error;
+  }
+
+  if (ValueGuard file_lock = StreamIsValid()) {
     bytes_read = ::fread(buf, 1, num_bytes, m_stream);
 
     if (bytes_read == 0) {
       if (::feof(m_stream))
         error.SetErrorString("feof");
       else if (::ferror(m_stream))
         error.SetErrorString("ferror");
       num_bytes = 0;
     } else
       num_bytes = bytes_read;
-  } else {
-    num_bytes = 0;
-    error.SetErrorString("invalid file handle");
-  }
+    return error;
+  }
+
   return error;
 }
 
 Status NativeFile::Write(const void *buf, size_t &num_bytes) {
   Status error;
 
 #if defined(MAX_WRITE_SIZE)
   if (num_bytes > MAX_WRITE_SIZE) {
while (bytes_left > 0) {
       if (error.Fail())
         break;
     }
     return error;
   }
 #endif
 
   ssize_t bytes_written = -1;
-  if (DescriptorIsValid()) {
+  if (ValueGuard descriptor_guard = DescriptorIsValid()) {
     bytes_written =
         llvm::sys::RetryAfterSignal(-1, ::write, m_descriptor, buf, num_bytes);
     if (bytes_written == -1) {
       error.SetErrorToErrno();
       num_bytes = 0;
     } else
       num_bytes = bytes_written;
-  } else if (StreamIsValid()) {
+    return error;
+  }
+
+  if (ValueGuard stream_guard = StreamIsValid()) {
     bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
 
     if (bytes_written == 0) {
       if (::feof(m_stream))
         error.SetErrorString("feof");
       else if (::ferror(m_stream))
         error.SetErrorString("ferror");
       num_bytes = 0;
     } else
       num_bytes = bytes_written;
+    return error;
+  }
 
-  } else {
-    num_bytes = 0;
-    error.SetErrorString("invalid file handle");
-  }
-
   return error;
 }
 
 Status NativeFile::Read(void *buf, size_t &num_bytes, off_t &offset) {
   Status error;
 
 #if defined(MAX_READ_SIZE)
   if (num_bytes > MAX_READ_SIZE) {