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) <noreply@anthropic.com>
This commit is contained in:
parent
dc94da207a
commit
cfa0a94166
4 changed files with 101 additions and 18 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue