From 90b99d91e98ea2c8ceedce64164ce5c9b892b533 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 29 Sep 2026 09:44:43 +0000 Subject: [PATCH] fix(scheduler): a batch replay exits 0 under LeakSanitizer (bug-1884, part 6) - kill_sched_resval() (every resource-value array's item killer) frees the value, not only a string's buffer: each replaced or torn-down value leaked its struct, including every WORKERS_PUSH value update. - put_resvals() shares the value it creates with later entries for the same index in one call; it created a second one that replaced (and now freed) the first while still writing through it. - JOB_UPDATE and the duplicate-JOBS_PUSH path kill_resreq() the old parsed resreq, and free the rejected job's schedobj (xce_core_free_schedobj(), exported from core.c), instead of freeing only the containers. - The previous attempt's (or ALTER alternative's) pend reason is freed when a new one replaces it. - kill_resreq_ctx() frees the parser context's error message, and the primary-resreq PEG action frees an earlier primary's message first. - write_snapshot_entry() frees the snapshot it builds (config and resource objects detached, they are the core's); the save side strdup()s its strings so the generated free_SnapshotData() applies. Test: test_cli_batch_replay_exits_clean_under_leak_detection (test_sim_discrete_clock.c) runs the real binary on a two-session fixture with detect_leaks=1 and requires exit 0: RED before (exit 1), GREEN now. All nine HA-soak recordings replay with exit 0 and no LeakSanitizer report. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/lib/resreq.c | 3 ++ src/lib/xce_resreq.peg | 2 ++ src/scheduler/include/core.h | 6 ++++ src/scheduler/src/core.c | 4 +-- src/scheduler/src/core_events.c | 9 +++++- src/scheduler/src/recording.c | 23 +++++++++++++++ src/scheduler/src/resource.c | 13 +++++++++ src/scheduler/src/schedobj.c | 7 ++++- src/scheduler/src/scheduler.c | 6 ++++ src/scheduler/src/snapshot.c | 6 ++-- src/scheduler/src/snapshot_wire.c | 14 +++++----- .../testsuite/test_sim_discrete_clock.c | 28 +++++++++++++++++++ 12 files changed, 107 insertions(+), 14 deletions(-) diff --git a/src/lib/resreq.c b/src/lib/resreq.c index f5cc3dca..55542d7e 100644 --- a/src/lib/resreq.c +++ b/src/lib/resreq.c @@ -505,6 +505,9 @@ void kill_resreq_ctx(resreq_context_t* ctx) if (ctx == NULL) { return; } + + /* bug-1884: a primary's parse error lands here (xce_resreq.peg) */ + FREEUP(ctx->errmsg); } int parse_resreq(xce_resreq_t* resreq, char* data, var resmgr, char** errmsg) diff --git a/src/lib/xce_resreq.peg b/src/lib/xce_resreq.peg index da2b7882..1628f33b 100644 --- a/src/lib/xce_resreq.peg +++ b/src/lib/xce_resreq.peg @@ -106,6 +106,8 @@ primary_resreq <- < (! ('{' / '}' ) .)* > { xce_primary_resreq_t *primary = (xce_primary_resreq_t *)calloc(1, sizeof(*primary)); if (primary != NULL) { auxil->primary = primary; + free(auxil->errmsg); /* bug-1884: an earlier primary's message */ + auxil->errmsg = NULL; parse_resreq_primary(primary, (char *)$0, auxil->resmgr, &(auxil->errmsg)); } } diff --git a/src/scheduler/include/core.h b/src/scheduler/include/core.h index fdda7a83..d1330c37 100644 --- a/src/scheduler/include/core.h +++ b/src/scheduler/include/core.h @@ -450,6 +450,12 @@ extern void xce_core_build_schedobj(xce_sched_obj_t*, const char* user, const char* queue, const char* project, xce_resreq_t*); +/* xce_core_free_schedobj - bug-1884: frees a schedobj built by + * xce_core_build_schedobj() (names, limit lists, per-primary wrappers and + * their pend reasons, last allocation and reason). Its resreqs belong to + * the job and are not freed. NULL-safe. */ +extern void xce_core_free_schedobj(xce_sched_obj_t* obj); + /* * sched_check_host_satisfy()/sched_check_shared_satisfy() - relocated here * from scheduler.h (scheduler-driver-equivalence fix, bug-1836), following diff --git a/src/scheduler/src/core.c b/src/scheduler/src/core.c index e441eb7a..f249b83a 100644 --- a/src/scheduler/src/core.c +++ b/src/scheduler/src/core.c @@ -209,7 +209,7 @@ int xce_sched_core_init(xce_sched_core_t* core) * used only by xce_sched_core_fini() after the table (and its shallow * datakiller) is gone -- see core_destroy_and_free_(). Here rather than in * schedobj.c so the core's teardown links on its own. */ -static void core_schedobj_free_(xce_sched_obj_t* obj) +void xce_core_free_schedobj(xce_sched_obj_t* obj) { int i; @@ -268,7 +268,7 @@ static void core_schedobj_free_(xce_sched_obj_t* obj) static void core_job_free_(xce_sched_job_t* job) { free_JobRusages_ptr(&(job->rusages)); - core_schedobj_free_(job->schedobj); + xce_core_free_schedobj(job->schedobj); if (job->resreq != NULL) { kill_resreq(job->resreq); free(job->resreq); diff --git a/src/scheduler/src/core_events.c b/src/scheduler/src/core_events.c index e2032634..8a3c8693 100644 --- a/src/scheduler/src/core_events.c +++ b/src/scheduler/src/core_events.c @@ -7,6 +7,7 @@ #include "core.h" #include "misc.h" #include "resobj.h" +#include "resreq.h" #include "sched/types.h" #include "utils.h" #include "xcealloc.h" @@ -334,6 +335,7 @@ static xce_sched_job_t* core_build_job(xce_sched_core_t* core, xce_job_t* j) job->schedobj = (xce_sched_obj_t*)calloc(1, sizeof(*(job->schedobj))); if (job->schedobj == NULL) { + kill_resreq(job->resreq); FREEUP(job->resreq); FREEUP(job); return NULL; @@ -1047,8 +1049,11 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, * FREEUP(job) below only frees the job shell itself, not * these two fields, matching this function's existing * "job->allocs/accounts are out of scope" convention. */ + /* bug-1884: free what the job owns, not only the + * containers */ + xce_core_free_schedobj(job->schedobj); + kill_resreq(job->resreq); FREEUP(job->resreq); - FREEUP(job->schedobj); FREEUP(job); continue; } @@ -1374,6 +1379,8 @@ int xce_core_push_events(xce_sched_core_t* core, xce_core_event_t* events, core_rebind_schedobj_resreq(job->schedobj, newreq); } + /* bug-1884: the schedobj now points at newreq */ + kill_resreq(job->resreq); FREEUP(job->resreq); job->resreq = newreq; } diff --git a/src/scheduler/src/recording.c b/src/scheduler/src/recording.c index 25d6a409..b4fab0f0 100644 --- a/src/scheduler/src/recording.c +++ b/src/scheduler/src/recording.c @@ -354,6 +354,27 @@ static int write_replay_header(FILE* f, xce_scheduler_t* sched, * save_snapshot() result unfreed for the same reason, detect_leaks=0 * across this whole suite) -- writing one is out of this task's boundary. */ +/* sched_snapshot_data_free - bug-1884: frees what + * xce_sched_core_save_snapshot() allocated in `snap` (not `snap` itself). + * The config and the resource objects are the core's (borrowed, see + * snapshot.c's field notes), so they are detached first. */ +static void sched_snapshot_data_free(sched_snapshot_data_t* snap) +{ + int i; + + if (snap == NULL) { + return; + } + + snap->config = NULL; + if (snap->resources != NULL) { + for (i = 0; i < snap->resources->count; i++) { + snap->resources->resources[i] = NULL; + } + } + free_SnapshotData(snap); +} + int write_snapshot_entry(FILE* f, xce_sched_core_t* core, uint64_t ts) { struct capn c; @@ -382,6 +403,8 @@ int write_snapshot_entry(FILE* f, xce_sched_core_t* core, uint64_t ts) rc = write_capnp_message(f, &c); capn_free(&c); + sched_snapshot_data_free(&snap); + return rc; } diff --git a/src/scheduler/src/resource.c b/src/scheduler/src/resource.c index 5a1b92e7..a87ae778 100644 --- a/src/scheduler/src/resource.c +++ b/src/scheduler/src/resource.c @@ -98,6 +98,19 @@ int put_resvals(zarray_t* array, int nres, int* idxs, float* vals) val->u.fvalue = 0.0; zarray_put(array, idxs[i], val); xvals[i] = val; + + /* bug-1884: a later entry for the same index shares this value; + * creating a second one replaced (and, with the array's killer, + * freed) this one while xvals[i] still pointed at it */ + { + int k; + + for (k = i + 1; k < nres; k++) { + if ((idxs[k] == idxs[i]) && (xvals[k] == NULL)) { + xvals[k] = val; + } + } + } } } diff --git a/src/scheduler/src/schedobj.c b/src/scheduler/src/schedobj.c index bc812cce..96d62592 100644 --- a/src/scheduler/src/schedobj.c +++ b/src/scheduler/src/schedobj.c @@ -276,13 +276,18 @@ void kill_sched_worker(void* arg) sched_worker_release_host_ctx((xce_sched_worker_t*)arg); } +/* kill_sched_resval - item killer of every resource-value array. bug-1884: + * frees the value itself too -- free_Value() frees only a string value's + * buffer, so every replaced or torn-down value leaked its struct. All + * values stored in these arrays are separately allocated (the wire + * decoder, dup_val(), put_resvals()). */ void kill_sched_resval(xce_val_t* val) { if (val == NULL) { return; } - free_Value(val); + free_Value_ptr(&val); } xce_sched_worker_t* make_sched_worker(void* arg, xce_worker_t* w) diff --git a/src/scheduler/src/scheduler.c b/src/scheduler/src/scheduler.c index 581b4409..50c9d93c 100644 --- a/src/scheduler/src/scheduler.c +++ b/src/scheduler/src/scheduler.c @@ -322,6 +322,9 @@ static int sched_make_primary_allocation(xce_sched_core_t* core, } if (ctx.reason != NULL) { + /* bug-1884: the previous attempt's reason (its decision has + * been published) or an earlier alternative's */ + free_PendReason_ptr(&(obj->reason)); MOVE_PTR(obj->reason, ctx.reason); } @@ -386,6 +389,9 @@ static int sched_make_compound_alloc_(xce_sched_core_t* core, } if (ctx.reason != NULL) { + /* bug-1884: the previous attempt's reason (its decision has + * been published) or an earlier alternative's */ + free_PendReason_ptr(&(obj->reason)); MOVE_PTR(obj->reason, ctx.reason); } diff --git a/src/scheduler/src/snapshot.c b/src/scheduler/src/snapshot.c index 2e873642..f2bbf75d 100644 --- a/src/scheduler/src/snapshot.c +++ b/src/scheduler/src/snapshot.c @@ -446,7 +446,7 @@ int xce_sched_core_save_snapshot(xce_sched_core_t* core, if (rv == NULL) { continue; } - rv->name = makeString((char*)rname); + rv->name = strdup(rname); rv->host = NULL; rv->val = dup_val(val); @@ -548,7 +548,7 @@ int xce_sched_core_save_snapshot(xce_sched_core_t* core, if (su == NULL) { continue; } - su->name = (u->name != NULL) ? makeString(u->name) : NULL; + su->name = (u->name != NULL) ? strdup(u->name) : NULL; if (arr_append((void***)&users_arr, &users_cap, &users_n, su) < 0) { FREEUP(su->name); @@ -575,7 +575,7 @@ int xce_sched_core_save_snapshot(xce_sched_core_t* core, if (sp == NULL) { continue; } - sp->name = (p->name != NULL) ? makeString(p->name) : NULL; + sp->name = (p->name != NULL) ? strdup(p->name) : NULL; sp->priority = p->priority; if (arr_append((void***)&proj_arr, &proj_cap, &proj_n, sp) < 0) { diff --git a/src/scheduler/src/snapshot_wire.c b/src/scheduler/src/snapshot_wire.c index a7809f65..5eda3aa4 100644 --- a/src/scheduler/src/snapshot_wire.c +++ b/src/scheduler/src/snapshot_wire.c @@ -148,10 +148,10 @@ xce_job_t* build_wire_job(xce_sched_job_t* job) } j->submit->queue - = (job->queue != NULL) ? makeString(job->queue->name) : NULL; - j->submit->user = (job->user != NULL) ? makeString(job->user->name) : NULL; + = (job->queue != NULL) ? strdup(job->queue->name) : NULL; + j->submit->user = (job->user != NULL) ? strdup(job->user->name) : NULL; j->submit->project_name - = (job->project != NULL) ? makeString(job->project->name) : NULL; + = (job->project != NULL) ? strdup(job->project->name) : NULL; j->submit->mintasks = job->mintasks; j->submit->maxtasks = job->maxtasks; @@ -206,13 +206,13 @@ xce_queue_t* build_wire_queue(xce_sched_queue_t* q) return NULL; } - wq->queue = (q->name != NULL) ? makeString(q->name) : NULL; + wq->queue = (q->name != NULL) ? strdup(q->name) : NULL; wq->attrs = q->attrs; wq->status = q->status; wq->priority = q->priority; wq->accept_interval = q->accept_interval; wq->sched_delay = q->sched_delay; - wq->resreq = (q->resreq != NULL) ? makeString(q->resreq) : NULL; + wq->resreq = (q->resreq != NULL) ? strdup(q->resreq) : NULL; /* project_name/job_pick_plugin: no wire slot -- see snapshot.c header * "Scope decisions". */ @@ -238,7 +238,7 @@ xce_worker_t* build_wire_worker(xce_sched_worker_t* w, return NULL; } - ww->name = (w->name != NULL) ? makeString(w->name) : NULL; + ww->name = (w->name != NULL) ? strdup(w->name) : NULL; ww->status = w->status; for (i = 0; i < w->resvals.count; i++) { @@ -275,7 +275,7 @@ xce_worker_t* build_wire_worker(xce_sched_worker_t* w, continue; } - nv->name = makeString((char*)rname); + nv->name = strdup(rname); nv->val = dup_val(val); if (arr_append((void***)&nvs, &cap, &n, nv) < 0) { diff --git a/src/scheduler/testsuite/test_sim_discrete_clock.c b/src/scheduler/testsuite/test_sim_discrete_clock.c index dd13252d..43e59697 100644 --- a/src/scheduler/testsuite/test_sim_discrete_clock.c +++ b/src/scheduler/testsuite/test_sim_discrete_clock.c @@ -1245,6 +1245,33 @@ static void test_cli_help_describes_sim_speed_as_deprecated(void) free(text); } +/* bug-1884: a batch replay frees what the simulator owns before exit, so an + * ASAN build with LeakSanitizer enabled exits 0. The test binary itself runs + * with detect_leaks=0 (meson.build), so the child re-enables it. Without + * ASAN the option is ignored and the test checks only the exit status. */ +static void test_cli_batch_replay_exits_clean_under_leak_detection(void) +{ + char fixture[PATH_MAX]; + char out[PATH_MAX]; + char log[PATH_MAX]; + char cmd[4 * PATH_MAX]; + + write_two_session_fixture(fixture, sizeof(fixture)); + make_tmp_file(out, PATH_MAX, "leakout"); + make_tmp_file(log, PATH_MAX, "leaklog"); + snprintf(cmd, sizeof(cmd), + "ASAN_OPTIONS=detect_leaks=1 " XCE_SCHEDULER_BIN + " --sim %s --sim-verify --sim-output %s --logfile %s " + "> /dev/null 2>&1", + fixture, out, log); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, run_child(cmd), + "a clean batch replay must exit 0 with " + "leak detection enabled"); + unlink(out); + unlink(log); + unlink(fixture); +} + int main(void) { UNITY_BEGIN(); @@ -1265,5 +1292,6 @@ int main(void) RUN_TEST(test_cli_trailing_sim_speed_without_value_fails); RUN_TEST(test_cli_interactive_sim_speed_logs_one_warning); RUN_TEST(test_cli_help_describes_sim_speed_as_deprecated); + RUN_TEST(test_cli_batch_replay_exits_clean_under_leak_detection); return UNITY_END(); }