From cfa0a941669e27767d667f6b989df9cbd90594b9 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 30 Sep 2026 02:47:24 +0000 Subject: [PATCH] fix(scheduler): the simulator's threads stop when its run returns (bug-1632, bug-1763) sched_sim_driver_run() started the picker and allocator threads on &sim->core and returned with them still running; only sched_sim_driver_fini() stopped them, and 24 test files (35 call sites) run the driver on a stack-local sim without calling it. The leftover threads kept a pointer into the dead frame, and ASAN occasionally caught one reading it (stack-use-after-return in cbuf_get) in sched_sim_verify_test and sched_sim_record_output_test. sched_sim_driver_run() now wraps the loop and stops both threads on every return (new sched_sim_driver_stop_threads(), shared with fini). If the allocator fails to start, the running picker is stopped too. The stop no longer pushes a jobid 0 to wake the allocator: cbuf_get/put wait in pthread_cond_wait, a cancellation point, and the fake end-of-session was visible to the allocator. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/scheduler/include/sim.h | 17 ++++++++-- src/scheduler/src/sim.c | 35 ++++++++++++------- src/scheduler/src/sim_loop.c | 26 ++++++++++++-- src/scheduler/testsuite/test_sim_verify.c | 41 +++++++++++++++++++++++ 4 files changed, 101 insertions(+), 18 deletions(-) diff --git a/src/scheduler/include/sim.h b/src/scheduler/include/sim.h index 019a3ad0..dc76a5c7 100644 --- a/src/scheduler/include/sim.h +++ b/src/scheduler/include/sim.h @@ -256,15 +256,26 @@ extern int sched_sim_driver_start(xce_sched_sim_driver_t* sim, * marker is reached, Req 5 AC 20), or a negative errno: whatever * sched_sim_driver_start() returned on a startup failure, or a pthread * error standing up the picker-driving thread the event loop needs (see - * sim_loop.c's own file header comment for why one exists). + * sim_loop.c's own file header comment for why one exists). Whatever it + * returns, the picker and allocator threads it started have been stopped + * (bug-1632/bug-1763): `sim` may go out of scope right after. */ extern int sched_sim_driver_run(xce_sched_sim_driver_t* sim, const sched_sim_args_t* args); +/* + * sched_sim_driver_stop_threads - bug-1632/bug-1763: cancels and joins the + * picker and allocator threads sched_sim_driver_run() started (the way + * sched_fini() does in normal mode). sched_sim_driver_run() calls it on + * every return, so no thread outlives the run holding a pointer to `sim` + * (callers commonly keep it on the stack). A no-op when none are running. + */ +extern void sched_sim_driver_stop_threads(xce_sched_sim_driver_t* sim); + /* * sched_sim_driver_fini - bug-1884: releases what a sched_sim_driver_run() - * that returned 0 left behind: cancels and joins the picker and allocator - * threads (the way sched_fini() does in normal mode), closes the replay, + * that returned 0 left behind: stops the picker and allocator threads if + * still running (see sched_sim_driver_stop_threads), closes the replay, * alternate-events and output files, and finalizes the core. Call it only * after sched_sim_driver_run() returned 0; a failed run has either * finalized the core already (a startup failure) or is about to exit. diff --git a/src/scheduler/src/sim.c b/src/scheduler/src/sim.c index ff8bfda0..637f65de 100644 --- a/src/scheduler/src/sim.c +++ b/src/scheduler/src/sim.c @@ -709,27 +709,36 @@ static void sim_free_alloc_wrappers_(xce_scheduler_t* wrappers[2]) } } +void sched_sim_driver_stop_threads(xce_sched_sim_driver_t* sim) +{ + void* res; + + if ((sim == NULL) || !sim->allocator_thread_live) { + return; + } + + /* Both threads block only in cbuf_get()/cbuf_put(), whose + * pthread_cond_wait() is a cancellation point (cbuf releases its mutex + * in a cleanup handler), so cancel + join needs no wake-up. A jobid 0 + * pushed to wake the allocator would read as a real end-of-session. */ + pthread_cancel(sim->core.jobpicker); + pthread_join(sim->core.jobpicker, &res); + + pthread_cancel(sim->core.allocator); + pthread_join(sim->core.allocator, &res); + + sim->allocator_thread_live = false; +} + void sched_sim_driver_fini(xce_sched_sim_driver_t* sim) { - uint64_t jobid = 0; xce_scheduler_t* wrappers[2] = { NULL, NULL }; - void* res; if (sim == NULL) { return; } - if (sim->allocator_thread_live) { - pthread_cancel(sim->core.jobpicker); - pthread_join(sim->core.jobpicker, &res); - - pthread_cancel(sim->core.allocator); - /* try to wake up allocator */ - cbuf_tryput(&(sim->core.jbuf), &jobid); - pthread_join(sim->core.allocator, &res); - - sim->allocator_thread_live = false; - } + sched_sim_driver_stop_threads(sim); if (sim->record_file != NULL) { fclose(sim->record_file); diff --git a/src/scheduler/src/sim_loop.c b/src/scheduler/src/sim_loop.c index 1812322f..0bd512ef 100644 --- a/src/scheduler/src/sim_loop.c +++ b/src/scheduler/src/sim_loop.c @@ -1379,8 +1379,8 @@ static void sim_reconcile_unaccounted_jobs(xce_sched_sim_driver_t* sim, * later, unrelated sim run in the same test binary) would read a * dangling/reused pointer. */ -int sched_sim_driver_run(xce_sched_sim_driver_t* sim, - const sched_sim_args_t* args) +static int sim_driver_run_body(xce_sched_sim_driver_t* sim, + const sched_sim_args_t* args) { xce_core_event_t* buf_events = NULL; replay_event_t* buf_decoded = NULL; @@ -1443,8 +1443,13 @@ int sched_sim_driver_run(xce_sched_sim_driver_t* sim, rc = pthread_create(&(sim->core.allocator), NULL, sched_allocator, &(sim->core)); if (rc != 0) { + void* res; + ERROR("%s failed to start the simulation allocator thread: %s", __func__, strerror(rc)); + /* bug-1632: the picker is already running and would outlive `sim` */ + pthread_cancel(sim->core.jobpicker); + pthread_join(sim->core.jobpicker, &res); xce_core_set_active_for_logging(NULL); return -rc; } @@ -1795,3 +1800,20 @@ int sched_sim_driver_run(xce_sched_sim_driver_t* sim, return 0; } + +/* + * bug-1632/bug-1763: the picker and allocator threads the body starts hold + * &sim->core, and most callers keep `sim` on their stack and never call + * sched_sim_driver_fini() -- a thread left running read that frame after it + * was gone (ASAN stack-use-after-return in cbuf_get). The threads end with + * the run, on every return path. + */ +int sched_sim_driver_run(xce_sched_sim_driver_t* sim, + const sched_sim_args_t* args) +{ + int rc = sim_driver_run_body(sim, args); + + sched_sim_driver_stop_threads(sim); + + return rc; +} diff --git a/src/scheduler/testsuite/test_sim_verify.c b/src/scheduler/testsuite/test_sim_verify.c index 8d52da61..3fe74548 100644 --- a/src/scheduler/testsuite/test_sim_verify.c +++ b/src/scheduler/testsuite/test_sim_verify.c @@ -1579,6 +1579,46 @@ static void test_verify_against_self_reports_zero_mismatches(void) printf("test_verify_against_self_reports_zero_mismatches: PASS\n"); } +/* + * bug-1632/bug-1763: sched_sim_driver_run() must not return with its picker + * and allocator threads still running. Nearly every caller (this file's + * run_sim_and_capture_stdout() among them) runs the driver on a stack-local + * xce_sched_sim_driver_t and never calls sched_sim_driver_fini(); threads + * left running kept a pointer into that dead frame, and ASAN caught one + * reading it (stack-use-after-return in cbuf_get). + */ +static void test_run_returns_with_its_threads_stopped(void) +{ + char path[PATH_MAX]; + FILE* f; + sched_sim_args_t args; + xce_sched_sim_driver_t sim; + int rc; + + f = write_header_and_snapshot(path, sizeof(path), "_threads"); + TEST_ASSERT_NOT_NULL(f); + + append_queue_update_event(f, 1000ULL, "normal"); + append_jobs_push_event(f, 1500ULL, 777ULL, "normal"); + append_replay_session_with_pend(f, 2000ULL, 777ULL, 0); + append_end_of_stream_marker(f); + fclose(f); + + memset(&args, 0, sizeof(args)); + args.replay_path = path; + args.sim_start_ts = SCHED_SIM_START_TS_LATEST; + + rc = sched_sim_driver_run(&sim, &args); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_FALSE_MESSAGE(sim.allocator_thread_live, + "bug-1632: sched_sim_driver_run() returned with its picker and " + "allocator threads still running"); + + sched_sim_driver_fini(&sim); + printf("test_run_returns_with_its_threads_stopped: PASS\n"); +} + /* * Observable (b): replaying against a deliberately-altered copy (recorded * main_reason flipped to 99, which the real picker thread can never @@ -2005,6 +2045,7 @@ int main(void) test_verify_with_multiple_already_resolved_jobs_across_rounds_reports_no_mismatch); RUN_TEST( test_reconciliation_no_longer_masks_a_pend_jobs_dispatch_mismatch); + RUN_TEST(test_run_returns_with_its_threads_stopped); return UNITY_END(); }