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

[Alex Langford via Phabricator via lldb-commits]

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