Merge pull request #1880 from CaptainMittens/fix/env-int-parse-strict

fix(env): stop an unreadable environment setting from picking a wrong default

Author: DeusData

src/cli/cli.h

@@ -525,6 +525,13 @@ char *cbm_hook_augment_lifecycle_json_for(const char *input, const char *forced_
 /* Thin daemon frontend support: preserve the hook's bounded stdin read and
  * hard fail-open deadline without constructing a local MCP/store instance. */
 void cbm_hook_augment_arm_deadline(void);
+
+/* The in-process deadline in milliseconds, as CBM_HOOK_DEADLINE_MS resolves it.
+ * Exposed so a test can check what an unreadable value falls back to. POSIX
+ * only: the Windows path arms a fixed timer and reads no environment value. */
+#ifndef _WIN32
+int cbm_hook_augment_deadline_ms_for_testing(void);
+#endif
 char *cbm_hook_augment_read_stdin(void);
 /* Pure no-op gate for the hook-client fast path (see hook_augment.c). */
 bool cbm_hook_augment_input_is_noop_bash(const char *input);

src/cli/hook_augment.c

@@ -20,6 +20,7 @@
 #include "foundation/compat_fs.h"
 #include "foundation/constants.h"
 #include "foundation/mem.h"
+#include "foundation/platform.h"
 #include "mcp/mcp.h"
 #include "pipeline/pipeline.h"
 #include "yyjson/yyjson.h"
@@ -70,18 +71,21 @@
  * hook "timeout" remains the outer backstop (and alone governs Windows,
  * where this whole in-process deadline block is compiled out). */
 static int ha_deadline_ms(void) {
-    const char *env = getenv("CBM_HOOK_DEADLINE_MS");
-    if (!env || !env[0]) {
+    /* A value this reader cannot read gets the DEFAULT, never the floor. atoi
+     * used to answer 0 for a typo, 0 is below the minimum, and the clamp then
+     * handed back the shortest deadline the setting allows — the opposite of
+     * what somebody raising CBM_HOOK_DEADLINE_MS is asking for. */
+    long v = 0;
+    if (!cbm_env_long("CBM_HOOK_DEADLINE_MS", &v)) {
         return HA_DEADLINE_DEFAULT_MS;
     }
-    int v = atoi(env);
     if (v < HA_DEADLINE_MIN_MS) {
         return HA_DEADLINE_MIN_MS;
     }
     if (v > HA_DEADLINE_MAX_MS) {
         return HA_DEADLINE_MAX_MS;
     }
-    return v;
+    return (int)v;
 }
 
 static int g_ha_crumb_fd = -1;
@@ -123,6 +127,10 @@ static void ha_open_crumb_log(int deadline_ms) {
     g_ha_crumb_len = (n > 0 && n < (int)sizeof(g_ha_crumb_msg)) ? (size_t)n : 0;
 }
 
+int cbm_hook_augment_deadline_ms_for_testing(void) {
+    return ha_deadline_ms();
+}
+
 void cbm_hook_augment_arm_deadline(void) {
     int ms = ha_deadline_ms();
     ha_open_crumb_log(ms);

src/foundation/platform.c

@@ -8,6 +8,8 @@
 #include "foundation/compat.h"
 #include "foundation/constants.h"
 #include "foundation/platform_internal.h"
+#include <ctype.h>
+#include <errno.h>
 #include <fcntl.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -435,6 +437,33 @@ const char *cbm_safe_getenv(const char *name, char *buf, size_t buf_sz, const ch
     return NULL;
 }
 
+/* See platform.h. The shape here is the one src/main.c:1104 already uses for
+ * --port=: an end pointer says where the read stopped, errno catches a number
+ * too large, and *end == '\0' catches anything left over. */
+bool cbm_env_long(const char *name, long *out) {
+    if (!out) {
+        return false;
+    }
+    char raw[CBM_SZ_64] = {0};
+    if (!cbm_safe_getenv(name, raw, sizeof(raw), NULL) || !raw[0]) {
+        return false;
+    }
+    /* strtol skips leading blanks of its own accord, so " 5" would read as 5.
+     * A blank in front of a setting is a slip, not a number, so refuse it here
+     * rather than let strtol quietly step over it. */
+    if (isspace((unsigned char)raw[0])) {
+        return false;
+    }
+    char *end = NULL;
+    errno = 0;
+    long value = strtol(raw, &end, CBM_DECIMAL_BASE);
+    if (errno != 0 || !end || end == raw || *end != '\0') {
+        return false;
+    }
+    *out = value;
+    return true;
+}
+
 /* ── Home directory (cross-platform) ───────────────────── */
 
 const char *cbm_get_home_dir(void) {

src/foundation/platform.h

@@ -121,6 +121,18 @@ int cbm_default_worker_count(bool initial);
  * Returns NULL when the variable is unset and fallback is NULL. */
 const char *cbm_safe_getenv(const char *name, char *buf, size_t buf_sz, const char *fallback);
 
+/* Read an environment variable as a whole number.
+ *
+ * Answers true only when the variable is set, is not empty, and reads cleanly
+ * from its first character to its last. Anything else — a typo, a trailing
+ * unit such as "30s", a leading or trailing space, or a number too large for a
+ * long — answers false and leaves *out untouched, so the caller picks its own
+ * fallback and can say that it did.
+ *
+ * This exists because atoi and atol answer 0 for text they cannot read, and 0
+ * is a real setting at every call site in this project. */
+bool cbm_env_long(const char *name, long *out);
+
 /* ── Home directory ─────────────────────────────────────────────── */
 
 /* Cross-platform home directory: tries HOME first, then USERPROFILE (Windows).

src/mcp/index_supervisor.c

@@ -12,6 +12,7 @@
 #include "foundation/profile.h"  /* cbm_profile_active (keep worker log under CBM_PROFILE) */
 #include "ui/http_server.h"      /* cbm_http_server_resolve_binary_path */
 
+#include <limits.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -322,14 +323,23 @@ static bool supervisor_disable_requested(void) {
  * CBM_INDEX_WORKER_TIMEOUT_S override (seconds → ms) tightens it for tests. */
 static int worker_quiet_timeout_ms(void) {
     enum { DEFAULT_QUIET_TIMEOUT_MS = 900000 }; /* 15 min with no progress */
+    enum { MS_PER_SECOND = 1000 };
     char timeout_seconds[CBM_SZ_32] = {0};
+    long s = 0;
+    /* The upper test only stops the seconds-to-ms multiply from overflowing an
+     * int. It sets no policy: a longer timeout than the default is still fine. */
+    if (cbm_env_long("CBM_INDEX_WORKER_TIMEOUT_S", &s) && s > 0 && s <= INT_MAX / MS_PER_SECOND) {
+        return (int)(s * MS_PER_SECOND);
+    }
+    /* atol used to answer 0 for a value it could not read, and 0 fell straight
+     * through to the 15-minute default with nothing on screen. A test set to
+     * give up after 30 seconds then hung for 15 minutes and nobody could see
+     * why. An unreadable value now says so before it is dropped. */
     if (cbm_safe_getenv("CBM_INDEX_WORKER_TIMEOUT_S", timeout_seconds, sizeof(timeout_seconds),
                         NULL) &&
         timeout_seconds[0]) {
-        long s = atol(timeout_seconds);
-        if (s > 0) {
-            return (int)(s * 1000);
-        }
+        cbm_log_warn("index.supervisor.worker_timeout_ignored", "value", timeout_seconds, "action",
+                     "using_default");
     }
     return DEFAULT_QUIET_TIMEOUT_MS;
 }

src/mcp/mcp.c

@@ -8439,6 +8439,34 @@ cbm_mcp_supervised_result_disposition_t cbm_mcp_supervised_result_disposition(
  *   - a contained-failure response only if even that cannot produce a clean run.
  * A physical CBM host never falls back to its in-process pipeline: an initial
  * start/protocol failure is returned as an explicit error response. */
+/* How many times a failed index worker may be re-run before the server gives
+ * up, as CBM_INDEX_MAX_RESTARTS sets it. Default 100. */
+static int index_restart_cap(void) {
+    enum { INDEX_RESTART_CAP_DEFAULT = 100 };
+    long v = 0;
+    if (!cbm_env_long("CBM_INDEX_MAX_RESTARTS", &v)) {
+        /* Unset is the ordinary case and says nothing. A value that is set but
+         * unreadable is a person's intent being dropped, so name it. */
+        char raw[CBM_SZ_64] = {0};
+        if (cbm_safe_getenv("CBM_INDEX_MAX_RESTARTS", raw, sizeof(raw), NULL) && raw[0]) {
+            cbm_log_warn("index.restart_cap.ignored", "value", raw, "action", "using_default");
+        }
+        return INDEX_RESTART_CAP_DEFAULT;
+    }
+    /* Zero is a real answer meaning no restarts. The old reader kept the
+     * default unless the number was above zero, so the one value somebody sets
+     * to leave the worker alone did the opposite. */
+    if (v < 0 || v > INT_MAX) {
+        cbm_log_warn("index.restart_cap.out_of_range", "action", "using_default");
+        return INDEX_RESTART_CAP_DEFAULT;
+    }
+    return (int)v;
+}
+
+int cbm_index_restart_cap_for_testing(void) {
+    return index_restart_cap();
+}
+
 static char *index_run_supervised(cbm_mcp_server_t *srv, const char *args) {
     invalidate_cached_store(srv);
 
@@ -8502,14 +8530,7 @@ static char *index_run_supervised(cbm_mcp_server_t *srv, const char *args) {
         (void)fclose(qinit);
     }
 
-    int cap = 100;
-    const char *cap_env = getenv("CBM_INDEX_MAX_RESTARTS");
-    if (cap_env && cap_env[0]) {
-        int v = atoi(cap_env);
-        if (v > 0) {
-            cap = v;
-        }
-    }
+    int cap = index_restart_cap();
 
     char *resp = NULL;
     int quarantined = 0;         /* files pinned + added to the quarantine list so far */

src/mcp/mcp.h

@@ -290,4 +290,8 @@ void cbm_mcp_server_request_scope_end(cbm_mcp_server_t *srv);
  * On Windows, strips leading / from /C:/path. */
 bool cbm_parse_file_uri(const char *uri, char *out_path, int out_size);
 
+/* How many restarts a failed index worker gets, as CBM_INDEX_MAX_RESTARTS sets
+ * it. Exposed so a test can check what the setting resolves to. */
+int cbm_index_restart_cap_for_testing(void);
+
 #endif /* CBM_MCP_H */

tests/test_cli.c

@@ -367,6 +367,83 @@ static void restore_test_env(const char *name, char *saved) {
     }
 }
 
+/* An unreadable CBM_HOOK_DEADLINE_MS must fall back to the DEFAULT budget, not
+ * to the shortest one the setting allows.
+ *
+ * atoi answers 0 for text it cannot read, and 0 is below HA_DEADLINE_MIN_MS, so
+ * the clamp used to hand back 50 ms -- the worst possible answer for a setting
+ * whose whole purpose is to give the hook more room. The comment above
+ * ha_deadline_ms records a hunt for hook runs that never finished (0 of 24 real
+ * sessions), which is exactly the symptom a silently-shortened deadline makes.
+ *
+ * POSIX only: the Windows path arms a fixed timer and reads no environment. */
+#ifndef _WIN32
+TEST(cli_hook_deadline_ignores_an_unreadable_value) {
+    enum { HOOK_DEADLINE_DEFAULT = 2000, HOOK_DEADLINE_MIN = 50, HOOK_DEADLINE_MAX = 10000 };
+    char *saved = save_test_env("CBM_HOOK_DEADLINE_MS");
+
+    /* Positive control: a good value is still used, so a failure below is about
+     * the unreadable case and not about the reader being broken outright. */
+    cbm_setenv("CBM_HOOK_DEADLINE_MS", "1234", 1);
+    ASSERT_EQ(cbm_hook_augment_deadline_ms_for_testing(), 1234);
+
+    /* Unset falls back to the default. */
+    cbm_unsetenv("CBM_HOOK_DEADLINE_MS");
+    ASSERT_EQ(cbm_hook_augment_deadline_ms_for_testing(), HOOK_DEADLINE_DEFAULT);
+
+    /* The claim: text the reader cannot read gets the default, never the floor. */
+    const char *unreadable[] = {"abc", "2000ms", " 2000", "2000 ", "", "1e3"};
+    for (size_t i = 0; i < sizeof(unreadable) / sizeof(unreadable[0]); i++) {
+        cbm_setenv("CBM_HOOK_DEADLINE_MS", unreadable[i], 1);
+        int ms = cbm_hook_augment_deadline_ms_for_testing();
+        if (ms != HOOK_DEADLINE_DEFAULT) {
+            printf("  unreadable value \"%s\" gave %d ms\n", unreadable[i], ms);
+        }
+        ASSERT_EQ(ms, HOOK_DEADLINE_DEFAULT);
+    }
+
+    /* Both clamps still hold for values that DO read. */
+    cbm_setenv("CBM_HOOK_DEADLINE_MS", "1", 1);
+    ASSERT_EQ(cbm_hook_augment_deadline_ms_for_testing(), HOOK_DEADLINE_MIN);
+    cbm_setenv("CBM_HOOK_DEADLINE_MS", "999999", 1);
+    ASSERT_EQ(cbm_hook_augment_deadline_ms_for_testing(), HOOK_DEADLINE_MAX);
+
+    restore_test_env("CBM_HOOK_DEADLINE_MS", saved);
+    PASS();
+}
+#endif
+
+/* CBM_INDEX_MAX_RESTARTS=0 means no restarts. It used to mean 100 of them.
+ *
+ * The old reader kept the default unless atoi answered greater than zero, so
+ * the one value a person sets when they want the worker left alone did the
+ * opposite. A typo did the same thing, with nothing on screen either way. */
+TEST(cli_index_restart_cap_honours_zero_and_refuses_junk) {
+    enum { INDEX_RESTART_CAP_DEFAULT = 100 };
+    char *saved = save_test_env("CBM_INDEX_MAX_RESTARTS");
+
+    /* Positive control: a good value is still used. */
+    cbm_setenv("CBM_INDEX_MAX_RESTARTS", "7", 1);
+    ASSERT_EQ(cbm_index_restart_cap_for_testing(), 7);
+
+    cbm_unsetenv("CBM_INDEX_MAX_RESTARTS");
+    ASSERT_EQ(cbm_index_restart_cap_for_testing(), INDEX_RESTART_CAP_DEFAULT);
+
+    /* The claim: zero is a real answer meaning no restarts. */
+    cbm_setenv("CBM_INDEX_MAX_RESTARTS", "0", 1);
+    ASSERT_EQ(cbm_index_restart_cap_for_testing(), 0);
+
+    /* Text the reader cannot read keeps the default. */
+    const char *unreadable[] = {"abc", "5x", " 5", "5 ", ""};
+    for (size_t i = 0; i < sizeof(unreadable) / sizeof(unreadable[0]); i++) {
+        cbm_setenv("CBM_INDEX_MAX_RESTARTS", unreadable[i], 1);
+        ASSERT_EQ(cbm_index_restart_cap_for_testing(), INDEX_RESTART_CAP_DEFAULT);
+    }
+
+    restore_test_env("CBM_INDEX_MAX_RESTARTS", saved);
+    PASS();
+}
+
 /* Helper: mkdirp */
 static int test_mkdirp(const char *path) {
     char tmp[1024];
@@ -13697,6 +13774,10 @@ TEST(cli_update_only_names_an_installer_that_exists_issue1632) {
 
 SUITE(cli) {
     RUN_TEST(cli_update_only_names_an_installer_that_exists_issue1632);
+#ifndef _WIN32
+    RUN_TEST(cli_hook_deadline_ignores_an_unreadable_value);
+#endif
+    RUN_TEST(cli_index_restart_cap_honours_zero_and_refuses_junk);
     RUN_TEST(cli_progress_visibility_policy);
     RUN_TEST(cli_raw_mcp_result_preserves_tool_error_status);
     RUN_TEST(cli_maintenance_cancellation_forces_failure_status);