From bc3abf91201cdd6a9bd5dd71e75114861930e016 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sima=CC=83o=20Seic=CC=A7a?= Date: Wed, 3 Jun 2026 19:10:46 +0100 Subject: [PATCH 1/2] RUM-16442: Fix profiler safe memory reads --- .../Mach/include/safe_read_testing.h | 57 +- .../Mach/mach_sampling_profiler.cpp | 398 +++++++++---- DatadogProfiling/Tests/SafeReadTests.swift | 553 ++++++++++++++++-- 3 files changed, 840 insertions(+), 168 deletions(-) diff --git a/DatadogProfiling/Mach/include/safe_read_testing.h b/DatadogProfiling/Mach/include/safe_read_testing.h index 89f6e193c5..78b22f288f 100644 --- a/DatadogProfiling/Mach/include/safe_read_testing.h +++ b/DatadogProfiling/Mach/include/safe_read_testing.h @@ -7,26 +7,73 @@ #ifndef DD_PROFILER_SAFE_READ_TESTING_H_ #define DD_PROFILER_SAFE_READ_TESTING_H_ -#include #include +#include #ifdef __APPLE__ #include #if !TARGET_OS_WATCH +#include "dd_profiler.h" // stack_trace_t + #ifdef __cplusplus extern "C" { #endif /** - * Helper to manually re-install handlers if needed by tests. + * Reads a contiguous region of the calling task's virtual memory starting at sp + * into buf (which must be at least buf_size bytes). + * + * Mirrors the production read_stack_region used inside stack_trace_sample_thread. + * Returns the number of contiguous bytes actually read from sp; returns 0 if sp + * itself is unmapped or invalid. + */ +size_t read_stack_region_for_testing(void* sp, void* buf, size_t buf_size); + +/** + * Walks a frame pointer chain entirely from a pre-populated local buffer. + * + * This is the same walk used inside stack_trace_sample_thread, extracted so tests + * can exercise the bounds-check guards and cycle-termination behavior directly + * without needing to suspend a real thread or craft live stack memory. + * + * @param trace Pre-allocated trace; trace->frames must point to at least + * max_depth entries. frame_count is reset to 0 on entry. + * @param initial_fp First frame pointer to walk from (typically the suspended + * thread's FP register). + * @param initial_pc First program counter to record (typically the suspended + * thread's PC register). + * @param stack_base Base address that stack_buf was read from (typically SP). + * @param stack_buf Buffer containing a snapshot of stack memory. + * @param bytes_read Valid bytes in stack_buf. + * @param max_depth Hard cap on frames to record. */ -void init_safe_read_handlers_for_testing(void); +void walk_frames_in_buffer( + stack_trace_t* trace, + void* initial_fp, + void* initial_pc, + uintptr_t stack_base, + const uint8_t* stack_buf, + size_t bytes_read, + uint32_t max_depth +); /** - * Validates the Safe Read mechanism. + * Same as walk_frames_in_buffer, but enables the production safe-read fallback + * for valid frame pointers that are outside the copied stack buffer. + * + * This lets tests verify that large-but-mapped frame gaps do not silently + * truncate otherwise valid stacks. */ -bool safe_read_memory_for_testing(void* addr, void* buffer, size_t size); +void walk_frames_with_safe_read_fallback_for_testing( + stack_trace_t* trace, + void* initial_fp, + void* initial_pc, + uintptr_t stack_base, + const uint8_t* stack_buf, + size_t bytes_read, + uint32_t max_depth +); #ifdef __cplusplus } diff --git a/DatadogProfiling/Mach/mach_sampling_profiler.cpp b/DatadogProfiling/Mach/mach_sampling_profiler.cpp index d81be50328..2d5f066715 100644 --- a/DatadogProfiling/Mach/mach_sampling_profiler.cpp +++ b/DatadogProfiling/Mach/mach_sampling_profiler.cpp @@ -10,18 +10,19 @@ #if defined(__APPLE__) && !TARGET_OS_WATCH #include +#include #include #include #include #include +#include #include -#include -#include #include #include #include #include #include +#include // Address validation constants and macros // @@ -56,47 +57,18 @@ static constexpr uintptr_t FRAME_POINTER_ALIGN = 0x7ULL; // 8-byte al static constexpr size_t PTHREAD_THREAD_NAME_MAX = 64; -// Main thread pthread identifier for comparison -static pthread_t g_main_pthread = NULL; - -// Safe memory read using signal handling -// Thread-local storage for signal-based safe memory reading -static thread_local sigjmp_buf g_safe_read_handler; -static thread_local volatile sig_atomic_t g_is_safe_read = false; +// Hard upper bound for the stack region read per thread sample. +// The actual bytes requested are computed dynamically from the thread's SP and FP, +// so this cap is only hit by threads with unusually large frames or very deep stacks. +static constexpr size_t STACK_REGION_MAX_READ = 65536; // 64 KB safety cap -// Previous signal handlers to restore if needed -static struct sigaction g_prev_sigbus_handler; -static struct sigaction g_prev_sigsegv_handler; +// Conservative per-frame size estimate used to compute the dynamic read size. +// ARM64/x86_64 frames range from 16 bytes (leaf) to ~512 bytes (heavy locals). +// 128 bytes covers the vast majority of real-world frames without over-reading. +static constexpr size_t STACK_REGION_FRAME_SIZE_ESTIMATE = 128; -/** - * Signal handler for catching memory access errors during stack unwinding. - * If safe_read is active, longjmp back to the safe point. - * Otherwise, call the previous handler or use default behavior. - */ -static void safe_read_signal_handler(int sig, siginfo_t* info, void* context) { - // If we're in a safe_read, recover via longjmp - if (g_is_safe_read) { - siglongjmp(g_safe_read_handler, 1); - } - - // Not in safe_read - forward to previous handler - struct sigaction* prev = (sig == SIGBUS) ? &g_prev_sigbus_handler : &g_prev_sigsegv_handler; - - if (prev->sa_flags & SA_SIGINFO) { - if (prev->sa_sigaction) { - prev->sa_sigaction(sig, info, context); - } - } else if (prev->sa_handler == SIG_DFL) { - // Restore default handler using sigaction (async-signal-safe) - struct sigaction dfl = {}; - dfl.sa_handler = SIG_DFL; - sigemptyset(&dfl.sa_mask); - sigaction(sig, &dfl, nullptr); - raise(sig); - } else if (prev->sa_handler != SIG_IGN) { - prev->sa_handler(sig); - } -} +// Main thread pthread identifier for comparison +static pthread_t g_main_pthread = NULL; /** * Sets the main thread pthread identifier. @@ -110,24 +82,94 @@ void set_main_thread(pthread_t thread) { } /** - * Install signal handlers for safe memory reading. + * Reads a contiguous region of the calling task's virtual memory. + * Uses vm_read_overwrite which returns an error code for unmapped/invalid addresses + * instead of raising EXC_BAD_ACCESS, making it safe to call with partially-valid + * thread stacks and compatible with Mach exception handlers such as Crashlytics. + * + * @param addr Starting address. + * @param buf Caller-supplied output buffer. + * @param size Maximum bytes to read (buf must be at least this large). + * @return Bytes actually read, 0 on any failure. */ -static void init_safe_read_handlers() { - static bool initialized = false; - if (initialized) { - return; +static size_t read_memory_region(void* addr, uint8_t* buf, size_t size) { + if (addr == nullptr || buf == nullptr || size == 0) { + return 0; } - initialized = true; - // Manually install the handler defined in this file - struct sigaction sa = {}; - sa.sa_sigaction = safe_read_signal_handler; - sigemptyset(&sa.sa_mask); - sa.sa_flags = SA_SIGINFO; + const uintptr_t address = reinterpret_cast(addr); + if (address > UINTPTR_MAX - size) { + return 0; + } - // Install handlers and save previous ones - sigaction(SIGBUS, &sa, &g_prev_sigbus_handler); - sigaction(SIGSEGV, &sa, &g_prev_sigsegv_handler); + vm_size_t read_size = static_cast(size); + kern_return_t kr = vm_read_overwrite( + mach_task_self(), + reinterpret_cast(addr), + static_cast(size), + reinterpret_cast(buf), + &read_size + ); + return (kr == KERN_SUCCESS && read_size == size) ? static_cast(read_size) : 0; +} + +/** + * Reads as much of a stack region as possible by walking page boundaries. + * + * This is only used as a fallback when the full-region read fails. Reading from + * SP upward preserves a contiguous prefix of the stack and stops at the first + * unreadable page, so the frame walker can still salvage frames already copied. + */ +static size_t read_stack_region_by_pages(void* sp, uint8_t* buf, size_t size) { + if (sp == nullptr || buf == nullptr || size == 0 || vm_page_size == 0) { + return 0; + } + + uintptr_t current_address = reinterpret_cast(sp); + size_t total_read = 0; + const size_t page_size = static_cast(vm_page_size); + + while (total_read < size) { + const size_t page_offset = current_address % page_size; + const size_t bytes_until_page_end = page_size - page_offset; + const size_t bytes_remaining = size - total_read; + const size_t chunk_size = std::min(bytes_remaining, bytes_until_page_end); + + const size_t bytes_read = read_memory_region( + reinterpret_cast(current_address), + buf + total_read, + chunk_size + ); + + if (bytes_read != chunk_size) { + break; + } + + total_read += bytes_read; + if (total_read == size || current_address > UINTPTR_MAX - bytes_read) { + break; + } + + current_address += bytes_read; + } + + return total_read; +} + +/** + * Reads a stack region with a fast full-region read and a page-by-page fallback. + * + * The common path is one vm_read_overwrite per sampled thread. If that all-or- + * nothing read fails because a later page is unmapped, the fallback returns the + * contiguous readable prefix from SP so the sample is degraded rather than lost. + */ +static size_t read_stack_region(void* sp, uint8_t* buf, size_t size) { + const size_t bytes_read = read_memory_region(sp, buf, size); + if (bytes_read == size) { + return bytes_read; + } + + return read_stack_region_by_pages(sp, buf, size); } /** @@ -186,20 +228,23 @@ void stack_trace_destroy(stack_trace_t* trace) { } /** - * Gets thread state and extracts frame pointer and program counter. + * Gets thread state and extracts stack pointer, frame pointer, and program counter. + * sp is needed to anchor the batch stack read; fp and pc seed the frame walk. * - * @param thread The thread to get the state from + * @param thread The thread to get the state from (must be suspended) * @param fp Output parameter for frame pointer * @param pc Output parameter for program counter + * @param sp Output parameter for stack pointer * @return true if successful, false if thread state could not be retrieved */ -bool thread_get_frame_pointers(thread_t thread, void** fp, void** pc) { +bool thread_get_frame_pointers(thread_t thread, void** fp, void** pc, void** sp) { #if defined(__x86_64__) x86_thread_state64_t state; mach_msg_type_number_t count = x86_THREAD_STATE64_COUNT; if (thread_get_state(thread, x86_THREAD_STATE64, (thread_state_t)&state, &count) == KERN_SUCCESS) { *fp = (void*)state.__rbp; *pc = (void*)state.__rip; + *sp = (void*)state.__rsp; return true; } #elif defined(__i386__) @@ -208,6 +253,7 @@ bool thread_get_frame_pointers(thread_t thread, void** fp, void** pc) { if (thread_get_state(thread, x86_THREAD_STATE32, (thread_state_t)&state, &count) == KERN_SUCCESS) { *fp = (void*)state.__ebp; *pc = (void*)state.__eip; + *sp = (void*)state.__esp; return true; } #elif defined(__arm64__) @@ -216,6 +262,7 @@ bool thread_get_frame_pointers(thread_t thread, void** fp, void** pc) { if (thread_get_state(thread, ARM_THREAD_STATE64, (thread_state_t)&state, &count) == KERN_SUCCESS) { *fp = (void*)arm_thread_state64_get_fp(state); *pc = (void*)arm_thread_state64_get_pc(state); + *sp = (void*)arm_thread_state64_get_sp(state); return true; } #elif defined(__arm__) @@ -225,6 +272,7 @@ bool thread_get_frame_pointers(thread_t thread, void** fp, void** pc) { // https://developer.apple.com/documentation/xcode/writing-armv6-code-for-ios#//apple_ref/doc/uid/TP40009021-SW1 *fp = (void*)state.__r[7]; // R7 is commonly used as frame pointer on iOS *pc = (void*)state.__pc; + *sp = (void*)state.__sp; return true; } #endif @@ -266,67 +314,200 @@ bool stack_trace_get_thread_info(stack_trace_t* trace, thread_t thread) { } /** - * Safely reads memory from a potentially invalid address. - * - * If memory is invalid, SIGBUS/SIGSEGV is caught and we return false. -*/ -bool safe_read_memory(void* addr, void* buffer, size_t size) { - // Set up the JUMP TARGET - if (sigsetjmp(g_safe_read_handler, 1) == 0) { - g_is_safe_read = true; - // try direct memory copy - memcpy(buffer, addr, size); - g_is_safe_read = false; - return true; + * Reads a saved frame pointer / program counter pair from a stack snapshot. + */ +static bool read_frame_pair_from_snapshot( + uintptr_t fp, + uintptr_t stack_base, + const uint8_t* stack_buf, + size_t bytes_read, + void* next_frame[2] +) { + if (stack_base == 0 || stack_buf == nullptr || bytes_read < sizeof(void*[2]) || fp < stack_base) { + return false; } - // Memory access failed - // We land here if safe_read_signal_handler() called siglongjmp() - g_is_safe_read = false; - return false; + const uintptr_t fp_offset = fp - stack_base; + if (fp_offset > bytes_read - sizeof(void*[2])) { + return false; + } + + memcpy(static_cast(next_frame), stack_buf + fp_offset, sizeof(void*[2])); + return true; } /** - * Samples a thread's stack to collect stack trace information. + * Reads a saved frame pointer / program counter pair directly from memory. * - * @param trace Pre-allocated stack trace to fill - * @param thread The thread to sample - * @param max_depth Maximum number of frames to capture + * This is only used when a valid frame pointer falls outside the initial stack + * snapshot. It preserves stack accuracy for large frames without returning to + * faulting memory reads; vm_read_overwrite reports failure instead of crashing. */ -void stack_trace_sample_thread(stack_trace_t* trace, thread_t thread, uint32_t max_depth) { - trace->timestamp = clock_gettime_nsec_np(CLOCK_UPTIME_RAW); - trace->frame_count = 0; +static bool read_frame_pair_from_memory(uintptr_t fp, void* next_frame[2]) { + return read_memory_region( + reinterpret_cast(fp), + reinterpret_cast(next_frame), + sizeof(void*[2]) + ) == sizeof(void*[2]); +} - void *fp = nullptr; - void *pc = nullptr; - if (!thread_get_frame_pointers(thread, &fp, &pc)) return; +/** + * Walks a frame pointer chain from a pre-populated local buffer. + * + * If allow_memory_fallback is true, a valid frame pointer outside the copied + * region is read with vm_read_overwrite so unusually large frames can still be + * followed safely. The common path remains local-buffer reads with no syscalls. + */ +static void walk_frames( + stack_trace_t* trace, + void* initial_fp, + void* initial_pc, + uintptr_t stack_base, + const uint8_t* stack_buf, + size_t bytes_read, + uint32_t max_depth, + bool allow_memory_fallback +) { + void* fp = initial_fp; + void* pc = initial_pc; + trace->frame_count = 0; while (trace->frame_count < max_depth && pc != nullptr) { auto& frame = trace->frames[trace->frame_count]; frame.instruction_ptr = (uint64_t)pc; - // Keep raw traces safe for callbacks by setting safe defaults frame.image.load_address = 0; memset(frame.image.uuid, 0, sizeof(uuid_t)); frame.image.filename = nullptr; - trace->frame_count++; - - if (fp == nullptr) break; - // Validate frame pointer before dereferencing - if (!is_valid_frame_pointer((uintptr_t)fp)) break; - // Read the next frame pointer and return address + const uintptr_t fp_addr = reinterpret_cast(fp); + if (!is_valid_frame_pointer(fp_addr)) break; + + // Frame pointers grow toward higher addresses (stack grows down), + // so fp must be at or above sp when a stack snapshot is available. + if (stack_base != 0 && fp_addr < stack_base) break; + void* next_frame[2]; - if (!safe_read_memory(fp, static_cast(next_frame), sizeof(next_frame))) break; + if (!read_frame_pair_from_snapshot(fp_addr, stack_base, stack_buf, bytes_read, next_frame)) { + if (!allow_memory_fallback || !read_frame_pair_from_memory(fp_addr, next_frame)) { + break; + } + } - fp = next_frame[0]; // Next frame pointer - pc = next_frame[1]; // Return address + fp = next_frame[0]; // next frame pointer (saved x29 / rbp) + pc = next_frame[1]; // return address (saved lr / return addr on stack) - // Validate the new PC - if (!is_valid_userspace_addr((uintptr_t)pc)) break; + if (!is_valid_userspace_addr(reinterpret_cast(pc))) break; } } +/** + * Walks a frame pointer chain entirely from a pre-populated local buffer. + * + * Resets trace->frame_count to 0, records initial_pc as the first frame, then + * follows the frame pointer chain by reading saved (fp, pc) word pairs from + * stack_buf with bounds checks. The walk stops on: max_depth reached, invalid + * FP/PC, FP outside the captured region, or null PC. + * + * No syscalls and no memory accesses outside [stack_buf, stack_buf + bytes_read) + * are performed, so this function cannot raise EXC_BAD_ACCESS or fault. + * + * Exposed via safe_read_testing.h so tests can exercise the bounds-check guards + * and cycle-termination behavior directly with crafted buffers. + * + * @param trace Pre-allocated trace; trace->frames must point to at least + * max_depth entries. + * @param initial_fp First frame pointer to walk from. + * @param initial_pc First program counter to record. + * @param stack_base Base address that stack_buf was read from. + * @param stack_buf Buffer containing a snapshot of stack memory. + * @param bytes_read Valid bytes in stack_buf. + * @param max_depth Hard cap on frames to record. + */ +extern "C" void walk_frames_in_buffer( + stack_trace_t* trace, + void* initial_fp, + void* initial_pc, + uintptr_t stack_base, + const uint8_t* stack_buf, + size_t bytes_read, + uint32_t max_depth +) { + walk_frames(trace, initial_fp, initial_pc, stack_base, stack_buf, bytes_read, max_depth, false); +} + +extern "C" void walk_frames_with_safe_read_fallback_for_testing( + stack_trace_t* trace, + void* initial_fp, + void* initial_pc, + uintptr_t stack_base, + const uint8_t* stack_buf, + size_t bytes_read, + uint32_t max_depth +) { + walk_frames(trace, initial_fp, initial_pc, stack_base, stack_buf, bytes_read, max_depth, true); +} + +/** + * Samples a thread's stack to collect stack trace information. + * + * Reads the thread's live stack region with a single vm_read_overwrite call, then + * walks frame pointers from the local copy. If a valid frame pointer lands outside + * the initial copy (for example because of a large stack allocation), that one + * saved FP/PC pair is read with vm_read_overwrite as a safe fallback. This avoids + * faulting memory reads while keeping the sampled stack internally consistent. + * + * Overhead: one vm_read_overwrite per thread sample on the common path, with extra + * safe reads only for out-of-window frame pointers. + * + * @param trace Pre-allocated stack trace to fill + * @param thread The thread to sample (must already be suspended) + * @param max_depth Maximum number of frames to capture + */ +static void stack_trace_sample_thread( + stack_trace_t* trace, + thread_t thread, + uint32_t max_depth +) { + trace->frame_count = 0; + + void* fp = nullptr; + void* pc = nullptr; + void* sp = nullptr; + if (!thread_get_frame_pointers(thread, &fp, &pc, &sp)) return; + + // Compute the minimum region that covers all frames we intend to walk. + // + // The stack grows downward: SP is the lowest live address, FP is above SP + // (within the current frame), and each subsequent frame pointer is higher still. + // Reading [SP, SP + region_size] therefore covers the entire walk. + // + // region_size = (FP - SP) // current frame + // + max_depth * frame_est // remaining frames above FP + // + // This is capped at STACK_REGION_MAX_READ as a safety bound for unusual stacks. + // Compared to always reading 64 KB, this typically reads 8-20 KB instead, + // reducing data movement by ~4-8x at the default depth of 128. + alignas(void*) uint8_t stack_buf[STACK_REGION_MAX_READ]; + size_t bytes_read = 0; + uintptr_t stack_base = 0; + + if (is_valid_userspace_addr(reinterpret_cast(sp))) { + stack_base = reinterpret_cast(sp); + + const uintptr_t fp_addr = reinterpret_cast(fp); + const size_t current_frame_size = (fp_addr > stack_base) ? (fp_addr - stack_base) : 0; + const size_t region_size = std::min( + current_frame_size + (static_cast(max_depth) * STACK_REGION_FRAME_SIZE_ESTIMATE), + STACK_REGION_MAX_READ + ); + + bytes_read = read_stack_region(sp, stack_buf, region_size); + } + + walk_frames(trace, fp, pc, stack_base, stack_buf, bytes_read, max_depth, true); +} + namespace dd::profiler { /** @@ -401,7 +582,6 @@ bool mach_sampling_profiler::start_sampling() { } sampling_thread_mach.store(MACH_PORT_NULL, std::memory_order_relaxed); - init_safe_read_handlers(); if (!worker || !worker->start()) { return false; } @@ -498,16 +678,24 @@ void mach_sampling_profiler::sample_thread(thread_t thread, uint64_t interval_na // Get thread info stack_trace_get_thread_info(&trace, thread); + trace.timestamp = clock_gettime_nsec_np(CLOCK_UPTIME_RAW); + if (thread_suspend(thread) == KERN_SUCCESS) { // CRITICAL: Thread is suspended - avoid operations that could deadlock // // The suspended thread may be holding system locks (memory allocator, pthread, etc). // If we try to acquire these same locks while the thread is suspended, we'll deadlock. // - // Specifically avoid: + // Specifically avoid nonessential: // - Memory allocations (new, malloc) - memory allocator locks - // - System calls that acquire locks - they may be held by suspended thread + // - System calls - they may acquire locks held by the suspended thread // - pthread functions - they share locks with system APIs + // + // Keep this window to the minimum required call-stack capture. The stack + // memory reads below intentionally use vm_read_overwrite while suspended + // to keep FP/PC/SP and caller frames from the same execution instant. + // Work that is not needed for stack consistency must stay outside this + // section. stack_trace_sample_thread(&trace, thread, config.max_stack_depth); thread_resume(thread); } @@ -584,12 +772,12 @@ void mach_sampling_profiler::main() { extern "C" { -void safe_read_memory_for_testing(void* addr, void* buffer, size_t size) { - safe_read_memory(addr, buffer, size); -} - -void init_safe_read_handlers_for_testing(void) { - init_safe_read_handlers(); +/** + * Reads a stack region for testing purposes. See read_stack_region for semantics. + * Returns the number of bytes actually read, 0 on failure. + */ +size_t read_stack_region_for_testing(void* sp, void* buf, size_t buf_size) { + return read_stack_region(sp, static_cast(buf), buf_size); } } // extern "C" diff --git a/DatadogProfiling/Tests/SafeReadTests.swift b/DatadogProfiling/Tests/SafeReadTests.swift index a7d9cabeb2..7bcc7ae5de 100644 --- a/DatadogProfiling/Tests/SafeReadTests.swift +++ b/DatadogProfiling/Tests/SafeReadTests.swift @@ -5,95 +5,532 @@ */ #if !os(watchOS) +import Foundation import XCTest + +// swiftlint:disable duplicate_imports +import DatadogMachProfiler import DatadogMachProfiler.Testing +// swiftlint:enable duplicate_imports -// Warning: To run successfully these tests, the LLDB debugger should be detached in the target schema, -// like when using xcodebuild where the tests run in a "headless" mode without the debugger. +// These tests exercise the safe-read path used by the Mach sampling profiler. +// Because vm_read_overwrite never raises EXC_BAD_ACCESS, the debugger does not +// need to be detached - no signal or Mach exception is generated for unmapped reads. final class SafeReadTests: XCTestCase { - override func setUp() { - super.setUp() - init_safe_read_handlers_for_testing() + // MARK: - Basic correctness + + func testReadStackRegionWithValidAddress() { + // Given: a known value on the current stack + var sentinel: UInt64 = 0x1122334455667788 + var buf = [UInt8](repeating: 0, count: 128) + + // When: reading the stack region starting at the sentinel's address + let bytesRead = withUnsafeMutablePointer(to: &sentinel) { spPtr in + buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(spPtr, bufPtr.baseAddress!, 128) + } + } + + // Then: at least 8 bytes are returned and the sentinel is at offset 0 + XCTAssertGreaterThanOrEqual(bytesRead, MemoryLayout.size) + let readBack = buf.withUnsafeBytes { $0.load(as: UInt64.self) } + XCTAssertEqual(readBack, 0x1122334455667788) + } + + func testReadStackRegionWithUnmappedAddress() { + // Given: an address that is not mapped in this process + let unmapped = UnsafeMutableRawPointer(bitPattern: 0xDEADBEEF)! + var buf = [UInt8](repeating: 0xFF, count: 128) + + // When + let bytesRead = buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(unmapped, bufPtr.baseAddress!, 128) + } + + // Then: vm_read_overwrite returns an error - no signal, no crash, 0 bytes + XCTAssertEqual(bytesRead, 0) } - func testValidMemoryRead() { + func testReadStackRegionWithNullAddress() { // Given - var validValue: UInt64 = 0x1122334455667788 - var result: UInt64 = 0 + var buf = [UInt8](repeating: 0, count: 16) // When - let success = safe_read_memory_for_testing(&validValue, &result, MemoryLayout.size) + let bytesRead = buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(nil, bufPtr.baseAddress!, 128) + } // Then - XCTAssertTrue(success) - XCTAssertEqual(result, 0x1122334455667788) + XCTAssertEqual(bytesRead, 0) } - func testInvalidMemoryRead() { - // Given - let invalidPtr = get_invalid_address() - var result: UInt64 = 0 + func testReadAcrossPartialUnmapReturnsReadablePrefix() { + // Given: three contiguous pages with the middle page deallocated. + // The fast full-region vm_read_overwrite fails, then the fallback reads + // the contiguous mapped prefix page-by-page. + let pageSize = vm_size_t(getpagesize()) + let totalSize = pageSize * 3 + + var addr: vm_address_t = 0 + let allocResult = vm_allocate(mach_task_self_, &addr, totalSize, VM_FLAGS_ANYWHERE) + XCTAssertEqual(allocResult, KERN_SUCCESS, "vm_allocate failed") + + // Fault the first page in so we know it is mapped and readable. + UnsafeMutableRawPointer(bitPattern: UInt(addr))! + .storeBytes(of: UInt64(0xAABBCCDD), as: UInt64.self) + + // Punch a hole: deallocate the middle page only. + let middleAddr = addr + vm_address_t(pageSize) + let deallocMiddle = vm_deallocate(mach_task_self_, middleAddr, pageSize) + XCTAssertEqual(deallocMiddle, KERN_SUCCESS, "vm_deallocate(middle) failed") + + defer { + // Cleanup remaining pages. + vm_deallocate(mach_task_self_, addr, pageSize) + vm_deallocate(mach_task_self_, addr + 2 * vm_address_t(pageSize), pageSize) + } + + var buf = [UInt8](repeating: 0, count: Int(totalSize)) + let startPtr = UnsafeMutableRawPointer(bitPattern: UInt(addr))! + + // When: reading across the unmapped hole. + let bytesAcross = buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(startPtr, bufPtr.baseAddress!, Int(totalSize)) + } + + // Then: the readable prefix is preserved instead of losing the sample. + XCTAssertEqual(bytesAcross, Int(pageSize), "Fallback should keep the first readable page") + let readBack = buf.withUnsafeBytes { $0.load(as: UInt64.self) } + XCTAssertEqual(readBack, 0xAABBCCDD) + + // And: reading only within the first mapped page still succeeds. + let bytesFirst = buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(startPtr, bufPtr.baseAddress!, Int(pageSize)) + } + XCTAssertEqual(bytesFirst, Int(pageSize), "Reading entirely within a mapped page must succeed") + } + + func testReadAcrossPartialUnmapFromUnalignedAddressReturnsReadablePrefix() { + // Given: a read starting near the end of a mapped page and crossing into + // an unmapped page. This exercises the fallback chunk sizing for SP + // values that are not page-aligned. + let pageSize = vm_size_t(getpagesize()) + let readablePrefixSize = 32 + let totalSize = pageSize * 2 + + var addr: vm_address_t = 0 + let allocResult = vm_allocate(mach_task_self_, &addr, totalSize, VM_FLAGS_ANYWHERE) + XCTAssertEqual(allocResult, KERN_SUCCESS, "vm_allocate failed") + + let secondPageAddr = addr + vm_address_t(pageSize) + let deallocSecond = vm_deallocate(mach_task_self_, secondPageAddr, pageSize) + XCTAssertEqual(deallocSecond, KERN_SUCCESS, "vm_deallocate(second) failed") + + defer { + vm_deallocate(mach_task_self_, addr, pageSize) + } + + let startAddress = addr + vm_address_t(Int(pageSize) - readablePrefixSize) + let startPtr = UnsafeMutableRawPointer(bitPattern: UInt(startAddress))! + memset(startPtr, 0xAB, readablePrefixSize) + + var buf = [UInt8](repeating: 0, count: readablePrefixSize * 2) + let requestedSize = buf.count // When - let success = safe_read_memory_for_testing(invalidPtr, &result, MemoryLayout.size) + let bytesRead = buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(startPtr, bufPtr.baseAddress!, requestedSize) + } // Then - XCTAssertFalse(success) + XCTAssertEqual(bytesRead, readablePrefixSize) + XCTAssertEqual(Array(buf.prefix(readablePrefixSize)), Array(repeating: 0xAB, count: readablePrefixSize)) } - func testRecoveryAndReuse() { - // Given - var result: UInt64 = 0 - let invalidPtr = get_invalid_address() - var validValue: UInt64 = 42 - - // When read INVALID memory - let failedRead = safe_read_memory_for_testing(invalidPtr, &result, MemoryLayout.size) - XCTAssertFalse(failedRead, "First invalid read should fail") - - // When immediately try a VALID read - let success2 = safe_read_memory_for_testing(&validValue, &result, MemoryLayout.size) - XCTAssertTrue(success2, "Subsequent valid read should succeed") - XCTAssertEqual(result, 42) - - // When trigger crash again - let failedRead2 = safe_read_memory_for_testing(invalidPtr, &result, MemoryLayout.size) - XCTAssertFalse(failedRead2, "Second invalid read should fail safely") + // MARK: - Recovery and reuse + + func testRecoveryAfterInvalidRead() { + // Given: an invalid address followed by a valid one + var sentinel: UInt64 = 42 + var buf = [UInt8](repeating: 0, count: 128) + let unmapped = UnsafeMutableRawPointer(bitPattern: 0xDEADBEEF)! + + // When: first read fails + let failedBytes = buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(unmapped, bufPtr.baseAddress!, 128) + } + XCTAssertEqual(failedBytes, 0, "Read from unmapped address should return 0 bytes") + + // When: subsequent read from a valid address succeeds + let successBytes = withUnsafeMutablePointer(to: &sentinel) { spPtr in + buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(spPtr, bufPtr.baseAddress!, 128) + } + } + XCTAssertGreaterThanOrEqual(successBytes, MemoryLayout.size) + let readBack = buf.withUnsafeBytes { $0.load(as: UInt64.self) } + XCTAssertEqual(readBack, 42) } - func testMultithreadedCrashes() { - // Given - let iterations = 1_000 - let expectation = self.expectation(description: "Concurrent reads") + // MARK: - Concurrency + + func testConcurrentReadsDoNotCrash() { + // Given: many threads simultaneously reading valid and invalid addresses + let iterations = 500 + let expectation = self.expectation(description: "Concurrent stack region reads") expectation.expectedFulfillmentCount = iterations - DispatchQueue.concurrentPerform(iterations: iterations) { i in - var result: UInt64 = 0 + DispatchQueue.concurrentPerform(iterations: iterations) { index in + var buf = [UInt8](repeating: 0, count: 128) - //Even threads - if i % 2 == 0 { - //Read VALID memory - var valid = UInt64(i) - let success = safe_read_memory_for_testing(&valid, &result, MemoryLayout.size) - XCTAssertTrue(success, "Thread \(i) valid read failed") - } - // Odd threads - else { - // Read INVALID memory (Crash) - let invalidPtr = self.get_invalid_address() - let success = safe_read_memory_for_testing(invalidPtr, &result, MemoryLayout.size) - XCTAssertFalse(success, "Thread \(i) invalid read should have failed safely") + if index.isMultiple(of: 2) { + // Valid: read from a live stack variable + var value = UInt64(index) + let bytes = withUnsafeMutablePointer(to: &value) { spPtr in + buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(spPtr, bufPtr.baseAddress!, 128) + } + } + XCTAssertGreaterThan(bytes, 0, "Valid read on iteration \(index) should return > 0 bytes") + } else { + // Invalid: unmapped address - must return 0, must not crash + let unmapped = UnsafeMutableRawPointer(bitPattern: 0xDEADBEEF)! + let bytes = buf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(unmapped, bufPtr.baseAddress!, 128) + } + XCTAssertEqual(bytes, 0, "Invalid read on iteration \(index) should return 0 bytes") } expectation.fulfill() } - // Then - wait(for: [expectation], timeout: 0.1) + wait(for: [expectation], timeout: 2.0) + } + + // MARK: - Walk termination on garbage frame pointers + + func testWalkTerminatesOnGarbageFramePointer() { + let maxDepth: UInt32 = 10 + let stackBase: UInt = 0x1_0000_0000 + let bufSize = 256 + let validPC = UnsafeMutableRawPointer(bitPattern: 0x40_0000)! + + let subcases: [(name: String, fp: UnsafeMutableRawPointer)] = [ + ("unaligned FP", UnsafeMutableRawPointer(bitPattern: stackBase + 0x11)!), + ("FP below stack_base", UnsafeMutableRawPointer(bitPattern: stackBase - 0x100)!), + ("FP below MIN_USERSPACE_ADDR", UnsafeMutableRawPointer(bitPattern: 0x8)!), + ("FP above MAX_USERSPACE_ADDR", UnsafeMutableRawPointer(bitPattern: 0x8000_0000_0000)!), + ("FP beyond bytes_read", UnsafeMutableRawPointer(bitPattern: stackBase + UInt(bufSize) + 0x100)!) + ] + + for subcase in subcases { + let count = runWalk( + initialFP: subcase.fp, + initialPC: validPC, + stackBase: stackBase, + bufSize: bufSize, + maxDepth: maxDepth + ) + let message = "\(subcase.name): walk should record only the initial PC and stop" + XCTAssertEqual(count, 1, message) + } } - private func get_invalid_address() -> UnsafeMutableRawPointer { - UnsafeMutableRawPointer(bitPattern: 0xDEADBEEF)! + func testWalkStopsAtInvalidSavedPC() { + let maxDepth: UInt32 = 10 + let stackBase: UInt = 0x1_0000_0000 + let bufSize = 256 + let validPC1 = UnsafeMutableRawPointer(bitPattern: 0x40_0000)! + let validPC2 = UnsafeMutableRawPointer(bitPattern: 0x50_0000)! + let initialFP = UnsafeMutableRawPointer(bitPattern: stackBase + 0x40)! + + let count = runWalk( + initialFP: initialFP, + initialPC: validPC1, + stackBase: stackBase, + bufSize: bufSize, + maxDepth: maxDepth + ) { ptr in + // Frame at offset 0x40: saved_fp = stackBase + 0x80 (valid), saved_pc = validPC2 (valid) + let frame1 = ptr.advanced(by: 0x40).assumingMemoryBound(to: UInt.self) + frame1[0] = stackBase + 0x80 + frame1[1] = UInt(bitPattern: validPC2) + + // Frame at offset 0x80: saved_fp = valid, saved_pc = 0 (invalid) + let frame2 = ptr.advanced(by: 0x80).assumingMemoryBound(to: UInt.self) + frame2[0] = stackBase + 0xC0 + frame2[1] = 0 + } + + let message = "Walk should record validPC1 + validPC2 then stop when the next saved PC is invalid" + XCTAssertEqual(count, 2, message) + } + + func testWalkUsesSafeReadFallbackWhenValidFramePointerIsOutsideSnapshot() { + let maxDepth: UInt32 = 4 + let pageSize = vm_size_t(getpagesize()) + let totalSize = pageSize * 2 + + var addr: vm_address_t = 0 + let allocResult = vm_allocate(mach_task_self_, &addr, totalSize, VM_FLAGS_ANYWHERE) + guard allocResult == KERN_SUCCESS else { + XCTFail("vm_allocate failed") + return + } + defer { vm_deallocate(mach_task_self_, addr, totalSize) } + + let stackBase = UInt(addr) + let firstFrameAddress = stackBase + UInt(pageSize) + 0x40 + let secondFrameAddress = firstFrameAddress + 0x40 + let validPC1 = UnsafeMutableRawPointer(bitPattern: 0x40_0000)! + let validPC2 = UnsafeMutableRawPointer(bitPattern: 0x50_0000)! + + let firstFrame = UnsafeMutableRawPointer(bitPattern: firstFrameAddress)!.assumingMemoryBound(to: UInt.self) + firstFrame[0] = secondFrameAddress + firstFrame[1] = UInt(bitPattern: validPC2) + + let secondFrame = UnsafeMutableRawPointer(bitPattern: secondFrameAddress)!.assumingMemoryBound(to: UInt.self) + secondFrame[0] = secondFrameAddress + 0x40 + secondFrame[1] = 0 + + let count = runWalk( + initialFP: UnsafeMutableRawPointer(bitPattern: firstFrameAddress)!, + initialPC: validPC1, + stackBase: stackBase, + bufSize: 128, + maxDepth: maxDepth, + useSafeReadFallback: true + ) + + XCTAssertEqual(count, 2, "Safe-read fallback should follow valid frame pointers outside the initial snapshot") + } + + func testWalkTerminatesOnFramePointerCycle() { + // A self-referential frame: the saved FP at offset 0x40 points back to + // offset 0x40, with a valid saved PC so the walk doesn't bail on the PC + // guard. The loop must still terminate at max_depth. + let maxDepth: UInt32 = 16 + let stackBase: UInt = 0x1_0000_0000 + let bufSize = 256 + let validPC = UnsafeMutableRawPointer(bitPattern: 0x40_0000)! + let cycleOffset: UInt = 0x40 + let cycleFP = UnsafeMutableRawPointer(bitPattern: stackBase + cycleOffset)! + + let count = runWalk( + initialFP: cycleFP, + initialPC: validPC, + stackBase: stackBase, + bufSize: bufSize, + maxDepth: maxDepth + ) { ptr in + let words = ptr.advanced(by: Int(cycleOffset)).assumingMemoryBound(to: UInt.self) + words[0] = stackBase + cycleOffset + words[1] = UInt(bitPattern: validPC) + } + + XCTAssertEqual(count, maxDepth, "FP cycle should iterate until max_depth and stop") + } + + // MARK: - Mach exception handler is not triggered + + #if !os(tvOS) + // thread_get_exception_ports / thread_set_exception_ports / mach_msg are not + // available on tvOS, so this regression test is iOS-only. + func testMachExceptionHandlerNotTriggeredByUnmappedRead() { + // Pins the Crashlytics fix: vm_read_overwrite must not raise EXC_BAD_ACCESS, + // otherwise a Mach exception handler installed on the current thread would + // receive a message before the kernel converts to SIGBUS/SIGSEGV. + + var exceptionPort: mach_port_t = 0 + var kr = mach_port_allocate(mach_task_self_, MACH_PORT_RIGHT_RECEIVE, &exceptionPort) + XCTAssertEqual(kr, KERN_SUCCESS, "mach_port_allocate failed") + defer { + // Release the send right we inserted (below), then the receive right + // we allocated above. mach_port_destroy is deprecated since macOS 12. + mach_port_deallocate(mach_task_self_, exceptionPort) + mach_port_mod_refs(mach_task_self_, exceptionPort, MACH_PORT_RIGHT_RECEIVE, -1) + } + + kr = mach_port_insert_right( + mach_task_self_, + exceptionPort, + exceptionPort, + mach_msg_type_name_t(MACH_MSG_TYPE_MAKE_SEND) + ) + XCTAssertEqual(kr, KERN_SUCCESS, "mach_port_insert_right failed") + + let currentThread = mach_thread_self() + defer { mach_port_deallocate(mach_task_self_, currentThread) } + + let mask = exception_mask_t(1 << EXC_BAD_ACCESS) | exception_mask_t(1 << EXC_BAD_INSTRUCTION) + + var savedMasks = [exception_mask_t](repeating: 0, count: Int(EXC_TYPES_COUNT)) + var savedPorts = [mach_port_t](repeating: 0, count: Int(EXC_TYPES_COUNT)) + var savedBehavior = [exception_behavior_t](repeating: 0, count: Int(EXC_TYPES_COUNT)) + var savedFlavor = [thread_state_flavor_t](repeating: 0, count: Int(EXC_TYPES_COUNT)) + var savedCount = mach_msg_type_number_t(EXC_TYPES_COUNT) + + kr = thread_get_exception_ports( + currentThread, + mask, + &savedMasks, + &savedCount, + &savedPorts, + &savedBehavior, + &savedFlavor + ) + XCTAssertEqual(kr, KERN_SUCCESS, "thread_get_exception_ports failed") + + kr = thread_set_exception_ports( + currentThread, + mask, + exceptionPort, + exception_behavior_t(EXCEPTION_DEFAULT), + THREAD_STATE_NONE + ) + XCTAssertEqual(kr, KERN_SUCCESS, "thread_set_exception_ports failed") + defer { + for index in 0.. mach_msg_return_t in + let header = ptr.baseAddress!.assumingMemoryBound(to: mach_msg_header_t.self) + return mach_msg( + header, + MACH_RCV_MSG | MACH_RCV_TIMEOUT, + 0, + bufferSize, + exceptionPort, + 500, + 0 + ) + } + receivedLock.lock() + didReceiveMessage = (result == MACH_MSG_SUCCESS) + receivedLock.unlock() + listenerDone.signal() + } + + // Trigger an unmapped read. vm_read_overwrite must return 0 without + // raising anything that reaches the exception port. + var readBuf = [UInt8](repeating: 0, count: 128) + let unmapped = UnsafeMutableRawPointer(bitPattern: 0xDEAD_BEEF)! + let bytesRead = readBuf.withUnsafeMutableBytes { bufPtr in + read_stack_region_for_testing(unmapped, bufPtr.baseAddress!, 128) + } + XCTAssertEqual(bytesRead, 0, "Unmapped vm_read_overwrite must return 0") + + let waitResult = listenerDone.wait(timeout: .now() + 1.5) + XCTAssertEqual(waitResult, .success, "Listener should complete (timed out, as expected)") + + receivedLock.lock() + let gotMessage = didReceiveMessage + receivedLock.unlock() + + let regressionMessage = "Mach exception handler must NOT receive a message - this is the Crashlytics fix" + XCTAssertFalse(gotMessage, regressionMessage) + } + #endif // !os(tvOS) + + // MARK: - End-to-end: frame capture via the full profiler + + func testProfilerCapturesFramesFromSuspendedThread() { + // This exercises the complete path: thread_get_frame_pointers -> read_stack_region + // -> memcpy frame walk, all without raising any memory fault. + let mockThread = MockThread { + XCTAssertEqual(dd_profiler_start(), 1) + + // Provide a non-trivial call stack so the profiler has frames to walk. + recursiveWork(depth: 10) { + Thread.sleep(forTimeInterval: 0.05) + } + + dd_profiler_stop() + + let sampleCount = dd_pprof_sample_count(dd_profiler_get_profile()) + XCTAssertGreaterThan(sampleCount, 0, "Profiler should capture stack frames via batch read") + + dd_profiler_destroy() + } + + mockThread.start() + XCTAssertTrue(mockThread.waitForWorkCompletion(timeout: 5.0)) + mockThread.cancel() + } +} + +// MARK: - Helpers + +private func recursiveWork(depth: Int, work: @escaping () -> Void) { + guard depth > 0 else { + work() + return + } + recursiveWork(depth: depth - 1, work: work) +} + +/// Drives the frame walker against a synthetic stack buffer. The caller can populate +/// the buffer via the `populate` closure before the walk runs. +/// +/// Returns the resulting `frame_count` so callers can assert how many frames the +/// walk recorded before terminating on a guard. +private func runWalk( + initialFP: UnsafeMutableRawPointer, + initialPC: UnsafeMutableRawPointer, + stackBase: UInt, + bufSize: Int, + maxDepth: UInt32, + useSafeReadFallback: Bool = false, + populate: (UnsafeMutableRawPointer) -> Void = { _ in } +) -> UInt32 { + var buf = [UInt8](repeating: 0, count: bufSize) + var frames = [stack_frame_t](repeating: stack_frame_t(), count: Int(maxDepth)) + var trace = stack_trace_t() + + return frames.withUnsafeMutableBufferPointer { framesPtr in + trace.frames = framesPtr.baseAddress + return buf.withUnsafeMutableBytes { bufPtr in + populate(bufPtr.baseAddress!) + if useSafeReadFallback { + walk_frames_with_safe_read_fallback_for_testing( + &trace, + initialFP, + initialPC, + stackBase, + bufPtr.baseAddress!.assumingMemoryBound(to: UInt8.self), + bufSize, + maxDepth + ) + } else { + walk_frames_in_buffer( + &trace, + initialFP, + initialPC, + stackBase, + bufPtr.baseAddress!.assumingMemoryBound(to: UInt8.self), + bufSize, + maxDepth + ) + } + return trace.frame_count + } } } #endif // !os(watchOS) From c95ed99f54838aed5c815415077242a61a7ae91e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sima=CC=83o=20Seic=CC=A7a?= Date: Thu, 18 Jun 2026 13:06:40 +0100 Subject: [PATCH 2/2] RUM-16442: Handle PR review comments --- .../Mach/mach_sampling_profiler.cpp | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/DatadogProfiling/Mach/mach_sampling_profiler.cpp b/DatadogProfiling/Mach/mach_sampling_profiler.cpp index 2d5f066715..710fee74cd 100644 --- a/DatadogProfiling/Mach/mach_sampling_profiler.cpp +++ b/DatadogProfiling/Mach/mach_sampling_profiler.cpp @@ -67,6 +67,13 @@ static constexpr size_t STACK_REGION_MAX_READ = 65536; // 64 KB safety cap // 128 bytes covers the vast majority of real-world frames without over-reading. static constexpr size_t STACK_REGION_FRAME_SIZE_ESTIMATE = 128; +struct frame_pointer_pair_t { + void* next_frame_pointer; + void* return_address; +}; + +static_assert(sizeof(frame_pointer_pair_t) == sizeof(void*) * 2, "Frame pointer pair must stay two pointers."); + // Main thread pthread identifier for comparison static pthread_t g_main_pthread = NULL; @@ -98,7 +105,8 @@ static size_t read_memory_region(void* addr, uint8_t* buf, size_t size) { } const uintptr_t address = reinterpret_cast(addr); - if (address > UINTPTR_MAX - size) { + const uintptr_t buffer = reinterpret_cast(buf); + if (address > UINTPTR_MAX - size || buffer > UINTPTR_MAX - size) { return 0; } @@ -146,7 +154,7 @@ static size_t read_stack_region_by_pages(void* sp, uint8_t* buf, size_t size) { } total_read += bytes_read; - if (total_read == size || current_address > UINTPTR_MAX - bytes_read) { + if (total_read == size) { break; } @@ -321,18 +329,18 @@ static bool read_frame_pair_from_snapshot( uintptr_t stack_base, const uint8_t* stack_buf, size_t bytes_read, - void* next_frame[2] + frame_pointer_pair_t* next_frame ) { - if (stack_base == 0 || stack_buf == nullptr || bytes_read < sizeof(void*[2]) || fp < stack_base) { + if (stack_base == 0 || stack_buf == nullptr || bytes_read < sizeof(frame_pointer_pair_t) || fp < stack_base) { return false; } const uintptr_t fp_offset = fp - stack_base; - if (fp_offset > bytes_read - sizeof(void*[2])) { + if (fp_offset > bytes_read - sizeof(frame_pointer_pair_t)) { return false; } - memcpy(static_cast(next_frame), stack_buf + fp_offset, sizeof(void*[2])); + memcpy(static_cast(next_frame), stack_buf + fp_offset, sizeof(frame_pointer_pair_t)); return true; } @@ -343,12 +351,12 @@ static bool read_frame_pair_from_snapshot( * snapshot. It preserves stack accuracy for large frames without returning to * faulting memory reads; vm_read_overwrite reports failure instead of crashing. */ -static bool read_frame_pair_from_memory(uintptr_t fp, void* next_frame[2]) { +static bool read_frame_pair_from_memory(uintptr_t fp, frame_pointer_pair_t* next_frame) { return read_memory_region( reinterpret_cast(fp), reinterpret_cast(next_frame), - sizeof(void*[2]) - ) == sizeof(void*[2]); + sizeof(frame_pointer_pair_t) + ) == sizeof(frame_pointer_pair_t); } /** @@ -387,15 +395,15 @@ static void walk_frames( // so fp must be at or above sp when a stack snapshot is available. if (stack_base != 0 && fp_addr < stack_base) break; - void* next_frame[2]; - if (!read_frame_pair_from_snapshot(fp_addr, stack_base, stack_buf, bytes_read, next_frame)) { - if (!allow_memory_fallback || !read_frame_pair_from_memory(fp_addr, next_frame)) { + frame_pointer_pair_t next_frame; + if (!read_frame_pair_from_snapshot(fp_addr, stack_base, stack_buf, bytes_read, &next_frame)) { + if (!allow_memory_fallback || !read_frame_pair_from_memory(fp_addr, &next_frame)) { break; } } - fp = next_frame[0]; // next frame pointer (saved x29 / rbp) - pc = next_frame[1]; // return address (saved lr / return addr on stack) + fp = next_frame.next_frame_pointer; // saved x29 / rbp + pc = next_frame.return_address; // saved lr / return address on stack if (!is_valid_userspace_addr(reinterpret_cast(pc))) break; }