From 9c135f1478cd9d7c81c11c6a6c9ac85b16d68d74 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Wed, 9 Aug 2023 12:31:30 -0700 Subject: [PATCH] [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 --- lldb/include/lldb/Host/File.h | 32 +++++++-- lldb/source/Host/common/File.cpp | 116 ++++++++++++++++++++----------- 2 files changed, 103 insertions(+), 45 deletions(-) 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 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 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; }