From e84a8d433203905c97851337f37a701d8ac917c6 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 27 Sep 2026 13:14:14 +0000 Subject: [PATCH] fix(worker): remove_worker frees every string add_worker copied (bug-1866) add_worker strndup's launch_type and the four queue/job pre/post-exec commands out of the startinfo, but remove_worker only freed exechome, execcwd, execuser, job_cmd, pids and exdata, so every finished job leaked up to five strings (LeakSanitizer at xcecd_virtual exit reported the launch_type one). New test in test_finished_job_retention.c compares ASAN's live-heap byte count across an add/remove round trip (the worker suite runs with detect_leaks=0): RED leaked exactly the 58 bytes of those five strings, GREEN after. Worker suite 12/12. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/worker/src/xcecd_worker_utils.c | 6 +++ .../testsuite/test_finished_job_retention.c | 50 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/src/worker/src/xcecd_worker_utils.c b/src/worker/src/xcecd_worker_utils.c index e88065f7..cf00ff37 100644 --- a/src/worker/src/xcecd_worker_utils.c +++ b/src/worker/src/xcecd_worker_utils.c @@ -297,6 +297,12 @@ void remove_worker(xcecd_worker_t** worker_list, int worker_pid) FREEUP(worker->job_cmd); FREEUP(worker->job_info.pidinfo.pids); FREEUP(worker->job_exdata); + /* bug-1866: every other string add_worker() copies */ + FREEUP(worker->launch_type); + FREEUP(worker->q_pre_exec); + FREEUP(worker->q_post_exec); + FREEUP(worker->job_pre_exec); + FREEUP(worker->job_post_exec); FREEUP(worker); } } diff --git a/src/worker/testsuite/test_finished_job_retention.c b/src/worker/testsuite/test_finished_job_retention.c index ff60612d..d5262105 100644 --- a/src/worker/testsuite/test_finished_job_retention.c +++ b/src/worker/testsuite/test_finished_job_retention.c @@ -371,6 +371,55 @@ static void test_unhealthy_connection_suspends_expiry(void) 70005, finish_time_ms + window_ms + 100, &out)); } +/* ════════════════════════════════════════════════════════════════════════ + * bug-1866: remove_worker() must free every string add_worker() copied + * out of the startinfo. This suite runs with detect_leaks=0, so compare + * ASAN's live-heap byte count across an add/remove round trip instead. + * ════════════════════════════════════════════════════════════════════════ */ + +#ifdef __SANITIZE_ADDRESS__ +/* from , which the nix toolchain lacks */ +size_t __sanitizer_get_current_allocated_bytes(void); + +static void add_remove_full_startinfo_worker(int pid, uint64_t jobid) +{ + xcecd_worker_t* list = NULL; + job_startinfo_t startinfo = { + .exechome = (char*)"/home/u", + .execcwd = (char*)"/home/u/run", + .execuser = (char*)"u", + .cmdline = (char*)"sleep 1", + .exdata = (char*)"exdata", + .launch_type = (char*)"openmpi", + .q_pre_exec = (char*)"/bin/q_pre", + .q_post_exec = (char*)"/bin/q_post", + .job_pre_exec = (char*)"/bin/job_pre", + .job_post_exec = (char*)"/bin/job_post", + }; + + add_worker(&list, pid, WORKER_RUNNING, jobid, 1, &startinfo); + TEST_ASSERT_NOT_NULL(list); + remove_worker(&list, pid); + TEST_ASSERT_NULL(list); +} +#endif + +static void test_remove_worker_frees_every_startinfo_copy(void) +{ +#ifdef __SANITIZE_ADDRESS__ + /* first round trip settles any lazy allocation (logging etc.) */ + add_remove_full_startinfo_worker(4101, 91001); + + size_t before = __sanitizer_get_current_allocated_bytes(); + add_remove_full_startinfo_worker(4102, 91002); + size_t after = __sanitizer_get_current_allocated_bytes(); + + TEST_ASSERT_EQUAL_UINT64((uint64_t)before, (uint64_t)after); +#else + TEST_IGNORE_MESSAGE("needs AddressSanitizer's allocator statistics"); +#endif +} + /* ════════════════════════════════════════════════════════════════════════ * main * ════════════════════════════════════════════════════════════════════════ */ @@ -391,6 +440,7 @@ int main(void) RUN_TEST(test_lookup_after_expiry_prunes_entry_permanently); RUN_TEST(test_reconnect_restarts_window_for_outcome_finished_before_outage); RUN_TEST(test_unhealthy_connection_suspends_expiry); + RUN_TEST(test_remove_worker_frees_every_startinfo_copy); return UNITY_END(); }