[release/6.0][EventPipe] Fix reverse connection socket leaking to chi…

…ld processes. (#65768) * [release/6.0][EventPipe] Fix reverse connection socket leaking to child processes. Sockets were getting leaked upon accepting connections. This meant that child processes could cause fully terminated tracing sessions and other IPC connections alive, causing clients to wait for input indefinitely. This sets the CLOEXEC bits on the socket atomically upon accepting if the OS provides the capability, falling back to a best effort fcntl on systems like macOS and x866 Android emulators. Port of #65365 to release/6.0. * Include well know cmake paths
dotnet · Mar 8, 2022 · 154cbb0 · 154cbb0
1 parent 21ec8d3
commit 154cbb0
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 4 deletions.
diff --git a/src/coreclr/CMakeLists.txt b/src/coreclr/CMakeLists.txt
@@ -124,6 +124,9 @@ endif(CLR_CMAKE_HOST_UNIX)
 # Add this subdir. We install the headers for the jit.
 add_subdirectory(pal/prebuilt/inc)
 
+# These need to happen before the VM and debug-pal includes.
+set(EP_GENERATED_HEADER_PATH "${GENERATED_INCLUDE_DIR}")
+include(${CLR_SRC_NATIVE_DIR}/eventpipe/configure.cmake)
 add_subdirectory(debug/debug-pal)
 
 add_subdirectory(minipal)

diff --git a/src/coreclr/debug/debug-pal/CMakeLists.txt b/src/coreclr/debug/debug-pal/CMakeLists.txt
@@ -1,6 +1,6 @@
-
 include_directories(../inc)
 include_directories(../../pal/inc)
+include_directories(${EP_GENERATED_HEADER_PATH})
 
 add_definitions(-DPAL_STDCPP_COMPAT)
 

diff --git a/src/coreclr/vm/eventing/eventpipe/CMakeLists.txt b/src/coreclr/vm/eventing/eventpipe/CMakeLists.txt
@@ -92,6 +92,7 @@ list(APPEND EVENTPIPE_SOURCES
   ${CORECLR_EVENTPIPE_SHIM_SOURCES}
   ${CORECLR_EVENTPIPE_SHIM_HEADERS}
   ${EVENTPIPE_HEADERS}
+  ${SHARED_EVENTPIPE_CONFIG_HEADERS}
 )
 
 add_library_clr(eventpipe_gen_objs OBJECT ${GEN_EVENTPIPE_SOURCES})

diff --git a/src/mono/mono/eventpipe/CMakeLists.txt b/src/mono/mono/eventpipe/CMakeLists.txt
@@ -1,3 +1,8 @@
+# For feature detection to work correctly, this needs to be outside of the conditional.
+set(EP_GENERATED_HEADER_PATH "${MONO_EVENTPIPE_GEN_INCLUDE_PATH}")
+include(../../../../eng/native/configurepaths.cmake)
+include(${SHARED_EVENTPIPE_SOURCE_PATH}configure.cmake)
+
 if(ENABLE_PERFTRACING)
 
     include(${MONO_EVENTPIPE_SHIM_SOURCE_PATH}/gen-eventing.cmake)
@@ -79,7 +84,7 @@ if(ENABLE_PERFTRACING)
     addprefix(shared_diagnostic_server_sources_base ${SHARED_EVENTPIPE_SOURCE_PATH} "${shared_diagnostic_server_sources_base}")
     addprefix(mono_diagnostic_server_shim_sources_base ${MONO_EVENTPIPE_SHIM_SOURCE_PATH} "${mono_diagnostic_server_shim_sources_base}")
 
-    set(eventpipe_sources ${shared_eventpipe_sources_base} ${mono_eventpipe_shim_sources_base} ${MONO_EVENTPIPE_GEN_HEADERS} ${MONO_EVENTPIPE_GEN_SOURCES})
+    set(eventpipe_sources ${shared_eventpipe_sources_base} ${SHARED_EVENTPIPE_CONFIG_HEADERS} ${mono_eventpipe_shim_sources_base} ${MONO_EVENTPIPE_GEN_HEADERS} ${MONO_EVENTPIPE_GEN_SOURCES})
     set(diagnostic_server_sources ${shared_diagnostic_server_sources_base} ${mono_diagnostic_server_shim_sources_base})
 
     set_source_files_properties(${SHARED_EVENTPIPE_SOURCE_PATH}/ep-sources.c PROPERTIES COMPILE_DEFINITIONS EP_FORCE_INCLUDE_SOURCE_FILES)

diff --git a/src/native/eventpipe/configure.cmake b/src/native/eventpipe/configure.cmake
@@ -0,0 +1,14 @@
+include(CheckSymbolExists)
+
+check_symbol_exists(
+    accept4
+    sys/socket.h
+    HAVE_ACCEPT4)
+
+if (NOT DEFINED EP_GENERATED_HEADER_PATH)
+    message(FATAL_ERROR "Required configuration EP_GENERATED_HEADER_PATH not set.")
+endif (NOT DEFINED EP_GENERATED_HEADER_PATH)
+
+configure_file(${CLR_SRC_NATIVE_DIR}/eventpipe/ep-shared-config.h.in ${EP_GENERATED_HEADER_PATH}/ep-shared-config.h)
+
+set (SHARED_EVENTPIPE_CONFIG_HEADERS "${EP_GENERATED_HEADER_PATH}/ep-shared-config.h")
diff --git a/src/native/eventpipe/ds-ipc-pal-socket.c b/src/native/eventpipe/ds-ipc-pal-socket.c
@@ -530,8 +530,22 @@ ipc_socket_accept (
 	ds_ipc_socket_t client_socket;
 	DS_ENTER_BLOCKING_PAL_SECTION;
 	do {
-		client_socket = accept (s, address, address_len);
+#if defined(HAVE_ACCEPT4) && defined(SOCK_CLOEXEC)
+    	client_socket = accept4 (s, address, address_len, SOCK_CLOEXEC);
+#else
+    	client_socket = accept (s, address, address_len);
+#endif
 	} while (ipc_retry_syscall (client_socket));
+
+#if !defined(HAVE_ACCEPT4) || !defined(SOCK_CLOEXEC)
+#if defined(FD_CLOEXEC)
+    if (client_socket != -1)
+    {
+        // ignore any failures; this is best effort
+        fcntl (client_socket, F_SETFD, FD_CLOEXEC);
+    }
+#endif
+#endif
 	DS_EXIT_BLOCKING_PAL_SECTION;
 	return client_socket;
 }
@@ -834,7 +848,7 @@ ipc_alloc_tcp_address (
 
 	const ep_char8_t *host_address = address;
 	const ep_char8_t *host_port = strrchr (address, ':');
-	
+
 	if (host_port && host_port != host_address) {
 		size_t host_address_len = host_port - address;
 		address [host_address_len] = 0;

diff --git a/src/native/eventpipe/ep-rt-config.h b/src/native/eventpipe/ep-rt-config.h
@@ -1,6 +1,8 @@
 #ifndef __EVENTPIPE_RT_CONFIG_H__
 #define __EVENTPIPE_RT_CONFIG_H__
 
+#include "ep-shared-config.h"
+
 #ifndef FEATURE_CORECLR
 
 #include <config.h>

diff --git a/src/native/eventpipe/ep-shared-config.h.in b/src/native/eventpipe/ep-shared-config.h.in
@@ -0,0 +1,7 @@
+#ifndef EP_SHARED_CONFIG_H_INCLUDED
+#define EP_SHARED_CONFIG_H_INCLUDED
+
+/* This platforms supports setting flags atomically when accepting connections. */
+#cmakedefine01 HAVE_ACCEPT4
+
+#endif //EP_SHARED_CONFIG_H_INCLUDED