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

fix(scheduler): invalidate stale builtin_fcfs picker cursors across calls (bug-1686)

Browse source 
User-reported live crash: xce_scheduler SIGSEGV in fcfs_cursor_enter_queue
(builtin_fcfs.c), called from builtin_fcfs_next_job <- sched_job_picker
(picker thread).

priv->cursor/priv->job_cursor (fcfs_plugin_data_t) are raw ordq_item_t*
pointers that persist across SEPARATE next_job() calls, each with its own
independent priv->lock acquisition/release. The existing mutual-exclusion
fix (task 2.4) correctly protects the cursor walk from corruption DURING
one locked call, but does nothing to stop a LATER call from dereferencing
a node a properly-locked, INTERVENING call already freed.
on_queue_update()'s re-sort (remove old ordq_item_t node, insert a fresh
one via ordq_put) is exactly such a free -- with only one queue in the
deployment, the picker's cursor is essentially always parked on it
between calls (sched_queue_is_ready/sched_job_is_ready are unconditional
true stubs today, so the cursor never advances away while jobs remain),
making a queue config update land in that window trivially reachable.

Fixed by invalidating (NULLing) whichever cursor points at a node right
before that node is freed, at all three sites that can free a node the
cursors might reference: on_queue_update's reuse-and-resort path
(priv->cursor + priv->job_cursor in lockstep), builtin_fcfs_on_job_state_change's
!JOB_IS_PEND removal, and fcfs_remove_job_everywhere's cross-queue removal
loop (both job_cursor only, via an ordq_peek-then-invalidate-then-delete
pattern). Always safe: next_job() treats a NULL cursor as "no more
candidates this round", and begin_session() resets both cursors fresh
every round regardless -- worst case is a bounded one-round delay, never
a crash or a permanently lost job.

New test_builtin_fcfs_cursor_uaf.c reproduces the bug fully single-threaded
(next_job() releases the lock between calls, so no real concurrency is
needed to hit the window). RED-phase independently confirmed a real ASAN
heap-use-after-free with the exact same call chain as the live crash
(freed by on_queue_update, read by next_job()); GREEN confirms 0 failures
and no job permanently lost across the invalidation.

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:49:43 +00:00
parent b9f13355df
commit bc3f76b42e
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
4 changed files with 324 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

9
.wolf/buglog.json
View file

File diff suppressed because one or more lines are too long

71
src/scheduler/src/plugins/builtin_fcfs.c
View file

