[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9c135f1478cd: [lldb] Fix data race in NativeFile (authored
by JDevlieghere).
Changed prior to commit:
https://reviews.llvm.org/D157347?vs=548849=549116#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
Files:
lldb/include/lldb/Host/File.h
lldb/source/Host/common/File.cpp
Index: lldb/source/Host/common/File.cpp
===
--- lldb/source/Host/common/File.cpp
+++ lldb/source/Host/common/File.cpp
@@ -219,7 +219,7 @@
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 @@
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 @@
}
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 @@
// 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 @@
}
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 @@
}
}
}
- 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 @@
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 @@
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 @@
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 @@
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 @@
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
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
bulbazord accepted this revision. bulbazord added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157347/new/ https://reviews.llvm.org/D157347 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
JDevlieghere added inline comments.
Comment at: lldb/source/Host/common/File.cpp:609
num_bytes = bytes_written;
- } else if (StreamIsValid()) {
+ } else if (ValueGuard stream_guard = StreamIsValid()) {
bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
augusto2112 wrote:
> bulbazord wrote:
> > augusto2112 wrote:
> > > 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.
> > If that's true, then this change needs further adjustment. `GetStream`
> > acquires the `ValueGuard`s 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).
> You're right, nice catch.
Thanks, I didn't know about that and that was certainly not the intention.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
JDevlieghere updated this revision to Diff 548849.
JDevlieghere marked 4 inline comments as done.
JDevlieghere added a comment.
Limit scope of `ValueGuard` by avoiding `else if`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
Files:
lldb/include/lldb/Host/File.h
lldb/source/Host/common/File.cpp
Index: lldb/source/Host/common/File.cpp
===
--- lldb/source/Host/common/File.cpp
+++ lldb/source/Host/common/File.cpp
@@ -219,7 +219,7 @@
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,20 @@
return file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
}
+bool NativeFile::IsValid() const {
+ return DescriptorIsValid() || StreamIsValid();
+}
+
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 +277,9 @@
}
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 +288,9 @@
// 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 +312,11 @@
}
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 +331,17 @@
}
}
}
- 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 +385,7 @@
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 +394,10 @@
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 +406,17 @@
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 +425,10 @@
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 +437,17 @@
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 +456,10 @@
else
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
augusto2112 added inline comments.
Comment at: lldb/source/Host/common/File.cpp:609
num_bytes = bytes_written;
- } else if (StreamIsValid()) {
+ } else if (ValueGuard stream_guard = StreamIsValid()) {
bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
bulbazord wrote:
> augusto2112 wrote:
> > 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.
> If that's true, then this change needs further adjustment. `GetStream`
> acquires the `ValueGuard`s 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).
You're right, nice catch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
bulbazord requested changes to this revision.
bulbazord added inline comments.
This revision now requires changes to proceed.
Comment at: lldb/source/Host/common/File.cpp:609
num_bytes = bytes_written;
- } else if (StreamIsValid()) {
+ } else if (ValueGuard stream_guard = StreamIsValid()) {
bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
augusto2112 wrote:
> 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.
If that's true, then this change needs further adjustment. `GetStream` acquires
the `ValueGuard`s 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).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
augusto2112 accepted this revision.
augusto2112 added a comment.
I have one nit that I'll leave up to you whether it should be addressed now or
not.
Comment at: lldb/source/Host/common/File.cpp:545
num_bytes = bytes_read;
- } else if (StreamIsValid()) {
+ } else if (ValueGuard file_lock = StreamIsValid()) {
bytes_read = ::fread(buf, 1, num_bytes, m_stream);
I believe `descriptor_guard` will still be in scope in the else branch (weird
scoping rules), but
Comment at: lldb/source/Host/common/File.cpp:609
num_bytes = bytes_written;
- } else if (StreamIsValid()) {
+ } else if (ValueGuard stream_guard = StreamIsValid()) {
bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
bulbazord accepted this revision. bulbazord added a comment. This revision is now accepted and ready to land. Looks right to me now. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157347/new/ https://reviews.llvm.org/D157347 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
JDevlieghere updated this revision to Diff 548729.
JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.
Address @bulbazord's code review feedback
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
Files:
lldb/include/lldb/Host/File.h
lldb/source/Host/common/File.cpp
Index: lldb/source/Host/common/File.cpp
===
--- lldb/source/Host/common/File.cpp
+++ lldb/source/Host/common/File.cpp
@@ -219,7 +219,7 @@
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,20 @@
return file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
}
+bool NativeFile::IsValid() const {
+ return DescriptorIsValid() || StreamIsValid();
+}
+
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 +277,9 @@
}
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());
@@ -306,8 +312,11 @@
}
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 +331,17 @@
}
}
}
- 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 +385,7 @@
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 +394,7 @@
else
error_ptr->Clear();
}
- } else if (StreamIsValid()) {
+ } else if (ValueGuard stream_guard = StreamIsValid()) {
result = ::fseek(m_stream, offset, SEEK_SET);
if (error_ptr) {
@@ -400,7 +411,7 @@
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 +420,7 @@
else
error_ptr->Clear();
}
- } else if (StreamIsValid()) {
+ } else if (ValueGuard stream_guard = StreamIsValid()) {
result = ::fseek(m_stream, offset, SEEK_CUR);
if (error_ptr) {
@@ -426,7 +437,7 @@
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 +446,7 @@
else
error_ptr->Clear();
}
- } else if (StreamIsValid()) {
+ } else if (ValueGuard stream_guard = StreamIsValid()) {
result = ::fseek(m_stream, offset, SEEK_END);
if (error_ptr) {
@@ -452,18 +463,23 @@
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)
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
bulbazord added inline comments.
Comment at: lldb/include/lldb/Host/File.h:425-433
+ ValueGuard DescriptorIsValid() const {
+m_descriptor_mutex.lock();
+return ValueGuard(m_descriptor_mutex,
+ File::DescriptorIsValid(m_descriptor));
+ }
+
+ ValueGuard StreamIsValid() const {
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).
Comment at: lldb/source/Host/common/File.cpp:222
+size_t written = s.size();
+;
Write(s.data(), written);
nit: delete this stray semicolon.
Comment at: lldb/source/Host/common/File.cpp:316
Status error;
- if (StreamIsValid()) {
-if (m_own_stream) {
- if (::fclose(m_stream) == EOF)
-error.SetErrorToErrno();
-} else {
- File::OpenOptions rw =
- m_options & (File::eOpenOptionReadOnly | File::eOpenOptionWriteOnly |
- File::eOpenOptionReadWrite);
+ {
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile
JDevlieghere updated this revision to Diff 548056.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
Files:
lldb/include/lldb/Host/File.h
lldb/source/Host/common/File.cpp
Index: lldb/source/Host/common/File.cpp
===
--- lldb/source/Host/common/File.cpp
+++ lldb/source/Host/common/File.cpp
@@ -78,21 +78,20 @@
}
Expected File::GetOptionsFromMode(llvm::StringRef mode) {
- OpenOptions opts =
- llvm::StringSwitch(mode)
- .Cases("r", "rb", eOpenOptionReadOnly)
- .Cases("w", "wb", eOpenOptionWriteOnly)
- .Cases("a", "ab",
- eOpenOptionWriteOnly | eOpenOptionAppend |
- eOpenOptionCanCreate)
- .Cases("r+", "rb+", "r+b", eOpenOptionReadWrite)
- .Cases("w+", "wb+", "w+b",
- eOpenOptionReadWrite | eOpenOptionCanCreate |
- eOpenOptionTruncate)
- .Cases("a+", "ab+", "a+b",
- eOpenOptionReadWrite | eOpenOptionAppend |
- eOpenOptionCanCreate)
- .Default(eOpenOptionInvalid);
+ OpenOptions opts = llvm::StringSwitch(mode)
+ .Cases("r", "rb", eOpenOptionReadOnly)
+ .Cases("w", "wb", eOpenOptionWriteOnly)
+ .Cases("a", "ab",
+eOpenOptionWriteOnly | eOpenOptionAppend |
+eOpenOptionCanCreate)
+ .Cases("r+", "rb+", "r+b", eOpenOptionReadWrite)
+ .Cases("w+", "wb+", "w+b",
+eOpenOptionReadWrite | eOpenOptionCanCreate |
+eOpenOptionTruncate)
+ .Cases("a+", "ab+", "a+b",
+eOpenOptionReadWrite | eOpenOptionAppend |
+eOpenOptionCanCreate)
+ .Default(eOpenOptionInvalid);
if (opts != eOpenOptionInvalid)
return opts;
return llvm::createStringError(
@@ -219,7 +218,8 @@
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,20 @@
return file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
}
+bool NativeFile::IsValid() const {
+ return DescriptorIsValid() || StreamIsValid();
+}
+
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 +277,9 @@
}
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());
@@ -307,30 +313,39 @@
Status NativeFile::Close() {
Status error;
- if (StreamIsValid()) {
-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)
+ {
+ValueGuard stream_guard = StreamIsValid();
+if (stream_guard) {
+ 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();
+}
}
}
+m_stream = kInvalidStream;
+m_own_stream = false;
}
- if (DescriptorIsValid() && m_own_descriptor) {
-if (::close(m_descriptor) != 0)
- error.SetErrorToErrno();
+
+ {
+ValueGuard descriptor_guard = DescriptorIsValid();
+if (descriptor_guard && m_own_descriptor) {
+ if (::close(m_descriptor) != 0)
+error.SetErrorToErrno();
+}
+m_descriptor = kInvalidDescriptor;
+m_own_descriptor = false;
}
- m_descriptor = kInvalidDescriptor;
-
