-
-
Notifications
You must be signed in to change notification settings - Fork 204
wip(native): add remote DWARF unwinding + symbol names for Linux #1747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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> | ||||||||||
|
|
@@ -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]; | ||||||||||
| 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")); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The code uses the raw loop index Suggested FixThe trust assignment check should use Prompt for AI AgentAlso affects:
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
|
||||||||||
|
Comment on lines
+948
to
+950
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double-free of When Evidence
Suggested fix: Null
Suggested change
Identified by Warden security-review · AUG-SHC There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing NULL assignment causes use-after-free and double-freeHigh Severity After Additional Locations (2)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)", | ||||||||||
|
|
||||||||||
| 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 */ |
| 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 |


There was a problem hiding this comment.
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. Eachsentry_remote_frame_tcontains a 256-bytesymbolbuffer. Combined with the existingtemp_frames[128]array and other locals, this adds significant stack pressure in a crash-handling context where reliability matters most.Reviewed by Cursor Bugbot for commit a6504fa. Configure here.