Skip to content

Commit

Permalink
[lldb] Fix data race in NativeFile
Browse files Browse the repository at this point in the history 
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
    rust-lang#2 lldb_private::Communication::Disconnect(...) Communication.cpp:61
    rust-lang#3 lldb_private::process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1164
    rust-lang#4 lldb_private::Process::SetExitStatus(...) Process.cpp:1097
    rust-lang#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
    rust-lang#2 lldb_private::Communication::IsConnected() const Communication.cpp:79
    rust-lang#3 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) GDBRemoteCommunication.cpp:256
    rust-lang#4 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) GDBRemoteCommunication.cpp:244
    rust-lang#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
  • Loading branch information
@JDevlieghere
JDevlieghere committed Aug 10, 2023
1 parent 13629b1 commit 9c135f1
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 45 deletions.
32 changes: 26 additions & 6 deletions lldb/include/lldb/Host/File.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
116 changes: 77 additions & 39 deletions lldb/source/Host/common/File.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand All @@ -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());
Expand All @@ -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;
}
Expand All @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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)
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}

Expand Down Expand Up @@ -577,15 +613,18 @@ 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) {
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) {
Expand All @@ -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;
}

Expand Down

0 comments on commit 9c135f1

Please sign in to comment.