From 9318f9977e45d84f6fc72f96f86ad46aeaa518e0 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Fri, 3 Jul 2026 02:40:52 +0000 Subject: [PATCH] fix: close bug-1401 ctrlstatus leak, bug-1400 jobsub leak; harden bug-1396 harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bug-1401: xce_free_ctrlstatus(s) was defined as free_CtrlStatus(s), which only frees the fields inside xce_ctrlstatus_t. The real client reply path (decode_CtrlStatus_ptr) calloc's the struct itself, so every ctrlstatus call leaked sizeof(xce_ctrlstatus_t) (120 bytes). Existing unit tests never caught it because they built/freed the struct via the raw stack-decode + bare free_CtrlStatus() helpers, never exercising the actual client macro. Fixed by switching the macro to free_CtrlStatus_ptr(&(s)) and added a regression test that exercises the real heap-decode path; RED confirmed the exact 120-byte/1-alloc LeakSanitizer signature before the fix. bug-1400: the originally reported SEGV in cmd_jobsub on malformed positional input is stale — NULLSTR guards added 2025-11-05 already reject it cleanly. The real residual defect was submit.start_info leaking on all 4 early-return validation paths; fixed by routing them through a shared `goto cleanup` (matches the existing goto-clean idiom in queue_ctrl). bug-1396 (job-count anomaly after cold-restart-all, not yet root-caused): instrumented test-commit-durability.sh's assert_job_count() to dump the live joblist and ACKED_FILE ledger on any count mismatch instead of only the count, per the bug's own recorded next step. Verified: controller ASAN build + full controller_unit_test green; lib xcelib_test 285/285 green, 0 ASAN leaks. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SDWVNQLKxqTVHh42cH7rV3 --- src/controller/testsuite/test.c | 13 +++++-- src/include/client.h | 9 +++-- src/lib/testsuite/test_ctrlstatus.c | 58 +++++++++++++++++++++++++++++ src/tools/test-commit-durability.sh | 10 ++++- 4 files changed, 82 insertions(+), 8 deletions(-) diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index f8ae710b..17d26d5f 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -1816,7 +1816,8 @@ int cmd_jobsub(int argc, char* argv[]) default: printf( "jobsub -u -q -R -c \n"); - return -EINVAL; + rc = -EINVAL; + goto cleanup; } } @@ -1827,17 +1828,20 @@ int cmd_jobsub(int argc, char* argv[]) if (NULLSTR(submit.user)) { printf("you need to specify job user with -u\n"); - return -EINVAL; + rc = -EINVAL; + goto cleanup; } if (NULLSTR(submit.queue)) { printf("you need to specify job queue with -q\n"); - return -EINVAL; + rc = -EINVAL; + goto cleanup; } if (NULLSTR(submit.start_info->cmdline)) { printf("you need to specify job command with -c\n"); - return -EINVAL; + rc = -EINVAL; + goto cleanup; } if (NULLSTR(submit.resreq)) { @@ -1866,6 +1870,7 @@ int cmd_jobsub(int argc, char* argv[]) jobsubmit.array_size = arsize; rc = xcecli_jobsubmit(active_ctx->ctx, &jobsubmit, &jobid); +cleanup: FREEUP(submit.user); FREEUP(submit.queue); FREEUP(submit.resreq); diff --git a/src/include/client.h b/src/include/client.h index e7eb065a..0cc4a0ff 100644 --- a/src/include/client.h +++ b/src/include/client.h @@ -114,9 +114,12 @@ extern int xcecli_jobinfo(xcecli_ctx_t, uint64_t, xce_job_t**); #define xcecli_free_queues(nqueues, queues) xce_free_queues(nqueues, queues) #define xcecli_free_groups(ngroups, groups) xce_free_groups(ngroups, groups) #define xcecli_free_clusterconf(conf) xce_free_clusterconf(conf) -/* ctrlstatus reply is freed by the generated codec free (free_CtrlStatus, - * declared in the generated controller.capnp.h). */ -#define xce_free_ctrlstatus(s) free_CtrlStatus(s) +/* ctrlstatus reply is freed by the generated codec free. free_CtrlStatus() + * only frees the fields INSIDE the struct; the xce_ctrlstatus_t itself is + * calloc'd by decode_CtrlStatus_ptr() and must be freed via free_CtrlStatus_ptr() + * (which also nulls the caller's pointer) or every call leaks sizeof(xce_ctrlstatus_t) + * (bug-1401). */ +#define xce_free_ctrlstatus(s) free_CtrlStatus_ptr(&(s)) #define xcecli_free_ctrlstatus(s) xce_free_ctrlstatus(s) #endif diff --git a/src/lib/testsuite/test_ctrlstatus.c b/src/lib/testsuite/test_ctrlstatus.c index 60c5943c..c8c13903 100644 --- a/src/lib/testsuite/test_ctrlstatus.c +++ b/src/lib/testsuite/test_ctrlstatus.c @@ -192,6 +192,63 @@ static void test_ctrlstatus_roundtrip_unknown_primary(void) printf("test_ctrlstatus_roundtrip_unknown_primary: PASS\n"); } +/* bug-1401: xce_free_ctrlstatus() must free the heap-allocated xce_ctrlstatus_t + * itself, not just the fields inside it. decode_CtrlStatus_ptr() (the real + * client reply path, via xce_decode -> ... -> xce_ctrlstatus) calloc's the + * struct; free_CtrlStatus() alone only frees the inner strings/peers list and + * leaks the struct. This mirrors that allocation path (decode_CtrlStatus_ptr, + * not the stack-struct decode_CtrlStatus used by the roundtrip tests above) + * and relies on ASan leak detection to catch a regression. */ +static void test_xce_free_ctrlstatus_frees_the_struct_itself(void) +{ + xce_ctrlpeer_t p0 = { .name = "ctrl-a", .host = "10.0.0.1", .port = 9991 }; + xce_ctrlpeer_t* peerlist[1] = { &p0 }; + xce_ctrlstatus_t in; + memset(&in, 0, sizeof(in)); + in.clusterName = "lab-cluster"; + in.primaryName = "ctrl-a"; + in.responderName = "ctrl-a"; + in.configChecksum = "deadbeef"; + in.peers.count = 1; + in.peers.list = peerlist; + in.peerCount = 1; + + struct capn c; + struct capn_segment* cs; + struct CtrlStatus cap_in; + CtrlStatus_ptr sp; + + capn_init_malloc(&c); + cs = capn_root(&c).seg; + memset(&cap_in, 0, sizeof(cap_in)); + encode_CtrlStatus(cs, &cap_in, &in); + sp = new_CtrlStatus(cs); + write_CtrlStatus(&cap_in, sp); + capn_setp(capn_root(&c), 0, sp.p); + + size_t sz = capn_size(&c); + uint8_t* buf = (uint8_t*)malloc(sz); + TEST_ASSERT_NOT_NULL(buf); + capn_write_mem(&c, buf, sz, 0); + capn_free(&c); + + struct capn dc; + CtrlStatus_ptr dp; + capn_init_mem(&dc, buf, sz, 0); + dp.p = capn_getp(capn_root(&dc), 0, 1); + + xce_ctrlstatus_t* out = NULL; + decode_CtrlStatus_ptr(&out, dp); + TEST_ASSERT_NOT_NULL(out); + + xce_free_ctrlstatus(out); + + capn_free(&dc); + free(buf); + + printf("test_xce_free_ctrlstatus_frees_the_struct_itself: PASS\n"); +} + /* ── no reachable controller ⇒ typed failure (Req 7.3) ──────────────────── */ static void test_xcecli_ctrlstatus_null_ctx_typed_failure(void) @@ -396,6 +453,7 @@ void test_ctrlstatus(void) RUN_TEST(test_handle_reply_ctrlstatus_does_not_free_foreign_slot); RUN_TEST(test_ctrlstatus_roundtrip_all_fields); RUN_TEST(test_ctrlstatus_roundtrip_unknown_primary); + RUN_TEST(test_xce_free_ctrlstatus_frees_the_struct_itself); RUN_TEST(test_xcecli_ctrlstatus_null_ctx_typed_failure); RUN_TEST(test_xcecli_ctrlstatus_primary_null_ctx_typed_failure); RUN_TEST(test_xcecli_ctrlstatus_primary_null_out_typed_failure); diff --git a/src/tools/test-commit-durability.sh b/src/tools/test-commit-durability.sh index fa35973f..faf2c78d 100755 --- a/src/tools/test-commit-durability.sh +++ b/src/tools/test-commit-durability.sh @@ -392,7 +392,15 @@ assert_job_count() { pass "$label: cluster lists exactly $expected jobs" return 0 else - fail "$label: job count != $expected after recovery (see expect-jobs output above)" + # bug-1396: a bare count mismatch isn't enough to diagnose an extra-job + # anomaly — dump the live joblist and the harness's own ACKED_FILE + # ledger side by side so the diverging job ID is visible in the log + # instead of only the count. + yellow " $label: job count mismatch — dumping live joblist vs ACKED_FILE for diagnosis" + xce_test_script "$(printf 'set masters %s\nclient\njoblist\n' "$MASTERS")" + echo " --- ACKED_FILE (tracked committed jobs, deduplicated) ---" + sort -u "$ACKED_FILE" | sed 's/^/ /' + fail "$label: job count != $expected after recovery (see joblist/ACKED_FILE dump above)" return 1 fi }