WIP: fix(crashtracker): unwind from saved crash context - #2398
Conversation
📚 Documentation Check Results📦
|
|
🔒 Cargo Deny Results📦
|
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
|
![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: 5ea16272ec
ℹ️ 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".
| if let Some(stack) = crashing_stack { | ||
| builder.with_stack(stack)?; |
There was a problem hiding this comment.
When Linux callers enable both collect_all_threads and EnabledWithInprocessSymbols, the original collector stack contains function names from unw_get_proc_name (collector/emitters.rs), but this unconditional replacement promotes a remote stack containing only ip/sp. resolve_frames does not enrich that configuration because it only handles EnabledWithSymbolsInReceiver, so every successfully captured crash loses its primary stack's function names and degrades Error Tracking grouping and display. Preserve the collector's enrichment when promoting the causal unwind, or restrict replacement to configurations that subsequently symbolize the remote frames.
Useful? React with 👍 / 👎.
| if tid == crashing_tid && !stack.frames.is_empty() { | ||
| crashing_stack = Some(stack.clone()); |
There was a problem hiding this comment.
When report_unhandled_exception is used with collect_all_threads, no ucontext is emitted, so this candidate stack is captured from the reporting thread's current registers while it is waiting for the receiver rather than from the exception site. Nevertheless, it is accepted here and later replaces the explicit StackTrace supplied by the caller, causing every successful all-thread collection for an unhandled exception to report the receiver-wait/reporting machinery as the primary error stack. Only promote this stack when a saved crash context was actually used, or limit promotion to UnixSignal reports.
Useful? React with 👍 / 👎.
| if crash_info | ||
| .sig_info | ||
| .as_ref() | ||
| .is_some_and(|sig_info| sig_info.si_code <= 0) | ||
| { | ||
| trim_signal_delivery_frames(&mut crash_info.error.stack); |
There was a problem hiding this comment.
A nonpositive si_code only says that the signal originated in user space; it does not prove that the receiving thread was executing raise or pthread_kill. For example, an external kill produces SI_USER, and pthread_kill can target a different thread whose saved context is anywhere. If that thread is interrupted while legitimately executing libc, this predicate causes the following path-based trimming to remove its real top frames and changes the reported grouping location. Restrict trimming to frames whose symbols identify the delivery path rather than classifying every user-originated signal as self-raised.
Useful? React with 👍 / 👎.
| if let Some(registers) = original_registers.as_mut() { | ||
| let _ = set_registers(tid, registers); |
There was a problem hiding this comment.
If the second PTRACE_SETREGSET fails while the tracee still exists, discarding the error and immediately detaching resumes the crashing thread with the temporary crash-time registers instead of the saved crash-handler registers. It can therefore re-execute the fault or otherwise leave the handler before the receiver symbolizes the report, violating the requirement that the process remain alive for /proc/<pid>/maps; restoration failure must be propagated or handled without resuming the tracee in the modified state.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
|
Do you want this to be taken a look at? or is this still WIP |
|
@gyuheon0h still WIP. I was looking at a crash today. first surfaced through crashtracking, then digging through coredump - I found hte actual crashing stack pointed to elsewhere. IDK how widespread this issue is or was it just my C handler implemetnation doing something really wrong. I need some time to properly understand what is up |
2 participants
Summary
ucontextbefore remote libunwind collectionValidation
cargo check -p libdd-crashtracker --features collector,receivercargo check -p libdd-crashtracker --features collector,receiver --tests --target x86_64-unknown-linux-musl-fomit-frame-pointercrash integration test