lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(scheduler): remove double-free in limit plugin config update (bug-1685)

Browse source 
User-reported live crash: xce_scheduler SIGABRT via glibc malloc_printerr
(heap corruption) inside kill_limit(), called from limit_update_config()
during a live config reload.

limit_update_config() called kill_limit(limit) in its error path after a
failed key_add(privdata->limits, ...). But privdata->limits is a kvmgr
created with .datakiller = kill_limit, so on a kvtab-level failure (e.g.
-EEXIST for a duplicate limit name -- exactly the rc=-17 captured in the
crash's own backtrace), xkvmgr_add() already rolls back via vclear() ->
xzarray_clear(), which fires kill_limit(limit) once automatically. The
caller's explicit second call was a double free.

Removed the redundant call; the kvmgr's own configured datakiller already
owns cleanup on this failure path (mirrors the ownership asymmetry
job.c's restore_job_array() already documents for the opposite case,
where its kvmgr has no datakiller configured and the caller must free
failed items itself). One accepted, documented residual gap: a vstore()-
level OOM failure (never reaching the kvtab step) would leak `limit`
instead -- vanishingly rare, and a leak is strictly better than the
guaranteed crash on the common duplicate-name case; xkvmgr_add's return
code doesn't distinguish which sub-case occurred, so there's no clean way
to free conditionally without a kvmgr API change (out of scope).

New test_limit_plugin.c (this plugin had zero test coverage before) --
RED-phase independently reproduced a genuine ASAN heap-use-after-free
matching the live crash's exact call chain; GREEN confirms 0 failures.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
This commit is contained in:
Rongsong Shen 2026-08-25 08:18:34 +00:00
parent 307f5b82e1
commit b040dfc068
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
4 changed files with 307 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

36
src/scheduler/src/plugins/limit.c
View file

@ -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;
}
}

18
src/scheduler/testsuite/meson.build
View file

@ -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'])

247
src/scheduler/testsuite/test_limit_plugin.c Normal file
View file

@ -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 <errno.h>
#include <stdlib.h>
#include <string.h>
#include <unity/unity.h>
#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();
}