[Lldb-commits] [PATCH] D77295: [lldb/Core] Fix a race in the Communication class

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: clayborg, JDevlieghere.
Herald added a subscriber: mgorny.
Herald added a project: LLDB.

Communication::SynchronizeWithReadThread is called whenever a process
stops to ensure that we process all of its stdout before we report the
stop. If the process exits, we first call this method, and then close
the connection.

However, when the child process exits, the thread reading its stdout
will usually (but not always) read an EOF because the other end of the
pty has been closed. In response to an EOF, the Communication read
thread closes it's end of the connection too.

This can result in a race where the read thread is closing the
connection while the synchronizing thread is attempting to get its
attention via Connection::InterruptRead.

The fix is to hold the synchronization mutex while closing the
connection.

I've found this issue while tracking down a rare flake in some of the
vscode tests. I am not sure this is the cause of those failures (as I
would have expected this issue to manifest itself differently), but it
is an issue nonetheless.

The attached test demonstrates the steps needed to reproduce the race.
It will fail under tsan without this patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77295

Files:
  lldb/source/Core/Communication.cpp
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/CommunicationTest.cpp


Index: lldb/unittests/Core/CommunicationTest.cpp
===================================================================
--- /dev/null
+++ lldb/unittests/Core/CommunicationTest.cpp
@@ -0,0 +1,36 @@
+//===-- CommunicationTest.cpp ---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Core/Communication.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
+#include "lldb/Host/Pipe.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+TEST(CommunicationTest, SynchronizeWhileClosing) {
+  // Set up a communication object reading from a pipe.
+  Pipe pipe;
+  ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
+                    llvm::Succeeded());
+  auto conn_up = std::make_unique<ConnectionFileDescriptor>(
+      pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true);
+
+  Communication comm("test");
+  comm.SetConnection(conn_up.release());
+  comm.SetCloseOnEOF(true);
+  ASSERT_TRUE(comm.StartReadThread());
+
+  // Ensure that we can safely synchronize with the read thread while it is
+  // closing the read end (in response to us closing the write end).
+  pipe.CloseWriteFileDescriptor();
+  comm.SynchronizeWithReadThread();
+
+  ASSERT_TRUE(comm.StopReadThread());
+}
Index: lldb/unittests/Core/CMakeLists.txt
===================================================================
--- lldb/unittests/Core/CMakeLists.txt
+++ lldb/unittests/Core/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_lldb_unittest(LLDBCoreTests
+  CommunicationTest.cpp
   MangledTest.cpp
   RichManglingContextTest.cpp
   StreamCallbackTest.cpp
Index: lldb/source/Core/Communication.cpp
===================================================================
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -315,14 +315,14 @@
   Status error;
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
+  bool disconnect = false;
   while (!done && comm->m_read_thread_enabled) {
     size_t bytes_read = comm->ReadFromConnection(
         buf, sizeof(buf), std::chrono::seconds(5), status, &error);
     if (bytes_read > 0)
       comm->AppendBytesToCache(buf, bytes_read, true, status);
     else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
-      if (comm->GetCloseOnEOF())
-        comm->Disconnect();
+      disconnect = comm->GetCloseOnEOF();
       comm->AppendBytesToCache(buf, bytes_read, true, status);
     }
 
@@ -336,7 +336,7 @@
     case eConnectionStatusError: // Check GetError() for details
       if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
         // EIO on a pipe is usually caused by remote shutdown
-        comm->Disconnect();
+        disconnect = comm->GetCloseOnEOF();
         done = true;
       }
       if (error.Fail())
@@ -365,9 +365,15 @@
   if (log)
     LLDB_LOGF(log, "%p Communication::ReadThread () thread exiting...", p);
 
-  comm->m_read_thread_did_exit = true;
-  // Let clients know that this thread is exiting
   comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  {
+    std::lock_guard<std::mutex> guard(comm->m_synchronize_mutex);
+    comm->m_read_thread_did_exit = true;
+    if (disconnect)
+      comm->Disconnect();
+  }
+
+  // Let clients know that this thread is exiting
   comm->BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77295.254489.patch
Type: text/x-patch
Size: 3659 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200402/307067f7/attachment-0001.bin>