fix(native): read frame records at pointer width in the crash daemon's FP walk - #2052
Conversation
4f2171f to
9dc11ee
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated read_stack_value implementation should avoid uint64_t overflow in bounds checks and use an endian-safe widening strategy when copying uintptr_t into uint64_t.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the crash daemon’s frame-pointer fallback stack walking to read frame records at the target’s pointer width, preventing incorrect slot reads and bounds-check failures on 32-bit targets while preserving 64-bit behavior.
Changes:
- Update
read_stack_valueto bounds-check and copysizeof(uintptr_t)bytes (instead of alwayssizeof(uint64_t)). - Step from saved FP to return address using pointer-sized offsets in the FP walk.
- Document the fix in
CHANGELOG.md.
File summaries
| File | Description |
|---|---|
| src/backends/native/sentry_crash_daemon.c | Switch stack reads/FP-walk offsets to pointer width for correct 32-bit frame record decoding. |
| CHANGELOG.md | Add an Unreleased fix entry describing the pointer-width FP-walk change. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (addr < stack_start | ||
| || addr + sizeof(uint64_t) > stack_start + stack_size) { | ||
| || addr + sizeof(uintptr_t) > stack_start + stack_size) { | ||
| return false; | ||
| } | ||
| uint64_t offset = addr - stack_start; |
`read_stack_value` bounds-checked and copied `sizeof(uint64_t)` bytes regardless of the target's pointer size, and the walk stepped to the return address with the same constant. On a 32-bit target every read therefore took two stack slots per pointer: the high half of each `saved_fp` and `return_addr` was the neighbouring word, the return address was read from the wrong slot, and the last legitimate frame record failed the bounds check. Read a `uintptr_t` and widen it, and step by pointer size; 64-bit targets are unchanged. While here, range-check by subtraction rather than `addr + size`: the address comes from a frame pointer in a crashed process, and a corrupted value near the top of the address space wraps the sum past the naive comparison.
9dc11ee to
dbb6acb
Compare
|
Addressed the review: |
…RM32 frame chains (#64) The pin moves 0.13.9 → 0.16.5. Two things this repo hand-wrote against 0.13.9 are upstream now and leave the patch: the non-regular-file guard when reading /proc/<pid>/maps entries as ELF (sentry__elf_open), and the ptrace snapshot of the other threads — #1747's per-thread remote libunwind unwinding replaces it with DWARF frames and symbol names. Also gone: previous-handler chaining and the flush_scope fields (release, dist, environment, sdk, event_id), which upstream fills. The patch shrinks from 600 lines to what upstream does not do. What it still does, and why: - process_vm_readv wrapper for glibc 2.12 (also needed by the daemon now). - ARM32 registers in the event, and a frame-pointer walk that reads BOTH ARM32 frame records. GCC's `push {..,fp,lr}; add fp,sp,#N` leaves fp on the LR slot ([fp-4] saved fp, [fp] return); rustc/LLVM leaves it on the saved-fp slot ([fp] saved fp, [fp+4] return). The old patch hard-coded the GCC shape, so a crash in Rust — nearly every crash — walked one frame and reported a saved fp as a return address. Candidates are judged against the crashed process's mappings, recorded with permission bits while maps is parsed: return address in an executable, non-writable mapping; saved fp above the current frame in a writable one. Fail closed when the mapping snapshot is incomplete or differs on a second read after the stack copy. Both halves are upstream PRs (getsentry/sentry-native#2052, #2053) and drop out of the patch once a pinned release contains them. - Pointer-width stack reads with a subtraction range check. - 32-frame cap for non-crashed threads (the 256 KiB record ceiling), the 30 s handler budget, and the two webOS-only signal-handler escapes. libsentry and our C are built with -funwind-tables so the remote unwinder gets through them (it stopped at threadpool_thread before). Device-verified on the debug install: SIGSEGV and SIGABRT → envelope → import (queued=1 rejected=0, native_wins=1) → flushed → visible in the Sentry project with sdk.version 0.16.5; crashed thread `crash_on_purpose ← plex_run ← __libc_start_main`; a GCC-built noinline chain `leaf ← f2 ← f1 ← __libc_start_main`. fwcompat matrix unchanged (OK 4.4.2→11.2.0); both binaries still need only GLIBC_2.12. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2052 +/- ##
==========================================
- Coverage 74.38% 74.34% -0.04%
==========================================
Files 103 103
Lines 26672 26676 +4
Branches 4853 4853
==========================================
- Hits 19839 19833 -6
- Misses 5487 5506 +19
+ Partials 1346 1337 -9 🚀 New features to boost your workflow:
|
Summary
read_stack_valuein the native backend's crash daemon bounds-checks and copiessizeof(uint64_t)bytes regardless of the target's pointer size, and the frame-pointer walk steps to the return address with the same constant. On a 32-bit target every read therefore spans two stack slots: the high half of eachsaved_fp/return_addris the neighbouring word, the return address is read from the wrong slot, and the last legitimate frame record fails the bounds check.This reads a
uintptr_tand widens it touint64_t(so the value is endian-independent), and steps by pointer size. On 64-bit targets the size and offsets are unchanged. While here, the range check is done by subtraction rather thanaddr + size, sinceaddris a frame pointer from a crashed process and a corrupted value near the top of the address space would wrap the sum past the naive comparison.Scope
This is the daemon's frame-pointer fallback. On Linux the signal handler captures a libunwind backtrace first and
build_stacktrace_for_threadreturns that when it is non-empty; the FP walk runs when that capture is unavailable or empty (for example a build that disables in-process libunwind, orunw_init_local2failing). Within that fallback, the pointer-width reads are wrong on every 32-bit target. On ARM32 this alone is not sufficient for GCC-built frames — see #2053, which is stacked on this PR.Verification
gsignal ← plex_run ← __libc_start_main(3 named frames); a GCC-built chain with frame pointers still stops after one real frame followed by a stack address, which is the record-shape problem feat(native): ARM32 registers and both frame-record shapes in the crash daemon #2053 addresses.sentry-crashcross-compiles forarm-linux-gnueabi(GCC 12); the SDK and daemon build on macOS arm64 withSENTRY_BACKEND=native;clang-formatclean.TEST_QEMU, which skips the native integration suite), andread_stack_valueis a static function inside the daemon with no unit-test seam, so this PR adds no test. Happy to add one if there is a preferred shape for daemon-internal tests.