diff --git a/src/lib/testsuite/test_xlist.c b/src/lib/testsuite/test_xlist.c index 4b192d53..042d70b3 100644 --- a/src/lib/testsuite/test_xlist.c +++ b/src/lib/testsuite/test_xlist.c @@ -128,6 +128,54 @@ static void test_xlist_iter(void) 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) { printf("\n>> xlist test\n\n"); @@ -136,4 +184,5 @@ void test_xlist(void) RUN_TEST(test_xlist_find); RUN_TEST(test_xlist_del); RUN_TEST(test_xlist_iter); + RUN_TEST(test_xlist_fini_resets_bounds_for_reuse); } diff --git a/src/lib/testsuite/test_zarray.c b/src/lib/testsuite/test_zarray.c index 73c9b495..6a27de66 100644 --- a/src/lib/testsuite/test_zarray.c +++ b/src/lib/testsuite/test_zarray.c @@ -160,6 +160,56 @@ static void test_zarray_clear() 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) { printf("\n>> zarray test\n\n"); @@ -170,4 +220,5 @@ void test_zarray(void) RUN_TEST(test_zarray_put); RUN_TEST(test_zarray_put_get_vec); RUN_TEST(test_zarray_iter); + RUN_TEST(test_zarray_fini_resets_bounds_for_reuse); } diff --git a/src/lib/xlist.c b/src/lib/xlist.c index ab48adbc..70ce0fa1 100644 --- a/src/lib/xlist.c +++ b/src/lib/xlist.c @@ -158,6 +158,17 @@ void* xlist_find(xlist_t* l, xlist_find_fn_t fn, ...) 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) { if (l == NULL) { @@ -165,5 +176,6 @@ void xlist_fini(xlist_t* l) } FREEUP(l->data); - return; + l->count = 0; + l->caps = 0; } diff --git a/src/lib/zarray.c b/src/lib/zarray.c index 42113e52..fbee96fd 100644 --- a/src/lib/zarray.c +++ b/src/lib/zarray.c @@ -117,6 +117,26 @@ int zarray_clear(zarray_t* array, int idx) 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) { if (array == NULL) { @@ -139,6 +159,10 @@ void zarray_fini(zarray_t* array) free(array->data); 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,