From 3cbb26b33fe545988eaa71ad71a202938a56b4f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Rivero?= Date: Wed, 3 Jul 2019 12:26:51 -0700 Subject: [PATCH 1/6] [WIP] --- src/debug/debug-pal/unix/diagnosticsipc.cpp | 40 ++++++++++++------- src/debug/debug-pal/win/diagnosticsipc.cpp | 3 +- src/debug/inc/diagnosticsipc.h | 11 ++++-- src/vm/diagnosticserver.cpp | 43 ++++++++++++++------- src/vm/diagnosticserver.h | 5 +++ 5 files changed, 69 insertions(+), 33 deletions(-) diff --git a/src/debug/debug-pal/unix/diagnosticsipc.cpp b/src/debug/debug-pal/unix/diagnosticsipc.cpp index 72c34b84ac0d..5571df222b67 100644 --- a/src/debug/debug-pal/unix/diagnosticsipc.cpp +++ b/src/debug/debug-pal/unix/diagnosticsipc.cpp @@ -15,7 +15,7 @@ IpcStream::DiagnosticsIpc::DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress) : _serverSocket(serverSocket), _pServerAddress(new sockaddr_un), - _isUnlinked(false) + _isClosed(false) { _ASSERTE(_pServerAddress != nullptr); _ASSERTE(_serverSocket != -1); @@ -28,15 +28,8 @@ IpcStream::DiagnosticsIpc::DiagnosticsIpc(const int serverSocket, sockaddr_un *c IpcStream::DiagnosticsIpc::~DiagnosticsIpc() { - if (_serverSocket != -1) - { - const int fSuccessClose = ::close(_serverSocket); - _ASSERTE(fSuccessClose != -1); // TODO: Add error handling? - - Unlink(); - - delete _pServerAddress; - } + Close(); + delete _pServerAddress; } IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const pIpcName, ErrorCallback callback) @@ -56,8 +49,10 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p _ASSERTE(!"Failed to create diagnostics IPC socket."); return nullptr; } + sockaddr_un serverAddress{}; serverAddress.sun_family = AF_UNIX; + const ProcessDescriptor pd = ProcessDescriptor::FromCurrentProcess(); PAL_GetTransportName( sizeof(serverAddress.sun_path), @@ -134,6 +129,25 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const return new IpcStream(clientSocket); } +void IpcStream::DiagnosticsIpc::Close(ErrorCallback callback) +{ + if (_isClosed) + return; + _isClosed = true; + + if (_serverSocket != -1) + { + if (::close(_serverSocket) == -1) + { + if (callback != nullptr) + callback(strerror(errno), errno); + _ASSERTE(!"Failed to close unix domain socket."); // TODO: Add error handling? + } + + Unlink(callback); + } +} + //! This helps remove the socket from the filesystem when the runtime exits. //! From: http://man7.org/linux/man-pages/man7/unix.7.html#NOTES //! Binding to a socket with a filename creates a socket in the @@ -143,16 +157,12 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const //! removed from the filesystem when the last reference to it is closed. void IpcStream::DiagnosticsIpc::Unlink(ErrorCallback callback) { - if (_isUnlinked) - return; - _isUnlinked = true; - const int fSuccessUnlink = ::unlink(_pServerAddress->sun_path); if (fSuccessUnlink == -1) { if (callback != nullptr) callback(strerror(errno), errno); - _ASSERTE(fSuccessUnlink == 0); + _ASSERTE(!"Failed to unlink server address."); } } diff --git a/src/debug/debug-pal/win/diagnosticsipc.cpp b/src/debug/debug-pal/win/diagnosticsipc.cpp index ef65f232a661..2482b19c6cb8 100644 --- a/src/debug/debug-pal/win/diagnosticsipc.cpp +++ b/src/debug/debug-pal/win/diagnosticsipc.cpp @@ -13,6 +13,7 @@ IpcStream::DiagnosticsIpc::DiagnosticsIpc(const char(&namedPipeName)[MaxNamedPip IpcStream::DiagnosticsIpc::~DiagnosticsIpc() { + Close(); } IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const pIpcName, ErrorCallback callback) @@ -73,7 +74,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const return new IpcStream(hPipe); } -void IpcStream::DiagnosticsIpc::Unlink(ErrorCallback) +void IpcStream::DiagnosticsIpc::Close(ErrorCallback) { } diff --git a/src/debug/inc/diagnosticsipc.h b/src/debug/inc/diagnosticsipc.h index 54277e1af9f3..f02da73435d9 100644 --- a/src/debug/inc/diagnosticsipc.h +++ b/src/debug/inc/diagnosticsipc.h @@ -34,18 +34,21 @@ class IpcStream final //! Enables the underlaying IPC implementation to accept connection. IpcStream *Accept(ErrorCallback callback = nullptr) const; - //! Used to unlink the socket so it can be removed from the filesystem - //! when the last reference to it is closed. - void Unlink(ErrorCallback callback = nullptr); + //! Closes an open IPC. + void Close(ErrorCallback callback = nullptr); private: #ifdef FEATURE_PAL const int _serverSocket; sockaddr_un *const _pServerAddress; - bool _isUnlinked = false; + bool _isClosed = false; DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress); + + //! Used to unlink the socket so it can be removed from the filesystem + //! when the last reference to it is closed. + void Unlink(ErrorCallback callback = nullptr); #else static const uint32_t MaxNamedPipeNameLength = 256; char _pNamedPipeName[MaxNamedPipeNameLength]; // https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-createnamedpipea diff --git a/src/vm/diagnosticserver.cpp b/src/vm/diagnosticserver.cpp index 9a1dce533622..db9e42c4484a 100644 --- a/src/vm/diagnosticserver.cpp +++ b/src/vm/diagnosticserver.cpp @@ -20,20 +20,21 @@ #ifdef FEATURE_PERFTRACING IpcStream::DiagnosticsIpc *DiagnosticServer::s_pIpc = nullptr; +Volatile DiagnosticServer::s_shuttingDown(false); +HANDLE DiagnosticServer::s_hServerThread = INVALID_HANDLE_VALUE; -static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter) +DWORD WINAPI DiagnosticServer::DiagnosticsServerThread(LPVOID) { CONTRACTL { NOTHROW; GC_TRIGGERS; MODE_PREEMPTIVE; - PRECONDITION(lpThreadParameter != nullptr); + PRECONDITION(s_pIpc != nullptr); } CONTRACTL_END; - auto pIpc = reinterpret_cast(lpThreadParameter); - if (pIpc == nullptr) + if (s_pIpc == nullptr) { STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ERROR, "Diagnostics IPC listener was undefined\n"); return 1; @@ -45,11 +46,11 @@ static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter) EX_TRY { - while (true) + while (!s_shuttingDown) { // FIXME: Ideally this would be something like a std::shared_ptr - IpcStream *pStream = pIpc->Accept(LoggingCallback); - + IpcStream *pStream = s_pIpc->Accept(LoggingCallback); + if (pStream == nullptr) continue; #ifdef FEATURE_AUTO_TRACE @@ -140,7 +141,7 @@ bool DiagnosticServer::Initialize() auto_trace_launch(); #endif DWORD dwThreadId = 0; - HANDLE hThread = ::CreateThread( // TODO: Is it correct to have this "lower" level call here? + s_hServerThread = ::CreateThread( // TODO: Is it correct to have this "lower" level call here? nullptr, // no security attribute 0, // default stack size DiagnosticsServerThread, // thread proc @@ -148,8 +149,11 @@ bool DiagnosticServer::Initialize() 0, // not suspended &dwThreadId); // returns thread ID - if (hThread == nullptr) + if (s_hServerThread == NULL) { + delete s_pIpc; + s_pIpc = nullptr; + // Failed to create IPC thread. STRESS_LOG1( LF_DIAGNOSTICS_PORT, // facility @@ -162,9 +166,6 @@ bool DiagnosticServer::Initialize() #ifdef FEATURE_AUTO_TRACE auto_trace_wait(); #endif - // FIXME: Maybe hold on to the thread to abort/cleanup at exit? - ::CloseHandle(hThread); - // TODO: Add error handling? fSuccess = true; } @@ -191,6 +192,8 @@ bool DiagnosticServer::Shutdown() bool fSuccess = false; + s_shuttingDown = true; + EX_TRY { if (s_pIpc != nullptr) @@ -203,7 +206,21 @@ bool DiagnosticServer::Shutdown() code, // data1 szMessage); // data2 }; - s_pIpc->Unlink(ErrorCallback); + s_pIpc->Close(ErrorCallback); // This will break the accept waiting for client connection. + + if (s_hServerThread != NULL) + { +#ifndef FEATURE_PAL + if (::CancelSynchronousIo(s_hServerThread) == 0) + _ASSERTE(!"Failed to mark pending synchronous I/O operations issued by Diagnostics Server Thread as canceled."); +#endif // FEATURE_PAL + + ::WaitForSingleObject(s_hServerThread, INFINITE); + ::CloseHandle(s_hServerThread); + } + + delete s_pIpc; + s_pIpc = nullptr; } fSuccess = true; } diff --git a/src/vm/diagnosticserver.h b/src/vm/diagnosticserver.h index adab6e0a9f69..f196c9cbb53c 100644 --- a/src/vm/diagnosticserver.h +++ b/src/vm/diagnosticserver.h @@ -42,8 +42,13 @@ class DiagnosticServer final //! Shutdown the event pipe. static bool Shutdown(); + //! Diagnostics server thread. + static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter); + private: static IpcStream::DiagnosticsIpc *s_pIpc; + static Volatile s_shuttingDown; + static HANDLE s_hServerThread; }; #endif // FEATURE_PERFTRACING From d1bac9f1324433ffe190b0531ec5ec2c68bd9c86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Rivero?= Date: Tue, 9 Jul 2019 10:20:12 -0700 Subject: [PATCH 2/6] Remove comment. --- src/debug/debug-pal/unix/diagnosticsipc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/debug/debug-pal/unix/diagnosticsipc.cpp b/src/debug/debug-pal/unix/diagnosticsipc.cpp index 5571df222b67..a44196e3ae22 100644 --- a/src/debug/debug-pal/unix/diagnosticsipc.cpp +++ b/src/debug/debug-pal/unix/diagnosticsipc.cpp @@ -141,7 +141,7 @@ void IpcStream::DiagnosticsIpc::Close(ErrorCallback callback) { if (callback != nullptr) callback(strerror(errno), errno); - _ASSERTE(!"Failed to close unix domain socket."); // TODO: Add error handling? + _ASSERTE(!"Failed to close unix domain socket."); } Unlink(callback); From 157e5ce19bfa8b3f6f4ba8196673e4a10927648d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Rivero?= Date: Tue, 9 Jul 2019 10:21:06 -0700 Subject: [PATCH 3/6] Adding braces around the if block (assert). --- src/vm/diagnosticserver.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/vm/diagnosticserver.cpp b/src/vm/diagnosticserver.cpp index db9e42c4484a..190da80121ce 100644 --- a/src/vm/diagnosticserver.cpp +++ b/src/vm/diagnosticserver.cpp @@ -212,7 +212,9 @@ bool DiagnosticServer::Shutdown() { #ifndef FEATURE_PAL if (::CancelSynchronousIo(s_hServerThread) == 0) + { _ASSERTE(!"Failed to mark pending synchronous I/O operations issued by Diagnostics Server Thread as canceled."); + } #endif // FEATURE_PAL ::WaitForSingleObject(s_hServerThread, INFINITE); From 295296c3176bc52ebc6ea7491e0ae0f24bced15c Mon Sep 17 00:00:00 2001 From: Jose Rivero Date: Tue, 9 Jul 2019 15:26:49 -0700 Subject: [PATCH 4/6] WaitForSingleObject under Windows only --- src/vm/diagnosticserver.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vm/diagnosticserver.cpp b/src/vm/diagnosticserver.cpp index 190da80121ce..853020f5c1d8 100644 --- a/src/vm/diagnosticserver.cpp +++ b/src/vm/diagnosticserver.cpp @@ -215,9 +215,9 @@ bool DiagnosticServer::Shutdown() { _ASSERTE(!"Failed to mark pending synchronous I/O operations issued by Diagnostics Server Thread as canceled."); } + ::WaitForSingleObject(s_hServerThread, INFINITE); #endif // FEATURE_PAL - ::WaitForSingleObject(s_hServerThread, INFINITE); ::CloseHandle(s_hServerThread); } From b82591e428de0a9bc42c613452dab58d33866703 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Rivero?= Date: Tue, 9 Jul 2019 15:31:57 -0700 Subject: [PATCH 5/6] Minor changes: Comment, error message, redundant initialization. --- src/debug/inc/diagnosticsipc.h | 2 +- src/vm/diagnosticserver.cpp | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/debug/inc/diagnosticsipc.h b/src/debug/inc/diagnosticsipc.h index f02da73435d9..fb0a64fdd097 100644 --- a/src/debug/inc/diagnosticsipc.h +++ b/src/debug/inc/diagnosticsipc.h @@ -42,7 +42,7 @@ class IpcStream final #ifdef FEATURE_PAL const int _serverSocket; sockaddr_un *const _pServerAddress; - bool _isClosed = false; + bool _isClosed; DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress); diff --git a/src/vm/diagnosticserver.cpp b/src/vm/diagnosticserver.cpp index 853020f5c1d8..cca77e518cf1 100644 --- a/src/vm/diagnosticserver.cpp +++ b/src/vm/diagnosticserver.cpp @@ -166,7 +166,6 @@ bool DiagnosticServer::Initialize() #ifdef FEATURE_AUTO_TRACE auto_trace_wait(); #endif - // TODO: Add error handling? fSuccess = true; } } @@ -202,7 +201,7 @@ bool DiagnosticServer::Shutdown() STRESS_LOG2( LF_DIAGNOSTICS_PORT, // facility LL_ERROR, // level - "Failed to unlink diagnostic IPC: error (%d): %s.\n", // msg + "Failed to close diagnostic IPC: error (%d): %s.\n", // msg code, // data1 szMessage); // data2 }; From 5a8b758268cb9e056c244a2b107272c55d8b5fa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Rivero?= Date: Mon, 15 Jul 2019 10:22:24 -0700 Subject: [PATCH 6/6] Avoid potential AV on non-Windows platforms by not deallocating the IPC on shutdown. --- src/vm/diagnosticserver.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/vm/diagnosticserver.cpp b/src/vm/diagnosticserver.cpp index cca77e518cf1..71abf8ed2346 100644 --- a/src/vm/diagnosticserver.cpp +++ b/src/vm/diagnosticserver.cpp @@ -214,14 +214,22 @@ bool DiagnosticServer::Shutdown() { _ASSERTE(!"Failed to mark pending synchronous I/O operations issued by Diagnostics Server Thread as canceled."); } - ::WaitForSingleObject(s_hServerThread, INFINITE); #endif // FEATURE_PAL + // At this point, IO operations on the server thread through the + // IPC channel has been closed/cancelled. + + // On non-Windows, this function is blocking on threads that already exit. + // ::WaitForSingleObject(s_hServerThread, INFINITE); + + // Close the thread handle (dispose OS resource). ::CloseHandle(s_hServerThread); + s_hServerThread = INVALID_HANDLE_VALUE; } - delete s_pIpc; - s_pIpc = nullptr; + // If we do not wait for thread to teardown, then we cannot delete this object. + // delete s_pIpc; + // s_pIpc = nullptr; } fSuccess = true; }