Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,10 @@ endif()

if(SENTRY_WITH_LIBUNWIND)
if(LINUX)
# Build remote unwinding support for the native backend's crash daemon
if(SENTRY_BACKEND_NATIVE)
set(LIBUNWIND_BUILD_REMOTE TRUE)
endif()
# Use vendored libunwind
add_subdirectory(vendor/libunwind)
target_link_libraries(sentry PRIVATE unwind)
Expand Down Expand Up @@ -835,6 +839,10 @@ elseif(SENTRY_BACKEND_NATIVE)
SENTRY_BUILD_STATIC
)

if(SENTRY_WITH_LIBUNWIND AND LINUX)
target_compile_definitions(sentry-crash PRIVATE SENTRY_WITH_REMOTE_UNWIND)
endif()

# Copy include directories from sentry target
target_include_directories(sentry-crash PRIVATE
${PROJECT_SOURCE_DIR}/include
Expand Down Expand Up @@ -874,7 +882,9 @@ elseif(SENTRY_BACKEND_NATIVE)
endif()

if(SENTRY_WITH_LIBUNWIND AND LINUX)
target_link_libraries(sentry-crash PRIVATE unwind)
# Use unwind_remote for the daemon (includes ptrace accessors
# for remote DWARF unwinding of the crashed process)
target_link_libraries(sentry-crash PRIVATE unwind_remote)
endif()

# Make sentry library depend on crash daemon so it's always built together
Expand Down
7 changes: 7 additions & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,13 @@ elseif(SENTRY_BACKEND_NATIVE)
backends/native/minidump/sentry_minidump_writer.h
)

# Remote unwinding (Linux only — uses libunwind ptrace accessors)
if(LINUX)
sentry_target_sources_cwd(sentry
backends/native/sentry_remote_unwind.c
)
endif()