@ -54,6 +54,25 @@
* that never touch queues/cursor/job_cursor, see each function's own
* comment; an earlier revision of this comment incorrectly included
* on_pend here, corrected in task 2.4.)
*
* bug-1686 (found live, 2026-08-25): mutual exclusion above closes the
* "torn mid-walk" race but is NOT by itself sufficient -- priv->cursor/
* job_cursor are raw ordq_item_t* values that PERSIST in fcfs_plugin_data_t
* across SEPARATE next_job() calls, each with its own independent lock
* acquisition/release. A properly-locked on_queue_update() or
* on_job_state_change()/on_job_update() call, running entirely BETWEEN two
* next_job() calls, can legitimately free the exact node one of these
* cursors is resting on (on_queue_update's remove-then-reinsert re-sort,
* or an ordq_del removing a no-longer-pending job) -- the lock prevents
* concurrent corruption but does nothing to stop a LATER call from
* dereferencing an already-freed node. Fixed by invalidating (NULLing)
* whichever cursor points at a node right before that node is freed, in
* on_queue_update (priv->cursor + priv->job_cursor in lockstep) and in
* fcfs_remove_job_everywhere/on_job_state_change (priv->job_cursor) -- see
* each site's own comment. Always safe: next_job() treats a NULL cursor as
* "no more candidates here", and begin_session() resets both fresh every
* round regardless, so the worst case is a bounded one-round delay, never
* a crash or lost job.
*/
#include <errno.h>
#include <pthread.h>
@ -359,7 +378,23 @@ static int fcfs_remove_job_everywhere(fcfs_plugin_data_t* priv, uint64_t jobid)
int removed = 0;
for (it = priv->queues.head; it != NULL; it = it->next) {
fcfs_queue_t* fq = (fcfs_queue_t*)it->data;
fcfs_queue_t* fq = (fcfs_queue_t*)it->data;
/* bug-1686: priv->job_cursor is a raw ordq_item_t* that survives
* across separate next_job() calls (each with its own priv->lock
* acquisition) -- unlike the walk itself, which this lock already
* protects mid-call (see the file header's Concurrency comment).
* If job_cursor is resting on the exact node ordq_del is about to
* free, invalidate it FIRST so the next next_job() call can never
* dereference freed memory; NULL is always safe here (next_job()
* treats a NULL job_cursor as "no more candidates in this queue",
* and begin_session() resets both cursors fresh every round
* anyway). */
ordq_item_t* victim = ordq_peek(&(fq->pendJobs),
(ordq_match_fn_t)job_match, jobid);
if ((victim != NULL) && (victim == priv->job_cursor)) {
priv->job_cursor = NULL;
}
removed += ordq_del(&(fq->pendJobs), (ordq_match_fn_t)job_match, jobid);
}
@ -426,6 +461,28 @@ int builtin_fcfs_on_queue_update(void* picker, xce_sched_queue_t* queue)
}
} else {
fq = (fcfs_queue_t*)it->data;
/* bug-1686: `it` is the exact ordq_item_t node priv->cursor may be
* resting on between separate next_job() calls -- each call takes
* its own priv->lock acquisition, so this lock does not protect a
* cursor value that outlives one call (only the walk WITHIN a
* call, see the file header's Concurrency comment). Reinserting
* `fq` below (ordq_put) allocates a BRAND NEW ordq_item_t, so `it`
* itself is genuinely freed here, not just repositioned -- a
* dangling priv->cursor would use-after-free on the very next
* next_job() call (live-reproduced: bug-1686). Invalidate BOTH
* cursors before freeing -- job_cursor is only ever meaningful
* relative to the queue priv->cursor currently names, so it must
* be reset in lockstep exactly as fcfs_cursor_enter_queue() itself
* always keeps them (see that function's own doc comment). NULL
* is always safe: next_job() treats it as "no more candidates this
* round", and begin_session() resets both cursors fresh on the
* next round regardless. */
if (priv->cursor == it) {
priv->cursor = NULL;
priv->job_cursor = NULL;
}
FREEUP(it);
}
@ -549,6 +606,18 @@ int builtin_fcfs_on_job_state_change(void* picker, xce_sched_job_t* job,
}
if (!JOB_IS_PEND(state)) {
/* bug-1686: see fcfs_remove_job_everywhere's identical comment --
* invalidate priv->job_cursor BEFORE freeing the node it might be
* resting on (a raw pointer that survives across separate
* next_job() calls, each with its own priv->lock acquisition). */
ordq_item_t* victim = ordq_peek(&(fq->pendJobs),
(ordq_match_fn_t)job_match,
job->jobid);
if ((victim != NULL) && (victim == priv->job_cursor)) {
priv->job_cursor = NULL;
}
ordq_del(&(fq->pendJobs), (ordq_match_fn_t)job_match, job->jobid);
mylock_unlock(&(priv->lock));
return 0;

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

@ -1139,6 +1139,25 @@ sched_limit_plugin_test = executable('sched_limit_plugin_test',
dependencies: [scheduler_test_deps],
install: false)
# White-box regression test for bug-1686 (live SIGSEGV in
# fcfs_cursor_enter_queue() <- builtin_fcfs_next_job(), a heap-use-after-free
# on priv->cursor/job_cursor -- raw ordq_item_t* values that persist across
# separate next_job() calls and can be freed in between by a properly-locked
# on_queue_update() re-sort of the same queue). It #includes
# ../src/plugins/builtin_fcfs.c directly (same white-box pattern as
# sched_picker_fcfs_session_test) so it exercises the real cursor logic and
# the real private fcfs_plugin_data_t layout -- including direct field
# access to priv->cursor/priv->job_cursor, which only a white-box #include
# gives a test -- not a re-implementation of any of it. Supplies its own
# always-ready sched_queue_is_ready()/sched_job_is_ready() test doubles and a
# plugin_register() stub, same convention as sched_picker_fcfs_session_test.
sched_builtin_fcfs_cursor_uaf_test = executable(
'sched_builtin_fcfs_cursor_uaf_test',
['test_builtin_fcfs_cursor_uaf.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',
@ -1288,3 +1307,6 @@ test('scheduler dispatch commit test',
env: ['ASAN_OPTIONS=detect_leaks=0'])
test('scheduler limit plugin test', sched_limit_plugin_test, suite: 'scheduler',
env: ['ASAN_OPTIONS=detect_leaks=0'])
test('scheduler builtin fcfs cursor uaf test',
sched_builtin_fcfs_cursor_uaf_test, suite: 'scheduler',
env: ['ASAN_OPTIONS=detect_leaks=0'])

227
src/scheduler/testsuite/test_builtin_fcfs_cursor_uaf.c Normal file
View file

@ -0,0 +1,227 @@
/*
* test_builtin_fcfs_cursor_uaf.c - white-box regression test for bug-1686: a
* live SIGSEGV in fcfs_cursor_enter_queue() (../src/plugins/builtin_fcfs.c),
* reached from builtin_fcfs_next_job() <- sched_job_picker() (scheduler.c)
* <- picker thread, captured from a real 3-node HA deployment.
*
* Root cause: fcfs_plugin_data_t.cursor/job_cursor are raw ordq_item_t*
* values that PERSIST in fcfs_plugin_data_t across SEPARATE
* builtin_fcfs_next_job() calls, each with its own independent
* fcfs_plugin_data_t.lock acquisition/release (see that field's own comment
* and the file's header "Concurrency" comment). fcfs_plugin_data_t.lock
* correctly makes each individual hook call atomic with respect to the
* others, but it cannot protect a pointer VALUE that outlives the call that
* read it: a properly-locked builtin_fcfs_on_queue_update() call for a
* queue that already has a registered fcfs_queue_t re-sorts it by removing
* the queue's existing ordq_item_t node (ordq_get + FREEUP(it)) and
* inserting a brand-new one (ordq_put, fresh calloc). If priv->cursor is
* resting on that exact node -- which it normally is, since the cursor
* stays parked on a queue for as long as it keeps yielding ready jobs
* (sched_queue_is_ready()/sched_job_is_ready() are unconditional `return 1`
* stubs today in scheduler.c, so a queue's cursor never advances away from
* it while jobs remain) -- the NEXT next_job() call dereferences
* priv->cursor->data from freed memory: a real heap-use-after-free, live-
* reproduced as the SIGSEGV this bug report captured (queue_item was a
* dangling, freed ordq_item_t*).
*
* Critically, this is deterministically reproducible SINGLE-THREADED: since
* next_job() releases priv->lock before returning, calling next_job(), then
* on_queue_update() for the SAME queue name, then next_job() again -- all
* sequentially on one thread, exactly as this test does -- hits the same
* use-after-free with no real concurrency required. ASAN catches it as a
* genuine heap-use-after-free even single-threaded.
*
* Fix (already applied, three sites, see builtin_fcfs.c's own file header
* comment and each site's comment for the full mechanism):
* 1. builtin_fcfs_on_queue_update()'s reuse branch: before FREEUP(it), if
* priv->cursor == it, NULLs both priv->cursor and priv->job_cursor.
* 2. builtin_fcfs_on_job_state_change()'s !JOB_IS_PEND removal branch:
* ordq_peek()s the about-to-be-deleted job node first; if it equals
* priv->job_cursor, NULLs priv->job_cursor before ordq_del().
* 3. fcfs_remove_job_everywhere(): same peek-then-invalidate-then-delete
* pattern inside its loop over all queues.
* Always safe: next_job() already treats a NULL cursor as "no more
* candidates this round" (a pre-existing, unrelated-to-this-bug branch --
* see test_picker_fcfs_session.c's own no-queues/empty-queue coverage), and
* begin_session() unconditionally resets both cursors fresh every round, so
* the worst case of an invalidation is a bounded one-round delay for the
* jobs on the invalidated queue, never a crash or a permanently lost job.
* This test exercises exactly site 1 (on_queue_update's reuse-and-resort
* path), the one the bug report's own crash actually walked into.
*
* RED (fix sites reverted): builtin_fcfs_next_job()'s second call
* dereferences the freed ordq_item_t node priv->cursor still points at --
* a genuine ASAN heap-use-after-free abort, not a clean TEST_ASSERT
* failure, matching bug-1686's captured live SIGSEGV signature
* (fcfs_cursor_enter_queue <- builtin_fcfs_next_job, dereferencing a
* freed/dangling ordq_item_t*).
* GREEN (fix applied): priv->cursor/job_cursor are observably NULLed by
* on_queue_update()'s reuse branch before the freed node's lifetime ends;
* the next next_job() call returns NULL cleanly (no crash); and a
* subsequent begin_session()/next_job() round finds every job again,
* proving no job is permanently lost -- only delayed one round.
*
* #include's the real ../src/plugins/builtin_fcfs.c (white-box, same
* convention as test_picker_fcfs.c/test_picker_fcfs_session.c) so this test
* exercises the real cursor logic and the real private fcfs_plugin_data_t
* layout -- including direct field access to priv->cursor/priv->job_cursor,
* which only a white-box #include can give a test -- not a re-implementation
* of any of it.
*
* builtin_fcfs.c's next_job() calls sched_queue_is_ready()/
* sched_job_is_ready() (declared in scheduler.h, defined in scheduler.c).
* This translation unit does not link scheduler.c (same as
* test_picker_fcfs_session.c), so it supplies its own always-ready test
* doubles below -- this test never needs to force a skip, unlike that
* file's coverage of the skip branches.
*/
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unity/unity.h>
#include "plugin.h"
#include "sched/types.h"
/* Always-ready test doubles -- see file header. Neither queue nor job
* readiness is under test here, only cursor lifetime across separate
* next_job()/on_queue_update() calls. */
int sched_queue_is_ready(xce_sched_queue_t* q)
{
(void)q;
return 1;
}
int sched_job_is_ready(xce_sched_job_t* j)
{
(void)j;
return 1;
}
/* register_builtin_fcfs() (bottom of builtin_fcfs.c) calls plugin_register()
* (plugin.c, not linked into this white-box TU). Stub to satisfy the
* linker -- this file's test drives the hooks directly, never
* register_builtin_fcfs() itself. Same convention as
* test_picker_fcfs_session.c's own stub. */
int plugin_register(void* arg1, void* arg2)
{
(void)arg1;
(void)arg2;
return 0;
}
#include "../src/plugins/builtin_fcfs.c"
void setUp(void) { return; }
void tearDown(void) { return; }
/* ---------------------------------------------------------------------- */
/* Fixture -- same convention as test_picker_fcfs_session.c's fixture_init:
* "picker" is passed to every hook as this struct's address. Nothing is
* torn down between tests (suite convention, see meson.build:
* detect_leaks=0). */
static void fixture_init(xce_sched_plugin_t* plugin)
{
memset(plugin, 0, sizeof(*plugin));
}
/*
* THE regression: reproduces bug-1686's exact use-after-free sequence
* single-threaded (see file header for why single-threaded is sufficient) --
* next_job() once, then on_queue_update() re-sorting the SAME queue (the
* live crash's own trigger), then next_job() again.
*/
static void test_cursor_invalidated_on_queue_resort_no_uaf(void)
{
xce_sched_plugin_t plugin;
xce_sched_queue_t q1 = { .name = "Q1", .priority = 5 };
xce_sched_job_t j1 = { .jobid = 1,
.priority = 5,
.queue_time = 1,
.state = JOB_STAT_PEND,
.queue = &q1 };
xce_sched_job_t j2 = { .jobid = 2,
.priority = 1,
.queue_time = 2,
.state = JOB_STAT_PEND,
.queue = &q1 };
fcfs_plugin_data_t* priv;
xce_sched_job_t* got;
fixture_init(&plugin);
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_init(&plugin, NULL));
priv = (fcfs_plugin_data_t*)plugin.data;
TEST_ASSERT_NOT_NULL(priv);
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_on_queue_update(&plugin, &q1));
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_on_job_push(&plugin, &j1));
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_on_job_push(&plugin, &j2));
/* (b) */
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_begin_session(&plugin));
/* (c) dispatch job 1; cursor stays parked on Q1's queue node (only
* queue, and it still has job 2 pending -- see fcfs_cursor_enter_queue's
* own doc comment: cursor only advances when a queue's job_cursor runs
* out or the queue itself is skipped, neither of which happens here). */
got = builtin_fcfs_next_job(&plugin);
TEST_ASSERT_NOT_NULL(got);
TEST_ASSERT_EQUAL_UINT64(1, got->jobid);
TEST_ASSERT_NOT_NULL(priv->cursor);
TEST_ASSERT_EQUAL_PTR(&q1, ((fcfs_queue_t*)priv->cursor->data)->queue);
TEST_ASSERT_NOT_NULL(priv->job_cursor);
/* (d) re-sort the SAME queue (different priority genuinely exercises
* the reuse-and-resort path) -- this is what frees the OLD ordq_item_t
* node priv->cursor is resting on. */
q1.priority = 9;
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_on_queue_update(&plugin, &q1));
/* (e) post-fix: both cursors observably NULLed before the free. */
TEST_ASSERT_NULL(priv->cursor);
TEST_ASSERT_NULL(priv->job_cursor);
/* (f) THE use-after-free reproduction point: pre-fix, priv->cursor
* still points at the node FREEUP(it) already freed in (d), and this
* call dereferences priv->cursor->data from freed memory -- a genuine
* ASAN heap-use-after-free abort matching bug-1686's live SIGSEGV.
* Post-fix, priv->cursor is NULL, so next_job()'s outer while loop never
* executes and this returns NULL cleanly: "no more candidates this
* round" for the (invalidated) Q1. */
got = builtin_fcfs_next_job(&plugin);
TEST_ASSERT_NULL(got);
/* stays NULL on a repeated call, same as ordinary exhaustion */
TEST_ASSERT_NULL(builtin_fcfs_next_job(&plugin));
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_end_session(&plugin));
/* (g) no job is permanently lost -- the next round's begin_session()
* resets the cursor to priv->queues.head fresh, regardless of the
* invalidation, and finds every pending job again (job 1, never
* on_dispatch()'d, is still a candidate; job 2 was never reached before
* the invalidation). */
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_begin_session(&plugin));
got = builtin_fcfs_next_job(&plugin);
TEST_ASSERT_NOT_NULL(got);
TEST_ASSERT_EQUAL_UINT64(1, got->jobid);
got = builtin_fcfs_next_job(&plugin);
TEST_ASSERT_NOT_NULL(got);
TEST_ASSERT_EQUAL_UINT64(2, got->jobid);
TEST_ASSERT_NULL(builtin_fcfs_next_job(&plugin));
TEST_ASSERT_EQUAL_INT(0, builtin_fcfs_end_session(&plugin));
builtin_fcfs_fini(&plugin);
}
int main(void)
{
UNITY_BEGIN();
RUN_TEST(test_cursor_invalidated_on_queue_resort_no_uaf);
return UNITY_END();
}