fix(live-debugger): fix use-after-free, tags leak, and Windows 7.x crash on probe removal#4036
Conversation
…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.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d4ab496dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
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.
Leiyks
changed the title
…; 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.
Benchmarks [ tracer ]Benchmark execution time: 2026-07-14 03:15:00 Comparing candidate commit f4b2785 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 193 metrics, 0 unstable metrics.
|
f728dc3 to
cef3cd0
Compare
8fdcc12 to
58f16aa
Compare
Leiyks
changed the title
…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.
58f16aa to
f4b2785
Compare
bwoebi
left a comment
bwoebi
left a comment
There was a problem hiding this comment.
I'm still not persuaded that probe_ids can effectively be shared, but it won't break anything. So let's do that.
2 participants
Fixes live-debugger probe-lifetime bugs surfaced by a flaky
LEAKEDondebugger_enable_dynamic_config.phpt, plus a Windows-only crash the new regression test exposed.Fixes
spans_mapwas keyed inconsistently —probe.idon apply/remove vsconfig_idon the DI-reinstall path. Two configs sharing aprobe.idorphaned a hook whose parsed-config box was then freed, so its next fire read the freedprobe.id. Now keyed byconfig_ideverywhere. Reproduced on PHP 7.1 & 8.3 under valgrind.tagsleak.probe.into()allocates aCharSliceVecfortagsthatdd_probe_dtornever freed. Addedddog_drop_probe(consumes the FFI probe by value) so drop glue freestags+ nested allocations in one place.zai_hook_remove's scope arg into an unconditionalZSTR_LEN(def->scope), a NULL deref for global functions. Build thezai_strviews with explicitif-guards. PHP 8.x (MSVC 2019) / Linux unaffected.Also drops the stale
active_live_debugger_hooksentry on removal so a later lookup of the same id can't use-after-free the freeddef.Test & verification
tests/ext/live-debugger/debugger_remove_shared_probe_id.phptinstalls two configs sharing aprobe.id(distinct tags), removes both without firing, then calls the target — exercising the UAF, the tags allocation, and the Windows removal path. Green across the fulltest_cmatrix: windows 7.2–8.5 and ASAN (Linux) 7.4–8.5; valgrind clean (0 bytes lost, no double-free).