From c1cd8f917d01d481f447e215c3613bc53e912ed8 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 29 Sep 2026 09:28:08 +0000 Subject: [PATCH] fix(scheduler): free registry entries after their tables at core teardown; fix dup_val and the unmeet re-init leak (bug-1884, part 5) A failed key_add() runs the table's datakiller on the rejected value, and callers keep using it (JOBS_PUSH merges a duplicate job into the existing one; sched_init_queues() re-adds every queue on each config apply), so the job, queue, worker and resource datakillers must stay shallow. Part 4 made kill_sched_job() free the whole job; that is reverted. Instead xce_sched_core_fini() collects each table's entries, destroys the table, then frees each entry with a full destructor (core.c). The resource table is released the same way (a reference per resobj). Also: - dup_val() tested which == XCE_VAL_STRING (0, the resource value type, equal to Value_u_ivalue32): real string values were shallow-copied and a nonzero int32 went to makeString() as a pointer. It now tests Value_u_svalue and strdup()s, as free_Value() expects. - The shared/host satisfy checks re-initialize the unmeet vector they are given; the allocators passed the ctx's live one, dropping its buffers on every candidate host. The callers finalize it first. - add_vres() registers a resobj via makeResource(), like every other resource, instead of a bare calloc(); test_limit.c drops it with dereference(). - plugin_register_core() rejects a duplicate name before init_fn/key_add, since a failed add now finalizes (frees) the plugin and callers free it on failure. - sharedInUse gets kill_sched_resval, as put_resvals() fills it. Exit-time leak on the reproduction replay 7,469 -> 928 bytes. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/lib/testsuite/test_limit.c | 12 +- src/lib/xcelimit.c | 42 ++--- src/scheduler/include/sched/types.h | 1 - src/scheduler/src/core.c | 175 +++++++++++++++++-- src/scheduler/src/host_iter.c | 2 + src/scheduler/src/plugin.c | 16 ++ src/scheduler/src/plugins/parjob_allocator.c | 2 + src/scheduler/src/plugins/seqjob_allocator.c | 2 + src/scheduler/src/resource.c | 2 + src/scheduler/src/schedobj.c | 73 +------- src/scheduler/src/snapshot_wire.c | 11 +- 11 files changed, 216 insertions(+), 122 deletions(-) diff --git a/src/lib/testsuite/test_limit.c b/src/lib/testsuite/test_limit.c index 50b07d1b..a0be2fd0 100644 --- a/src/lib/testsuite/test_limit.c +++ b/src/lib/testsuite/test_limit.c @@ -660,11 +660,10 @@ static void test_fmt_limit_wrapper_matches_fmt_limit_units_raw_mode(void) * private and its existing white-box test harness stubs get_resvals()/ * put_resvals() as no-ops, see test_repl_limit_table.c). * - * add_vres() (xcelimit.c) key_add()s the limit's own new "limit=" - * vres into resmgr with no datakiller, so each test frees that one entry - * by hand before tearing down the shared make_test_resmgr() fixture -- - * mirroring free_test_resmgr()'s own manual-free pattern for its 8 - * pre-registered resources. + * add_vres() (xcelimit.c) registers the limit's own new "limit=" + * vres resobj in resmgr, and this fixture's resmgr has no datakiller, so + * each test drops that one entry by hand before tearing down the shared + * make_test_resmgr() fixture. */ static void free_instantiated_vres(var resmgr, xce_vres_t* vres) { @@ -674,8 +673,7 @@ static void free_instantiated_vres(var resmgr, xce_vres_t* vres) if (vres->vres != NULL) { key_del(resmgr, (void*)vres->vres->name); - FREEUP(vres->vres->name); - FREEUP(vres->vres); + dereference(vres->vres); /* a resobj since bug-1884 */ } FREEUP(vres); diff --git a/src/lib/xcelimit.c b/src/lib/xcelimit.c index 84e0049b..e5bafaac 100644 --- a/src/lib/xcelimit.c +++ b/src/lib/xcelimit.c @@ -8,6 +8,7 @@ #include "group.h" #include "kvmgr.h" +#include "resobj.h" #include "utils.h" #include "xce/types.h" #include "xcelimit.h" @@ -507,40 +508,31 @@ static const char* percent_capacity_resname(const char* resname) return NULL; } +/* add_vres - registers a limit's virtual resource. bug-1884: built with + * makeResource() like every other resmgr entry (a reference-counted + * resobj the manager releases), not a bare calloc(). */ int add_vres(xce_resource_t** pvres, var resmgr, char* name, int global) { - xce_resource_t* vres = NULL; - int rc; + xce_resource_t tmpl; - vres = (xce_resource_t*)calloc(1, sizeof(*vres)); - *pvres = vres; - - if (vres == NULL) { - return -ENOMEM; - } - - vres->name = strdup(name); - vres->idx = -1; - vres->desc = NULL; - vres->valtype = XCE_VAL_NUMERIC; - vres->ordtype = XCE_ORD_DECR; - vres->flags = XCE_RESF_LIMIT | XCE_RESF_CONSUMABLE | XCE_RESF_RELEASE; + memset(&tmpl, 0, sizeof(tmpl)); + tmpl.name = name; + tmpl.idx = -1; + tmpl.valtype = XCE_VAL_NUMERIC; + tmpl.ordtype = XCE_ORD_DECR; + tmpl.flags = XCE_RESF_LIMIT | XCE_RESF_CONSUMABLE | XCE_RESF_RELEASE; if (global) { - vres->flags |= (XCE_RESF_SHARED | XCE_RESF_GLOBAL); + tmpl.flags |= (XCE_RESF_SHARED | XCE_RESF_GLOBAL); } else { - vres->flags |= XCE_RESF_DEFAULT_HOST; + tmpl.flags |= XCE_RESF_DEFAULT_HOST; } - lock(resmgr, RTTI_LOCK_WRITE); - rc = key_add(resmgr, (void*)vres->name, (void*)vres); - unlock(resmgr); - - if (rc < 0) { - return rc; + *pvres = makeResource(resmgr, &tmpl); + if (*pvres == NULL) { + return (errno > 0) ? -errno : -ENOMEM; } - vres->idx = rc; - return rc; + return (*pvres)->idx; } int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit, diff --git a/src/scheduler/include/sched/types.h b/src/scheduler/include/sched/types.h index a9ad3124..25d3f880 100644 --- a/src/scheduler/include/sched/types.h +++ b/src/scheduler/include/sched/types.h @@ -221,7 +221,6 @@ extern int sched_ctx_get_uncomit_shared(xvec_t**, xce_sched_ctx_t*); extern int sched_ctx_get_uncomit_host(xvec_t**, xce_sched_ctx_t*, int); extern int sched_ctx_uncommit_host(xce_sched_ctx_t*, int, xvec_idval_t*); extern void reset_sched_obj(xce_sched_obj_t*); -extern void kill_sched_obj(xce_sched_obj_t*); /* item killer for xce_sched_obj_t.hlimits: frees one per-worker xlist_t * (never its vres entries, which the limit plugin's instance table owns). */ extern void kill_hlimits_list(void*); diff --git a/src/scheduler/src/core.c b/src/scheduler/src/core.c index ec562ac5..e441eb7a 100644 --- a/src/scheduler/src/core.c +++ b/src/scheduler/src/core.c @@ -7,7 +7,9 @@ #include "core.h" #include "decision.h" #include "group.h" +#include "misc.h" #include "plugin.h" +#include "resreq.h" #include "sched/types.h" #include "xstring.h" @@ -196,11 +198,159 @@ int xce_sched_core_init(xce_sched_core_t* core) zarray_init(&(core->sharedInst), 128, (zarray_item_killer_t)kill_sched_resval); - zarray_init(&(core->sharedInUse), 128, NULL); + /* bug-1884: put_resvals() fills this with values it allocates */ + zarray_init(&(core->sharedInUse), 128, + (zarray_item_killer_t)kill_sched_resval); return 0; } +/* bug-1884: full destructors for the job, queue and worker tables' entries, + * 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) +{ + int i; + + if (obj == NULL) { + return; + } + + if (obj->user != NULL) { + dereference(obj->user); + } + if (obj->queue != NULL) { + dereference(obj->queue); + } + if (obj->project != NULL) { + dereference(obj->project); + } + + xlist_fini(&(obj->glimits)); + zarray_fini(&(obj->hlimits)); + + switch (obj->tag) { + case XCE_RESREQ_PRIMARY: + if (obj->u.primary != NULL) { + free_PendReason_ptr(&(obj->u.primary->reason)); + free(obj->u.primary); + } + break; + case XCE_RESREQ_COMPOUND: + if (obj->u.compound != NULL) { + free_PendReason_ptr(&(obj->u.compound->reason)); + free(obj->u.compound); + } + break; + case XCE_RESREQ_ALTER: + for (i = 0; i < obj->u.alters.count; i++) { + xce_compound_schedobj_t* cobj + = (xce_compound_schedobj_t*)xlist_get(&(obj->u.alters), i); + + if (cobj != NULL) { + free_PendReason_ptr(&(cobj->reason)); + free(cobj); + } + } + xlist_fini(&(obj->u.alters)); + break; + default: + break; + } + + free_JobAlloc_ptr(&(obj->alloc)); + free_PendReason_ptr(&(obj->reason)); + free(obj); +} + +/* the schedobj's resreqs point into job->resreq, freed after it */ +static void core_job_free_(xce_sched_job_t* job) +{ + free_JobRusages_ptr(&(job->rusages)); + core_schedobj_free_(job->schedobj); + if (job->resreq != NULL) { + kill_resreq(job->resreq); + free(job->resreq); + } + free_JobAlloc_ptr(&(job->allocs)); + free_job_account(job->accounts); + free(job); +} + +static void core_queue_free_(xce_sched_queue_t* queue) +{ + if (queue->name != NULL) { + dereference(queue->name); + } + if (queue->resreq != NULL) { + dereference(queue->resreq); + } + free(queue); +} + +/* kill_sched_worker(), the table's datakiller, released the host eval + * context already */ +static void core_worker_free_(xce_sched_worker_t* worker) +{ + if (worker->name != NULL) { + dereference(worker->name); + } + zarray_fini(&(worker->resvals)); + zarray_fini(&(worker->resInUse)); + free(worker); +} + +/* core_destroy_and_free_ - bug-1884: collects every entry of the registry + * *mgr, destroys the registry (running its datakiller on each entry), then + * calls free_fn on each entry. */ +static void core_destroy_and_free_(var* mgr, void (*free_fn)(void*)) +{ + iterator_t it; + void** items = NULL; + int n = 0; + int cap = 0; + int i; + + if (*mgr == NULL) { + return; + } + + if (iter_make(&it, *mgr) >= 0) { + for (;;) { + void* item = NULL; + int rc = iter_next(&it, &item); + + if ((rc < 0) || (rc == RTTI_ITER_NO_DATA)) { + break; + } + if (item == NULL) { + continue; + } + if (n == cap) { + int ncap = cap ? (cap * 2) : 64; + void** tmp = (void**)realloc(items, ncap * sizeof(*tmp)); + + if (tmp == NULL) { + break; /* OOM at exit: the rest stays unfreed */ + } + items = tmp; + cap = ncap; + } + items[n++] = item; + } + iter_destroy(&it); + } + + destroy(*mgr); + *mgr = NULL; + + for (i = 0; i < n; i++) { + free_fn(items[i]); + } + free(items); +} + void xce_sched_core_fini(xce_sched_core_t* core) { if (core == NULL) { @@ -222,23 +372,18 @@ void xce_sched_core_fini(xce_sched_core_t* core) core->pend_updates[1] = NULL; /* bug-1884: every registry xce_sched_core_init() makes. Plugins go - * first -- their fini hooks may still read the entity tables. */ + * first -- their fini hooks may still read the entity tables. The job, + * worker, queue and resource tables keep shallow datakillers (a failed + * add runs them too, and callers keep using that value), so their + * entries are collected, the table destroyed, then each entry freed. */ if (core->plugins != NULL) { destroy(core->plugins); core->plugins = NULL; } - if (core->jobs != NULL) { - destroy(core->jobs); - core->jobs = NULL; - } - if (core->workers != NULL) { - destroy(core->workers); - core->workers = NULL; - } - if (core->queues != NULL) { - destroy(core->queues); - core->queues = NULL; - } + core_destroy_and_free_(&(core->jobs), (void (*)(void*))core_job_free_); + core_destroy_and_free_(&(core->workers), + (void (*)(void*))core_worker_free_); + core_destroy_and_free_(&(core->queues), (void (*)(void*))core_queue_free_); if (core->users != NULL) { destroy(core->users); core->users = NULL; @@ -250,7 +395,7 @@ void xce_sched_core_fini(xce_sched_core_t* core) zarray_fini(&(core->sharedInst)); zarray_fini(&(core->sharedInUse)); - destroy(core->resmgr); + core_destroy_and_free_(&(core->resmgr), (void (*)(void*))dereference); destroy(core->ugmgr); destroy(core->qgmgr); diff --git a/src/scheduler/src/host_iter.c b/src/scheduler/src/host_iter.c index 3be6eb90..203f7b0b 100644 --- a/src/scheduler/src/host_iter.c +++ b/src/scheduler/src/host_iter.c @@ -155,6 +155,8 @@ xce_sched_worker_t* sched_host_iter_next(xce_host_iter_t* iter) continue; } + /* bug-1884: the check (re)initializes unmeet itself */ + xvec_idval_fini(&(ctx->unmeet)); rc = sched_check_host_satisfy(core, w, uncomit, &(ctx->alloc_req.host), &(ctx->unmeet)); diff --git a/src/scheduler/src/plugin.c b/src/scheduler/src/plugin.c index 6b3a2bf6..58e8afaa 100644 --- a/src/scheduler/src/plugin.c +++ b/src/scheduler/src/plugin.c @@ -189,6 +189,22 @@ int plugin_register_core(xce_sched_core_t* core, xce_sched_plugin_t* plugin) * plugin registered via this entry point had no way to reach shared * core state at all. plugin->sched is deliberately left NULL, exactly * as before. */ + /* bug-1884: reject a duplicate name before init_fn and key_add(): a + * failed key_add() runs the registry's datakiller (plugin_unregister(), + * which frees the plugin), and callers free the plugin themselves on + * any failure. */ + { + void* existing = NULL; + + lock(core->plugins, RTTI_LOCK_READ); + rc = key_find(core->plugins, (void*)plugin->name, &existing); + unlock(core->plugins); + if ((rc > 0) && (existing != NULL)) { + return -EEXIST; + } + rc = 0; + } + plugin->core = core; if (plugin->init_fn != NULL) { diff --git a/src/scheduler/src/plugins/parjob_allocator.c b/src/scheduler/src/plugins/parjob_allocator.c index 491c38c7..e061e523 100644 --- a/src/scheduler/src/plugins/parjob_allocator.c +++ b/src/scheduler/src/plugins/parjob_allocator.c @@ -609,6 +609,8 @@ static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx) * takes core directly (already core-typed since task 1.3) -- read * from the plugin's own honest `core` field, not derived from * plugin->sched. */ + /* bug-1884: the check (re)initializes unmeet itself */ + xvec_idval_fini(&(ctx->unmeet)); rc = sched_check_shared_satisfy(core, uncomit, &(ctx->alloc_req.shared), &(ctx->unmeet)); if (rc < 0) { diff --git a/src/scheduler/src/plugins/seqjob_allocator.c b/src/scheduler/src/plugins/seqjob_allocator.c index 329cf80f..ab1edd35 100644 --- a/src/scheduler/src/plugins/seqjob_allocator.c +++ b/src/scheduler/src/plugins/seqjob_allocator.c @@ -248,6 +248,8 @@ static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx) * takes core directly (already core-typed since task 1.3) -- read * from the plugin's own honest `core` field, not derived from * plugin->sched. */ + /* bug-1884: the check (re)initializes unmeet itself */ + xvec_idval_fini(&(ctx->unmeet)); rc = sched_check_shared_satisfy(core, uncomit, &(ctx->alloc_req.shared), &(ctx->unmeet)); if (rc < 0) { diff --git a/src/scheduler/src/resource.c b/src/scheduler/src/resource.c index ebc1ce1c..5a1b92e7 100644 --- a/src/scheduler/src/resource.c +++ b/src/scheduler/src/resource.c @@ -537,6 +537,8 @@ int sched_try_alloc_on_host_(xce_sched_worker_t* w, xce_sched_core_t* core, return rc; } + /* bug-1884: the check (re)initializes unmeet itself */ + xvec_idval_fini(&(ctx->unmeet)); rc = sched_check_host_satisfy(core, w, uncomit, &(ctx->alloc_req.host), &(ctx->unmeet)); if (rc < 0) { diff --git a/src/scheduler/src/schedobj.c b/src/scheduler/src/schedobj.c index fb024788..bc812cce 100644 --- a/src/scheduler/src/schedobj.c +++ b/src/scheduler/src/schedobj.c @@ -5,8 +5,6 @@ #include "controller.capnp.h" #include "kvmgr.h" -#include "misc.h" -#include "resreq.h" #include "sched/types.h" #include "scheduler.h" #include "utils.h" @@ -16,67 +14,6 @@ #include "xvec.h" #include "xstring.h" -/* kill_sched_obj - bug-1884: frees a job's schedobj: its interned names, - * limit lists, the per-primary/compound wrappers and their pend reasons, - * and the allocation/pend reason of its last attempt. The resreqs the - * wrappers point at belong to the job (job->resreq). */ -void kill_sched_obj(xce_sched_obj_t* obj) -{ - int i; - - if (obj == NULL) { - return; - } - - if (obj->user != NULL) { - dereference(obj->user); - } - if (obj->queue != NULL) { - dereference(obj->queue); - } - if (obj->project != NULL) { - dereference(obj->project); - } - - xlist_fini(&(obj->glimits)); - zarray_fini(&(obj->hlimits)); - - switch (obj->tag) { - case XCE_RESREQ_PRIMARY: - if (obj->u.primary != NULL) { - free_PendReason_ptr(&(obj->u.primary->reason)); - FREEUP(obj->u.primary); - } - break; - case XCE_RESREQ_COMPOUND: - if (obj->u.compound != NULL) { - free_PendReason_ptr(&(obj->u.compound->reason)); - FREEUP(obj->u.compound); - } - break; - case XCE_RESREQ_ALTER: - for (i = 0; i < obj->u.alters.count; i++) { - xce_compound_schedobj_t* cobj - = (xce_compound_schedobj_t*)xlist_get(&(obj->u.alters), i); - - if (cobj != NULL) { - free_PendReason_ptr(&(cobj->reason)); - free(cobj); - } - } - xlist_fini(&(obj->u.alters)); - break; - default: - break; - } - - free_JobAlloc_ptr(&(obj->alloc)); - free_PendReason_ptr(&(obj->reason)); - free(obj); -} - -/* kill_sched_job - datakiller of core->jobs (core.c). bug-1884: frees the - * whole job, not only its rusage report (Req 6 / bug-765 G-5). */ void kill_sched_job(void* arg) { xce_sched_job_t* job = (xce_sched_job_t*)arg; @@ -85,15 +22,9 @@ void kill_sched_job(void* arg) return; } + /* release the stored rusage report, if any (Req 6 / bug-765 G-5). + * free_JobRusages_ptr is NULL-safe and clears the field. */ free_JobRusages_ptr(&job->rusages); - kill_sched_obj(job->schedobj); - if (job->resreq != NULL) { - kill_resreq(job->resreq); - FREEUP(job->resreq); - } - free_JobAlloc_ptr(&(job->allocs)); - free_job_account(job->accounts); - free(job); } request_res_t* make_resource_request(char* res, float value) diff --git a/src/scheduler/src/snapshot_wire.c b/src/scheduler/src/snapshot_wire.c index df4d1766..a7809f65 100644 --- a/src/scheduler/src/snapshot_wire.c +++ b/src/scheduler/src/snapshot_wire.c @@ -48,10 +48,15 @@ xce_val_t* dup_val(const xce_val_t* src) return NULL; } + /* bug-1884: `which` is the wire union tag, so a string is + * Value_u_svalue -- XCE_VAL_STRING (0) is the resource value TYPE and + * equals Value_u_ivalue32, so strings were shallow-copied (shared with + * the source) and a nonzero int32 was passed to makeString() as a + * pointer. The copy is strdup()'d: its owners release it with + * free_Value(), which free()s the string. */ v->which = src->which; - if (src->which == XCE_VAL_STRING) { - v->u.svalue - = (src->u.svalue != NULL) ? makeString(src->u.svalue) : NULL; + if (src->which == Value_u_svalue) { + v->u.svalue = (src->u.svalue != NULL) ? strdup(src->u.svalue) : NULL; } else { v->u = src->u; }