Skip to content

fix(native): read frame records at pointer width in the crash daemon's FP walk - #2052

Merged
jpnurmi merged 2 commits into
getsentry:masterfrom
GLinnik21:fix/read-stack-value-32bit
Sep 3, 2026
Merged

fix(native): read frame records at pointer width in the crash daemon's FP walk#2052
jpnurmi merged 2 commits into
getsentry:masterfrom
GLinnik21:fix/read-stack-value-32bit

Conversation

@GLinnik21

@GLinnik21 GLinnik21 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

read_stack_value in the native backend's crash daemon bounds-checks and copies sizeof(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 each saved_fp / return_addr is 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_t and widens it to uint64_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 than addr + size, since addr is 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_thread returns 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, or unw_init_local2 failing). 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

  • ARM32 hardware (LG webOS 4.x television, Cortex-A9, glibc 2.24, in-process libunwind disabled so the FP fallback is exercised): with this fix alone, a SIGABRT in a rustc-built application walks 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-crash cross-compiles for arm-linux-gnueabi (GCC 12); the SDK and daemon build on macOS arm64 with SENTRY_BACKEND=native; clang-format clean.
  • No automated 32-bit coverage of the native backend runs in CI today (the ARM32 job runs under TEST_QEMU, which skips the native integration suite), and read_stack_value is 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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_value to bounds-check and copy sizeof(uintptr_t) bytes (instead of always sizeof(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.

Comment on lines 593 to 597
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.
@GLinnik21
GLinnik21 force-pushed the fix/read-stack-value-32bit branch from 9dc11ee to dbb6acb Compare September 2, 2026 21:45
@GLinnik21

Copy link
Copy Markdown
Contributor Author

Addressed the review: read_stack_value now range-checks by subtraction (no addr + size that could wrap for a corrupted frame pointer) and reads into a uintptr_t before widening to uint64_t, so the value is endian-independent. Re-verified on ARM32 hardware; macOS native-backend build and arm-linux-gnueabi cross-compile still clean.

GLinnik21 added a commit to GLinnik21/plx-native that referenced this pull request Sep 2, 2026
…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>

@jpnurmi jpnurmi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! LGTM 👍

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.34%. Comparing base (d95373f) to head (1437550).

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jpnurmi
jpnurmi merged commit 938c7ac into getsentry:master Sep 3, 2026
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants