fix(loader): avoid background dlclose at shutdown

.gitlab/build-loader.sh

@@ -22,4 +22,13 @@ phpize
 ./configure
 make clean
 make -j "${MAKE_JOBS}" all ECHO_ARG="-e" CFLAGS="-std=gnu11 -O2 -g -Wall -Wextra -Werror -DPHP_DD_LIBRARY_LOADER_VERSION='\"$(cat ../VERSION)\"'"
+
+# The reaper is copied out of the loader. Reject accidental references to its
+# code/data, including compiler instrumentation, before packaging the library.
+REAPER_RELOCATIONS="$(LC_ALL=C objdump -r -j ddloader_reaper_code .libs/telemetry_reaper.o)"
+if [[ "${REAPER_RELOCATIONS}" == *"RELOCATION RECORDS"* ]]; then
+  printf 'Telemetry reaper must not contain relocations:\n%s\n' "${REAPER_RELOCATIONS}"
+  exit 1
+fi
+
 cp modules/dd_library_loader.so "../dd_library_loader-$(uname -m)-${HOST_OS}.so"

loader/config.m4

@@ -8,5 +8,5 @@ if test "$PHP_DD_LIBRARY_LOADER" != "no"; then
   dnl In case of no dependencies
   AC_DEFINE(HAVE_DD_LIBRARY_LOADER, 1, [ Have dd_library_loader support ])
 
-  PHP_NEW_EXTENSION(dd_library_loader, dd_library_loader.c dd_library_loader_module.c compat_php.c, $ext_shared, , , , yes)
+  PHP_NEW_EXTENSION(dd_library_loader, dd_library_loader.c dd_library_loader_module.c compat_php.c telemetry_reaper.c, $ext_shared, , , , yes)
 fi

loader/dd_library_loader.c

@@ -9,13 +9,14 @@
 #include <php_ini.h>
 #include <stdbool.h>
 #include <errno.h>
-#include <pthread.h>
+#include <signal.h>
 #include <sys/wait.h>
 #include <main/SAPI.h>
 #include <ext/standard/basic_functions.h>
 
 #include "compat_php.h"
 #include "php_dd_library_loader.h"
+#include "telemetry_reaper.h"
 
 #define MIN_API_VERSION 320151012
 #define MAX_API_VERSION 420250925
@@ -368,43 +369,9 @@ void ddloader_logf(injected_ext *config, log_level level, const char *format, ..
     va_end(va);
 }
 
