From af3e003ad8e67cd1fe3b20dafaf4d33a4b351852 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 18:15:33 +0200 Subject: [PATCH 1/2] fix(mcp): one release point for handle_index_repository's argument strings main fails the memory-core linter: src/mcp/mcp.c: grew by 3 (814 -> 817) PR #1723 (merged as 92abefa3) added a new early-return path to handle_index_repository and, following the function's convention, freed its three argument strings there with raw free(). Its CI lint was green on 2026-09-09. On 2026-09-19 the memwaste merge cut this file's raw sites and ratcheted the baseline down to 814, so by the time #1723 landed its green was measured against a rule that no longer existed. The linter is right and the merge-result check that let it through was incomplete: it built and ran the suites and did not run the linter. It does now. The fix is not cbm_free. The three strings come from cbm_mcp_get_string_arg and resolved_repo_path_from_project_arg and are raw heap memory, released raw by every other path in the function; routing three of them through the core would free raw memory through the accounting layer. The actual defect is that the function had 41 raw free() calls of the same three arguments across a dozen early returns, and #1723 added one more block in the same shape. One helper, index_args_free(repo_path, mode_str, name_override), now replaces twelve of those blocks. free(NULL) is a no-op, so the one path where repo_path is still NULL passes NULL. On the cross-repo branch mode_str used to be freed before handle_cross_repo_mode and the other two after it; mode_str is neither passed to nor read by that call, so releasing all three after it is not observable. Five sites on the normal-completion path are left as they were: they free the strings at different points as ownership is handed to the pipeline, and collapsing them would change that order for no gain. Raw sites in this file: 817 -> 785. The baseline follows the improvement, 814 -> 785, as the linter asks; it can only tighten. Build: clean, -Werror, ASan/UBSan. mcp cli daemon_application: 692 passed, 4 skipped, 0 failed -- index_repository_honors_allowed_root, which runs through a replaced early return, included. Signed-off-by: Martin Vogel --- scripts/memory-core-baseline.txt | 2 +- src/mcp/mcp.c | 57 ++++++++++++-------------------- 2 files changed, 23 insertions(+), 36 deletions(-) diff --git a/scripts/memory-core-baseline.txt b/scripts/memory-core-baseline.txt index 8bc450f7f..19fdd363d 100644 --- a/scripts/memory-core-baseline.txt +++ b/scripts/memory-core-baseline.txt @@ -52,7 +52,7 @@ src/git/git_context.c 18 src/main.c 32 src/mcp/compact_out.c 39 src/mcp/index_supervisor.c 8 -src/mcp/mcp.c 814 +src/mcp/mcp.c 785 src/pipeline/artifact.c 38 src/pipeline/fqn.c 23 src/pipeline/lsp_resolve.h 4 diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 39aaf0dcc..7bfca8d7a 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -11062,6 +11062,16 @@ static bool project_db_is_servable(const char *project, const char *db_path) { return servable; } +/* The three heap strings handle_index_repository owns from + * cbm_mcp_get_string_arg / resolved_repo_path_from_project_arg. One release + * point keeps the dozen early-return paths in step; free(NULL) is a no-op, so + * a path may pass NULL for an argument it never obtained. */ +static void index_args_free(char *repo_path, char *mode_str, char *name_override) { + free(repo_path); + free(mode_str); + free(name_override); +} + static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { char *repo_path = cbm_mcp_get_string_arg(args, "repo_path"); char *mode_str = cbm_mcp_get_string_arg(args, "mode"); @@ -11074,15 +11084,12 @@ static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { } if (!repo_path) { - free(mode_str); - free(name_override); + index_args_free(NULL, mode_str, name_override); return cbm_mcp_text_result("repo_path is required", true); } if (!resolve_session_repo_path(srv, &repo_path)) { - free(mode_str); - free(name_override); - free(repo_path); + index_args_free(repo_path, mode_str, name_override); return cbm_mcp_text_result("failed to resolve repo_path", true); } @@ -11102,26 +11109,20 @@ static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { if (repo_path && repo_path[0] && !cbm_workspace_root_allowed(repo_path, cbm_workspace_home_dir(), cbm_workspace_cache_dir(), allowed_root, boundary_err, sizeof(boundary_err))) { - free(mode_str); - free(name_override); - free(repo_path); + index_args_free(repo_path, mode_str, name_override); return cbm_mcp_text_result(boundary_err, true); } if (mode_str && strcmp(mode_str, "cross-repo-intelligence") == 0) { - free(mode_str); char *result = handle_cross_repo_mode(srv, repo_path, name_override, args); - free(name_override); - free(repo_path); + index_args_free(repo_path, mode_str, name_override); return result; } cbm_index_resource_policy_t resource_policy; char policy_error[CBM_SZ_256] = {0}; if (!load_index_policy(srv, args, &resource_policy, policy_error, sizeof(policy_error))) { - free(mode_str); - free(name_override); - free(repo_path); + index_args_free(repo_path, mode_str, name_override); return cbm_mcp_text_result(policy_error, true); } @@ -11133,9 +11134,7 @@ static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { worker_args ? srv->index_executor(srv->index_executor_context, repo_path, worker_args) : NULL; free(worker_args); - free(repo_path); - free(mode_str); - free(name_override); + index_args_free(repo_path, mode_str, name_override); return coordinated ? coordinated : cbm_mcp_text_result( "daemon index coordinator could not start the operation", true); @@ -11148,9 +11147,7 @@ static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { char *mutation_project = cbm_project_name_from_path(name_override && name_override[0] ? name_override : repo_path); if (!mutation_project) { - free(repo_path); - free(mode_str); - free(name_override); + index_args_free(repo_path, mode_str, name_override); return cbm_mcp_text_result("could not resolve index project name", true); } @@ -11163,24 +11160,18 @@ static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { char *worker_args = index_args_with_repo_path(args, repo_path, &resource_policy); if (!worker_args) { free(mutation_project); - free(repo_path); - free(mode_str); - free(name_override); + index_args_free(repo_path, mode_str, name_override); return cbm_mcp_text_result("failed to prepare supervised index request", true); } char *supervised = index_run_supervised(srv, worker_args); free(worker_args); if (supervised) { free(mutation_project); - free(repo_path); - free(mode_str); - free(name_override); + index_args_free(repo_path, mode_str, name_override); return supervised; } free(mutation_project); - free(repo_path); - free(mode_str); - free(name_override); + index_args_free(repo_path, mode_str, name_override); return cbm_mcp_text_result( "index supervision failed before a contained worker could start; no " "in-process fallback was attempted", @@ -11189,18 +11180,14 @@ static char *handle_index_repository(cbm_mcp_server_t *srv, const char *args) { if (!mcp_project_mutation_begin(srv, mutation_project)) { free(mutation_project); - free(repo_path); - free(mode_str); - free(name_override); + index_args_free(repo_path, mode_str, name_override); return cbm_mcp_text_result("index operation blocked by another mutation for this project", true); } if (mcp_request_cancelled(srv)) { mcp_project_mutation_end(srv, mutation_project); free(mutation_project); - free(repo_path); - free(mode_str); - free(name_override); + index_args_free(repo_path, mode_str, name_override); return cbm_mcp_text_result("index operation cancelled for this request", true); } From c1128db8187db2dac27a7ad14120fc09802c8018 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 21:40:37 +0200 Subject: [PATCH 2/2] fix(test): the worker-scope script sends the policy a supervisor sends main fails scripts/test.sh at Step 5f on every platform: === Step 5f: worker request-scope regression === worker did not index the admitted repository {"content":[{"type":"text","text":"missing or incomplete trusted worker policy"}], ... "isError":true} Second collision from the #1723 merge (92abefa3), and the same kind as the first. #1723 made an index worker refuse any request that does not carry `_cbm_index_policy`: a worker never resolves the resource policy from config or environment, the supervisor does and sends it along. tests/test_worker_session_scope.sh, added on 2026-09-06 by a08b9ecb, plays the supervisor by hand -- it runs `cli --index-worker index_repository '{"repo_path":...,"mode":"fast"}'` directly -- and #1723's branch never contained that script, so its CI never ran the two together. The merge was textually clean and the result was wrong. It stayed hidden behind the lint failure the previous commit fixes, because the test stage only runs once lint is green. The worker is right and the script was incomplete: it now sends what the supervisor sends, both limits off, which is the default (`"index_max_files":"off","index_max_source_mb":"off"`, the same object tests/test_index_policy.c uses). The fail-closed case carries it too, so that a request without repo_path is still refused for its missing scope ("request workspace scope invalid") and not, by accident, for a missing policy. No assertion changed. RED, production binary built from this branch, script unmodified: worker did not index the admitted repository ... "missing or incomplete trusted worker policy" ... rc=1 GREEN: ok: index worker is scoped to the admitted request, not the daemon environment rc=0 On the failed CI run of this PR, 12 of the 13 red test jobs carry exactly that signature and nothing else. The 13th, test-windows-guards, is test_daemon_stability.py section_cold_storm ("secure CLI coordination could not be created (endpoint)", a racing cold start) -- a known nondeterministic guard, unrelated to either commit here. Step 6, which Step 5f's failure kept CI from reaching, passes locally: security-strings allow-list 4 passed, destructive-ordering contract PASS. Signed-off-by: Martin Vogel --- tests/test_worker_session_scope.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/test_worker_session_scope.sh b/tests/test_worker_session_scope.sh index fed32e97e..ab018ba76 100644 --- a/tests/test_worker_session_scope.sh +++ b/tests/test_worker_session_scope.sh @@ -70,8 +70,15 @@ run_worker() { --response-out "${response}" >"${out}" 2>"${err}" } +# A worker never reads the resource policy from config or environment: the +# supervisor resolves it and sends it inside the request, and a request +# without one is refused ("missing or incomplete trusted worker policy"). +# This script plays the supervisor, so it sends what the supervisor sends -- +# both limits off, the default. +policy='"_cbm_index_policy":{"index_max_files":"off","index_max_source_mb":"off"}' + response="${tmpdir}/scoped.response" -if ! run_worker "{\"repo_path\":\"${repo}\",\"mode\":\"fast\"}" "${response}" \ +if ! run_worker "{\"repo_path\":\"${repo}\",\"mode\":\"fast\",${policy}}" "${response}" \ "${tmpdir}/scoped.out" "${tmpdir}/scoped.err"; then echo "worker exited nonzero for an admitted request" >&2 cat "${tmpdir}/scoped.err" >&2 @@ -94,7 +101,7 @@ fi # Fail closed: no repo_path means no request scope, so no indexing at all. unscoped="${tmpdir}/unscoped.response" -if run_worker '{"mode":"fast"}' "${unscoped}" "${tmpdir}/unscoped.out" "${tmpdir}/unscoped.err"; then +if run_worker "{\"mode\":\"fast\",${policy}}" "${unscoped}" "${tmpdir}/unscoped.out" "${tmpdir}/unscoped.err"; then echo "worker ran without a request workspace scope" >&2 exit 1 fi