Merge pull request #1606 from ertankucukoglu/fix/search-temp-isolation

fix(platform): isolate concurrent temporary directories

src/foundation/compat.c

@@ -6,6 +6,7 @@
  */
 #include "foundation/compat.h"
 #include "foundation/constants.h"
+#include "foundation/secure_random.h"
 
 #include <errno.h>
 #include <stdlib.h>
@@ -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.

tests/test_platform.c

@@ -24,6 +24,7 @@
 #endif
 
 enum { PLATFORM_TIME_THREADS = 8 };
+enum { PLATFORM_MKDTEMP_THREADS = 8, PLATFORM_MKDTEMP_ITERATIONS = 32 };
 
 #include <stdio.h>
 #include <string.h>
@@ -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);