[Lldb-commits] [lldb] 9c135f1 - [lldb] Fix data race in NativeFile

Jonas Devlieghere via lldb-commits lldb-commits at lists.llvm.org
Thu Aug 10 11:14:41 PDT 2023


Author: Jonas Devlieghere
Date: 2023-08-10T11:13:47-07:00
New Revision: 9c135f1478cd9d7c81c11c6a6c9ac85b16d68d74

URL: https://github.com/llvm/llvm-project/commit/9c135f1478cd9d7c81c11c6a6c9ac85b16d68d74
DIFF: https://github.com/llvm/llvm-project/commit/9c135f1478cd9d7c81c11c6a6c9ac85b16d68d74.diff

LOG: [lldb] Fix data race in NativeFile

TSan reports the following data race:

  Write of size 4 at 0x000109e0b160 by thread T2 (...):
    #0 lldb_private::NativeFile::Close() File.cpp:329
    #1 lldb_private::ConnectionFileDescriptor::Disconnect(...) ConnectionFileDescriptorPosix.cpp:232
    #2 lldb_private::Communication::Disconnect(...) Communication.cpp:61
    #3 lldb_private::process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1164
    #4 lldb_private::Process::SetExitStatus(...) Process.cpp:1097
    #5 lldb_private::process_gdb_remote::ProcessGDBRemote::MonitorDebugserverProcess(...) ProcessGDBRemote.cpp:3387

  Previous read of size 4 at 0x000109e0b160 by main thread (...):
    #0 lldb_private::NativeFile::IsValid() const File.h:393
    #1 lldb_private::ConnectionFileDescriptor::IsConnected() const ConnectionFileDescriptorPosix.cpp:121
    #2 lldb_private::Communication::IsConnected() const Communication.cpp:79
    #3 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) GDBRemoteCommunication.cpp:256
    #4 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) GDBRemoteCommunication.cpp:244
    #5 lldb_private::process_gdb_remote::GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(...) GDBRemoteClientBase.cpp:246

I originally tried fixing the problem at the ConnectionFileDescriptor
level, but that operates on an IOObject which can have different thread
safety guarantees depending on its implementation.

For this particular issue, the problem is specific to NativeFile.
NativeFile can hold a file descriptor and/or a file stream. Throughout
its implementation, it checks if the descriptor or stream is valid and
do some operation on it if it is. 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.

Differential revision: https://reviews.llvm.org/D157347

Added: 
    

Modified: 
    lldb/include/lldb/Host/File.h
    lldb/source/Host/common/File.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Host/File.h b/lldb/include/lldb/Host/File.h
index a1199a51b8a691..5ce53c93b1b910 100644
--- a/lldb/include/lldb/Host/File.h
+++ b/lldb/include/lldb/Host/File.h
@@ -389,9 +389,7 @@ class NativeFile : public File {
 
   ~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;
@@ -417,15 +415,37 @@ class NativeFile : public File {
   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;

diff  --git a/lldb/source/Host/common/File.cpp b/lldb/source/Host/common/File.cpp
index 7c5d71d9426ead..82cfcedd616fc2 100644
--- a/lldb/source/Host/common/File.cpp
+++ b/lldb/source/Host/common/File.cpp
@@ -219,7 +219,7 @@ size_t File::Printf(const char *format, ...) {
 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;
   }
@@ -247,15 +247,21 @@ uint32_t File::GetPermissions(Status &error) const {
   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
@@ -272,8 +278,9 @@ IOObject::WaitableHandle NativeFile::GetWaitableHandle() {
 }
 
 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());
@@ -282,9 +289,9 @@ FILE *NativeFile::GetStream() {
 // 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;
         }
@@ -306,8 +313,11 @@ FILE *NativeFile::GetStream() {
 }
 
 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();
@@ -322,15 +332,17 @@ Status NativeFile::Close() {
       }
     }
   }
-  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;
@@ -374,7 +386,7 @@ Status NativeFile::GetFileSpec(FileSpec &file_spec) const {
 
 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) {
@@ -383,7 +395,10 @@ off_t NativeFile::SeekFromStart(off_t offset, Status *error_ptr) {
       else
         error_ptr->Clear();
     }
-  } else if (StreamIsValid()) {
+    return result;
+  }
+
+  if (ValueGuard stream_guard = StreamIsValid()) {
     result = ::fseek(m_stream, offset, SEEK_SET);
 
     if (error_ptr) {
@@ -392,15 +407,17 @@ off_t NativeFile::SeekFromStart(off_t offset, Status *error_ptr) {
       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) {
@@ -409,7 +426,10 @@ off_t NativeFile::SeekFromCurrent(off_t offset, Status *error_ptr) {
       else
         error_ptr->Clear();
     }
-  } else if (StreamIsValid()) {
+    return result;
+  }
+
+  if (ValueGuard stream_guard = StreamIsValid()) {
     result = ::fseek(m_stream, offset, SEEK_CUR);
 
     if (error_ptr) {
@@ -418,15 +438,17 @@ off_t NativeFile::SeekFromCurrent(off_t offset, Status *error_ptr) {
       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) {
@@ -435,7 +457,10 @@ off_t NativeFile::SeekFromEnd(off_t offset, Status *error_ptr) {
       else
         error_ptr->Clear();
     }
-  } else if (StreamIsValid()) {
+    return result;
+  }
+
+  if (ValueGuard stream_guard = StreamIsValid()) {
     result = ::fseek(m_stream, offset, SEEK_END);
 
     if (error_ptr) {
@@ -444,26 +469,32 @@ off_t NativeFile::SeekFromEnd(off_t offset, Status *error_ptr) {
       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()) {
-    error.SetErrorString("invalid file handle");
+    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)
@@ -518,14 +549,18 @@ Status NativeFile::Read(void *buf, size_t &num_bytes) {
 #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) {
@@ -536,10 +571,11 @@ Status NativeFile::Read(void *buf, size_t &num_bytes) {
       num_bytes = 0;
     } else
       num_bytes = bytes_read;
-  } else {
-    num_bytes = 0;
-    error.SetErrorString("invalid file handle");
+    return error;
   }
+
+  num_bytes = 0;
+  error.SetErrorString("invalid file handle");
   return error;
 }
 
@@ -577,7 +613,7 @@ Status NativeFile::Write(const void *buf, size_t &num_bytes) {
 #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) {
@@ -585,7 +621,10 @@ Status NativeFile::Write(const void *buf, size_t &num_bytes) {
       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) {
@@ -596,12 +635,11 @@ Status NativeFile::Write(const void *buf, size_t &num_bytes) {
       num_bytes = 0;
     } else
       num_bytes = bytes_written;
-
-  } else {
-    num_bytes = 0;
-    error.SetErrorString("invalid file handle");
+    return error;
   }
 
+  num_bytes = 0;
+  error.SetErrorString("invalid file handle");
   return error;
 }
 


        


More information about the lldb-commits mailing list