[Lldb-commits] [PATCH] D157347: [lldb] Fix data race in NativeFile

[Jonas Devlieghere via Phabricator via lldb-commits]

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