diff options
author | José Rivero <jorive@microsoft.com> | 2019-03-21 09:08:46 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-03-21 09:08:46 -0700 |
commit | a8624052c9e7b759e7f943cb2a84a2520bef1b79 (patch) | |
tree | 39a8a1ee347b401427760183c763ca56350f5b34 | |
parent | ad30997d41d9652a644f6a2fe1b73bd8416d87b2 (diff) | |
download | coreclr-a8624052c9e7b759e7f943cb2a84a2520bef1b79.tar.gz coreclr-a8624052c9e7b759e7f943cb2a84a2520bef1b79.tar.bz2 coreclr-a8624052c9e7b759e7f943cb2a84a2520bef1b79.zip |
[EventPipe] Fix linker warning on Debug/Checked build and unlink Unix Domain Socket #23334 (#23357)
- Attempt to close-behind unix domain socket and repurpose LF_REMOTING facility.
- On shutdown, attempt to unlink the bound socket so it can be removed from the file system when the last reference to it it closed.
- Rename unused LF_REMOTING to LF_DIAGNOSTICS_PORT. This new flag will be used by the diagnostic server stress log calls.
- libcmtd.lib(initializers.obj) : warning LNK4098: defaultlib 'libcmt.lib' conflicts with use of other libs; use /NODEFAULTLIB:library [S:\github.com\jorive\coreclr\bin\obj\Windows_NT.x64.Checked\src\dlls\mscoree\coreclr\coreclr.vcxproj]
- Move some preprocessors and includes around.
-rw-r--r-- | src/debug/debug-pal/CMakeLists.txt | 4 | ||||
-rw-r--r-- | src/debug/debug-pal/unix/diagnosticsipc.cpp | 51 | ||||
-rw-r--r-- | src/debug/debug-pal/win/diagnosticsipc.cpp | 4 | ||||
-rw-r--r-- | src/debug/inc/diagnosticsipc.h | 19 | ||||
-rw-r--r-- | src/inc/loglf.h | 2 | ||||
-rw-r--r-- | src/vm/diagnosticserver.cpp | 38 | ||||
-rw-r--r-- | src/vm/diagnosticserver.h | 8 | ||||
-rw-r--r-- | src/vm/fastserializer.cpp | 1 | ||||
-rw-r--r-- | src/vm/fastserializer.h | 3 |
9 files changed, 99 insertions, 31 deletions
diff --git a/src/debug/debug-pal/CMakeLists.txt b/src/debug/debug-pal/CMakeLists.txt index c1b5b443a9..ac0f97e468 100644 --- a/src/debug/debug-pal/CMakeLists.txt +++ b/src/debug/debug-pal/CMakeLists.txt @@ -5,12 +5,10 @@ include_directories(../../pal/inc) add_definitions(-DPAL_STDCPP_COMPAT=1) if(WIN32) - #use static crt - add_definitions(-MT) add_definitions(-DWIN32_LEAN_AND_MEAN) include_directories(../../inc) #needed for warning control - set(TWO_WAY_PIPE_SOURCES + set(TWO_WAY_PIPE_SOURCES win/diagnosticsipc.cpp win/twowaypipe.cpp win/processdescriptor.cpp diff --git a/src/debug/debug-pal/unix/diagnosticsipc.cpp b/src/debug/debug-pal/unix/diagnosticsipc.cpp index 182dff01be..d9e0e94c9f 100644 --- a/src/debug/debug-pal/unix/diagnosticsipc.cpp +++ b/src/debug/debug-pal/unix/diagnosticsipc.cpp @@ -7,7 +7,6 @@ #include <new> #include <unistd.h> #include <fcntl.h> -#include <sys/types.h> #include <sys/socket.h> #include <sys/un.h> #include "diagnosticsipc.h" @@ -15,7 +14,8 @@ IpcStream::DiagnosticsIpc::DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress) : _serverSocket(serverSocket), - _pServerAddress(new (std::nothrow) sockaddr_un) + _pServerAddress(new (std::nothrow) sockaddr_un), + _isUnlinked(false) { _ASSERTE(_pServerAddress != nullptr); _ASSERTE(_serverSocket != -1); @@ -33,8 +33,7 @@ IpcStream::DiagnosticsIpc::~DiagnosticsIpc() const int fSuccessClose = ::close(_serverSocket); _ASSERTE(fSuccessClose != -1); // TODO: Add error handling? - const int fSuccessUnlink = ::unlink(_pServerAddress->sun_path); - _ASSERTE(fSuccessUnlink != -1); // TODO: Add error handling? + Unlink(); delete _pServerAddress; } @@ -68,6 +67,26 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p if (callback != nullptr) callback(strerror(errno), errno); _ASSERTE(fSuccessBind != -1); + + const int fSuccessClose = ::close(serverSocket); + _ASSERTE(fSuccessClose != -1); + + return nullptr; + } + + const int fSuccessfulListen = ::listen(serverSocket, /* backlog */ 255); + if (fSuccessfulListen == -1) + { + if (callback != nullptr) + callback(strerror(errno), errno); + _ASSERTE(fSuccessfulListen != -1); + + const int fSuccessUnlink = ::unlink(serverAddress.sun_path); + _ASSERTE(fSuccessUnlink != -1); + + const int fSuccessClose = ::close(serverSocket); + _ASSERTE(fSuccessClose != -1); + return nullptr; } @@ -76,8 +95,6 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const { - if (::listen(_serverSocket, /* backlog */ 255) == -1) - return nullptr; sockaddr_un from; socklen_t fromlen = sizeof(from); const int clientSocket = ::accept(_serverSocket, (sockaddr *)&from, &fromlen); @@ -94,6 +111,28 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const return pIpcStream; } +//! 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 +//! filesystem that must be deleted by the caller when it is no longer +//! needed (using unlink(2)). The usual UNIX close-behind semantics +//! apply; the socket can be unlinked at any time and will be finally +//! 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); + } +} + IpcStream::~IpcStream() { if (_clientSocket != -1) diff --git a/src/debug/debug-pal/win/diagnosticsipc.cpp b/src/debug/debug-pal/win/diagnosticsipc.cpp index 7d9f84e7e5..46f212836b 100644 --- a/src/debug/debug-pal/win/diagnosticsipc.cpp +++ b/src/debug/debug-pal/win/diagnosticsipc.cpp @@ -77,6 +77,10 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const return pIpcStream; } +void IpcStream::DiagnosticsIpc::Unlink(ErrorCallback callback) +{ +} + IpcStream::~IpcStream() { if (_hPipe != INVALID_HANDLE_VALUE) diff --git a/src/debug/inc/diagnosticsipc.h b/src/debug/inc/diagnosticsipc.h index 81789f0803..54277e1af9 100644 --- a/src/debug/inc/diagnosticsipc.h +++ b/src/debug/inc/diagnosticsipc.h @@ -26,20 +26,31 @@ public: class DiagnosticsIpc final { public: - static DiagnosticsIpc *Create(const char *const pIpcName, ErrorCallback callback = nullptr); ~DiagnosticsIpc(); + + //! Creates an IPC object + static DiagnosticsIpc *Create(const char *const pIpcName, ErrorCallback callback = nullptr); + + //! 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); + private: #ifdef FEATURE_PAL - DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress); - const int _serverSocket = -1; + const int _serverSocket; sockaddr_un *const _pServerAddress; + bool _isUnlinked = false; + + DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress); #else static const uint32_t MaxNamedPipeNameLength = 256; - DiagnosticsIpc(const char(&namedPipeName)[MaxNamedPipeNameLength]); char _pNamedPipeName[MaxNamedPipeNameLength]; // https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-createnamedpipea + + DiagnosticsIpc(const char(&namedPipeName)[MaxNamedPipeNameLength]); #endif /* FEATURE_PAL */ DiagnosticsIpc() = delete; diff --git a/src/inc/loglf.h b/src/inc/loglf.h index e7fbd519d9..76a9897b53 100644 --- a/src/inc/loglf.h +++ b/src/inc/loglf.h @@ -16,7 +16,7 @@ DEFINE_LOG_FACILITY(LF_GCALLOC ,0x00000100) DEFINE_LOG_FACILITY(LF_CORDB ,0x00000200) DEFINE_LOG_FACILITY(LF_CLASSLOADER ,0x00000400) DEFINE_LOG_FACILITY(LF_CORPROF ,0x00000800) -DEFINE_LOG_FACILITY(LF_REMOTING ,0x00001000) +DEFINE_LOG_FACILITY(LF_DIAGNOSTICS_PORT ,0x00001000) DEFINE_LOG_FACILITY(LF_DBGALLOC ,0x00002000) DEFINE_LOG_FACILITY(LF_EH ,0x00004000) DEFINE_LOG_FACILITY(LF_ENC ,0x00008000) diff --git a/src/vm/diagnosticserver.cpp b/src/vm/diagnosticserver.cpp index c0e11d100f..3a700383ac 100644 --- a/src/vm/diagnosticserver.cpp +++ b/src/vm/diagnosticserver.cpp @@ -4,7 +4,6 @@ #include "common.h" #include "diagnosticserver.h" -#include "diagnosticsipc.h" #include "eventpipeprotocolhelper.h" #ifdef FEATURE_PAL @@ -13,6 +12,8 @@ #ifdef FEATURE_PERFTRACING +IpcStream::DiagnosticsIpc *DiagnosticServer::s_pIpc = nullptr; + static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter) { CONTRACTL @@ -28,17 +29,13 @@ static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter) auto pIpc = reinterpret_cast<IpcStream::DiagnosticsIpc *>(lpThreadParameter); if (pIpc == nullptr) { - STRESS_LOG0(LF_STARTUP, LL_ERROR,"Diagnostics IPC listener was undefined\n"); + STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ERROR, "Diagnostics IPC listener was undefined\n"); return 1; } -#ifdef _DEBUG ErrorCallback LoggingCallback = [](const char *szMessage, uint32_t code) { - LOG((LF_REMOTING, LL_WARNING, "warning (%d): %s.\n", code, szMessage)); + STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_WARNING, "warning (%d): %s.\n", code, szMessage); }; -#else - ErrorCallback LoggingCallback = nullptr; -#endif while (true) { @@ -69,7 +66,7 @@ static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter) break; default: - LOG((LF_REMOTING, LL_WARNING, "Received unknow request type (%d)\n", header.RequestType)); + LOG((LF_DIAGNOSTICS_PORT, LL_WARNING, "Received unknow request type (%d)\n", header.RequestType)); break; } } @@ -93,23 +90,25 @@ bool DiagnosticServer::Initialize() { auto ErrorCallback = [](const char *szMessage, uint32_t code) { STRESS_LOG2( - LF_STARTUP, // facility + LF_DIAGNOSTICS_PORT, // facility LL_ERROR, // level "Failed to create diagnostic IPC: error (%d): %s.\n", // msg code, // data1 szMessage); // data2 }; - IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create( + + // TODO: Should we handle/assert that (s_pIpc == nullptr)? + s_pIpc = IpcStream::DiagnosticsIpc::Create( "dotnetcore-diagnostic", ErrorCallback); - if (pIpc != nullptr) + if (s_pIpc != nullptr) { DWORD dwThreadId = 0; HANDLE hThread = ::CreateThread( // TODO: Is it correct to have this "lower" level call here? nullptr, // no security attribute 0, // default stack size DiagnosticsServerThread, // thread proc - (LPVOID)pIpc, // thread parameter + (LPVOID)s_pIpc, // thread parameter 0, // not suspended &dwThreadId); // returns thread ID @@ -117,7 +116,7 @@ bool DiagnosticServer::Initialize() { // Failed to create IPC thread. STRESS_LOG1( - LF_STARTUP, // facility + LF_DIAGNOSTICS_PORT, // facility LL_ERROR, // level "Failed to create diagnostic server thread (%d).\n", // msg ::GetLastError()); // data1 @@ -155,7 +154,18 @@ bool DiagnosticServer::Shutdown() EX_TRY { - // FIXME: Stop IPC server thread? + if (s_pIpc != nullptr) + { + auto ErrorCallback = [](const char *szMessage, uint32_t code) { + STRESS_LOG2( + LF_DIAGNOSTICS_PORT, // facility + LL_ERROR, // level + "Failed to unlink diagnostic IPC: error (%d): %s.\n", // msg + code, // data1 + szMessage); // data2 + }; + s_pIpc->Unlink(ErrorCallback); + } fSuccess = true; } EX_CATCH diff --git a/src/vm/diagnosticserver.h b/src/vm/diagnosticserver.h index 95399a2ef8..51f32ae09f 100644 --- a/src/vm/diagnosticserver.h +++ b/src/vm/diagnosticserver.h @@ -5,10 +5,11 @@ #ifndef __DIAGNOSTIC_SERVER_H__ #define __DIAGNOSTIC_SERVER_H__ -#include <stdint.h> - #ifdef FEATURE_PERFTRACING // This macro should change to something more generic than performance. +#include <stdint.h> +#include "diagnosticsipc.h" + //! TODO: Temp class. enum class DiagnosticMessageType : uint32_t { @@ -51,6 +52,9 @@ public: //! Shutdown the event pipe. static bool Shutdown(); + +private: + static IpcStream::DiagnosticsIpc *s_pIpc; }; #endif // FEATURE_PERFTRACING diff --git a/src/vm/fastserializer.cpp b/src/vm/fastserializer.cpp index 89dbe50728..e02ec93010 100644 --- a/src/vm/fastserializer.cpp +++ b/src/vm/fastserializer.cpp @@ -4,6 +4,7 @@ #include "common.h" #include "fastserializer.h" +#include "diagnosticsipc.h" #ifdef FEATURE_PERFTRACING diff --git a/src/vm/fastserializer.h b/src/vm/fastserializer.h index 3b4de65bf5..98208c0eb1 100644 --- a/src/vm/fastserializer.h +++ b/src/vm/fastserializer.h @@ -11,7 +11,8 @@ #include "fastserializableobject.h" #include "fstream.h" -#include "diagnosticsipc.h" + +class IpcStream; // the enumeration has a specific set of values to keep it compatible with consumer library // it's sibling is defined in https://github.com/Microsoft/perfview/blob/10d1f92b242c98073b3817ac5ee6d98cd595d39b/src/FastSerialization/FastSerialization.cs#L2295 |