# Platform-specific minidump writers
if(LINUX OR ANDROID)
sentry_target_sources_cwd(sentry
Expand Down
70 changes: 67 additions & 3 deletions src/backends/native/sentry_crash_daemon.c
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@
# include <mach-o/dyld.h>
# include <spawn.h>
# endif
# if defined(SENTRY_PLATFORM_LINUX)
# include "sentry_remote_unwind.h"
# endif
#elif defined(SENTRY_PLATFORM_WINDOWS)
# include <dbghelp.h>
# include <fcntl.h>
Expand Down Expand Up @@ -900,10 +903,71 @@
sentry_value_t temp_frames[MAX_STACK_FRAMES];
int frame_count = 0;

#if defined(SENTRY_PLATFORM_LINUX)
// Remote DWARF unwinding via libunwind ptrace accessors.
// Works for all threads (including crashing) and resolves symbol names.
{
pid_t tid = 0;
if (thread_idx == SIZE_MAX || thread_idx == 0) {
tid = ctx->crashed_tid;
} else if (thread_idx < ctx->platform.num_threads) {
tid = ctx->platform.threads[thread_idx].tid;
}

if (tid > 0) {
sentry_remote_frame_t remote_frames[MAX_STACK_FRAMES];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Large 34KB stack allocation in crash handler path

Low Severity

sentry_remote_frame_t remote_frames[MAX_STACK_FRAMES] allocates 128 × 272 = ~34KB on the stack. Each sentry_remote_frame_t contains a 256-byte symbol buffer. Combined with the existing temp_frames[128] array and other locals, this adds significant stack pressure in a crash-handling context where reliability matters most.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a6504fa. Configure here.

size_t remote_count = sentry__remote_unwind_thread(
tid, remote_frames, MAX_STACK_FRAMES);

if (remote_count > 0) {
SENTRY_DEBUGF("Remote unwound %zu frames for thread %d",
remote_count, tid);

for (size_t i = 0;
i < remote_count && frame_count < MAX_STACK_FRAMES; i++) {
if (remote_frames[i].ip == 0
|| !is_valid_code_addr(remote_frames[i].ip)) {
continue;
}
temp_frames[frame_count] = sentry_value_new_object();
sentry_value_set_by_key(temp_frames[frame_count],
"instruction_addr",
sentry__value_new_addr(remote_frames[i].ip));
sentry_value_set_by_key(temp_frames[frame_count], "trust",
sentry_value_new_string(i == 0 ? "context" : "cfi"));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The code uses the raw loop index i to assign "context" trust. If the first frame is skipped, the next valid frame is incorrectly assigned "cfi" trust.
Severity: LOW

Suggested Fix

The trust assignment check should use frame_count == 0 instead of i == 0. This ensures that the first valid frame added to the backtrace correctly receives "context" trust, regardless of how many preceding frames were skipped.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/backends/native/sentry_crash_daemon.c#L937

Potential issue: The logic for assigning trust to stack frames uses the raw loop index
`i` to identify the first frame, which should receive `"context"` trust. However, the
loop may skip frames if their instruction pointer is invalid. If the first frame at `i
== 0` is filtered out, the next valid frame becomes the first frame in the final
backtrace but is incorrectly assigned `"cfi"` trust instead of `"context"`. This
mislabeling can affect the reliability and quality of symbolication, as `"context"`
frames are considered more trustworthy. The same bug pattern exists for both remote
unwinding and pre-captured backtraces.

Also affects:

  • src/backends/native/sentry_crash_daemon.c:977-991

Did we get this right? 👍 / 👎 to inform future reviews.

enrich_frame_with_module_info(
ctx, temp_frames[frame_count], remote_frames[i].ip);
if (remote_frames[i].symbol[0]) {
sentry_value_set_by_key(temp_frames[frame_count],
"function",
sentry_value_new_string(remote_frames[i].symbol));
}
frame_count++;
}

if (stack_buf) {
sentry_free(stack_buf);
}

Check warning on line 950 in src/backends/native/sentry_crash_daemon.c

View check run for this annotation

@sentry/warden / warden: security-review

Double-free of `stack_buf` when remote unwinding succeeds but all frames are filtered

When `remote_count > 0` but every frame fails the `is_valid_code_addr` check (leaving `frame_count == 0`), `stack_buf` is freed at line 948–950 without being set to `NULL`, and the code falls through to the pre-captured backtrace block (line 993–994) or the final cleanup block (line 1109–1110), where `stack_buf` is freed a second time. Set `stack_buf = NULL` immediately after the free.
Comment on lines +948 to +950
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Double-free of stack_buf when remote unwinding succeeds but all frames are filtered

When remote_count > 0 but every frame fails the is_valid_code_addr check (leaving frame_count == 0), stack_buf is freed at line 948–950 without being set to NULL, and the code falls through to the pre-captured backtrace block (line 993–994) or the final cleanup block (line 1109–1110), where stack_buf is freed a second time. Set stack_buf = NULL immediately after the free.

Evidence
  • stack_buf is allocated at line ~767 or ~801 and starts non-NULL when stack memory was captured.
  • The new block (lines 948–950) calls sentry_free(stack_buf) unconditionally when remote_count > 0, but never assigns stack_buf = NULL.
  • If every frame produced by sentry__remote_unwind_thread satisfies ip == 0 || !is_valid_code_addr(ip), frame_count stays 0 and execution falls out of the if (frame_count > 0) branch.
  • At line 993–994 (pre-captured backtrace path) and line 1109–1110 (final cleanup), both guarded by if (stack_buf), the now-dangling pointer passes the NULL check and is freed again, causing a double-free heap corruption in the crash daemon process.

Suggested fix: Null stack_buf immediately after freeing it in the new block so subsequent guards correctly skip the second free.

Suggested change
if (stack_buf) {
sentry_free(stack_buf);
}
stack_buf = NULL;

Identified by Warden security-review · AUG-SHC

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing NULL assignment causes use-after-free and double-free

High Severity

After sentry_free(stack_buf) at line 949, stack_buf is not set to NULL. When frame_count == 0 (all remote frames failed is_valid_code_addr), the code falls through without returning. Subsequent code at line 993 will double-free stack_buf, and if that path doesn't execute, line 1028's if (stack_buf && ...) check passes on the dangling pointer, causing use-after-free in read_stack_value. The final cleanup at line 1109 can also trigger another free of already-freed memory.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a6504fa. Configure here.


if (frame_count > 0) {
for (int i = frame_count - 1; i >= 0; i--) {
sentry_value_append(frames, temp_frames[i]);
}
sentry_value_set_by_key(stacktrace, "frames", frames);
sentry_value_set_by_key(stacktrace, "registers",
build_registers_from_ctx(ctx, thread_idx));
return stacktrace;
}
}
}
}
// Fall through to pre-captured backtrace or FP-walking if remote
// unwinding failed
#endif

#if defined(SENTRY_PLATFORM_LINUX) || defined(SENTRY_PLATFORM_ANDROID)
// Use pre-captured libunwind backtrace if available (DWARF-based, works
// without frame pointers). This is preferred over FP-based walking for
// the crashed thread.
// Fallback: use pre-captured libunwind backtrace if available
// (DWARF-based, works without frame pointers).
if (ctx->platform.backtrace_count > 0
&& (thread_idx == SIZE_MAX || thread_idx == 0)) {
SENTRY_DEBUGF("Using pre-captured libunwind backtrace (%zu frames)",
Expand Down
111 changes: 111 additions & 0 deletions src/backends/native/sentry_remote_unwind.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// Remote stack unwinding via libunwind ptrace accessors.
// This file must NOT define UNW_LOCAL_ONLY so that unw_init_remote()
// and other generic (_U prefix) symbols are used.
//
// Only compiled into sentry-crash daemon (SENTRY_WITH_REMOTE_UNWIND).
// The sentry library compiles this file too (shared sources) but the
// function body is empty without the define.

#include "sentry_remote_unwind.h"

#ifdef SENTRY_WITH_REMOTE_UNWIND

# include "sentry_logger.h"

# include <errno.h>
# include <string.h>
# include <sys/ptrace.h>
# include <sys/wait.h>

# include <libunwind-ptrace.h>
# include <libunwind.h>

size_t
sentry__remote_unwind_thread(
pid_t tid, sentry_remote_frame_t *frames, size_t max_frames)
{
if (ptrace(PTRACE_ATTACH, tid, NULL, NULL) != 0) {
SENTRY_WARNF("remote_unwind: ptrace attach failed for %d: %s", tid,
strerror(errno));
return 0;
}

int status;
if (waitpid(tid, &status, __WALL) < 0) {
SENTRY_WARNF(
"remote_unwind: waitpid failed for %d: %s", tid, strerror(errno));
ptrace(PTRACE_DETACH, tid, NULL, NULL);
return 0;
}

size_t n = 0;

unw_addr_space_t as = unw_create_addr_space(&_UPT_accessors, 0);
if (!as) {
SENTRY_WARN("remote_unwind: unw_create_addr_space failed");
goto detach;
}

void *upt = _UPT_create(tid);
if (!upt) {
SENTRY_WARN("remote_unwind: _UPT_create failed");
unw_destroy_addr_space(as);
goto detach;
}

unw_cursor_t cursor;
int ret = unw_init_remote(&cursor, as, upt);
if (ret < 0) {
SENTRY_WARNF("remote_unwind: unw_init_remote failed: %d", ret);
_UPT_destroy(upt);
unw_destroy_addr_space(as);
goto detach;
}

while (n < max_frames) {
unw_word_t ip = 0;
if (unw_get_reg(&cursor, UNW_REG_IP, &ip) < 0 || ip == 0) {
break;
}

frames[n].ip = (uint64_t)ip;
frames[n].symbol[0] = '\0';
frames[n].symbol_offset = 0;

unw_word_t off = 0;
if (unw_get_proc_name(
&cursor, frames[n].symbol, sizeof(frames[n].symbol), &off)
== 0) {
frames[n].symbol_offset = (uint64_t)off;
}

n++;

if (unw_step(&cursor) <= 0) {
break;
}
}

_UPT_destroy(upt);
unw_destroy_addr_space(as);

detach:
ptrace(PTRACE_DETACH, tid, NULL, NULL);

SENTRY_DEBUGF("Remote unwound %zu frames for thread %d", n, tid);
return n;
}

#else /* !SENTRY_WITH_REMOTE_UNWIND */

size_t
sentry__remote_unwind_thread(
pid_t tid, sentry_remote_frame_t *frames, size_t max_frames)
{
(void)tid;
(void)frames;
(void)max_frames;
return 0;
}

#endif /* SENTRY_WITH_REMOTE_UNWIND */
28 changes: 28 additions & 0 deletions src/backends/native/sentry_remote_unwind.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#ifndef SENTRY_REMOTE_UNWIND_H_INCLUDED
#define SENTRY_REMOTE_UNWIND_H_INCLUDED

#include <stddef.h>
#include <stdint.h>
#include <sys/types.h>

#define SENTRY_REMOTE_UNWIND_MAX_SYMBOL 256

typedef struct {
uint64_t ip;
char symbol[SENTRY_REMOTE_UNWIND_MAX_SYMBOL];
uint64_t symbol_offset;
} sentry_remote_frame_t;

/**
* Remotely unwind a thread's stack using libunwind ptrace accessors.
* Handles ptrace attach/detach internally.
*
* @param tid Thread ID to unwind
* @param frames Output array of frames
* @param max_frames Maximum number of frames to capture
* @return Number of frames captured, or 0 on failure
*/
size_t sentry__remote_unwind_thread(
pid_t tid, sentry_remote_frame_t *frames, size_t max_frames);

#endif
Loading
Loading