From 5d0b0c03f091229d48a145f4439a6197dc8aef79 Mon Sep 17 00:00:00 2001 From: John Salem Date: Wed, 21 Oct 2020 16:40:41 -0700 Subject: [PATCH 1/2] initial commit of fix --- .../debug/debug-pal/win/diagnosticsipc.cpp | 108 +++++++++--------- src/coreclr/src/vm/ipcstreamfactory.cpp | 5 + 2 files changed, 60 insertions(+), 53 deletions(-) diff --git a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp index 9a28e482342f5..4f8a04c61f97b 100644 --- a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp +++ b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp @@ -134,6 +134,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) _ASSERTE(mode == ConnectionMode::LISTEN); DWORD dwDummy = 0; + IpcStream *pStream = nullptr; bool fSuccess = GetOverlappedResult( _hPipe, // handle &_oOverlap, // overlapped @@ -144,11 +145,14 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) { if (callback != nullptr) callback("Failed to GetOverlappedResults for NamedPipe server", ::GetLastError()); - return nullptr; + // clean up the pipe + ::CloseHandle(_hPipe); + } + else + { + // create new IpcStream using handle (passes ownership to pStream) + pStream = new IpcStream(_hPipe, ConnectionMode::LISTEN); } - - // create new IpcStream using handle and reset the Server object so it can listen again - IpcStream *pStream = new IpcStream(_hPipe, ConnectionMode::LISTEN); // reset the server _hPipe = INVALID_HANDLE_VALUE; @@ -159,7 +163,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) if (!fSuccess) { delete pStream; - return nullptr; + pStream = nullptr; } return pStream; @@ -434,43 +438,42 @@ bool IpcStream::Read(void *lpBuffer, const uint32_t nBytesToRead, uint32_t &nByt if (!fSuccess) { + DWORD dwError = GetLastError(); + // if we're waiting infinitely, only make one syscall - if (timeoutMs == InfiniteTimeout) + if (timeoutMs == InfiniteTimeout && dwError == ERROR_IO_PENDING) { fSuccess = GetOverlappedResult(_hPipe, // pipe overlap, // overlapped &nNumberOfBytesRead, // out actual number of bytes read true) != 0; // block until async IO completes } - else + else if (dwError == ERROR_IO_PENDING) { - DWORD dwError = GetLastError(); - if (dwError == ERROR_IO_PENDING) + // Wait on overlapped IO event (triggers when async IO is complete regardless of success) + // or timeout + DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs); + if (dwWait == WAIT_OBJECT_0) { - // Wait on overlapped IO event (triggers when async IO is complete regardless of success) - // or timeout - DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs); - if (dwWait == WAIT_OBJECT_0) - { - // async IO compelted, get the result - fSuccess = GetOverlappedResult(_hPipe, // pipe - overlap, // overlapped - &nNumberOfBytesRead, // out actual number of bytes read - true) != 0; // block until async IO completes - } - else + // async IO compelted, get the result + fSuccess = GetOverlappedResult(_hPipe, // pipe + overlap, // overlapped + &nNumberOfBytesRead, // out actual number of bytes read + true) != 0; // block until async IO completes + } + else + { + // We either timed out or something else went wrong. + // For any error, attempt to cancel IO and ensure the cancel happened + if (CancelIoEx(_hPipe, overlap) != 0) { - // We either timed out or something else went wrong. - // For any error, attempt to cancel IO and ensure the cancel happened - if (CancelIoEx(_hPipe, overlap) != 0) - { - // check if the async write beat the cancellation - fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesRead, true) != 0; - // Failure here isn't recoverable, so return as such - } + // check if the async write beat the cancellation + fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesRead, true) != 0; + // Failure here isn't recoverable, so return as such } } } + // error is unrecoverable, so return as such } nBytesRead = static_cast(nNumberOfBytesRead); @@ -492,43 +495,42 @@ bool IpcStream::Write(const void *lpBuffer, const uint32_t nBytesToWrite, uint32 if (!fSuccess) { + DWORD dwError = GetLastError(); + // if we're waiting infinitely, only make one syscall - if (timeoutMs == InfiniteTimeout) + if (timeoutMs == InfiniteTimeout && dwError == ERROR_IO_PENDING) { fSuccess = GetOverlappedResult(_hPipe, // pipe overlap, // overlapped &nNumberOfBytesWritten, // out actual number of bytes written true) != 0; // block until async IO completes } - else + else if (dwError == ERROR_IO_PENDING) { - DWORD dwError = GetLastError(); - if (dwError == ERROR_IO_PENDING) + // Wait on overlapped IO event (triggers when async IO is complete regardless of success) + // or timeout + DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs); + if (dwWait == WAIT_OBJECT_0) { - // Wait on overlapped IO event (triggers when async IO is complete regardless of success) - // or timeout - DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs); - if (dwWait == WAIT_OBJECT_0) - { - // async IO compelted, get the result - fSuccess = GetOverlappedResult(_hPipe, // pipe - overlap, // overlapped - &nNumberOfBytesWritten, // out actual number of bytes written - true) != 0; // block until async IO completes - } - else + // async IO compelted, get the result + fSuccess = GetOverlappedResult(_hPipe, // pipe + overlap, // overlapped + &nNumberOfBytesWritten, // out actual number of bytes written + true) != 0; // block until async IO completes + } + else + { + // We either timed out or something else went wrong. + // For any error, attempt to cancel IO and ensure the cancel happened + if (CancelIoEx(_hPipe, overlap) != 0) { - // We either timed out or something else went wrong. - // For any error, attempt to cancel IO and ensure the cancel happened - if (CancelIoEx(_hPipe, overlap) != 0) - { - // check if the async write beat the cancellation - fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesWritten, true) != 0; - // Failure here isn't recoverable, so return as such - } + // check if the async write beat the cancellation + fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesWritten, true) != 0; + // Failure here isn't recoverable, so return as such } } } + // error is unrecoverable, so return as such } nBytesWritten = static_cast(nNumberOfBytesWritten); diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index 1116f84dee2bc..b11d5542ab652 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -325,6 +325,11 @@ IpcStream *IpcStreamFactory::GetNextAvailableStream(ErrorCallback callback) if (pStream == nullptr) // only use first signaled stream; will get others on subsequent calls { pStream = ((DiagnosticPort*)(rgIpcPollHandles[i].pUserData))->GetConnectedStream(callback); + if (pStream == nullptr) + { + fSawError = true; + break; + } s_currentPort = (DiagnosticPort*)(rgIpcPollHandles[i].pUserData); } STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - SIG :: Poll attempt: %d, connection %d signalled.\n", nPollAttempts, i); From 3413e5e86ed18a5592c0fc81f20fc39141dd5c7a Mon Sep 17 00:00:00 2001 From: John Salem Date: Tue, 3 Nov 2020 13:41:45 -0800 Subject: [PATCH 2/2] PR feedback --- src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp | 2 +- src/coreclr/src/vm/ipcstreamfactory.cpp | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp index 4f8a04c61f97b..fe7828daf78b5 100644 --- a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp +++ b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp @@ -145,7 +145,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) { if (callback != nullptr) callback("Failed to GetOverlappedResults for NamedPipe server", ::GetLastError()); - // clean up the pipe + // close the pipe (cleaned up and reset below) ::CloseHandle(_hPipe); } else diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index b11d5542ab652..dfaf4b93feaef 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -328,7 +328,6 @@ IpcStream *IpcStreamFactory::GetNextAvailableStream(ErrorCallback callback) if (pStream == nullptr) { fSawError = true; - break; } s_currentPort = (DiagnosticPort*)(rgIpcPollHandles[i].pUserData); }