-typedef struct {
-    pid_t pid;
-    void *self_handle; // dlopen handle for this .so, closed by the thread
-} ddloader_reaper_arg;
-
-// Reaps the telemetry child process, then tail-calls dlclose() to release the
-// extra reference on this .so that was acquired before thread creation.
-// The tail call ensures dlclose() returns directly to libpthread's start_thread
-// without ever returning into this .so's code, which may be unmapped when
-// dlclose() releases the last reference and runs munmap.
-//
-// [[clang::musttail]] guarantees the tail call at the source level (compile
-// error if not possible). __attribute__((optimize("O2"))) is the GCC fallback
-// to enable sibling-call optimisation.  Both are needed because musttail
-// requires a single CK_IntegralToPointer cast, while GCC -Wint-to-pointer-cast
-// requires the (intptr_t) intermediate; using __has_attribute lets us pick the
-// right form for each compiler.
-// Redeclare dlclose under a private name with void* return type so the tail
-// call is type-correct without any cast.  int and void* share the same return
-// register on all supported ABIs; the return value is discarded anyway.
-extern void *ddloader_dlclose(void *) __asm__("dlclose");
-
-#if defined(__has_attribute) && __has_attribute(musttail)
-# define DDLOADER_MUSTTAIL __attribute__((musttail))
-#elif defined(__clang__) && __clang_major__ >= 13
-# define DDLOADER_MUSTTAIL [[clang::musttail]]
-#else
-# define DDLOADER_MUSTTAIL
-__attribute__((optimize("O2")))
-#endif
-static void *ddloader_reap_child(void *arg_) {
-    ddloader_reaper_arg *arg = (ddloader_reaper_arg *)arg_;
-    pid_t pid = arg->pid;
-    void *handle = arg->self_handle;
-    free(arg);
-    waitpid(pid, NULL, 0);
-    DDLOADER_MUSTTAIL return ddloader_dlclose(handle);
+static void ddloader_wait_for_child(pid_t pid) {
+    while (waitpid(pid, NULL, 0) == -1 && errno == EINTR) {
+    }
 }
 
 /**
@@ -501,22 +468,16 @@ static void ddloader_telemetryf(telemetry_reason reason, injected_ext *config, c
         return;
     }
     if (pid > 0) {
-        // reap the child in a background thread to avoid leaking it
-        ddloader_reaper_arg *reaper_arg = malloc(sizeof(*reaper_arg));
-        reaper_arg->pid = pid;
-        // Bump our own refcount so this .so stays mapped while the reaper
-        // thread is running. The thread will tail-call dlclose() to release it.
-        Dl_info info;
-        reaper_arg->self_handle =
-            (dladdr((void *)ddloader_telemetryf, &info) && info.dli_fname)
-            ? dlopen(info.dli_fname, RTLD_LAZY)
-            : NULL;
-        pthread_t reaper;
-        pthread_attr_t attr;
-        pthread_attr_init(&attr);
-        pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-        pthread_create(&reaper, &attr, ddloader_reap_child, reaper_arg);
-        pthread_attr_destroy(&attr);
+        // The reaper owns an independent code page, allowing Zend to unload
+        // this DSO during shutdown without waiting for telemetry delivery.
+        int error_code = ddloader_reaper_start(pid);
+        if (error_code) {
+            LOG(config, ERROR, "Telemetry error: cannot start child reaper: %s", strerror(error_code))
+            // Do not let the forwarder block startup when no reaper thread is
+            // available. Reap it synchronously after forcing it to exit.
+            kill(pid, SIGKILL);
+            ddloader_wait_for_child(pid);
+        }
         return;  // parent
     }
 
@@ -609,7 +570,7 @@ static void ddloader_telemetryf(telemetry_reason reason, injected_ext *config, c
 
     // If execv failed, exit immediately
     // Return 127 for the most likely case of a missing file
-    exit(127);
+    _exit(127);
 }
 
 static char *ddloader_find_ext_path(const char *ext_dir, const char *ext_name, int module_api, bool is_zts, bool is_debug) {

loader/telemetry_reaper.c

@@ -0,0 +1,122 @@
+#include "telemetry_reaper.h"
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdint.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#if (!defined(__x86_64__) && !defined(__aarch64__)) || defined(__ILP32__)
+#error Unsupported architecture for the telemetry reaper
+#endif
+
+#if defined(__aarch64__) && defined(__ARM_FEATURE_BTI_DEFAULT)
+#include <asm/hwcap.h>
+#include <sys/auxv.h>
+
+// Compatibility with the CentOS 7 headers used for release builds.
+#ifndef HWCAP2_BTI
+#define HWCAP2_BTI (1UL << 17)
+#endif
+#ifndef PROT_BTI
+#define PROT_BTI 0x10
+#endif
+#endif
+
+// The context precedes the copied code; keep its entry point aligned.
+typedef struct __attribute__((aligned(16))) {
+    size_t mapping_size;
+    pid_t pid;
+    pid_t (*wait_for_child)(pid_t, int *, int);
+    int *(*error_location)(void);
+    int (*unmap)(void *, size_t);
+} ddloader_reaper_context;
+
+// Linker-provided bounds for the reaper code size.
+extern const char __start_ddloader_reaper_code[] __attribute__((visibility("hidden")));
+extern const char __stop_ddloader_reaper_code[] __attribute__((visibility("hidden")));
+
+#if __has_attribute(musttail)
+#define DDLOADER_MUSTTAIL __attribute__((musttail))
+#elif defined(__clang__) && __clang_major__ >= 13
+#define DDLOADER_MUSTTAIL [[clang::musttail]]
+#else
+#define DDLOADER_MUSTTAIL
+#endif
+
+// No code or data reference may point back into the loader, including compiler
+// instrumentation. All libc calls go through pointers in the copied context.
+// The release build checks that this section contains no relocations.
+__attribute__((section("ddloader_reaper_code"), used, noinline, no_instrument_function,
+               no_profile_instrument_function))
+#if defined(__clang__)
+// no_sanitize covers frontend checks; disabling instrumentation also removes
+// TSan's function entry/exit hooks, which otherwise survive no_sanitize("all").
+__attribute__((no_stack_protector, no_sanitize("all"), disable_sanitizer_instrumentation))
+#else
+// Older GCC has no musttail attribute; force sibling calls even in debug builds.
+__attribute__((no_sanitize_address, no_sanitize_thread, no_sanitize_undefined,
+               optimize("O2", "optimize-sibling-calls", "no-stack-protector")))
+#endif
+static int ddloader_reap_child(void *mapping, size_t unused) {
+    (void)unused;
+    ddloader_reaper_context *context = mapping;
+    while (context->wait_for_child(context->pid, NULL, 0) == -1 &&
+           *context->error_location() == EINTR) {
+    }
+    // Match munmap's signature so musttail can guarantee that it returns straight
+    // to pthread's startup routine, never into the page it has just unmapped.
+    DDLOADER_MUSTTAIL return context->unmap(mapping, context->mapping_size);
+}
+
+int ddloader_reaper_start(pid_t pid) {
+    size_t code_size = (uintptr_t)__stop_ddloader_reaper_code - (uintptr_t)__start_ddloader_reaper_code;
+    size_t entry_offset = (uintptr_t)ddloader_reap_child - (uintptr_t)__start_ddloader_reaper_code;
+    long page_size = sysconf(_SC_PAGESIZE);
+    if (page_size <= 0 || sizeof(ddloader_reaper_context) + code_size > (size_t)page_size) {
+        return EINVAL;
+    }
+    size_t mapping_size = (size_t)page_size;
+    ddloader_reaper_context *context = mmap(NULL, mapping_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (context == MAP_FAILED) {
+        return errno;
+    }
+    // Taking libc function addresses resolves them before this DSO can unload;
+    // the copied function must not use the loader's PLT or GOT, even for errno.
+    *context = (ddloader_reaper_context){mapping_size, pid, waitpid, __errno_location, munmap};
+    void *code = context + 1;
+    memcpy(code, __start_ddloader_reaper_code, code_size);
+    __builtin___clear_cache(code, (char *)code + code_size);
+
+    int protection = PROT_READ | PROT_EXEC;
+#if defined(__aarch64__) && defined(__ARM_FEATURE_BTI_DEFAULT)
+    // Only enable BTI when the compiler emitted its landing pad in the copy.
+    if (getauxval(AT_HWCAP2) & HWCAP2_BTI) {
+        protection |= PROT_BTI;
+    }
+#endif
+    if (mprotect(context, mapping_size, protection)) {
+        int error = errno;
+        munmap(context, mapping_size);
+        return error;
+    }
+
+    pthread_attr_t attributes;
+    int error = pthread_attr_init(&attributes);
+    if (!error) {
+        error = pthread_attr_setdetachstate(&attributes, PTHREAD_CREATE_DETACHED);
+        if (!error) {
+            pthread_t thread;
+            // On the supported 64-bit ABIs the unused second argument needs no
+            // initialization, and a detached thread's return value is discarded.
+            error = pthread_create(&thread, &attributes, (void *(*)(void *))((char *)code + entry_offset), context);
+        }
+        pthread_attr_destroy(&attributes);
+    }
+    if (error) {
+        munmap(context, mapping_size);
+    }
+    return error;
+}

loader/telemetry_reaper.h

@@ -0,0 +1,10 @@
+#ifndef DDLOADER_TELEMETRY_REAPER_H
+#define DDLOADER_TELEMETRY_REAPER_H
+
+#include <sys/types.h>
+
+// Start a detached reaper that can outlive the loader and frees its own code.
+// Returns 0 on success, or an error number; on failure the caller must reap pid.
+int ddloader_reaper_start(pid_t pid);
+
+#endif

loader/tests/functional/fixtures/gated_forwarder.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+exec </dev/null >/dev/null 2>&1
+
+# Bound the wait even if the test runner is killed before releasing the gate.
+for ((i = 0; i < 3000; ++i)); do
+    if [[ -e "${FAKE_FORWARDER_RELEASE_PATH}" ]]; then
+        echo "${*:2}" >> "${FAKE_FORWARDER_LOG_PATH}"
+        exit 0
+    fi
+    sleep 0.01
+done
+exit 1

loader/tests/functional/fixtures/reaper_shutdown.c

@@ -0,0 +1,115 @@
+#define _GNU_SOURCE
+#include <dirent.h>
+#include <dlfcn.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <stdatomic.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/wait.h>
+#include <time.h>
+#include <unistd.h>
+
+static pid_t php_pid;
+static pthread_t php_thread;
+static char *exit_path;
+static atomic_int exiting;
+static atomic_int background_dlclose;
+
+static void check_reapers_at_exit(void);
+static int thread_count(void);
+static void mark_exit(const char *state);
+static void fail(const char *message);
+
+void *dlopen(const char *filename, int flags) {
+    // PHP's DEEPBIND would otherwise hide the loader's calls from our dlclose hook.
+    if (filename) {
+        const char *name = strrchr(filename, '/');
+        if (!strcmp(name ? name + 1 : filename, "dd_library_loader.so")) {
+            flags &= ~RTLD_DEEPBIND;
+        }
+    }
+    void *(*open_library)(const char *, int) =
+        (void *(*)(const char *, int))dlsym(RTLD_NEXT, "dlopen");
+    if (!open_library) fail("Cannot resolve libc dlopen\n");
+    return open_library(filename, flags);
+}
+
+int dlclose(void *handle) {
+    if (getpid() == php_pid && atomic_load(&exiting) &&
+        !pthread_equal(pthread_self(), php_thread)) {
+        // Observe the unsafe overlap without letting it corrupt destructor state.
+        atomic_store(&background_dlclose, 1);
+        return 0;
+    }
+    int (*close_library)(void *) = (int (*)(void *))dlsym(RTLD_NEXT, "dlclose");
+    if (!close_library) fail("Cannot resolve libc dlclose\n");
+    return close_library(handle);
+}
+
+__attribute__((constructor)) static void register_exit_check(void) {
+    const char *expected_pid = getenv("DD_REAPER_TEST_PID");
+    // The forwarder inherits LD_PRELOAD, but only the PHP process owns this check.
+    if (!expected_pid || getpid() != (pid_t)strtol(expected_pid, NULL, 10)) return;
+    php_pid = getpid();
+    php_thread = pthread_self();
+    // PHP can tear down its environment before libc starts running exit handlers.
+    const char *path = getenv("DD_REAPER_TEST_EXIT_PATH");
+    if (!path || !(exit_path = strdup(path))) fail("Cannot save exit marker path\n");
+    if (atexit(check_reapers_at_exit)) fail("Cannot register exit check\n");
+}
+
+static void check_reapers_at_exit(void) {
+    if (getpid() != php_pid) return;
+    if (thread_count() <= 1) fail("No telemetry reapers active at process exit\n");
+
+    // PHP has shut down its modules. Tell the test it can release the forwarders,
+    // and keep this real exit handler active until their reaper threads finish.
+    atomic_store(&exiting, 1);
+    mark_exit("entered\n");
+    struct timespec start, current;
+    if (clock_gettime(CLOCK_MONOTONIC, &start)) fail("Cannot read clock\n");
+    while (thread_count() > 1) {
+        if (clock_gettime(CLOCK_MONOTONIC, &current)) fail("Cannot read clock\n");
+        if (current.tv_sec - start.tv_sec >= 5) fail("Telemetry reapers did not finish\n");
+        struct timespec delay = {0, 1000000};
+        nanosleep(&delay, NULL);
+    }
+    if (atomic_load(&background_dlclose)) {
+        fail("Telemetry reaper called dlclose while an atexit handler was running\n");
+    }
+    // Thread exit alone is insufficient: every telemetry child must be reaped.
+    siginfo_t child;
+    if (waitid(P_ALL, 0, &child, WEXITED | WNOHANG | WNOWAIT) != -1 || errno != ECHILD) {
+        fail("Telemetry children remain after their reapers exited\n");
+    }
+    mark_exit("passed\n");
+    free(exit_path);
+}
+
+static int thread_count(void) {
+    DIR *tasks = opendir("/proc/self/task");
+    if (!tasks) fail("Cannot inspect PHP threads\n");
+    int count = 0;
+    struct dirent *task;
+    while ((task = readdir(tasks))) {
+        if (task->d_name[0] != '.') ++count;
+    }
+    closedir(tasks);
+    return count;
+}
+
+static void mark_exit(const char *state) {
+    int fd = open(exit_path, O_WRONLY | O_CREAT | O_TRUNC, 0600);
+    if (fd < 0) fail("Cannot open exit marker\n");
+    size_t length = strlen(state);
+    if (write(fd, state, length) != (ssize_t)length) fail("Cannot write exit marker\n");
+    if (close(fd)) fail("Cannot close exit marker\n");
+}
+
+static void fail(const char *message) {
+    ssize_t written = write(STDERR_FILENO, message, strlen(message));
+    (void)written;
+    _exit(86);
+}