diff --git a/.wolf/buglog.json b/.wolf/buglog.json index a0c4a0b1..32a73815 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -18720,20 +18720,23 @@ "id": "bug-1685", "timestamp": "2026-08-25T15:47:00Z", "error_message": "User-reported live crash: xce_scheduler SIGABRT via glibc malloc_printerr (heap corruption detected during a malloc/free bookkeeping check -- 'double free or corruption'-class abort, not a clean segfault). Core captured from a live 3-node HA deployment run (--peers ctrl-1/ctrl-2/ctrl-3, -vvv). Backtrace: malloc_printerr -> kill_limit (src/lib/xcelimit.c:181) <- limit_update_config (src/scheduler/src/plugins/limit.c:76) <- plugin_config_update (src/scheduler/src/plugin.c:246) <- sched_init_config (src/scheduler/src/scheduler.c:936) <- scheduler_req_handler (src/scheduler/src/handler.c:687) <- vtask_entry (src/lib/xcectx.c:172) <- run_task/vcpu_entry (src/lib/vtaskmgr.c). limit_update_config's own locals at the crash frame: rc=-17 (-EEXIST), nlimits=7, i=0 -- crashes on (or near) the FIRST limit in a 7-limit config update.", - "file": "src/lib/xcelimit.c", - "root_cause": "Not yet root-caused. kill_limit() (xcelimit.c:181) is called from limit_update_config() (limit.c:76) during a live config update/reload (sched_init_config -> plugin_config_update fan-out) while nlimits=7 limits are being processed and rc=-17 (-EEXIST) is already set in the caller's frame -- suggests kill_limit is being called on a limit object that's already been freed or is being double-freed as part of an add-conflicts-with-existing (-EEXIST) error-handling/cleanup path in limit_update_config's loop over the new config's limits vs the existing limit table. Needs source read of limit_update_config()'s loop body and kill_limit()'s own free logic to confirm whether this is a double-free, a use-after-free, or a heap metadata corruption from an earlier out-of-bounds write that only surfaces later at this free call.", - "fix": "Not yet fixed -- investigating.", + "file": "src/scheduler/src/plugins/limit.c", + "root_cause": "FIXED (2026-08-25). Confirmed double-free: limit_update_config()'s privdata->limits is a kvmgr created with .datakiller = kill_limit (limit_priv_data_init(), limit.c). Its key_add() call (limit.c, then rc=-17/-EEXIST on the duplicate-name entry -- exactly the crash frame's own captured locals) dispatches to xkvmgr_add() (src/lib/kvmgr.c:107-129): vstore(kvm->data, value) succeeds first, THEN key_add(kvm->kvtab, key, idx) fails (EEXIST for a name already present), so xkvmgr_add rolls back via vclear(kvm->data, idx) -- which dispatches to xzarray_clear() (src/lib/zarray.c:614-637), and since the slot is in-use, fires a->killer(a->data[idx].data), i.e. kill_limit(limit), ONCE, automatically, as part of that rollback. limit_update_config()'s OLD error path then called kill_limit(limit) a SECOND time on the same already-freed pointer -- the double free glibc's malloc_printerr aborts on. Established precedent for this exact ownership-ambiguity shape: src/controller/job.c's restore_job_array() documents the identical asymmetry for a DIFFERENT kvmgr (c->jobmgr, .datakiller = NULL there) -- that kvmgr's own internal rollback is a no-op (killer is NULL), so THAT caller correctly must free the rejected value itself. Whether a caller must free a rejected key_add() value depends entirely on whether THAT specific kvmgr instance was configured with a non-NULL datakiller; privdata->limits has one (kill_limit), so limit_update_config must NOT free it itself. One accepted, documented residual gap (mirrors job.c's own accepted trade-off): if vstore() itself fails (e.g. zarray growth OOM, before ever reaching the kvtab key_add step), xkvmgr_add returns negative WITHOUT having stored or freed `limit` at all, and xkvmgr_add's return code doesn't distinguish this from the common EEXIST case -- removing the kill_limit() call trades a guaranteed crash on the common (duplicate-name) case for a leak in that vanishingly rare OOM-only sub-case. Leak is strictly better than crash; closing this gap cleanly would need a new kvmgr API, out of scope here.", + "fix": "FIXED (2026-08-25). Removed the redundant `kill_limit(limit);` call in limit_update_config()'s error path (src/scheduler/src/plugins/limit.c, `if (rc < 0) { ... return rc; }` block) -- single-line removal, replaced with a comment explaining the exact double-free mechanism above so a future reader doesn't reintroduce it 'to be safe'. Nothing else in limit.c/kvmgr.c/zarray.c touched.\n\nTest: added src/scheduler/testsuite/test_limit_plugin.c (this plugin had ZERO test coverage before this fix -- confirmed via `find src/scheduler/testsuite -iname '*limit*'` returning nothing pre-fix), wired into src/scheduler/testsuite/meson.build (new `sched_limit_plugin_test` executable + `scheduler limit plugin test` meson test, suite 'scheduler', ASAN_OPTIONS=detect_leaks=0 matching every sibling test). White-box #include of ../src/plugins/limit.c (same convention as test_plugin_picker_registration.c/test_handlers.c) to reach limit_update_config()/limit_init()/limit_priv_data_init(), all `static`; stubs reset_sched_obj()/get_resvals()/put_resvals()/plugin_register() (scheduler/src functions outside this TU, referenced only by limit.c code paths this test never exercises) purely to satisfy the linker. test_duplicate_limit_name_no_double_free() builds a real xce_scheduler_t fixture (real groupmgr_init()'d core.ugmgr/qgmgr/pgmgr/hgmgr, real limit_init()'d plugin.data), then drives limit_update_config() with an xce_conf4sched_t containing TWO xce_limit_config_t entries sharing the SAME name ('dup_limit') -- the exact duplicate-name collision bug-1685's live crash captured -- and asserts rc == -EEXIST with no crash. test_distinct_limit_names_both_succeed() is the control case (two DISTINCT names, rc == 0), proving the fix didn't just suppress freeing on failure in general.\n\nRED (pre-fix, verified live): built via `nix develop ./src --command bash src/tools/xce-build.sh scheduler asan build-asan --incremental` with the new test present but the buggy kill_limit(limit) call still in place -- sched_limit_plugin_test aborted with a genuine ASAN heap-use-after-free (not a clean assertion failure): 'AddressSanitizer: heap-use-after-free ... in kill_limit ../lib/xcelimit.c:181', READ stack `kill_limit <- limit_update_config (limit.c:76, the buggy call) <- test_duplicate_limit_name_no_double_free`, freed-by stack `kill_limit (xcelimit.c:192) <- xzarray_clear (zarray.c:626) <- xkvmgr_add (kvmgr.c:124) <- limit_update_config (limit.c:72, the key_add call)` -- an exact match to the diagnosed mechanism and to bug-1685's own captured crash signature (malloc_printerr <- kill_limit <- limit_update_config).\nGREEN (post-fix, verified live): rebuilt with the single-line fix applied -- `scheduler limit plugin test OK (0.05s)`. Full scheduler suite re-run: 52/53 OK, 1 pre-existing failure (`test_recordstart_symlink_escape_replies_error`, bug-1560, unrelated flake -- same failure present in the RED run too) -- zero new regressions.", "tags": [ "scheduler", "limit-plugin", "heap-corruption", "crash", "sigabrt", - "live-deployment" + "live-deployment", + "double-free", + "kvmgr-rollback", + "fixed" ], "related_bugs": [], "occurrences": 1, - "last_seen": "2026-08-25T15:47:00Z" + "last_seen": "2026-08-25T16:30:00Z" } ] } diff --git a/src/scheduler/src/plugins/limit.c b/src/scheduler/src/plugins/limit.c index 5bcffd31..0067f669 100644 --- a/src/scheduler/src/plugins/limit.c +++ b/src/scheduler/src/plugins/limit.c @@ -73,8 +73,40 @@ static int limit_update_config(xce_sched_plugin_t* plugin, unlock(privdata->limits); if (rc < 0) { - kill_limit(limit); - + /* bug-1685: do NOT kill_limit(limit) here -- privdata->limits + * is a kvmgr created with .datakiller = kill_limit + * (limit_priv_data_init() above). key_add() on it dispatches + * to xkvmgr_add() (../../lib/kvmgr.c), which -- when the + * kvtab-level key_add() fails after the value was already + * vstore()'d (e.g. -EEXIST on a duplicate limit name, this + * function's own common failure mode) -- itself rolls back via + * vclear(kvm->data, idx). vclear() dispatches to + * xzarray_clear() (../../lib/zarray.c), which fires the + * configured killer (kill_limit) on `limit` exactly once as + * part of that rollback. Calling kill_limit(limit) again here + * would free the same already-freed pointer a second time -- + * a double free that glibc's malloc_printerr aborts on + * (bug-1685's captured live crash: malloc_printerr <- + * kill_limit <- limit_update_config, SIGABRT on a real 3-node + * HA deployment). See job.c's restore_job_array() doc comment + * for the same asymmetry on a DIFFERENT kvmgr (c->jobmgr, + * .datakiller = NULL there) -- whether a caller must free a + * rejected key_add() value depends entirely on whether THAT + * specific kvmgr instance was configured with a non-NULL + * datakiller. privdata->limits has one, so this caller must + * NOT free it itself. + * + * Accepted residual gap: if vstore() itself fails (before + * ever reaching the kvtab key_add step -- e.g. zarray growth + * OOM), xkvmgr_add() returns negative WITHOUT having stored or + * freed `limit` at all, and this function has no way to tell + * that sub-case apart from the common EEXIST one from rc + * alone -- so removing this call trades a guaranteed crash on + * the common case for a leak in that vanishingly rare, + * OOM-only sub-case. Leak is strictly better than crash; + * closing this gap cleanly would need a new kvmgr API, which + * is out of scope here (mirrors this codebase's own accepted + * trade-off for restore_job_array()'s analogous gap). */ return rc; } } diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index 755b2333..12131372 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -1123,6 +1123,22 @@ sched_scheduler_dispatch_commit_test = executable( dependencies: [scheduler_deps, unity_dep], install: false) +# White-box regression test for bug-1685 (double-free/heap-corruption +# SIGABRT in limit_update_config() on a duplicate limit-name config +# update). It #includes ../src/plugins/limit.c directly (same white-box +# pattern as sched_picker_registration_test) so it exercises the real +# limit_update_config()/limit_init()/limit_priv_data_init() -- all +# `static`, otherwise unreachable from a test binary -- not a +# re-implementation of any of them. Stubs reset_sched_obj()/get_resvals()/ +# put_resvals() (scheduler/src, not part of this TU) purely to satisfy the +# linker for limit.c code paths this test never exercises -- see the +# file's own header comment. +sched_limit_plugin_test = executable('sched_limit_plugin_test', + ['test_limit_plugin.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + # Register as meson tests so `meson test --suite scheduler` runs them all. # detect_leaks=0 matches the suite convention (fixtures are not torn down). test('scheduler test', scheduler_test, suite: 'scheduler', @@ -1270,3 +1286,5 @@ test('scheduler decision redelivery test', test('scheduler dispatch commit test', sched_scheduler_dispatch_commit_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +test('scheduler limit plugin test', sched_limit_plugin_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_limit_plugin.c b/src/scheduler/testsuite/test_limit_plugin.c new file mode 100644 index 00000000..9b62ee8f --- /dev/null +++ b/src/scheduler/testsuite/test_limit_plugin.c @@ -0,0 +1,247 @@ +/* + * test_limit_plugin.c - white-box regression test for bug-1685: a live + * SIGABRT (glibc malloc_printerr, "double free or corruption"-class heap + * corruption abort) in limit_update_config() (../src/plugins/limit.c) + * whenever a scheduler config update contains two xce_limit_config_t + * entries with the SAME limit name. + * + * Root cause: privdata->limits is a kvmgr created with + * .datakiller = kill_limit (limit_priv_data_init(), limit.c). key_add() on + * that kvmgr dispatches to xkvmgr_add() (../../lib/kvmgr.c), which -- when + * the SECOND, kvtab-level key_add() fails (e.g. -EEXIST for a duplicate + * name, after the value has already been vstore()'d) -- itself rolls back + * via vclear(kvm->data, idx). vclear() dispatches to xzarray_clear() + * (../../lib/zarray.c), which fires the configured killer (kill_limit) on + * the just-stored value exactly once as part of that rollback. Before this + * fix, limit_update_config()'s own error path called kill_limit(limit) a + * SECOND time on the same already-freed pointer -- a double free that + * glibc's malloc_printerr aborts on. The fix removes that second, + * redundant kill_limit() call from limit_update_config()'s error path (see + * that function's own comment for the full mechanism and the accepted, + * documented residual gap for the separate vstore()-failure sub-case). + * + * RED (pre-fix): running test_duplicate_limit_name_no_double_free() + * against a limit_update_config() that still calls kill_limit(limit) in + * its error path crashes with a real SIGABRT ("double free or corruption") + * on the second (duplicate-name) config entry -- not a clean + * TEST_ASSERT failure, an actual process abort, matching bug-1685's + * captured live crash signature (malloc_printerr <- kill_limit <- + * limit_update_config, rc=-17/-EEXIST at the crash frame). + * GREEN (post-fix): limit_update_config() returns a negative rc (-EEXIST) + * on the duplicate-name collision, with no crash and no double free. + * + * #include's the real ../src/plugins/limit.c (white-box, same convention + * as test_plugin_picker_registration.c/test_handlers.c/ + * test_job_array_expand.c) so this test exercises the real + * limit_update_config()/limit_priv_data_init()/limit_init() -- not a + * re-implementation of any of them. All three are `static`; limit_init()/ + * limit_priv_data_init() are otherwise unreachable outside this TU, and + * limit_update_config() is reachable in production only via + * plugin_config_update()'s hooks.config_hook fan-out (plugin.c) -- calling + * it directly here is the same "drive the static entry point directly" + * convention test_job_update.c/test_handlers.c use for their own static + * targets. + * + * Stubs: limit.c's limit_job_election()/limit_prepare_allocation() + * (neither exercised by this test -- only limit_update_config() is) + * reference reset_sched_obj() (../src/schedobj.c) and get_resvals()/ + * put_resvals() (../src/resource.c), none of which is part of this + * white-box TU. Stubbed as no-ops purely to satisfy the linker -- same + * pattern test_plugin_picker_registration.c uses for its + * sched_fill_limit_requests() stub. + */ +#include +#include +#include + +#include + +#include "group.h" +#include "kvmgr.h" +#include "scheduler.h" +#include "xce/types.h" +#include "xcelimit.h" + +void reset_sched_obj(xce_sched_obj_t* obj) +{ + (void)obj; +} + +int get_resvals(zarray_t* array, int nres, int* idxs, float* vals) +{ + (void)array; + (void)nres; + (void)idxs; + (void)vals; + return 0; +} + +int put_resvals(zarray_t* array, int nres, int* idxs, float* vals) +{ + (void)array; + (void)nres; + (void)idxs; + (void)vals; + return 0; +} + +/* register_limit() (limit.c, the plugin's public entry point) calls + * plugin_register() (../src/plugin.c, not part of this TU); this test + * never calls register_limit() itself (it drives limit_init()/ + * limit_update_config() directly), so a no-op stub is sufficient purely + * to satisfy the linker -- same rationale as the other stubs above. */ +int plugin_register(void* sched, void* plugin) +{ + (void)sched; + (void)plugin; + return 0; +} + +#include "../src/plugins/limit.c" + +void setUp(void) { return; } +void tearDown(void) { return; } + +/* ---------------------------------------------------------------------- */ +/* Fixture: a real xce_scheduler_t with real group managers for + * core.ugmgr/qgmgr/pgmgr/hgmgr (build_limit()'s build_consumer() calls + * get_group(mgr, "all") for every consumer type our test configs leave + * NULL -- same minimal setup ../../lib/testsuite/test_limit.c's own + * test_build_limit_ok1() uses for build_limit() directly), plus a real + * limit plugin (limit_init()) so plugin.data is a real + * limit_plugin_data_t with privdata->limits configured with + * .datakiller = kill_limit -- the exact kvmgr shape bug-1685 crashed on. + * ---------------------------------------------------------------------- */ +static void fixture_init(xce_scheduler_t* sched, xce_sched_plugin_t* plugin) +{ + int rc; + + memset(sched, 0, sizeof(*sched)); + groupmgr_init(&(sched->core.ugmgr), 4, 4); + groupmgr_init(&(sched->core.qgmgr), 4, 4); + groupmgr_init(&(sched->core.pgmgr), 4, 4); + groupmgr_init(&(sched->core.hgmgr), 4, 4); + TEST_ASSERT_NOT_NULL(sched->core.ugmgr); + TEST_ASSERT_NOT_NULL(sched->core.qgmgr); + TEST_ASSERT_NOT_NULL(sched->core.pgmgr); + TEST_ASSERT_NOT_NULL(sched->core.hgmgr); + + memset(plugin, 0, sizeof(*plugin)); + plugin->sched = sched; + + rc = limit_init(plugin); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NOT_NULL(plugin->data); +} + +/* A minimal, heap-allocated xce_limit_config_t: no per-consumer groups + * (per_user/per_queue/per_project/per_host all 0, users/queues/projects/ + * hosts all NULL), so build_limit()'s build_consumer() falls back to + * get_group(mgr, "all") for every consumer -- the same "all"-group path + * ../../lib/testsuite/test_limit.c's own fixtures rely on. `name` is + * expected to be a string literal (build_limit() strdup()s it into the + * built xce_limit_t, so no ownership is transferred here). */ +static xce_limit_config_t* make_limit_config(const char* name) +{ + xce_limit_config_t* cfg = (xce_limit_config_t*)calloc(1, sizeof(*cfg)); + xce_limit_val_t* val = (xce_limit_val_t*)calloc(1, sizeof(*val)); + + TEST_ASSERT_NOT_NULL(cfg); + TEST_ASSERT_NOT_NULL(val); + + val->resname = "mem"; + val->percent = 0; + val->val = 10.0; + + cfg->name = (char*)name; + cfg->val = val; + + return cfg; +} + +static void free_limit_config(xce_limit_config_t* cfg) +{ + FREEUP(cfg->val); + FREEUP(cfg); +} + +/* + * THE regression: a config update with TWO xce_limit_config_t entries + * sharing the SAME name ("dup_limit") -- the exact duplicate-name + * collision bug-1685's live crash captured (rc=-17/-EEXIST at the crash + * frame, on/near the first limit of a 7-limit config). Pre-fix, this + * aborts the process (SIGABRT, "double free or corruption"); post-fix, + * limit_update_config() must return a negative rc cleanly, with no crash. + * Under ASAN, a double free is caught unconditionally regardless of + * whether this test also makes an explicit assertion about it -- the + * absence of an abort IS the proof (same rationale the task description + * for this test uses: "that's literally what caught the original bug"). + */ +static void test_duplicate_limit_name_no_double_free(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t plugin; + xce_limit_config_t* limits[2]; + xce_conf4sched_t config; + int rc; + + fixture_init(&sched, &plugin); + + limits[0] = make_limit_config("dup_limit"); + limits[1] = make_limit_config("dup_limit"); + + memset(&config, 0, sizeof(config)); + config.nlimits = 2; + config.limits = limits; + + rc = limit_update_config(&plugin, &config); + + TEST_ASSERT_LESS_THAN_INT(0, rc); + TEST_ASSERT_EQUAL_INT(-EEXIST, rc); + + free_limit_config(limits[0]); + free_limit_config(limits[1]); +} + +/* + * Control case: a config update with DISTINCT limit names succeeds for + * both entries, proving the fix didn't just suppress freeing on failure + * in general -- it only stopped the redundant free on THIS specific + * (kvtab-key_add-failure) rollback path. Both built xce_limit_t values + * are now owned by privdata->limits (torn down, if at all, only by the + * kvmgr's own datakiller at process exit -- suite convention: + * ASAN_OPTIONS=detect_leaks=0, fixtures are never explicitly torn down, + * see meson.build). + */ +static void test_distinct_limit_names_both_succeed(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t plugin; + xce_limit_config_t* limits[2]; + xce_conf4sched_t config; + int rc; + + fixture_init(&sched, &plugin); + + limits[0] = make_limit_config("limit_a"); + limits[1] = make_limit_config("limit_b"); + + memset(&config, 0, sizeof(config)); + config.nlimits = 2; + config.limits = limits; + + rc = limit_update_config(&plugin, &config); + + TEST_ASSERT_EQUAL_INT(0, rc); + + free_limit_config(limits[0]); + free_limit_config(limits[1]); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_duplicate_limit_name_no_double_free); + RUN_TEST(test_distinct_limit_names_both_succeed); + return UNITY_END(); +}