diff --git a/src/testsuite_support/mkdtemp_clean.c b/src/testsuite_support/mkdtemp_clean.c index ad7729ff..f0d83b62 100644 --- a/src/testsuite_support/mkdtemp_clean.c +++ b/src/testsuite_support/mkdtemp_clean.c @@ -11,19 +11,28 @@ * This file is the fix at the source. It provides a replacement `mkdtemp()` * that is linked into every unit-test executable in this project (via a * declare_dependency() appended to each testsuite's shared *_test_deps list, - * see controller/meson.build and scheduler/testsuite/meson.build). Every - * scratch dir it creates is registered and recursively removed by an - * atexit() handler, so a test binary leaves /tmp clean regardless of which - * test "owns" the dir. Cleanup runs at process exit (after ALL TEST_ - * functions), so tests that legitimately inspect a fixture's on-disk - * contents across multiple TEST functions in the same file are unaffected -- - * the very property that made retrofitting cleanup into 70+ individual test - * files invasive. + * see controller/meson.build and scheduler/testsuite/meson.build). Each + * scratch dir it creates is registered with the pid of the process that + * created it and removed at process exit (atexit) -- but ONLY by the process + * that created it. + * + * Per-entry owner pid is what makes this fork-safe without any pthread_atfork + * hook: atexit() handlers are inherited across fork(), so a forked child that + * later calls exit() would otherwise run the cleanup and delete every dir the + * PARENT registered -- including the dir the repl/sim suites' real-stdout- + * capture helper just wrote its capture file into, so the parent's subsequent + * fopen() sees ENOENT ("Expected Non-NULL" cascade). Filtering cleanup to + * entries whose owner pid == getpid() means a child's inherited atexit removes + * nothing the parent owns (parent pid != child pid), while dirs the child + * itself mkdtemp()s after the fork are owned by the child and cleaned at its + * own exit. No fork hooks are registered, so forking tests are completely + * unaffected (a global pthread_atfork was tried and regressed the HA/commit + * suites that fork heavily). * * Contract is glibc mkdtemp's: `tmpl` must end in XXXXXX (overwritten with a * unique directory name); returns `tmpl` on success, NULL on error. If the - * dir was subsequently removed by the test itself, the atexit sweep is a - * harmless no-op (rmdir fails with ENOENT). + * dir was subsequently removed by the test itself, the sweep is a harmless + * no-op (rmdir fails with ENOENT). */ #define _GNU_SOURCE #include @@ -34,14 +43,20 @@ #include #include #include +#include #include #include #define MKDTEMP_SUFFIX "XXXXXX" #define MKDTEMP_TRIES 1000 +typedef struct mkd_entry { + char* path; + pid_t owner; +} mkd_entry_t; + static pthread_mutex_t g_clean_mu = PTHREAD_MUTEX_INITIALIZER; -static char** g_clean_dirs = NULL; +static mkd_entry_t* g_clean_dirs = NULL; static size_t g_clean_count = 0; static size_t g_clean_capacity = 0; static int g_clean_armed = 0; @@ -78,29 +93,34 @@ static void rmrf(const char* path) rmdir(path); } +/* Remove every registered dir owned by THIS process. Forks inherit a copy of + * the parent's registry, but every entry carries the pid that created it, so + * a child's inherited atexit removes nothing the parent owns. */ static void mkdtemp_cleanup_all(void) { + pid_t me = getpid(); size_t i; pthread_mutex_lock(&g_clean_mu); for (i = 0; i < g_clean_count; i++) { - if (g_clean_dirs[i] != NULL) { - rmrf(g_clean_dirs[i]); - free(g_clean_dirs[i]); - g_clean_dirs[i] = NULL; + if (g_clean_dirs[i].path != NULL && g_clean_dirs[i].owner == me) { + rmrf(g_clean_dirs[i].path); + free(g_clean_dirs[i].path); + g_clean_dirs[i].path = NULL; + g_clean_dirs[i].owner = 0; } } - g_clean_count = 0; pthread_mutex_unlock(&g_clean_mu); } /* Own a copy of `path` (the caller's tmpl buffer is reused across mkdtemp - * calls) and arrange for it to be removed at process exit (atexit). */ + * calls), tagged with the creating pid, and arrange for it to be removed at + * process exit by that same pid. */ static void track_dir(const char* path) { - char* copy; - size_t ncap; - char** ndirs; + char* copy; + size_t ncap; + mkd_entry_t* ndirs; copy = strdup(path); if (copy == NULL) { @@ -113,7 +133,7 @@ static void track_dir(const char* path) } if (g_clean_count == g_clean_capacity) { ncap = g_clean_capacity ? g_clean_capacity * 2 : 64; - ndirs = (char**)realloc(g_clean_dirs, ncap * sizeof(*ndirs)); + ndirs = (mkd_entry_t*)realloc(g_clean_dirs, ncap * sizeof(*ndirs)); if (ndirs == NULL) { pthread_mutex_unlock(&g_clean_mu); free(copy); @@ -122,7 +142,9 @@ static void track_dir(const char* path) g_clean_dirs = ndirs; g_clean_capacity = ncap; } - g_clean_dirs[g_clean_count++] = copy; + g_clean_dirs[g_clean_count].path = copy; + g_clean_dirs[g_clean_count].owner = getpid(); + g_clean_count++; pthread_mutex_unlock(&g_clean_mu); }