feat(crashtracking)!: API to unpatch crashtracker GOT patches - #2492
feat(crashtracking)!: API to unpatch crashtracker GOT patches#2492gyuheon0h wants to merge 1 commit into
Conversation
gyuheon0h
changed the title
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
gyuheon0h
changed the title
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results✅ No issues found! 📦
|
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: e906f40 | Docs | View more details | Give us feedback! |
BenchmarksComparisonBenchmark execution time: 2026-09-11 18:41:13 Comparing candidate commit e906f40 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.
|
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8175M CPU @ 2.50GHz | e906f40 | 1789151263 | gyuheon0h/PROF-15940-unpatch-crashtracker-GOT-api |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| receiver_entry_point/report/2644 | execution_time | 6.299ms | 6.460ms ± 0.035ms | 6.462ms ± 0.017ms | 6.481ms | 6.502ms | 6.519ms | 6.529ms | 1.05% | -1.722 | 5.022 | 0.54% | 0.002ms | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| receiver_entry_point/report/2644 | execution_time | [6.455ms; 6.464ms] or [-0.074%; +0.074%] | None | None | None |
Baseline
Baseline benchmark details
Group 1
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8175M CPU @ 2.50GHz | 3dff449 | 1789151263 | gyuheon0h/got-patch-sigaction |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| receiver_entry_point/report/2644 | execution_time | 6.286ms | 6.372ms ± 0.033ms | 6.370ms ± 0.012ms | 6.384ms | 6.414ms | 6.455ms | 6.644ms | 4.30% | 2.840 | 22.726 | 0.51% | 0.002ms | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| receiver_entry_point/report/2644 | execution_time | [6.367ms; 6.377ms] or [-0.071%; +0.071%] | None | None | None |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
bc7c43c to
9e0b34b
Compare
![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: 9e0b34bd99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| target_os = "linux", | ||
| target_pointer_width = "64" | ||
| ))] | ||
| pub use collector::uninstall_hooks; |
There was a problem hiding this comment.
The new function is only a Rust API, so the SDKs that load the generated libdatadog shared library cannot call it before dlclose. The existing initialization path is exposed as ddog_crasht_init through libdd-crashtracker-ffi, and libdd-profiling-ffi only re-exports that FFI crate; a repository-wide search finds no corresponding #[no_mangle] extern "C" uninstall function. Add the FFI wrapper so the intended unloading tracers can actually use this feature.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| super::assert_interceptor::uninstall_assert_hook(); | ||
| super::sigaction_interceptor::uninstall_sigaction_hook(); |
There was a problem hiding this comment.
When init succeeds, it also registers handle_posix_sigaction as the process signal handler and installs a Rust panic-hook closure (api.rs lines 87-88), but this teardown only removes the two GOT hooks. After the containing shared library is unloaded, those process-global callbacks still point into its unmapped code, so a monitored signal or Rust panic jumps to a dangling address despite the caller having invoked the documented pre-unload API. The teardown needs to restore the previous signal and panic handlers as well, or it must not be presented as the operation to call before unloading.
Useful? React with 👍 / 👎.
| // SAFETY: all libraries patched at hook time are still loaded | ||
| // (the crashtracker itself is live, so the process hasn't exited). | ||
| // The caller must ensure sigaction is not called concurrently. | ||
| unsafe { libdd_gotter::unhook_symbol(&result, hook_sigaction as *const () as usize) }; |
There was a problem hiding this comment.
If mprotect fails or a slot no longer contains this hook—for example because another interposer was installed later—unhook_symbol reports fewer restored entries, but this call discards that result and then clears both the saved HookResult and ORIG_SIGACTION_FN. The public API consequently reports success and cannot retry even though GOT slots or a later hook's saved original may still reference libdatadog, making the subsequent unload unsafe. Return a structured error and retain enough state to retry or tell the caller not to unload; the identical assert-hook call has the same issue.
AGENTS.md reference: AGENTS.md:L72-L73
Useful? React with 👍 / 👎.
| // SAFETY: all libraries patched at hook time are still loaded. | ||
| unsafe { libdd_gotter::unhook_symbol(&result, hook_assert_fail as *const () as usize) }; |
There was a problem hiding this comment.
If any DSO whose GOT was patched during init is dlclosed before this API runs, the saved raw slot addresses no longer satisfy unhook_symbol's requirement that every address remain mapped. The public safe API neither documents nor enforces that lifetime; worse, if another mapping reuses an old address, make_writable can succeed and the CAS can modify unrelated memory. This is especially relevant to an API intended for runtimes that unload extensions, so the implementation must associate slots with loaded-object identity and skip stale/reused mappings, or require and expose the lifetime precondition as an unsafe contract.
Useful? React with 👍 / 👎.
9e0b34b to
62f5c05
Compare
4405231 to
4180f3b
Compare
4180f3b to
4c9f499
Compare
62f5c05 to
42bfd62
Compare
4c9f499 to
b63b056
Compare
0b72ac4 to
c3a07e9
Compare
b63b056 to
16ba2cd
Compare
bdc3300 to
9b1f7c1
Compare
16ba2cd to
3dff449
Compare
9b1f7c1 to
e906f40
Compare
1 participant
What does this PR do?
Provides an API to unpatch the two symbols that crashtracker patches --
assert_failandsigaction.Motivation
Some tracers unload extensions. Unloading libdatadog after its already updated the GOT entries means that symbols now point to now-unmapped areas. We should provide an API for users to call before unloading.
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.