Author: Pavel Labath Date: 2020-04-09T12:06:47+02:00 New Revision: ebb071345cdae2509c55f9eec76090926fee86a2
URL: https://github.com/llvm/llvm-project/commit/ebb071345cdae2509c55f9eec76090926fee86a2 DIFF: https://github.com/llvm/llvm-project/commit/ebb071345cdae2509c55f9eec76090926fee86a2.diff LOG: [lldb/Core] Fix a race in the Communication class Summary: 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. Reviewers: clayborg, JDevlieghere Subscribers: mgorny, lldb-commits Tags: #lldb Differential Revision: https://reviews.llvm.org/D77295 Added: lldb/unittests/Core/CommunicationTest.cpp Modified: lldb/source/Core/Communication.cpp lldb/unittests/Core/CMakeLists.txt Removed: ################################################################################ diff --git a/lldb/source/Core/Communication.cpp b/lldb/source/Core/Communication.cpp index f4163847e4bb..c3e421191b01 100644 --- a/lldb/source/Core/Communication.cpp +++ b/lldb/source/Core/Communication.cpp @@ -315,16 +315,12 @@ lldb::thread_result_t Communication::ReadThread(lldb::thread_arg_t p) { 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) + if (bytes_read > 0 || status == eConnectionStatusEndOfFile) comm->AppendBytesToCache(buf, bytes_read, true, status); - else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) { - if (comm->GetCloseOnEOF()) - comm->Disconnect(); - comm->AppendBytesToCache(buf, bytes_read, true, status); - } switch (status) { case eConnectionStatusSuccess: @@ -332,11 +328,12 @@ lldb::thread_result_t Communication::ReadThread(lldb::thread_arg_t p) { case eConnectionStatusEndOfFile: done = true; + disconnect = comm->GetCloseOnEOF(); break; 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 +362,15 @@ lldb::thread_result_t Communication::ReadThread(lldb::thread_arg_t p) { 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 {}; } diff --git a/lldb/unittests/Core/CMakeLists.txt b/lldb/unittests/Core/CMakeLists.txt index 688a39d9a10f..6393c6fe38c2 100644 --- a/lldb/unittests/Core/CMakeLists.txt +++ b/lldb/unittests/Core/CMakeLists.txt @@ -1,4 +1,5 @@ add_lldb_unittest(LLDBCoreTests + CommunicationTest.cpp MangledTest.cpp RichManglingContextTest.cpp StreamCallbackTest.cpp diff --git a/lldb/unittests/Core/CommunicationTest.cpp b/lldb/unittests/Core/CommunicationTest.cpp new file mode 100644 index 000000000000..6ad0bc720d93 --- /dev/null +++ b/lldb/unittests/Core/CommunicationTest.cpp @@ -0,0 +1,35 @@ +//===-- 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()); + + Communication comm("test"); + comm.SetConnection(std::make_unique<ConnectionFileDescriptor>( + pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true)); + 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()); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
