fix(live-debugger): fix use-after-free, tags leak, and Windows 7.x crash on probe removal (#4036)

* fix(live-debugger): key spans_map by config_id to prevent probe hook use-after-free

The live debugger tracks installed probe hooks in `spans_map`, but keyed it
inconsistently: apply_config and remove_config used the probe's `id` field,
while the DI-enable reinstall path used the `active` map key (the config_id).

`active` is keyed by config_id, which is unique per remote-config path, whereas
probe.id is not: two distinct configs can carry the same probe id. When they do
(or when a probe is installed via different paths), the second install overwrites
the probe.id-keyed spans_map entry, orphaning the first hook. Removing a config
then tears down the wrong hook (or none) and frees the parsed-config box while an
orphaned hook still borrows its strings. The orphan's first fire afterwards reads
the freed probe id in ddog_debugger_diagnostics_create_unboxed -> use-after-free.

Only valgrind observes it (plain runs read freed-but-intact bytes), which is why
it surfaced intermittently as a LEAKED test in test_extension_ci.

Key spans_map by config_id everywhere (apply_config, remove_config, and the
already-correct reinstall path) so every config's hook is tracked and removed
independently. Adds tests/ext/live-debugger/debugger_remove_shared_probe_id.phpt,
which reproduces the UAF (two configs sharing a probe id) and is clean with the fix.

Verified under valgrind on PHP 7.1 and 8.3: the reproducer leaks before the fix
and passes after; the whole live-debugger suite is clean.

* test(live-debugger): distinguish configs by an ignored field, not tags

The regression test used a distinct `tags` entry per config only to give the two
configs different remote-config paths. But a non-empty `tags` makes probe.into()
allocate an FFI CharSliceVec that the C side never frees (a separate latent leak
for tagged probes), which LeakSanitizer flags in the ASAN 'multiple observers'
job (32 bytes / 2 objects) and breaks the test's expected output.

Use an ignored unknown field instead: same two distinct config paths, same probe
id, no per-config allocation -> no unrelated LSan noise. Verified under valgrind
(clean) and the two configs still install at distinct paths with the shared id.

* fix(live-debugger): free the probe tags CharSliceVec on uninstall

probe.into() heap-allocates a CharSliceVec for the probe `tags`, but the C side
(def->probe) only freed the nested span-decoration / log allocations in
dd_probe_dtor -- never the tags vec -- so every probe carrying tags leaked it
(LeakSanitizer: 32 bytes / 2 objects in the ASAN 'multiple observers' job).

Add ddog_drop_probe, which consumes the FFI probe by value so its drop glue
frees the tags CharSliceVec together with the nested span-decoration / log
allocations, and call it from dd_probe_dtor in place of the piecemeal drops.
Cleanup is now a single, consistent operation covering every FFI-owned field.

Restore the shared-probe-id regression test to distinguish its two configs by
`tags`, so it also exercises this path (a tags leak would resurface under LSan).

Verified on PHP 8.3: the live-debugger suite is clean under valgrind (no UAF,
no double-free), and valgrind --leak-check=full on a tagged probe shows 0 bytes
definitely lost with no CharSliceVec leak record.

* fix(live-debugger): remove stale hook on re-add; wait for both probes; trim comments

Address Codex review:
- apply_config: remove any hook already installed for a config id before
  installing the replacement, so an in-place Add (Occupied entry) can't orphan
  the previous hook into the dropped parsed config.
- regression test: await both colliding probes (2) before removing, so it
  actually exercises the shared-probe-id collision.

Also condense the comments added in this PR.

* fix(live-debugger): fix PHP 7.x Windows crash and drop stale removal map entry

The regression test exposed a PHP 7.x Windows-only crash (0xC0000005) in
dd_remove_live_debugger_probe: MSVC 2017 if-converts the inline ternary that
builds zai_hook_remove's scope/function args into an unconditional ZSTR_LEN
load, a NULL deref for global functions (scope == NULL). Build the zai_str
views with explicit if-guards, as the install path already does. PHP 8.x
(MSVC 2019) and Linux were unaffected.

Also drop the stale active_live_debugger_hooks entry on removal so a later
lookup of the same id can't use-after-free the already-freed def.

Author: Leiyks

components-rs/datadog.h

@@ -189,6 +189,8 @@ void ddog_rshutdown_remote_config(struct ddog_RemoteConfigState *remote_config);
 
 void ddog_shutdown_remote_config(struct ddog_RemoteConfigState*);
 
+void ddog_drop_probe(struct ddog_Probe probe);
+
 void ddog_log_debugger_data(const struct ddog_Vec_DebuggerPayload *payloads);
 
 void ddog_log_debugger_datum(const struct ddog_DebuggerPayload *payload);

components-rs/remote_config.rs

@@ -392,6 +392,7 @@ 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 config_id = value.config_id.clone();
                             let entry = remote_config.live_debugger.active.entry(value.config_id);
                             let (parsed, limiter) = match entry {
                                 Entry::Occupied(mut e) => {
@@ -405,7 +406,7 @@ pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigSt
                                 }
                             };
                             if let Some(debugger) = parsed.downcast::<LiveDebuggingData>() {
-                                apply_config(rc_ref, debugger, limiter);
+                                apply_config(rc_ref, &config_id, debugger, limiter);
                             }
                         }
                         RemoteConfigProduct::ApmTracing => {
@@ -440,7 +441,7 @@ pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigSt
                 RemoteConfigProduct::LiveDebugger => {
                     if let Some(boxed) = remote_config.live_debugger.active.remove(&path.config_id) {
                         if let Some(debugger) = boxed.0.downcast::<LiveDebuggingData>() {
-                            remove_config(remote_config, debugger);
+                            remove_config(remote_config, &path.config_id, debugger);
                         }
                     }
                 }
@@ -472,6 +473,7 @@ pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigSt
 
 fn apply_config(
     remote_config: &mut RemoteConfigState,
+    config_id: &str,
     debugger: &LiveDebuggingData,
     limiter: &MaybeShmLimiter,
 ) {
@@ -480,12 +482,21 @@ fn apply_config(
             LiveDebuggingData::Probe(probe) => {
                 debug!("Applying live debugger probe {probe:?}");
                 if remote_config.live_debugger.di_enabled {
+                    // Tear down any hook already installed for this config before
+                    // replacing it, so it isn't left dangling into the dropped config.
+                    if let Some(old_hook_id) =
+                        remote_config.live_debugger.spans_map.remove(config_id)
+                    {
+                        (callbacks.remove_probe)(old_hook_id);
+                    }
                     let hook_id = (callbacks.set_probe)(probe.into(), limiter);
                     if hook_id >= 0 {
+                        // Key by config_id, not probe.id: distinct configs can share a
+                        // probe id, so a probe.id key would orphan a hook on removal (UAF).
                         remote_config
                             .live_debugger
                             .spans_map
-                            .insert(probe.id.clone(), hook_id);
+                            .insert(config_id.to_string(), hook_id);
                     }
                 }
                 // If di_enabled is false, probe is stored in `active` but hook is not installed.
@@ -522,12 +533,12 @@ fn apply_config(
     }
 }
 
-fn remove_config(remote_config: &mut RemoteConfigState, debugger: &LiveDebuggingData) {
+fn remove_config(remote_config: &mut RemoteConfigState, config_id: &str, debugger: &LiveDebuggingData) {
     if let Some(callbacks) = unsafe { &LIVE_DEBUGGER_CALLBACKS } {
         match debugger {
             LiveDebuggingData::Probe(probe) => {
-                if let Some(id) = remote_config.live_debugger.spans_map.remove(&probe.id) {
-                    debug!("Removing live debugger probe {}", probe.id);
+                if let Some(id) = remote_config.live_debugger.spans_map.remove(config_id) {
+                    debug!("Removing live debugger probe {} (config {})", probe.id, config_id);
                     (callbacks.remove_probe)(id);
                 }
             }
@@ -719,15 +730,15 @@ pub extern "C" fn ddog_set_dynamic_instrumentation_enabled(
                 (callbacks.remove_probe)(hook_id);
             }
         } else {
-            // Reinstall all probes currently stored in `active`.
-            for (probe_id, boxed) in remote_config.live_debugger.active.iter() {
+            // Reinstall all probes in `active`, keyed by config_id (like apply/remove).
+            for (config_id, boxed) in remote_config.live_debugger.active.iter() {
                 if let Some(LiveDebuggingData::Probe(probe)) = boxed.0.downcast::<LiveDebuggingData>() {
                     let hook_id = (callbacks.set_probe)(probe.into(), &boxed.1);
                     if hook_id >= 0 {
                         remote_config
                             .live_debugger
                             .spans_map
-                            .insert(probe_id.clone(), hook_id);
+                            .insert(config_id.clone(), hook_id);
                     }
                 }
             }
@@ -750,6 +761,12 @@ pub extern "C" fn ddog_rshutdown_remote_config(remote_config: &mut RemoteConfigS
 #[no_mangle]
 pub extern "C" fn ddog_shutdown_remote_config(_: Box<RemoteConfigState>) {}
 
+/// Free the FFI-owned allocations in a `Probe` (the `tags` vec and the nested
+/// span-decoration / log allocations) by consuming it; borrowed `CharSlice`s are
+/// left untouched. Called from `dd_probe_dtor` when a probe is uninstalled.
+#[no_mangle]
+pub extern "C" fn ddog_drop_probe(_: Probe) {}
+
 #[no_mangle]
 pub extern "C" fn ddog_log_debugger_data(payloads: &Vec<DebuggerPayload>) {
     if !payloads.is_empty() {

tests/ext/live-debugger/debugger_remove_shared_probe_id.phpt

@@ -0,0 +1,61 @@
+--TEST--
+Removing live debugger configs that share a probe id must not leave a dangling hook
+--SKIPIF--
+<?php include __DIR__ . '/../includes/skipif_no_dev_env.inc'; ?>
+--ENV--
+DD_AGENT_HOST=request-replayer
+DD_TRACE_AGENT_PORT=80
+DD_TRACE_GENERATE_ROOT_SPAN=0
+DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS=0.1
+--INI--
+datadog.trace.agent_test_session_token=live-debugger/remove_shared_probe_id
+--FILE--
+<?php
+
+require __DIR__ . "/live_debugger.inc";
+
+reset_request_replayer();
+
+function foo() {
+    $span = \DDTrace\active_span();
+    return $span ? $span->name : "removed";
+}
+
+put_dynamic_config_file([
+    "dynamic_instrumentation_enabled" => true,
+]);
+
+// Two configs at different paths sharing the SAME probe id. Keying the hook map
+// by probe id would orphan a hook on removal (use-after-free); distinct non-empty
+// tags also make the paths differ and exercise the tags allocation freed on drop.
+function put_span_probe_id1($tag) {
+    return put_live_debugger_file([
+        "id" => "1",
+        "language" => "php",
+        "evaluateAt" => "EXIT",
+        "type" => "SPAN_PROBE",
+        "where" => ["methodName" => "foo"],
+        "tags" => [$tag],
+    ]);
+}
+
+$p1 = put_span_probe_id1("a");
+$p2 = put_span_probe_id1("b");
+
+// Wait for BOTH probes to install, without calling foo() (keeps them pre-EMITTING
+// so the diagnostics read of the probe id happens on the first call, after removal).
+await_probe_installation(function () {}, 2);
+
+// Remove both configs and let the remote-config poll process the removals.
+del_rc_file($p1);
+del_rc_file($p2);
+for ($i = 0; $i < 300; $i++) {
+    usleep(10000);
+}
+
+// No hook must remain: an orphaned one would read freed memory here.
+var_dump(foo());
+
+?>
+--EXPECT--
+string(7) "removed"

tracer/live_debugger.c

@@ -128,11 +128,8 @@ static bool dd_probe_file_mismatch(dd_probe_def *def, zend_execute_data *execute
 
 static void dd_probe_dtor(void *data) {
     dd_probe_def *def = data;
-    if (def->probe.probe.tag == DDOG_PROBE_TYPE_SPAN_DECORATION) {
-        ddog_drop_span_decoration_probe(def->probe.probe.span_decoration);
-    } else if (def->probe.probe.tag == DDOG_PROBE_TYPE_LOG) {
-        ddog_drop_log_probe_capture_expressions(def->probe.probe.log);
-    }
+    // Frees the probe's FFI-owned allocations (tags + span-decoration/log).
+    ddog_drop_probe(def->probe);
     if (def->file) {
         zend_string_release(def->file);
     }
@@ -854,16 +851,28 @@ static void dd_remove_live_debugger_probe(int64_t id) {
         zend_string *scope = def->scope ? zend_string_copy(def->scope) : NULL;
         zend_string *func = def->function ? zend_string_copy(def->function) : NULL;
         def->removed = true;
-        zai_hook_remove(
-                def->scope ? (zai_str)ZAI_STR_FROM_ZSTR(def->scope) : (zai_str)ZAI_STR_EMPTY,
-                def->function ? (zai_str)ZAI_STR_FROM_ZSTR(def->function) : (zai_str)ZAI_STR_EMPTY,
-                id);
+        // Use explicit if-guards, not an inline ternary in the call args:
+        // MSVC 2017 (PHP 7.x Windows) if-converts
+        // `def->scope ? ZAI_STR_FROM_ZSTR(def->scope) : ZAI_STR_EMPTY` into an
+        // unconditional ZSTR_LEN(def->scope) — a NULL deref for global functions
+        // (scope == NULL). See PR #4036.
+        zai_str scope_str = ZAI_STR_EMPTY, func_str = ZAI_STR_EMPTY;
+        if (def->scope) {
+            scope_str = (zai_str) ZAI_STR_FROM_ZSTR(def->scope);
+        }
+        if (def->function) {
+            func_str = (zai_str) ZAI_STR_FROM_ZSTR(def->function);
+        }
+        zai_hook_remove(scope_str, func_str, id);
         if (scope) {
             zend_string_release(scope);
         }
         if (func) {
             zend_string_release(func);
         }
+        // zai_hook_remove freed `def` via the hook's aux dtor; drop the now-stale
+        // map entry so a later lookup of the same id can't use-after-free it.
+        zend_hash_index_del(&DDTRACE_G(active_live_debugger_hooks), (zend_ulong)id);
     }
 }