fix(lib): zarray_fini/xlist_fini must reset caps/count, not just free data
A user-reported core dump showed xce_scheduler crashing with SIGSEGV in
its allocator thread: zarray_get_ex dereferencing a NULL array->data
inside match_job_host_limit_() (plugins/limit.c), reached via schedobj.c's
reset_sched_obj().
Root cause: zarray_fini() freed array->data and set it to NULL, but left
array->caps/count/pos at their stale pre-fini values instead of resetting
to the safe (0,0,0) state zarray_init() establishes. xlist_fini() had the
identical gap. Both structs' own bounds checks (`idx >= caps`) treat a
nonzero caps as a guarantee that data is a valid allocation -- a
guarantee broken for any caller that reuses the same struct after
_fini() without re-init.
schedobj.c's reset_sched_obj() does exactly that: it tears down a job's
own PERSISTENT schedobj (reused for the job's whole lifetime, not a
disposable scratch object) whenever the cluster's config version has
advanced, with no matching re-init. Once a host-limit match had grown
hlimits' capacity, and the config version bumped again while that job
sat PEND, the next allocation attempt for the same job dereferenced the
torn array -- exactly matching the reported backtrace. This same crash
class had already caused at least one earlier, never-successfully-root-
caused crash near the same call site (2026-08-17).
Fixed at the library-primitive level rather than reset_sched_obj()'s
call site: zarray_fini()/xlist_fini() now reset caps/count/pos to 0,
matching what _init() establishes. This makes every existing
_fini()-then-reuse caller correct by construction, with zero changes
needed at any call site. Audited every non-test zarray_fini()/
xlist_fini() call site first -- none relies on caps/count surviving
_fini(), so this is a pure safety improvement, not a behavior change.
RED confirmed by temporarily disabling both resets: new tests failed
cleanly ("Expected 0 Was 4" / "Expected 0 Was 2", the stale pre-fini
caps). GREEN confirmed restored: full lib suite 314/314, scheduler
suite 77/77, zero regressions.
bug-1817.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
parent
7f3237aee1
commit
63de947305
4 changed files with 137 additions and 1 deletions
|
|
@ -128,6 +128,54 @@ static void test_xlist_iter(void)
|
||||||
xlist_fini(&lst);
|
xlist_fini(&lst);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/*
|
||||||
|
* test_xlist_fini_resets_bounds_for_reuse - sibling of zarray's own
|
||||||
|
* test_zarray_fini_resets_bounds_for_reuse (test_zarray.c): xlist_get()'s
|
||||||
|
* bounds check has the identical shape as zarray_get_ex()'s
|
||||||
|
* (`idx >= l->caps`), so the same live SIGSEGV class (schedobj.c's
|
||||||
|
* reset_sched_obj() -> xlist_fini(&(obj->glimits)), reused without a
|
||||||
|
* matching xlist_init()) applies here too -- caps/count must not survive
|
||||||
|
* xlist_fini() as stale-nonzero while data is NULL.
|
||||||
|
*/
|
||||||
|
static void test_xlist_fini_resets_bounds_for_reuse(void)
|
||||||
|
{
|
||||||
|
xlist_t lst;
|
||||||
|
void* p;
|
||||||
|
int idx;
|
||||||
|
|
||||||
|
xlist_init(&lst, 2);
|
||||||
|
|
||||||
|
idx = xlist_store(&lst, (void*)"Hello");
|
||||||
|
TEST_ASSERT_TRUE(idx >= 0);
|
||||||
|
TEST_ASSERT_TRUE(lst.caps > 0);
|
||||||
|
|
||||||
|
/* Tear down for reuse WITHOUT calling xlist_init() again -- exactly
|
||||||
|
* reset_sched_obj()'s own pattern. */
|
||||||
|
xlist_fini(&lst);
|
||||||
|
|
||||||
|
TEST_ASSERT_EQUAL_INT(0, lst.caps);
|
||||||
|
TEST_ASSERT_EQUAL_INT(0, lst.count);
|
||||||
|
TEST_ASSERT_NULL(lst.data);
|
||||||
|
|
||||||
|
/* Reading an index that was valid before fini must now safely return
|
||||||
|
* NULL instead of dereferencing freed memory. */
|
||||||
|
p = xlist_get(&lst, idx);
|
||||||
|
TEST_ASSERT_NULL(p);
|
||||||
|
|
||||||
|
/* And the list must be genuinely reusable afterward, matching
|
||||||
|
* xlist_init()'s own contract -- including xlist_store(), which
|
||||||
|
* before this fix could ALSO write through the freed pointer if
|
||||||
|
* count/caps had survived stale (xlist_ensure()'s own
|
||||||
|
* `count < caps` check would wrongly skip reallocating). */
|
||||||
|
idx = xlist_store(&lst, (void*)"World");
|
||||||
|
TEST_ASSERT_TRUE(idx >= 0);
|
||||||
|
p = xlist_get(&lst, idx);
|
||||||
|
TEST_ASSERT_TRUE(p != NULL);
|
||||||
|
TEST_ASSERT_TRUE(strcmp((char*)p, "World") == 0);
|
||||||
|
|
||||||
|
xlist_fini(&lst);
|
||||||
|
}
|
||||||
|
|
||||||
void test_xlist(void)
|
void test_xlist(void)
|
||||||
{
|
{
|
||||||
printf("\n>> xlist test\n\n");
|
printf("\n>> xlist test\n\n");
|
||||||
|
|
@ -136,4 +184,5 @@ void test_xlist(void)
|
||||||
RUN_TEST(test_xlist_find);
|
RUN_TEST(test_xlist_find);
|
||||||
RUN_TEST(test_xlist_del);
|
RUN_TEST(test_xlist_del);
|
||||||
RUN_TEST(test_xlist_iter);
|
RUN_TEST(test_xlist_iter);
|
||||||
|
RUN_TEST(test_xlist_fini_resets_bounds_for_reuse);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -160,6 +160,56 @@ static void test_zarray_clear()
|
||||||
|
|
||||||
destroy(a); /* no memory leak */
|
destroy(a); /* no memory leak */
|
||||||
}
|
}
|
||||||
|
/*
|
||||||
|
* test_zarray_fini_resets_bounds_for_reuse - live SIGSEGV found 2026-09-15
|
||||||
|
* (scheduler's allocator thread, zarray_get_ex dereferencing a NULL
|
||||||
|
* array->data): schedobj.c's reset_sched_obj() calls zarray_fini() on a
|
||||||
|
* still-in-use xce_sched_obj_t.hlimits and expects it to be safely reusable
|
||||||
|
* afterward (the object itself isn't freed, just reset for the next
|
||||||
|
* scheduling round) -- but the pre-fix zarray_fini() only NULLed data,
|
||||||
|
* leaving caps/count/pos at their pre-fini values. A later zarray_get()
|
||||||
|
* for any idx below that stale caps then passed the bounds check and
|
||||||
|
* dereferenced NULL. This drives the raw zarray_init/zarray_put/
|
||||||
|
* zarray_fini/zarray_get API directly (not the xzarray RTTI wrapper this
|
||||||
|
* file's other tests use) since that's the exact API schedobj.c calls.
|
||||||
|
*/
|
||||||
|
static void test_zarray_fini_resets_bounds_for_reuse()
|
||||||
|
{
|
||||||
|
zarray_t array;
|
||||||
|
int rc;
|
||||||
|
void* p;
|
||||||
|
|
||||||
|
zarray_init(&array, 4, NULL);
|
||||||
|
|
||||||
|
rc = zarray_put(&array, 0, (void*)"Hello");
|
||||||
|
TEST_ASSERT_EQUAL_INT(0, rc);
|
||||||
|
TEST_ASSERT_TRUE(array.caps > 0);
|
||||||
|
|
||||||
|
/* Tear down for reuse WITHOUT calling zarray_init() again -- exactly
|
||||||
|
* reset_sched_obj()'s own pattern. */
|
||||||
|
zarray_fini(&array);
|
||||||
|
|
||||||
|
TEST_ASSERT_EQUAL_INT(0, array.caps);
|
||||||
|
TEST_ASSERT_EQUAL_INT(0, array.count);
|
||||||
|
TEST_ASSERT_NULL(array.data);
|
||||||
|
|
||||||
|
/* The exact call that used to SEGV: reading an index that was valid
|
||||||
|
* before fini must now safely return NULL instead of dereferencing
|
||||||
|
* freed memory. */
|
||||||
|
p = zarray_get(&array, 0);
|
||||||
|
TEST_ASSERT_NULL(p);
|
||||||
|
|
||||||
|
/* And the array must be genuinely reusable afterward, matching
|
||||||
|
* zarray_init()'s own contract. */
|
||||||
|
rc = zarray_put(&array, 0, (void*)"World");
|
||||||
|
TEST_ASSERT_EQUAL_INT(0, rc);
|
||||||
|
p = zarray_get(&array, 0);
|
||||||
|
TEST_ASSERT_TRUE(p != NULL);
|
||||||
|
TEST_ASSERT_TRUE(strcmp((char*)p, "World") == 0);
|
||||||
|
|
||||||
|
zarray_fini(&array);
|
||||||
|
}
|
||||||
|
|
||||||
void test_zarray(void)
|
void test_zarray(void)
|
||||||
{
|
{
|
||||||
printf("\n>> zarray test\n\n");
|
printf("\n>> zarray test\n\n");
|
||||||
|
|
@ -170,4 +220,5 @@ void test_zarray(void)
|
||||||
RUN_TEST(test_zarray_put);
|
RUN_TEST(test_zarray_put);
|
||||||
RUN_TEST(test_zarray_put_get_vec);
|
RUN_TEST(test_zarray_put_get_vec);
|
||||||
RUN_TEST(test_zarray_iter);
|
RUN_TEST(test_zarray_iter);
|
||||||
|
RUN_TEST(test_zarray_fini_resets_bounds_for_reuse);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -158,6 +158,17 @@ void* xlist_find(xlist_t* l, xlist_find_fn_t fn, ...)
|
||||||
return it;
|
return it;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/*
|
||||||
|
* xlist_fini - tears down `l`'s backing storage, leaving it in the SAME
|
||||||
|
* safe, empty state xlist_init() establishes (data=NULL, caps=0, count=0)
|
||||||
|
* -- NOT just data=NULL with caps/count still holding their pre-fini
|
||||||
|
* values. See zarray_fini()'s own doc comment (zarray.c) for the exact
|
||||||
|
* live crash this class of bug caused (schedobj.c's reset_sched_obj(),
|
||||||
|
* the analogous xlist_t field obj->glimits) -- xlist_get()'s bounds check
|
||||||
|
* has the identical shape (`idx >= l->caps`), so a stale-nonzero caps with
|
||||||
|
* data==NULL is just as exploitable here. No existing caller relies on
|
||||||
|
* caps/count surviving an xlist_fini() call.
|
||||||
|
*/
|
||||||
void xlist_fini(xlist_t* l)
|
void xlist_fini(xlist_t* l)
|
||||||
{
|
{
|
||||||
if (l == NULL) {
|
if (l == NULL) {
|
||||||
|
|
@ -165,5 +176,6 @@ void xlist_fini(xlist_t* l)
|
||||||
}
|
}
|
||||||
|
|
||||||
FREEUP(l->data);
|
FREEUP(l->data);
|
||||||
return;
|
l->count = 0;
|
||||||
|
l->caps = 0;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -117,6 +117,26 @@ int zarray_clear(zarray_t* array, int idx)
|
||||||
return 0;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/*
|
||||||
|
* zarray_fini - tears down `array`'s backing storage, leaving it in the
|
||||||
|
* SAME safe, empty state zarray_init() establishes (data=NULL, caps=0,
|
||||||
|
* count=0, pos=0) -- NOT just data=NULL with caps/count/pos still holding
|
||||||
|
* their pre-fini values. Before this fix, a caller that called
|
||||||
|
* zarray_fini() and then reused the SAME zarray_t (rather than discarding
|
||||||
|
* it) without an intervening zarray_init() left `caps`/`count` stale-
|
||||||
|
* nonzero while `data` was NULL: zarray_get_ex()'s own bounds check
|
||||||
|
* (`idx >= array->caps`) would then pass for any idx below the stale caps,
|
||||||
|
* and `array->data[idx]` would dereference NULL -- a live SIGSEGV
|
||||||
|
* (scheduler-framework, 2026-09-15, xce_sched_obj_t.hlimits via
|
||||||
|
* schedobj.c's reset_sched_obj() -> plugins/limit.c's match_job_host_
|
||||||
|
* limit_(), crashing in the scheduler's allocator thread) that had also
|
||||||
|
* caused at least one earlier, never-root-caused crash near the same call
|
||||||
|
* site (2026-08-17). No existing caller relies on caps/count surviving a
|
||||||
|
* zarray_fini() call -- every current call site either discards the
|
||||||
|
* zarray_t immediately after (a stack-local scratch array) or is tearing
|
||||||
|
* down the struct that embeds it -- so this is a pure safety improvement,
|
||||||
|
* not a behavior change for any correct caller.
|
||||||
|
*/
|
||||||
void zarray_fini(zarray_t* array)
|
void zarray_fini(zarray_t* array)
|
||||||
{
|
{
|
||||||
if (array == NULL) {
|
if (array == NULL) {
|
||||||
|
|
@ -139,6 +159,10 @@ void zarray_fini(zarray_t* array)
|
||||||
free(array->data);
|
free(array->data);
|
||||||
array->data = NULL;
|
array->data = NULL;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
array->count = 0;
|
||||||
|
array->caps = 0;
|
||||||
|
array->pos = 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
int zarray_iter_ex(zarray_t* array, zarray_iter_skip_fn_t skipfn, xlist_t* l,
|
int zarray_iter_ex(zarray_t* array, zarray_iter_skip_fn_t skipfn, xlist_t* l,
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue