From c92d03e8408ba796598928903c6ec4131577792c Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Tue, 30 Jun 2026 14:36:17 +0200 Subject: [PATCH 1/2] fix(live-debugger): retire removed probe configs to prevent use-after-free The C side (tracer/live_debugger.c) stores a shallow copy of the FFI ddog_Probe in def->probe, whose ddog_CharSlice fields (probe id, capture config, etc.) borrow strings owned by the boxed RemoteConfigParsed kept in live_debugger.active. The Box exists specifically to give that data a stable heap address for these borrows. On a LiveDebugger Remove (and on an Entry::Occupied re-add), the box was dropped immediately, freeing those strings. But zai_hook_remove can defer the actual hook teardown, so a still-installed probe could fire afterwards and read the freed strings via ddog_send_debugger_diagnostics(&def->probe, ...) -> probe.id, a use-after-free. Only the valgrind master job observes it (plain runs read the freed-but-intact bytes); confirmed by the .mem from test_extension_ci: [7.1]. Retire removed/replaced boxes into live_debugger.retired instead of dropping them, and free them in ddog_rshutdown_remote_config, by which point all probe hooks are torn down and no PHP user code can fire a probe. Verified: builds on PHP 7.1 (libdatadog 6a6d4a535); the leaking test passes 10/10 under valgrind with the fix and is non-regressing. --- components-rs/remote_config.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/components-rs/remote_config.rs b/components-rs/remote_config.rs index 5b08de64a8..bb80b9779b 100644 --- a/components-rs/remote_config.rs +++ b/components-rs/remote_config.rs @@ -111,6 +111,13 @@ pub struct LiveDebuggerCallbacks { pub struct LiveDebuggerState { pub spans_map: HashMap, pub active: HashMap>, // Box<> for stable heap address! + // Parsed configs removed (or replaced) from `active` whose probe hooks may still be + // installed: the C side stores shallow `ddog_Probe` copies that borrow `CharSlice`s + // (probe id, capture config, shm limiter, ...) into these boxes, and `zai_hook_remove` + // can defer the actual teardown of those hooks past this point. Dropping the box here + // would free the borrowed strings while a deferred hook can still fire and read them + // (use-after-free). Keep them alive until request shutdown, when all hooks are gone. + pub retired: Vec>, pub config_id: String, pub allow_dfa: Option, pub deny_dfa: Option, @@ -392,10 +399,13 @@ pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigSt RemoteConfigProduct::LiveDebugger => { let val = Box::new((data, MaybeShmLimiter::open(limiter_index))); let rc_ref: &mut RemoteConfigState = unsafe { mem::transmute(remote_config as *mut _) }; // sigh, borrow checker + let mut retired_old = None; let entry = remote_config.live_debugger.active.entry(value.config_id); let (parsed, limiter) = match entry { Entry::Occupied(mut e) => { - e.insert(val); + // Retire (don't drop) the replaced box: an orphaned hook may + // still borrow its strings until request shutdown. + retired_old = Some(e.insert(val)); let r = e.into_mut(); (&r.0, &r.1) } @@ -407,6 +417,9 @@ pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigSt if let Some(debugger) = parsed.downcast::() { apply_config(rc_ref, debugger, limiter); } + if let Some(old) = retired_old { + rc_ref.live_debugger.retired.push(old); + } } RemoteConfigProduct::ApmTracing => { if let Some(config_data) = data.downcast::() { @@ -442,6 +455,9 @@ pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigSt if let Some(debugger) = boxed.0.downcast::() { remove_config(remote_config, debugger); } + // Don't drop `boxed` here: a deferred hook teardown can still borrow + // its strings. Retire it until request shutdown instead. + remote_config.live_debugger.retired.push(boxed); } } RemoteConfigProduct::ApmTracing => { @@ -738,6 +754,9 @@ pub extern "C" fn ddog_set_dynamic_instrumentation_enabled( #[no_mangle] pub extern "C" fn ddog_rshutdown_remote_config(remote_config: &mut RemoteConfigState) { remote_config.live_debugger.spans_map.clear(); + // All probe hooks are torn down by request shutdown, so the borrowed strings in + // retired parsed configs are no longer referenced and can be freed. + remote_config.live_debugger.retired.clear(); remote_config.dynamic_config.old_config_values.clear(); remote_config.dynamic_config.active_configs.clear(); remote_config.dynamic_config.merged_configs.clear(); From a7cfd7d2a0a378741ab27c6b2c09ff5159a85ff6 Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Tue, 30 Jun 2026 16:06:56 +0200 Subject: [PATCH 2/2] fix(live-debugger): free retired configs after hook teardown, not at rc rshutdown Code review found the previous commit freed live_debugger.retired in ddog_rshutdown_remote_config, which runs at the very start of RSHUTDOWN (ext/datadog.c:647) -- BEFORE ddtrace_rshutdown -> dd_force_shutdown_tracing / zai_hook_clean (tracer/ddtrace.c:582,588) tear down the probe hooks that borrow into those boxes. The shutdown-time span flush can itself fire a probe, so the use-after-free window was not actually closed. Move the free to a new FFI ddog_live_debugger_free_retired, invoked at the end of ddtrace_live_debugger_rshutdown() (tracer/ddtrace.c:618) -- after both zai_hook_clean calls and the active_live_debugger_hooks destroy, i.e. the last point at which a probe can fire. Still per-request, so retired stays bounded. Verified on PHP 7.1 (libdatadog 6a6d4a535): builds/links; debugger_enable_dynamic_config passes 10/10 under valgrind and the whole live-debugger dir is clean. --- components-rs/datadog.h | 2 ++ components-rs/remote_config.rs | 20 ++++++++++++++++---- tracer/live_debugger.c | 6 ++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/components-rs/datadog.h b/components-rs/datadog.h index bb08a554b1..35fa8b0781 100644 --- a/components-rs/datadog.h +++ b/components-rs/datadog.h @@ -187,6 +187,8 @@ void ddog_set_dynamic_instrumentation_enabled(struct ddog_RemoteConfigState *rem void ddog_rshutdown_remote_config(struct ddog_RemoteConfigState *remote_config); +void ddog_live_debugger_free_retired(struct ddog_RemoteConfigState *remote_config); + void ddog_shutdown_remote_config(struct ddog_RemoteConfigState*); void ddog_log_debugger_data(const struct ddog_Vec_DebuggerPayload *payloads); diff --git a/components-rs/remote_config.rs b/components-rs/remote_config.rs index bb80b9779b..c893f83520 100644 --- a/components-rs/remote_config.rs +++ b/components-rs/remote_config.rs @@ -116,7 +116,9 @@ pub struct LiveDebuggerState { // (probe id, capture config, shm limiter, ...) into these boxes, and `zai_hook_remove` // can defer the actual teardown of those hooks past this point. Dropping the box here // would free the borrowed strings while a deferred hook can still fire and read them - // (use-after-free). Keep them alive until request shutdown, when all hooks are gone. + // (use-after-free). Keep them alive until the probe hooks are torn down, then free + // them via `ddog_live_debugger_free_retired` (called from ddtrace_live_debugger_rshutdown, + // after zai_hook_clean), which bounds growth to a single request's config churn. pub retired: Vec>, pub config_id: String, pub allow_dfa: Option, @@ -754,9 +756,6 @@ pub extern "C" fn ddog_set_dynamic_instrumentation_enabled( #[no_mangle] pub extern "C" fn ddog_rshutdown_remote_config(remote_config: &mut RemoteConfigState) { remote_config.live_debugger.spans_map.clear(); - // All probe hooks are torn down by request shutdown, so the borrowed strings in - // retired parsed configs are no longer referenced and can be freed. - remote_config.live_debugger.retired.clear(); remote_config.dynamic_config.old_config_values.clear(); remote_config.dynamic_config.active_configs.clear(); remote_config.dynamic_config.merged_configs.clear(); @@ -766,6 +765,19 @@ pub extern "C" fn ddog_rshutdown_remote_config(remote_config: &mut RemoteConfigS ]); } +/// Free the parsed configs retired during this request (see `LiveDebuggerState::retired`). +/// +/// MUST be called only after all live debugger probe hooks have been torn down for the +/// request (i.e. after `zai_hook_clean`), since those hooks hold C-side `def->probe` +/// shallow copies whose `CharSlice`s / limiter pointer borrow into these boxes. It is +/// invoked from `ddtrace_live_debugger_rshutdown()` right after the hook table is +/// destroyed — NOT from `ddog_rshutdown_remote_config`, which runs at the very start of +/// RSHUTDOWN, before the shutdown-time flush that can still fire (and tear down) probes. +#[no_mangle] +pub extern "C" fn ddog_live_debugger_free_retired(remote_config: &mut RemoteConfigState) { + remote_config.live_debugger.retired.clear(); +} + #[no_mangle] pub extern "C" fn ddog_shutdown_remote_config(_: Box) {} diff --git a/tracer/live_debugger.c b/tracer/live_debugger.c index 81a5109e4a..91b2566e06 100644 --- a/tracer/live_debugger.c +++ b/tracer/live_debugger.c @@ -1677,6 +1677,12 @@ void ddtrace_live_debugger_rinit(void) { void ddtrace_live_debugger_rshutdown(void) { zend_hash_destroy(&DDTRACE_G(active_live_debugger_hooks)); + // The probe hooks (and their def->probe shallow copies borrowing into retired + // parsed configs) are now fully torn down, so it is finally safe to free the + // configs retired during this request. + if (DATADOG_G(remote_config_state)) { + ddog_live_debugger_free_retired(DATADOG_G(remote_config_state)); + } } bool ddtrace_alter_dynamic_instrumentation_config(zval *old_value, zval *new_value, zend_string *new_str) {