feat(crashtracking): GOT patch sigaction - #2448
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
gyuheon0h
changed the title
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
| oldact: *mut libc::sigaction, | ||
| ) -> libc::c_int { | ||
| // Check if this signal is one we monitor. | ||
| if (0..64).contains(&signum) |
There was a problem hiding this comment.
I don't like this but clippy wont let me make the inverse check. Havent checked but probably compiles down to the same thing
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 1875df8 | Docs | View more details | Give us feedback! |
BenchmarksComparisonBenchmark execution time: 2026-09-07 03:29:18 Comparing candidate commit 1875df8 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 | 1875df8 | 1788751023 | gyuheon0h/PROF-15892-sigaction-got |
| 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.311ms | 6.368ms ± 0.020ms | 6.368ms ± 0.013ms | 6.381ms | 6.403ms | 6.409ms | 6.425ms | 0.90% | 0.052 | 0.135 | 0.31% | 0.001ms | 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.366ms; 6.371ms] or [-0.043%; +0.043%] | 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 | 64ad2ed | 1788533433 | main |
| 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.324ms | 6.392ms ± 0.029ms | 6.389ms ± 0.017ms | 6.406ms | 6.434ms | 6.481ms | 6.542ms | 2.40% | 1.430 | 4.716 | 0.45% | 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.388ms; 6.396ms] or [-0.063%; +0.063%] | 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
|
8440a0f to
070422d
Compare
070422d to
6f13ff4
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: 6f13ff416d
ℹ️ 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".
| if (0..64).contains(&sig) { | ||
| mask |= 1u64 << sig; |
There was a problem hiding this comment.
register_crash_handlers explicitly supports signal numbers through 64 (MAX_SIGNALS is 65), but this exclusive 0..64 range omits signal 64 from the mask. When a configuration monitors SIGRTMAX on Linux, its crash handler is installed but subsequent overwrites never produce this warning. Map valid signals 1..=64 onto bits 0..63, or otherwise represent all supported signal numbers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We won't ever monitor signal num 64, and its honestly a micro optimization that isn't necessary in the first place
6f13ff4 to
54f9590
Compare
54f9590 to
1875df8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1875df89bc
ℹ️ 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".
|
|
||
| // SAFETY: hook_sigaction has the same signature as sigaction. | ||
| let result = unsafe { | ||
| libdd_gotter::hook_symbol_excluding_self(c"sigaction", hook_sigaction as *const () as usize) |
There was a problem hiding this comment.
When crashtracking is delivered through the repository's supported libdatadog_profiling.a artifact, hook_sigaction resides in the main executable, and hook_symbol_excluding_self skips that entire ELF object. Consequently, sigaction calls originating in the executable—the primary application code this feature is meant to observe—are never patched, so interception only works for calls from separate DSOs. Use a forwarding strategy that permits patching the containing executable, or exclude only the internal call path rather than the whole object.
AGENTS.md reference: AGENTS.md:L70-L71
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'll let you assess this as I don't know in which ways libdd-profiling is linked for end customers, but I suppose a client app isn't directly statically linked against libdatadog_profiling.a eventually, is it?
| #[cfg(all(target_os = "linux", target_pointer_width = "64"))] | ||
| super::assert_interceptor::install_assert_hook(); | ||
| #[cfg(all(target_os = "linux", target_pointer_width = "64"))] | ||
| super::sigaction_interceptor::install_sigaction_hook(config.signals()); |
There was a problem hiding this comment.
| #[cfg(all(target_os = "linux", target_pointer_width = "64"))] | |
| super::assert_interceptor::install_assert_hook(); | |
| #[cfg(all(target_os = "linux", target_pointer_width = "64"))] | |
| super::sigaction_interceptor::install_sigaction_hook(config.signals()); | |
| #[cfg(all(target_os = "linux", target_pointer_width = "64"))] | |
| { | |
| super::assert_interceptor::install_assert_hook(); | |
| super::sigaction_interceptor::install_sigaction_hook(config.signals()); | |
| } |
|
|
||
| // SAFETY: hook_sigaction has the same signature as sigaction. | ||
| let result = unsafe { | ||
| libdd_gotter::hook_symbol_excluding_self(c"sigaction", hook_sigaction as *const () as usize) |
There was a problem hiding this comment.
I'll let you assess this as I don't know in which ways libdd-profiling is linked for end customers, but I suppose a client app isn't directly statically linked against libdatadog_profiling.a eventually, is it?
2 participants
What does this PR do?
Intercepts sigaction calls with
hook_symbolGOT patching so the crashtracker can detect when application code overwrites a monitored signal handler after initialization.The hook covers all callers including the statically-linked test binary; internal
sigactioncalls fromchain_signal_handler(which runs inside the signal handler) are safe because SIG_DFL/SIG_IGN handlers are filtered out before any async work is attempted (this is done before thehook_symbolanyways), and the hook always forwards using the stored original function pointer rather than through a GOT entry so there.Motivation
Some runtimes may overwrite our handlers. It would be good to know when this is happening.
Additional Notes
This is a stacked PR. This change is minimal -- just adding the patch, and printing out. Sending actual telemetry is done feat(crashtracking): send telemetry if instrumented application sigactions our signal
How to test the change?
Describe here in detail how the change can be validated.