Merge pull request #2275 from DeusData/fix/win-endpoint-dir-create-race

fix(daemon): losing the runtime-directory creation race is not a failure on Windows

src/daemon/ipc.c

@@ -214,6 +214,31 @@ static void ipc_startup_gate_run(void) {
 }
 #endif
 
+#ifdef _WIN32
+static cbm_daemon_ipc_win_directory_create_hook_fn g_win_directory_create_hook_for_test;
+static void *g_win_directory_create_hook_context_for_test;
+#endif
+
+void cbm_daemon_ipc_win_directory_create_hook_set_for_test(
+    cbm_daemon_ipc_win_directory_create_hook_fn hook, void *context) {
+#ifdef _WIN32
+    g_win_directory_create_hook_context_for_test = context;
+    g_win_directory_create_hook_for_test = hook;
+#else
+    (void)hook;
+    (void)context;
+#endif
+}
+
+#ifdef _WIN32
+static void ipc_win_directory_create_hook_run(const wchar_t *path) {
+    cbm_daemon_ipc_win_directory_create_hook_fn hook = g_win_directory_create_hook_for_test;
+    if (hook) {
+        hook(path, g_win_directory_create_hook_context_for_test);
+    }
+}
+#endif
+
 bool cbm_daemon_ipc_windows_legacy_names(const char *canonical_runtime_parent,
                                          const char *instance_key,
                                          char pipe_out[CBM_DAEMON_IPC_WINDOWS_NAME_CAP],
@@ -3994,6 +4019,16 @@ static char *wide_to_utf8(const wchar_t *value) {
     return utf8;
 }
 
+/* Record "<path>: <message>" as the validation detail, with the wide path
+ * rendered as UTF-8. One place owns the conversion buffer, so every refusal on
+ * the directory walk can name its component without each call site carrying
+ * its own allocation. */
+static void ipc_validation_detail_set_for_path(const wchar_t *path, const char *message) {
+    char *path_utf8 = wide_to_utf8(path);
+    ipc_validation_detail_set("%s: %s", path_utf8 ? path_utf8 : "<path>", message);
+    free(path_utf8);
+}
+
 static wchar_t *wide_copy(const wchar_t *value) {
     if (!value) {
         return NULL;
@@ -4861,13 +4896,28 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) {
         return false;
     }
     bool created = CreateDirectoryW(runtime_dir, &security.directory_attributes) != 0;
-    if (!created && GetLastError() != ERROR_ALREADY_EXISTS) {
+    DWORD create_error = created ? ERROR_SUCCESS : GetLastError();
+    if (!created && create_error != ERROR_ALREADY_EXISTS) {
+        char message[96];
+        (void)snprintf(message, sizeof(message),
+                       "could not create the directory (Windows error %lu)",
+                       (unsigned long)create_error);
+        ipc_validation_detail_set_for_path(runtime_dir, message);
         win_security_destroy(&security);
         return false;
     }
     DWORD attributes = GetFileAttributesW(runtime_dir);
     if (attributes == INVALID_FILE_ATTRIBUTES || (attributes & FILE_ATTRIBUTE_DIRECTORY) == 0 ||
         (attributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) {
+        /* What already exists at this path is adopted only if it is a plain
+         * directory. Say which rule refused it: the walk in front of this
+         * function tolerates losing a creation race, so this check is what
+         * stands between that tolerance and a planted file or junction. */
+        const char *rule = attributes == INVALID_FILE_ATTRIBUTES ? "cannot be inspected"
+                           : (attributes & FILE_ATTRIBUTE_DIRECTORY) == 0
+                               ? "exists but is not a directory"
+                               : "is a reparse point (junction or symlink)";
+        ipc_validation_detail_set_for_path(runtime_dir, rule);
         win_security_destroy(&security);
         return false;
     }
@@ -5021,8 +5071,35 @@ static bool win_private_directory_tree_secure(const wchar_t *directory_path) {
             DWORD attributes = GetFileAttributesW(path);
             if (attributes == INVALID_FILE_ATTRIBUTES) {
                 DWORD error = GetLastError();
-                ok = (error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND) &&
-                     CreateDirectoryW(path, &security.directory_attributes) != 0;
+                bool absent = error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND;
+                if (absent) {
+                    ipc_win_directory_create_hook_run(path);
+                    /* Several processes first-starting together all observe
+                     * this component absent; one CreateDirectoryW wins and
+                     * the rest get ERROR_ALREADY_EXISTS. Losing that race is
+                     * not a failure -- the directory this process wanted now
+                     * exists, exactly as if it had been there all along. It
+                     * is NOT trusted for that: an ancestor still goes through
+                     * win_directory_component_secure() below, and the final
+                     * component through win_runtime_directory_secure(), which
+                     * refuses a non-directory or reparse point and enforces
+                     * owner and DACL. The POSIX walk tolerates EEXIST at the
+                     * same point for the same reason. */
+                    if (CreateDirectoryW(path, &security.directory_attributes) == 0) {
+                        error = GetLastError();
+                        absent = error == ERROR_ALREADY_EXISTS;
+                    }
+                }
+                ok = absent;
+                if (!ok) {
+                    /* Never a bare "(endpoint)": name the component and the
+                     * Windows error that refused it. */
+                    char message[96];
+                    (void)snprintf(message, sizeof(message),
+                                   "could not create or inspect the directory (Windows error %lu)",
+                                   (unsigned long)error);
+                    ipc_validation_detail_set_for_path(path, message);
+                }
             }
             /* Ancestors are observe-only and must already be secure.  The
              * final current-user directory is intentionally handled below by
@@ -5034,10 +5111,7 @@ static bool win_private_directory_tree_secure(const wchar_t *directory_path) {
                      * at this walk position; the helper set the inner rule. */
                     char inner[384];
                     (void)snprintf(inner, sizeof(inner), "%s", ipc_validation_detail_buffer);
-                    char *component_utf8 = wide_to_utf8(path);
-                    ipc_validation_detail_set(
-                        "%s: %s", component_utf8 ? component_utf8 : "<component>", inner);
-                    free(component_utf8);
+                    ipc_validation_detail_set_for_path(path, inner);
                 }
             }
         }

src/daemon/ipc_internal.h

@@ -110,4 +110,15 @@ void cbm_daemon_ipc_posix_record_write_failure_set_for_test(int errno_value);
 typedef void (*cbm_daemon_ipc_startup_gate_fn)(void *context);
 void cbm_daemon_ipc_startup_gate_set_for_test(cbm_daemon_ipc_startup_gate_fn gate, void *context);
 
+/* Deterministic-interleaving seam for the Windows private-directory walk:
+ * fires after a path component was observed ABSENT and before this process
+ * tries to create it. A test plays the concurrent process that wins that
+ * creation, which pins the cold-start race (N processes first-starting
+ * against a runtime directory that does not exist yet) by construction
+ * instead of by thread timing. `path` is the NUL-terminated component path at
+ * the walk position. No-op off Windows. */
+typedef void (*cbm_daemon_ipc_win_directory_create_hook_fn)(const wchar_t *path, void *context);
+void cbm_daemon_ipc_win_directory_create_hook_set_for_test(
+    cbm_daemon_ipc_win_directory_create_hook_fn hook, void *context);
+
 #endif /* CBM_DAEMON_IPC_INTERNAL_H */

tests/test_daemon_ipc.c

@@ -882,6 +882,129 @@ TEST(daemon_ipc_windows_private_directory_ace_is_inheritable) {
     PASS();
 }
 
+typedef struct {
+    int calls;
+    bool plant_file; /* false: play a racer that creates the DIRECTORY first */
+    bool planted;
+} ipc_test_win_create_racer_t;
+
+/* Plays the concurrent process that wins the creation of a path component the
+ * walk has just observed absent. Runs inside the walk, at the exact point the
+ * race lives, so the interleaving is pinned by construction: no threads, no
+ * timing. */
+static void ipc_test_win_create_racer(const wchar_t *path, void *context) {
+    ipc_test_win_create_racer_t *racer = context;
+    racer->calls++;
+    if (racer->calls != 1) {
+        return;
+    }
+    if (racer->plant_file) {
+        HANDLE file =
+            CreateFileW(path, GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL);
+        racer->planted = file != INVALID_HANDLE_VALUE;
+        if (racer->planted) {
+            (void)CloseHandle(file);
+        }
+    } else {
+        racer->planted = CreateDirectoryW(path, NULL) != 0;
+    }
+}
+
+/* Cold-start race (test-windows-guards section_cold_storm, 2026-09-21; the
+ * same thing a user gets when a host launches several MCP servers on its very
+ * first run): N processes first-start against a runtime directory that does
+ * not exist yet. All of them observe the final component absent, one
+ * CreateDirectoryW wins, and every other process gets ERROR_ALREADY_EXISTS.
+ * The walk treated that as a failure and recorded no detail, so the losers
+ * exited with a bare "secure CLI coordination could not be created
+ * (endpoint)". Losing the creation race is not a failure: the directory this
+ * process wanted now exists, and the owner / DACL / not-a-reparse-point checks
+ * that follow are exactly the ones a pre-existing directory gets. The POSIX
+ * walk has always tolerated EEXIST here. */
+TEST(daemon_ipc_windows_private_directory_survives_lost_creation_race) {
+    char parent[TEST_PATH_CAP] = {0};
+    char cache[TEST_PATH_CAP] = {0};
+    bool paths_ok = false;
+    bool secured = false;
+    bool owner_only = false;
+    ipc_test_win_create_racer_t racer = {0};
+
+    if (ipc_test_parent_new(parent, "win-create-race")) {
+        int written = snprintf(cache, sizeof(cache), "%s/cache", parent);
+        paths_ok = written > 0 && written < (int)sizeof(cache);
+    }
+    if (paths_ok) {
+        cbm_daemon_ipc_win_directory_create_hook_set_for_test(ipc_test_win_create_racer, &racer);
+        secured = cbm_daemon_ipc_private_directory_secure(cache);
+        cbm_daemon_ipc_win_directory_create_hook_set_for_test(NULL, NULL);
+    }
+    if (secured) {
+        /* The racer created the directory with an inherited DACL; the winner's
+         * directory must still end up owner-only, like any adopted one. */
+        PACL dacl = NULL;
+        PSECURITY_DESCRIPTOR descriptor = NULL;
+        ACL_SIZE_INFORMATION information;
+        memset(&information, 0, sizeof(information));
+        if (GetNamedSecurityInfoA(cache, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL,
+                                  &dacl, NULL, &descriptor) == ERROR_SUCCESS &&
+            dacl &&
+            GetAclInformation(dacl, &information, sizeof(information), AclSizeInformation)) {
+            owner_only = information.AceCount == 1;
+        }
+        if (descriptor) {
+            (void)LocalFree(descriptor);
+        }
+    }
+
+    (void)RemoveDirectoryA(cache);
+    ipc_test_remove_flat_dir(parent);
+
+    ASSERT_TRUE(paths_ok);
+    /* The seam sat on the path and the racer really won: without these two the
+     * test could pass without ever exercising the race. */
+    ASSERT_EQ(racer.calls, 1);
+    ASSERT_TRUE(racer.planted);
+    ASSERT_TRUE(secured);
+    ASSERT_TRUE(owner_only);
+    PASS();
+}
+
+/* The tolerance above must not admit anything: when what appeared at the path
+ * is NOT a directory, the walk still refuses -- and it says why. A bare
+ * "(endpoint)" is what four reporters in #1533/#1574 were left with; every
+ * refusal on this path names the component and the rule. */
+TEST(daemon_ipc_windows_private_directory_refuses_and_names_a_planted_file) {
+    char parent[TEST_PATH_CAP] = {0};
+    char cache[TEST_PATH_CAP] = {0};
+    char detail[512] = {0};
+    bool paths_ok = false;
+    bool refused = false;
+    ipc_test_win_create_racer_t racer = {.plant_file = true};
+
+    if (ipc_test_parent_new(parent, "win-create-race-file")) {
+        int written = snprintf(cache, sizeof(cache), "%s/cache", parent);
+        paths_ok = written > 0 && written < (int)sizeof(cache);
+    }
+    if (paths_ok) {
+        cbm_daemon_ipc_win_directory_create_hook_set_for_test(ipc_test_win_create_racer, &racer);
+        refused = !cbm_daemon_ipc_private_directory_secure(cache);
+        cbm_daemon_ipc_win_directory_create_hook_set_for_test(NULL, NULL);
+        const char *why = cbm_daemon_ipc_validation_detail();
+        (void)snprintf(detail, sizeof(detail), "%s", why ? why : "");
+    }
+
+    (void)DeleteFileA(cache);
+    ipc_test_remove_flat_dir(parent);
+
+    ASSERT_TRUE(paths_ok);
+    ASSERT_EQ(racer.calls, 1);
+    ASSERT_TRUE(racer.planted);
+    ASSERT_TRUE(refused);
+    ASSERT_NOT_NULL(strstr(detail, "cache"));
+    ASSERT_NOT_NULL(strstr(detail, "not a directory"));
+    PASS();
+}
+
 TEST(daemon_ipc_windows_private_directory_allows_add_subdirectory_only_ancestor) {
     char parent[TEST_PATH_CAP] = {0};
     char add_only[TEST_PATH_CAP] = {0};
@@ -5361,6 +5484,8 @@ SUITE(daemon_ipc) {
     RUN_TEST(daemon_ipc_windows_default_endpoint_ignores_temp_environment);
     RUN_TEST(daemon_ipc_windows_private_directory_rejects_untrusted_ancestor_acl);
     RUN_TEST(daemon_ipc_windows_private_directory_ace_is_inheritable);
+    RUN_TEST(daemon_ipc_windows_private_directory_survives_lost_creation_race);
+    RUN_TEST(daemon_ipc_windows_private_directory_refuses_and_names_a_planted_file);
     RUN_TEST(daemon_ipc_windows_private_directory_allows_add_subdirectory_only_ancestor);
     RUN_TEST(daemon_ipc_windows_sid_trust_accepts_local_admin_rejects_foreign_500);
     RUN_TEST(daemon_ipc_windows_legacy_bridge_covers_handoff_and_lifetime);