From fcca4c9cfa67d6190a3375f8f2cd99652b62b9d7 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 8 Oct 2026 15:44:53 +0000 Subject: [PATCH] fix(scheduler): stop sim threads before releasing trace settings on quit (bug-1925) repl_cmd_quit() (quit and EOF) released the trace settings while the picker and allocator threads could still run; at a mid-session breakpoint the allocator reads core->trace_ctl unlocked on every job pop, a possible use-after-free just before exit. quit now cancels and joins both threads first, as sched_sim_driver_run() does on return. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/scheduler/src/repl.c | 7 ++ src/scheduler/testsuite/meson.build | 3 + .../testsuite/test_repl_trace_wiring.c | 105 +++++++++++++++++- 3 files changed, 109 insertions(+), 6 deletions(-) diff --git a/src/scheduler/src/repl.c b/src/scheduler/src/repl.c index 3ead469a..c00dac5b 100644 --- a/src/scheduler/src/repl.c +++ b/src/scheduler/src/repl.c @@ -347,9 +347,16 @@ static void repl_print_help(void) * sim->record_file (sim_loop.c) so a `quit` mid-session leaves any partial * --sim-output write as durable as a normal completion would. Never * returns. + * + * bug-1925: at a breakpoint mid-session the allocator keeps working and + * reads core->trace_ctl unlocked on every job pop, so the picker and + * allocator are stopped (cancel + join; both park only in cbuf_get/put, + * cancellation points; a no-op when none run) before + * repl_trace_shutdown() frees the ctl. Nothing after this needs them. */ static void repl_cmd_quit(xce_sched_sim_driver_t* sim) { + sched_sim_driver_stop_threads(sim); repl_trace_shutdown(sim); /* flush and close trace files (8.6) */ if (sim->record_file != NULL) { fflush(sim->record_file); diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index a5bc10e2..5c73840e 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -2863,6 +2863,9 @@ sched_repl_trace_wiring_test = executable( sched_repl_trace_wiring_test_srcs, include_directories:[incdir], dependencies: [scheduler_deps, unity_dep], + # bug-1925: the wrap checks the quit/EOF path stops the picker and + # allocator threads before it releases the trace settings. + link_args: ['-Wl,--wrap=repl_trace_shutdown'], install: false) test('scheduler repl trace wiring test', diff --git a/src/scheduler/testsuite/test_repl_trace_wiring.c b/src/scheduler/testsuite/test_repl_trace_wiring.c index c53462a0..31dfd839 100644 --- a/src/scheduler/testsuite/test_repl_trace_wiring.c +++ b/src/scheduler/testsuite/test_repl_trace_wiring.c @@ -19,7 +19,8 @@ * - `set output` rejects a trace file still open for writing, including * one kept open by the session in progress and a hard link to it, and * leaves the output file unchanged (8.4); - * - quit and end of input shut tracing down (8.6); + * - quit and end of input shut tracing down (8.6), and only after the + * picker and allocator threads are stopped (bug-1925); * - the continue-mode rollup reports a traced round on its own, with the * range-flush idle rule, and never starts a collapsed range from it * (7.3, 7.4); @@ -28,6 +29,7 @@ #define _GNU_SOURCE #include #include +#include #include #include #include @@ -52,6 +54,13 @@ static char g_dir[64]; static char g_cwd[4096]; static int g_report_fd = -1; +/* bug-1925: what the first repl_trace_shutdown() call saw (-1: not called + * yet; 0: no thread live; 1: a picker/allocator thread still live), and how + * many stand-in threads have been cancelled. */ +static int g_shutdown_saw_live = -1; +static int g_standins_started = 0; +static int g_standins_exited = 0; + /* stdout capture */ static int g_cap_fd = -1; static int g_cap_saved = -1; @@ -66,6 +75,9 @@ void setUp(void) memset(&g_sim, 0, sizeof(g_sim)); memset(&g_rs, 0, sizeof(g_rs)); + g_shutdown_saw_live = -1; + g_standins_started = 0; + g_standins_exited = 0; g_rs.units_mode = XCE_REPL_UNITS_BOTH; g_rs.mode = XCE_REPL_MODE_STEP; g_sim.interactive = true; @@ -460,10 +472,64 @@ static void test_set_output_rejects_file_kept_open_by_session(void) TEST_ASSERT_FALSE(repl_trace_file_conflicts(&g_sim, "a.trace")); } -/* ---- quit and end of input (8.6) ----------------------------------------- */ +/* ---- quit and end of input (8.6, bug-1925) ------------------------------ */ + +/* Linked with -Wl,--wrap=repl_trace_shutdown: notes whether a picker or + * allocator thread was still live when the trace settings were released. + * The allocator reads core->trace_ctl unlocked on every job pop + * (sched_allocator()), so a live one could use the ctl after the free. */ +void __real_repl_trace_shutdown(xce_sched_sim_driver_t* sim); +void __wrap_repl_trace_shutdown(xce_sched_sim_driver_t* sim) +{ + if (g_shutdown_saw_live < 0) { + g_shutdown_saw_live = + (sim->allocator_thread_live + || (__atomic_load_n(&g_standins_exited, __ATOMIC_SEQ_CST) + < g_standins_started)) + ? 1 + : 0; + } + __real_repl_trace_shutdown(sim); +} + +static void standin_exited(void* arg) +{ + (void)arg; + __atomic_add_fetch(&g_standins_exited, 1, __ATOMIC_SEQ_CST); +} + +/* Stands in for the picker or allocator of a session paused at the REPL: + * parked in a cancellation point, as the real ones are in cbuf_get/put. */ +static void* standin_thread(void* arg) +{ + (void)arg; + pthread_cleanup_push(standin_exited, NULL); + for (;;) { + pause(); + } + pthread_cleanup_pop(0); + return NULL; +} + +/* Gives the child a live picker and allocator, as a session paused at a + * breakpoint has (sim_loop.c sets allocator_thread_live once both run). */ +static void start_standin_threads(void) +{ + if ((pthread_create(&(g_sim.core.jobpicker), NULL, standin_thread, NULL) + != 0) + || (pthread_create(&(g_sim.core.allocator), NULL, standin_thread, + NULL) + != 0)) { + _exit(98); + } + g_standins_started = 2; + g_sim.allocator_thread_live = true; +} /* Runs in the child at exit(), after the quit path ran: reports whether - * tracing was shut down, then frees the fixture so the child exits clean. */ + * tracing was shut down ('N' if not) and, with threads, whether they were + * stopped before it ('L' if one was still live), then frees the fixture so + * the child exits clean. */ static void report_at_exit(void) { char ok = ((g_rs.trace.next == NULL) && (g_rs.trace.cur == NULL) @@ -471,12 +537,15 @@ static void report_at_exit(void) ? 'Y' : 'N'; + if ((ok == 'Y') && (g_shutdown_saw_live != 0)) { + ok = 'L'; + } (void)!write(g_report_fd, &ok, 1); close(g_report_fd); fixture_fini(); } -static void assert_child_shuts_down(bool eof) +static void assert_child_shuts_down(bool eof, bool threads) { int fds[2]; pid_t pid; @@ -499,6 +568,9 @@ static void assert_child_shuts_down(bool eof) g_report_fd = fds[1]; dup2(devnull, STDIN_FILENO); dup2(devnull, STDOUT_FILENO); + if (threads) { + start_standin_threads(); + } atexit(report_at_exit); if (eof) { repl_command_loop(&g_sim); /* readline() hits EOF */ @@ -513,12 +585,31 @@ static void assert_child_shuts_down(bool eof) TEST_ASSERT_EQUAL_INT(pid, waitpid(pid, &status, 0)); TEST_ASSERT_TRUE(WIFEXITED(status)); TEST_ASSERT_EQUAL_INT(0, WEXITSTATUS(status)); + TEST_ASSERT_NOT_EQUAL_MESSAGE( + 'L', got, "trace settings released while threads were live"); TEST_ASSERT_EQUAL_CHAR_MESSAGE('Y', got, "tracing still active at exit"); } -static void test_quit_shuts_tracing_down(void) { assert_child_shuts_down(false); } +static void test_quit_shuts_tracing_down(void) +{ + assert_child_shuts_down(false, false); +} -static void test_eof_shuts_tracing_down(void) { assert_child_shuts_down(true); } +static void test_eof_shuts_tracing_down(void) +{ + assert_child_shuts_down(true, false); +} + +/* bug-1925: `quit` at a breakpoint mid-session, picker and allocator live. */ +static void test_quit_stops_threads_before_trace_shutdown(void) +{ + assert_child_shuts_down(false, true); +} + +static void test_eof_stops_threads_before_trace_shutdown(void) +{ + assert_child_shuts_down(true, true); +} /* ---- continue-mode rollup (7.3, 7.4) ------------------------------------- */ @@ -667,6 +758,8 @@ int main(void) RUN_TEST(test_set_output_rejects_file_kept_open_by_session); RUN_TEST(test_quit_shuts_tracing_down); RUN_TEST(test_eof_shuts_tracing_down); + RUN_TEST(test_quit_stops_threads_before_trace_shutdown); + RUN_TEST(test_eof_stops_threads_before_trace_shutdown); RUN_TEST(test_rollup_traced_round_emitted_alone_after_flush); RUN_TEST(test_rollup_traced_idle_round_reported_as_idle); RUN_TEST(test_rollup_traced_pend_only_round_not_idle);