From d0cc55ec283def1186ac66900e12aa4db01140b4 Mon Sep 17 00:00:00 2001 From: Ertan Date: Thu, 13 Aug 2026 20:45:08 +0300 Subject: [PATCH] fix(platform): isolate concurrent temporary directories Signed-off-by: Ertan --- src/foundation/compat.c | 87 +++++++++++++++++++++++++++-------------- tests/test_platform.c | 74 +++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 29 deletions(-) diff --git a/src/foundation/compat.c b/src/foundation/compat.c index 2ebddb13a..f0c4d3571 100644 --- a/src/foundation/compat.c +++ b/src/foundation/compat.c @@ -6,6 +6,7 @@ */ #include "foundation/compat.h" #include "foundation/constants.h" +#include "foundation/secure_random.h" #include #include @@ -114,52 +115,80 @@ static bool win_mkdtemp_private_create(const char *path) { } char *cbm_mkdtemp(char *tmpl) { - /* Build path in static buffer, then copy back to caller. - * Callers must provide buffers >= CBM_SZ_256 bytes (all test code does). */ - static char buf[CBM_SZ_512]; + /* Per-call storage is required: daemon sessions invoke mkdtemp concurrently. + * A process-global buffer lets one request overwrite another request's path + * between expansion, creation, and the copy back to its caller. */ + char buf[CBM_SZ_512]; + int written; if (strncmp(tmpl, "/tmp/", 5) == 0) { const char *tmp = getenv("TEMP"); if (!tmp) tmp = getenv("TMP"); if (!tmp) tmp = "."; - snprintf(buf, sizeof(buf), "%s\\%s", tmp, tmpl + 5); + written = snprintf(buf, sizeof(buf), "%s\\%s", tmp, tmpl + 5); } else { - snprintf(buf, sizeof(buf), "%s", tmpl); + written = snprintf(buf, sizeof(buf), "%s", tmpl); } - /* Wide-API template expansion: the ANSI CRT interprets the UTF-8 bytes of - * non-ASCII cache/temp components in the local codepage and fails. */ - wchar_t *wide_template = cbm_utf8_to_wide(buf); - if (!wide_template || !_wmktemp(wide_template)) { - free(wide_template); + if (written < 0 || (size_t)written >= sizeof(buf)) { + errno = ENAMETOOLONG; return NULL; } - char *expanded = cbm_wide_to_utf8(wide_template); - free(wide_template); - if (!expanded || strlen(expanded) >= sizeof(buf)) { - free(expanded); + + size_t length = strlen(buf); + if (length < 6 || strcmp(buf + length - 6, "XXXXXX") != 0) { + errno = EINVAL; return NULL; } - strcpy(buf, expanded); - free(expanded); - if (!win_mkdtemp_private_create(buf)) { - /* One-time note: every private-namespace validation downstream - * depends on the explicit descriptor, so a silent fallback turns - * into unexplained owner/DACL refusals far from this call site. */ - static bool fallback_reported; + + static const char alphabet[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + bool created = false; + for (int attempt = 0; attempt < 128; attempt++) { + unsigned char random_suffix[6]; + if (!cbm_secure_random(random_suffix, sizeof(random_suffix))) { + errno = EIO; + return NULL; + } + for (size_t index = 0; index < sizeof(random_suffix); index++) { + buf[length - sizeof(random_suffix) + index] = + alphabet[random_suffix[index] % (sizeof(alphabet) - 1)]; + } + + if (win_mkdtemp_private_create(buf)) { + created = true; + break; + } + + /* Keep the existing compatibility fallback when an explicit private + * descriptor is unavailable. A name collision is retried; any other + * filesystem refusal is returned to the caller immediately. */ DWORD create_error = GetLastError(); wchar_t *wide_directory = cbm_utf8_to_wide(buf); + errno = 0; int mkdir_result = wide_directory ? _wmkdir(wide_directory) : -1; + int mkdir_error = errno; free(wide_directory); - if (mkdir_result != 0) - return NULL; - if (!fallback_reported) { - fallback_reported = true; - (void)fprintf(stderr, - "warning: private temp-directory descriptor unavailable " - "(os %lu); using default directory security\n", - (unsigned long)create_error); + if (mkdir_result == 0) { + static volatile LONG fallback_reported; + if (InterlockedCompareExchange(&fallback_reported, 1, 0) == 0) { + (void)fprintf(stderr, + "warning: private temp-directory descriptor unavailable " + "(os %lu); using default directory security\n", + (unsigned long)create_error); + } + created = true; + break; } + if (create_error == ERROR_ALREADY_EXISTS || create_error == ERROR_FILE_EXISTS || + mkdir_error == EEXIST) { + continue; + } + errno = mkdir_error != 0 ? mkdir_error : EACCES; + return NULL; + } + if (!created) { + errno = EEXIST; + return NULL; } /* Normalize to forward slashes. Callers embed this path in JSON repo_path * (where "\t"/"\a" are invalid escapes → index fails) and pass it to git -C. diff --git a/tests/test_platform.c b/tests/test_platform.c index d705c672f..1b55df699 100644 --- a/tests/test_platform.c +++ b/tests/test_platform.c @@ -24,6 +24,7 @@ #endif enum { PLATFORM_TIME_THREADS = 8 }; +enum { PLATFORM_MKDTEMP_THREADS = 8, PLATFORM_MKDTEMP_ITERATIONS = 32 }; #include #include @@ -102,6 +103,78 @@ TEST(platform_mkstemp_and_mkdtemp_survive_non_ascii_directory) { PASS(); } +typedef struct { + atomic_int *ready; + atomic_bool *go; + int worker_index; + int created; + bool ok; + char paths[PLATFORM_MKDTEMP_ITERATIONS][CBM_SZ_512]; +} platform_mkdtemp_worker_t; + +static void *platform_mkdtemp_concurrent_worker(void *opaque) { + platform_mkdtemp_worker_t *worker = opaque; + (void)atomic_fetch_add_explicit(worker->ready, 1, memory_order_acq_rel); + while (!atomic_load_explicit(worker->go, memory_order_acquire)) { + atomic_signal_fence(memory_order_seq_cst); + } + worker->ok = true; + for (int index = 0; index < PLATFORM_MKDTEMP_ITERATIONS; index++) { + int written = + snprintf(worker->paths[index], sizeof(worker->paths[index]), + "/tmp/cbm-mkdtemp-concurrent-%d-%d-XXXXXX", worker->worker_index, index); + if (written <= 0 || written >= (int)sizeof(worker->paths[index]) || + !cbm_mkdtemp(worker->paths[index])) { + worker->ok = false; + break; + } + worker->created++; + } + return NULL; +} + +/* MCP daemon sessions can enter cbm_mkdtemp simultaneously. Every request must + * retain its own expanded path and create a distinct directory. */ +TEST(platform_mkdtemp_is_thread_safe) { + cbm_thread_t threads[PLATFORM_MKDTEMP_THREADS]; + platform_mkdtemp_worker_t workers[PLATFORM_MKDTEMP_THREADS] = {0}; + atomic_int ready; + atomic_bool go; + atomic_init(&ready, 0); + atomic_init(&go, false); + + int started = 0; + for (; started < PLATFORM_MKDTEMP_THREADS; started++) { + workers[started].ready = &ready; + workers[started].go = &go; + workers[started].worker_index = started; + if (cbm_thread_create(&threads[started], 0, platform_mkdtemp_concurrent_worker, + &workers[started]) != 0) { + break; + } + } + while (started == PLATFORM_MKDTEMP_THREADS && + atomic_load_explicit(&ready, memory_order_acquire) != PLATFORM_MKDTEMP_THREADS) { + atomic_signal_fence(memory_order_seq_cst); + } + atomic_store_explicit(&go, true, memory_order_release); + for (int index = 0; index < started; index++) { + (void)cbm_thread_join(&threads[index]); + } + + bool all_ok = started == PLATFORM_MKDTEMP_THREADS; + for (int index = 0; index < started; index++) { + all_ok = + all_ok && workers[index].ok && workers[index].created == PLATFORM_MKDTEMP_ITERATIONS; + for (int path_index = 0; path_index < workers[index].created; path_index++) { + all_ok = all_ok && cbm_is_dir(workers[index].paths[path_index]); + (void)cbm_rmdir(workers[index].paths[path_index]); + } + } + ASSERT_TRUE(all_ok); + PASS(); +} + TEST(platform_counter_scaling_avoids_intermediate_overflow) { const uint64_t frequency = UINT64_C(10000000); const uint64_t counter = UINT64_C(20000000000); @@ -613,6 +686,7 @@ TEST(cgroup_no_mem_files) { SUITE(platform) { RUN_TEST(platform_file_apis_survive_max_path_overflow); RUN_TEST(platform_mkstemp_and_mkdtemp_survive_non_ascii_directory); + RUN_TEST(platform_mkdtemp_is_thread_safe); RUN_TEST(platform_counter_scaling_avoids_intermediate_overflow); RUN_TEST(platform_counter_scaling_preserves_monotonic_deadlines); RUN_TEST(platform_now_ns_concurrent_first_call);