diff --git a/samply-mac-preload/Cargo.lock b/samply-mac-preload/Cargo.lock index cfb93c6d..df63d53a 100644 --- a/samply-mac-preload/Cargo.lock +++ b/samply-mac-preload/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "autocfg" @@ -8,6 +8,31 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa" +[[package]] +name = "byteorder" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fd0f2584146f6f2ef48085050886acf353beff7305ebd1ae69500e27c67f64b" + +[[package]] +name = "hash32" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47d60b12902ba28e2730cd37e95b8c9223af2808df9e902d4df49588d1470606" +dependencies = [ + "byteorder", +] + +[[package]] +name = "heapless" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0bfb9eb618601c89945a70e254898da93b13be0388091d42117462b265bb3fad" +dependencies = [ + "hash32", + "stable_deref_trait", +] + [[package]] name = "libc" version = "0.2.127" @@ -24,11 +49,19 @@ dependencies = [ "scopeguard", ] +[[package]] +name = "log" +version = "0.4.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "113b30b4cd05f7c06868fdb2854f66a7b9fece9a48425351cd532e810d74024f" + [[package]] name = "samply-mac-preload" version = "0.1.0" dependencies = [ + "heapless", "libc", + "log", "spin", ] @@ -46,3 +79,9 @@ checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67" dependencies = [ "lock_api", ] + +[[package]] +name = "stable_deref_trait" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" diff --git a/samply-mac-preload/Cargo.toml b/samply-mac-preload/Cargo.toml index 87595aa7..67f6d9ca 100644 --- a/samply-mac-preload/Cargo.toml +++ b/samply-mac-preload/Cargo.toml @@ -20,5 +20,7 @@ panic = 'abort' [dependencies] libc = { version = "0.2", default-features = false } +heapless = { version = "0.8", default-features = false } +log = { version = "0.4", default-features = false } spin = "0.9.8" diff --git a/samply-mac-preload/binaries/libsamply_mac_preload.dylib b/samply-mac-preload/binaries/libsamply_mac_preload.dylib index 0c1fc394..fcbc14fd 100755 Binary files a/samply-mac-preload/binaries/libsamply_mac_preload.dylib and b/samply-mac-preload/binaries/libsamply_mac_preload.dylib differ diff --git a/samply-mac-preload/binaries/libsamply_mac_preload_arm64.dylib b/samply-mac-preload/binaries/libsamply_mac_preload_arm64.dylib index efa29040..63d5d1ad 100755 Binary files a/samply-mac-preload/binaries/libsamply_mac_preload_arm64.dylib and b/samply-mac-preload/binaries/libsamply_mac_preload_arm64.dylib differ diff --git a/samply-mac-preload/binaries/libsamply_mac_preload_arm64e.dylib b/samply-mac-preload/binaries/libsamply_mac_preload_arm64e.dylib index 500af622..93c2255e 100755 Binary files a/samply-mac-preload/binaries/libsamply_mac_preload_arm64e.dylib and b/samply-mac-preload/binaries/libsamply_mac_preload_arm64e.dylib differ diff --git a/samply-mac-preload/binaries/libsamply_mac_preload_x86_64.dylib b/samply-mac-preload/binaries/libsamply_mac_preload_x86_64.dylib index 646f38df..5c1daec5 100755 Binary files a/samply-mac-preload/binaries/libsamply_mac_preload_x86_64.dylib and b/samply-mac-preload/binaries/libsamply_mac_preload_x86_64.dylib differ diff --git a/samply-mac-preload/build.sh b/samply-mac-preload/build.sh index 2612e3d6..bf45529b 100755 --- a/samply-mac-preload/build.sh +++ b/samply-mac-preload/build.sh @@ -1,9 +1,9 @@ -export SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk +export SDKROOT="$(xcrun --show-sdk-path)" MACOSX_DEPLOYMENT_TARGET=10.12 cargo build --release --target=x86_64-apple-darwin mv target/x86_64-apple-darwin/release/libsamply_mac_preload.dylib binaries/libsamply_mac_preload_x86_64.dylib MACOSX_DEPLOYMENT_TARGET=11.0 cargo build --release --target=aarch64-apple-darwin mv target/aarch64-apple-darwin/release/libsamply_mac_preload.dylib binaries/libsamply_mac_preload_arm64.dylib -MACOSX_DEPLOYMENT_TARGET=11.0 RUSTC_BOOTSTRAP=1 cargo build --release --target=arm64e-apple-darwin -Zbuild-std=core +MACOSX_DEPLOYMENT_TARGET=11.0 RUSTC_BOOTSTRAP=1 CARGO_TARGET_ARM64E_APPLE_DARWIN_LINKER=clang cargo build --release --target=arm64e-apple-darwin -Zbuild-std=core mv target/arm64e-apple-darwin/release/libsamply_mac_preload.dylib binaries/libsamply_mac_preload_arm64e.dylib lipo binaries/libsamply_mac_preload_* -create -output binaries/libsamply_mac_preload.dylib gzip -cvf binaries/libsamply_mac_preload.dylib > ../samply/resources/libsamply_mac_preload.dylib.gz diff --git a/samply-mac-preload/src/lib.rs b/samply-mac-preload/src/lib.rs index 894dbe9f..4deb2a94 100644 --- a/samply-mac-preload/src/lib.rs +++ b/samply-mac-preload/src/lib.rs @@ -5,8 +5,10 @@ use libc::{c_char, c_int, mode_t, FILE}; use core::ffi::CStr; +mod logging; mod mach_ipc; mod mach_sys; +mod sip_warn; use mach_ipc::{channel, mach_task_self, OsIpcChannel, OsIpcSender}; @@ -30,12 +32,59 @@ fn panic(_panic: &core::panic::PanicInfo<'_>) -> ! { #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] static __SETUP_SAMPLY_CONNECTION: unsafe extern "C" fn() = { unsafe extern "C" fn __load_samply_lib() { + logging::init(); let _ = set_up_samply_connection(); } __load_samply_lib }; +/// Returns true if the current process is an Apple "platform binary" +/// (`CS_PLATFORM_BINARY`). Such processes are given an *immovable* task-self +/// mach port by the kernel: any attempt to transfer that port to another +/// process — which is exactly what samply's task handoff below does — raises a +/// fatal `EXC_GUARD` (`ILLEGAL_MOVE`) and the kernel SIGKILLs the process. +/// +/// This is how `samply record -- ` would otherwise kill `dsymutil` (and +/// other Apple toolchain binaries) that a build invokes: they inherit samply's +/// `DYLD_INSERT_LIBRARIES`, load this preload, and crash in the handoff. +/// +/// samply cannot profile platform binaries through this mechanism regardless +/// (their task port is protected), so detecting this case and skipping the +/// handoff loses nothing and keeps the process alive. +fn is_platform_binary() -> bool { + // `csops(getpid(), CS_OPS_STATUS, &flags, sizeof(flags))` reports the + // process's code-signing status flags. It works on the calling process + // without any privilege. CS_PLATFORM_BINARY == 0x04000000. + const CS_OPS_STATUS: u32 = 0; + const CS_PLATFORM_BINARY: u32 = 0x0400_0000; + extern "C" { + fn csops( + pid: libc::pid_t, + ops: u32, + useraddr: *mut libc::c_void, + usersize: libc::size_t, + ) -> libc::c_int; + } + let mut flags: u32 = 0; + let r = unsafe { + csops( + libc::getpid(), + CS_OPS_STATUS, + &mut flags as *mut u32 as *mut libc::c_void, + core::mem::size_of::() as libc::size_t, + ) + }; + r == 0 && (flags & CS_PLATFORM_BINARY) != 0 +} + fn set_up_samply_connection() -> Option<()> { + // Don't hand our task port to samply if we're a platform binary: the port + // is immovable and sending it would get us SIGKILLed. See + // `is_platform_binary`. + if is_platform_binary() { + log::debug!("samply_preload: skipping handoff (platform binary)"); + return None; + } let (tx0, rx0) = channel().ok()?; // Safety: // - b"SAMPLY_BOOTSTRAP_SERVER_NAME\0" is a nul-terminated c string @@ -64,6 +113,7 @@ fn set_up_samply_connection() -> Option<()> { let mut recv_buf = [0; 256]; let result = rx0.recv(&mut recv_buf).ok()?; assert_eq!(b"Proceed", &result); + log::debug!("samply_preload: ready"); Some(()) } @@ -159,6 +209,15 @@ pub struct InterposeEntry { _old: *const (), } +impl InterposeEntry { + pub const fn new(new: *const (), old: *const ()) -> Self { + InterposeEntry { + _new: new, + _old: old, + } + } +} + #[used] #[allow(dead_code)] #[allow(non_upper_case_globals)] diff --git a/samply-mac-preload/src/logging.rs b/samply-mac-preload/src/logging.rs new file mode 100644 index 00000000..b64f9ab1 --- /dev/null +++ b/samply-mac-preload/src/logging.rs @@ -0,0 +1,38 @@ +use core::fmt::Write; +use libc::c_void; + +struct StderrLogger; + +impl log::Log for StderrLogger { + fn enabled(&self, _: &log::Metadata) -> bool { + true + } + + fn log(&self, record: &log::Record) { + // Format into a fixed stack buffer — there is no allocator in a preload. + // Messages are short; on overflow we just write the truncated prefix. + let mut buf = heapless::String::<512>::new(); + let _ = writeln!(buf, "{}", record.args()); + unsafe { + libc::write( + libc::STDERR_FILENO, + buf.as_ptr() as *const c_void, + buf.len(), + ); + } + } + + fn flush(&self) {} +} + +static LOGGER: StderrLogger = StderrLogger; + +/// Install the stderr logger iff `SAMPLY_PRELOAD_DEBUG` is set. Idempotent +/// (a second `set_logger` simply fails and is ignored). Call once, early, from +/// the preload constructor; if never called the max level stays `Off`. +pub(crate) fn init() { + let enabled = unsafe { !libc::getenv(c"SAMPLY_PRELOAD_DEBUG".as_ptr()).is_null() }; + if enabled && log::set_logger(&LOGGER).is_ok() { + log::set_max_level(log::LevelFilter::Trace); + } +} diff --git a/samply-mac-preload/src/sip_warn/detect.rs b/samply-mac-preload/src/sip_warn/detect.rs new file mode 100644 index 00000000..00661e46 --- /dev/null +++ b/samply-mac-preload/src/sip_warn/detect.rs @@ -0,0 +1,150 @@ +//! Decide whether an exec will drop into a SIP-protected (un-injectable) +//! process, and if so warn the user. We never modify the exec — we only observe +//! and print. +//! +//! The warning explains *why* the process can't be profiled (SIP strips the +//! `DYLD_*` variable samply relies on) and what the user can do about it, and is +//! emitted as a GitHub Actions `::warning::` annotation when running in CI so it +//! surfaces in the workflow log. + +use core::ffi::{c_void, CStr}; +use core::fmt::Write; +use libc::{c_char, c_int}; + +/// Called from the exec/posix_spawn interposers. If SIP is active and the +/// effective binary (the target itself, or — for a `#!` script — its shebang +/// interpreter) is SIP-protected, print a warning. Always returns; the caller +/// then performs the real, unmodified exec. +pub(super) unsafe fn check_and_warn(path: *const c_char) { + // If SIP is off, DYLD_* survives even for protected binaries, so the preload + // still loads and there's nothing to warn about. + if path.is_null() || !sip_enabled() { + return; + } + let path = CStr::from_ptr(path); + + if is_sip_protected(path) { + warn(path); + return; + } + + // Not a protected binary itself — but it may be a script whose shebang + // interpreter is one (e.g. `pnpm` -> `#!/usr/bin/env node`). The kernel + // execs that interpreter, and that's where DYLD_* gets stripped. + let mut buf = [0u8; libc::PATH_MAX as usize]; + if let Some(interp) = shebang_interpreter(path, &mut buf) { + if is_sip_protected(interp) { + warn(interp); + } + } +} + +/// Is System Integrity Protection enabled? +fn sip_enabled() -> bool { + // `csr_check(mask)` returns 0 when the active configuration *allows* the + // operation in `mask` (i.e. that protection is off); non-zero means the + // protection is active. If unrestricted filesystem access is not allowed, + // SIP is on. + const CSR_ALLOW_UNRESTRICTED_FS: u32 = 1 << 1; + extern "C" { + fn csr_check(mask: u32) -> c_int; + } + unsafe { csr_check(CSR_ALLOW_UNRESTRICTED_FS) != 0 } +} + +/// Is `path` a SIP-protected system binary? Such binaries carry the restricted +/// file flag, which is what makes the kernel strip `DYLD_*` on exec. +unsafe fn is_sip_protected(path: &CStr) -> bool { + const SF_RESTRICTED: u32 = 0x0008_0000; // from + let mut st: libc::stat = core::mem::zeroed(); + if libc::stat(path.as_ptr(), &mut st) != 0 { + return false; + } + st.st_flags & SF_RESTRICTED != 0 +} + +const SHEBANG_READ_SZ: usize = 256; + +/// If `path` is a regular file with a `#!` shebang, return the interpreter path +/// (written into `buf`). Only the interpreter is needed — we don't rebuild argv. +unsafe fn shebang_interpreter<'a>(path: &CStr, buf: &'a mut [u8]) -> Option<&'a CStr> { + // Only regular files can be scripts (also resolves relative paths via CWD). + let mut st: libc::stat = core::mem::zeroed(); + if libc::stat(path.as_ptr(), &mut st) != 0 || (st.st_mode & libc::S_IFMT) != libc::S_IFREG { + return None; + } + + // Read the first bytes looking for a shebang. `libc::open` here is NOT routed + // back through our own interpose hook: dyld does not re-interpose an image's + // references to symbols it itself interposes. + let mut read_buf = [0u8; SHEBANG_READ_SZ]; + let fd = libc::open(path.as_ptr(), libc::O_RDONLY); + if fd < 0 { + return None; + } + let nread = libc::read(fd, read_buf.as_mut_ptr() as *mut c_void, read_buf.len()); + libc::close(fd); + if nread < 2 || read_buf[0] != b'#' || read_buf[1] != b'!' { + return None; + } + + // Parse "#! [] \n" — we only want . + let line = match read_buf[..nread as usize].iter().position(|&b| b == b'\n') { + Some(pos) => &read_buf[..pos], + None => return None, // truncated / no newline within the window + }; + + let mut start = 2; + while start < line.len() && (line[start] == b' ' || line[start] == b'\t') { + start += 1; + } + if start >= line.len() { + return None; + } + let interp_end = line[start..] + .iter() + .position(|&b| b == b' ' || b == b'\t') + .map(|p| start + p) + .unwrap_or(line.len()); + + let interp = &line[start..interp_end]; + if interp.is_empty() || interp.len() >= buf.len() { + return None; + } + buf[..interp.len()].copy_from_slice(interp); + buf[interp.len()] = 0; + CStr::from_bytes_with_nul(&buf[..=interp.len()]).ok() +} + +/// Print the warning to stderr, as a GitHub Actions annotation when in CI. +unsafe fn warn(binary: &CStr) { + let bin = core::str::from_utf8(binary.to_bytes()).unwrap_or(""); + + let mut msg = heapless::String::<1024>::new(); + let prefix = if in_github_actions() { + "::warning title=CodSpeed cannot profile a system process::" + } else { + "[CodSpeed] warning: " + }; + + const SIP_DOCS_URL: &str = "https://codspeed.io/docs/instruments/walltime/macos-profiling"; + + let _ = writeln!( + msg, + "{prefix}CodSpeed could not profile the system process `{bin}`. System Integrity Protection (SIP) removes the DYLD_INSERT_LIBRARIES \ + environment variable that samply uses to attach to a process whenever a protected Apple system binary is executed, so this process and its children \ + are invisible to the profiler. See {SIP_DOCS_URL} for more information." + ); + + libc::write( + libc::STDERR_FILENO, + msg.as_ptr() as *const c_void, + msg.len(), + ); +} + +/// True iff `GITHUB_ACTIONS=true` in the environment. +unsafe fn in_github_actions() -> bool { + let v = libc::getenv(c"GITHUB_ACTIONS".as_ptr()); + !v.is_null() && CStr::from_ptr(v).to_bytes() == b"true" +} diff --git a/samply-mac-preload/src/sip_warn/mod.rs b/samply-mac-preload/src/sip_warn/mod.rs new file mode 100644 index 00000000..0ebbe50a --- /dev/null +++ b/samply-mac-preload/src/sip_warn/mod.rs @@ -0,0 +1,78 @@ +//! Interpose `execve` + `posix_spawn{,p}` to detect when a process is about to +//! exec a SIP-protected Apple system binary. +//! +//! macOS strips every `DYLD_*` environment variable when the kernel execs a +//! protected platform binary (`/usr/bin/env`, `/bin/sh`, system `python`, …). +//! Once stripped, this preload no longer loads into that process *or any of its +//! descendants*, so samply stops seeing the whole subtree. We can't keep the +//! preload alive across that boundary here — we only *observe* it and print a +//! warning naming the offending binary (see [`detect`]). The real exec then +//! proceeds completely unmodified. + +use libc::{c_char, c_int, pid_t, posix_spawn_file_actions_t, posix_spawnattr_t}; + +use crate::InterposeEntry; + +mod detect; + +unsafe extern "C" fn samply_sip_execve( + path: *const c_char, + argv: *const *const c_char, + envp: *const *const c_char, +) -> c_int { + detect::check_and_warn(path); + libc::execve(path, argv, envp) +} + +unsafe extern "C" fn samply_sip_posix_spawn( + pid: *mut pid_t, + path: *const c_char, + file_actions: *const posix_spawn_file_actions_t, + attrp: *const posix_spawnattr_t, + argv: *const *mut c_char, + envp: *const *mut c_char, +) -> c_int { + detect::check_and_warn(path); + libc::posix_spawn(pid, path, file_actions, attrp, argv, envp) +} + +unsafe extern "C" fn samply_sip_posix_spawnp( + pid: *mut pid_t, + file: *const c_char, + file_actions: *const posix_spawn_file_actions_t, + attrp: *const posix_spawnattr_t, + argv: *const *mut c_char, + envp: *const *mut c_char, +) -> c_int { + // `file` may be a bare name (e.g. "sh") that the real call resolves against + // `$PATH`. No need to resolve it here: libc's `posix_spawnp` funnels into + // `posix_spawn` with the resolved path, which hits our interposer above. + detect::check_and_warn(file); + libc::posix_spawnp(pid, file, file_actions, attrp, argv, envp) +} + +// We only interpose `execve` + `posix_spawn{,p}`: the other `exec*` wrappers +// (`execv`, `execvp`, `execl`, …) funnel through `execve` inside libc, and +// `posix_spawn{,p}` are the separate spawn syscalls used by libuv/Node and +// CPython. +#[used] +#[allow(non_upper_case_globals)] +#[link_section = "__DATA,__interpose"] +pub static mut _interpose_execve: InterposeEntry = + InterposeEntry::new(samply_sip_execve as *const (), libc::execve as *const ()); + +#[used] +#[allow(non_upper_case_globals)] +#[link_section = "__DATA,__interpose"] +pub static mut _interpose_posix_spawn: InterposeEntry = InterposeEntry::new( + samply_sip_posix_spawn as *const (), + libc::posix_spawn as *const (), +); + +#[used] +#[allow(non_upper_case_globals)] +#[link_section = "__DATA,__interpose"] +pub static mut _interpose_posix_spawnp: InterposeEntry = InterposeEntry::new( + samply_sip_posix_spawnp as *const (), + libc::posix_spawnp as *const (), +); diff --git a/samply/resources/libsamply_mac_preload.dylib.gz b/samply/resources/libsamply_mac_preload.dylib.gz index e8085588..82a163de 100644 Binary files a/samply/resources/libsamply_mac_preload.dylib.gz and b/samply/resources/libsamply_mac_preload.dylib.gz differ diff --git a/samply/src/linux_shared/converter.rs b/samply/src/linux_shared/converter.rs index ade6b740..2e671a66 100644 --- a/samply/src/linux_shared/converter.rs +++ b/samply/src/linux_shared/converter.rs @@ -273,6 +273,7 @@ where e, &process.unwinder, &mut self.cache, + &mut process.stack_read_cache, stack, self.fold_recursive_prefix, self.call_chain_return_addresses_are_preadjusted, @@ -402,6 +403,7 @@ where e, &process.unwinder, &mut self.cache, + &mut process.stack_read_cache, stack, self.fold_recursive_prefix, self.call_chain_return_addresses_are_preadjusted, @@ -514,6 +516,7 @@ where e, &process.unwinder, &mut self.cache, + &mut process.stack_read_cache, stack, self.fold_recursive_prefix, self.call_chain_return_addresses_are_preadjusted, @@ -565,6 +568,7 @@ where e, &process.unwinder, &mut self.cache, + &mut process.stack_read_cache, stack, self.fold_recursive_prefix, self.call_chain_return_addresses_are_preadjusted, @@ -620,6 +624,7 @@ where e: &SampleRecord, unwinder: &U, cache: &mut U::Cache, + stack_read_cache: &mut std::collections::HashMap, stack: &mut Vec, fold_recursive_prefix: bool, call_chain_return_addresses_are_preadjusted: bool, @@ -658,10 +663,22 @@ where let ustack_bytes = RawDataU64::from_raw_data::(user_stack); let (pc, sp, regs) = C::convert_regs(regs); let mut read_stack = |addr: u64| { - // ustack_bytes has the stack bytes starting from the current stack pointer. - let offset = addr.checked_sub(sp).ok_or(())?; - let index = usize::try_from(offset / 8).map_err(|_| ())?; - ustack_bytes.get(index).ok_or(()) + // Prefer this sample's freshly captured stack window. ustack_bytes + // has the stack bytes starting from the current stack pointer. + if let Some(value) = addr + .checked_sub(sp) + .and_then(|offset| usize::try_from(offset / 8).ok()) + .and_then(|index| ustack_bytes.get(index)) + { + // Remember it: the upper stack is stable across samples, so a + // later sample whose window doesn't reach this far can still + // satisfy the read. + stack_read_cache.insert(addr, value); + return Ok(value); + } + // The read is below sp or past the captured window. Fall back to + // a value seen in an earlier sample, if any. + stack_read_cache.get(&addr).copied().ok_or(()) }; // Unwind. diff --git a/samply/src/linux_shared/process.rs b/samply/src/linux_shared/process.rs index bdc1fc50..844628e3 100644 --- a/samply/src/linux_shared/process.rs +++ b/samply/src/linux_shared/process.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::path::{Path, PathBuf}; use framehop::Unwinder; @@ -33,6 +34,12 @@ pub struct Process { pub jit_app_cache_mapping_ops: LibMappingOpQueue, pub jit_function_recycler: Option, marker_file_paths: Vec<(ThreadHandle, PathBuf, Vec)>, + /// Per-process cache of stack memory previously read during unwinding, + /// keyed by absolute stack address. The upper part of the stack (the frames + /// above the churning leaf) is stable across samples, so when an unwind + /// walks past the current sample's captured stack window we can satisfy the + /// read from a value seen in an earlier sample instead of truncating. + pub stack_read_cache: HashMap, pub prev_mm_filepages_size: i64, pub prev_mm_anonpages_size: i64, pub prev_mm_swapents_size: i64, @@ -80,6 +87,7 @@ where jit_app_cache_mapping_ops: LibMappingOpQueue::default(), jit_function_recycler, marker_file_paths: Vec::new(), + stack_read_cache: HashMap::new(), prev_mm_filepages_size: 0, prev_mm_anonpages_size: 0, prev_mm_swapents_size: 0, diff --git a/samply/src/mac/process_launcher.rs b/samply/src/mac/process_launcher.rs index 071b5dae..b9fd8fe7 100644 --- a/samply/src/mac/process_launcher.rs +++ b/samply/src/mac/process_launcher.rs @@ -162,6 +162,17 @@ impl TaskAccepter { add_env("DYLD_INSERT_LIBRARIES", preload_lib_path.as_os_str()); add_env("SAMPLY_BOOTSTRAP_SERVER_NAME", OsStr::new(&server_name)); + // Expose the preload path under a second, non-`DYLD_`-prefixed name. + // macOS SIP strips every `DYLD_*` variable when execing a protected + // system binary (`/usr/bin/env`, `/bin/sh`, …). + // + // This alias survives the stripping, so a script closer to the process + // under observation can re-enable `DYLD_INSERT_LIBRARIES`. + added_env.push(( + "SAMPLY_DYLD_INSERT_LIBRARIES".into(), + preload_lib_path.as_os_str().to_owned(), + )); + Ok(TaskAccepter { server, added_env, diff --git a/samply/src/shared/jit_category_manager.rs b/samply/src/shared/jit_category_manager.rs index 47acf6db..560e4e14 100644 --- a/samply/src/shared/jit_category_manager.rs +++ b/samply/src/shared/jit_category_manager.rs @@ -231,9 +231,8 @@ impl JitCategoryManager { name: &str, profile: &mut Profile, ) -> (SubcategoryHandle, Option) { - static DISABLED: std::sync::LazyLock = std::sync::LazyLock::new(|| { - std::env::var("SAMPLY_DISABLE_JIT_CLASSIFICATION").is_ok() - }); + static DISABLED: std::sync::LazyLock = + std::sync::LazyLock::new(|| std::env::var("SAMPLY_DISABLE_JIT_CLASSIFICATION").is_ok()); if *DISABLED { return (self.generic_jit_category.get(profile).into(), None); } diff --git a/samply/tests/dsymutil_sigkill.rs b/samply/tests/dsymutil_sigkill.rs new file mode 100644 index 00000000..c8a5b43a --- /dev/null +++ b/samply/tests/dsymutil_sigkill.rs @@ -0,0 +1,122 @@ +//! Regression test for: `samply record` SIGKILLs `dsymutil`. +//! +//! samply injects `DYLD_INSERT_LIBRARIES` (+ `SAMPLY_BOOTSTRAP_SERVER_NAME`) into +//! the *entire* descendant process tree it launches. Any descendant that loads +//! the preload hands its mach task port to samply and lets it "control us +//! completely" (see `samply-mac-preload`). For `dsymutil` this takeover ends in a +//! deterministic `SIGKILL`, which breaks builds run under `samply record` on +//! macOS (the linker invokes `dsymutil` and reports `running dsymutil failed: +//! signal: killed`). +//! +//! This test launches, under the built `samply` binary, a small locally-built +//! "spawner" that execs `dsymutil` on a Mach-O with DWARF, and asserts that +//! `dsymutil` is NOT killed (the desired behaviour). +//! +//! macOS-only (`cfg(target_os = "macos")`), so it compiles out elsewhere. It +//! needs Xcode's `dsymutil` and a working `cc`, both present on `macos-latest` +//! CI runners. Launch-mode profiling does not need `task_for_pid` entitlements +//! (the child volunteers its task port), so no `samply setup` is required. +//! +//! Run with: +//! cargo test -p samply --test dsymutil_sigkill -- --nocapture +#![cfg(target_os = "macos")] + +use std::path::Path; +use std::process::Command; + +fn cc(args: &[&str]) { + let status = Command::new("cc").args(args).status().expect("failed to run cc"); + assert!(status.success(), "cc {args:?} failed"); +} + +fn xcrun_dsymutil() -> String { + let out = Command::new("xcrun") + .args(["-f", "dsymutil"]) + .output() + .expect("failed to run xcrun"); + assert!(out.status.success(), "xcrun -f dsymutil failed"); + String::from_utf8(out.stdout).unwrap().trim().to_string() +} + +#[test] +fn dsymutil_is_not_killed_under_samply() { + let samply = env!("CARGO_BIN_EXE_samply"); + let dsymutil = xcrun_dsymutil(); + + let tmp = std::env::temp_dir().join(format!("samply_dsym_repro_{}", std::process::id())); + std::fs::create_dir_all(&tmp).unwrap(); + + // A Mach-O with enough DWARF that dsymutil does real work. + let src = tmp.join("big.cpp"); + { + use std::fmt::Write as _; + let mut s = String::from("#include \n"); + for i in 0..1200 { + writeln!(s, "template struct S{i} {{ int v[N%7+1]; int f(int x){{return x*{i}+N;}} }};").unwrap(); + writeln!(s, "int g{i}(int x){{ S{i}<{}> s; return s.f(x)+{i}; }}", i % 9 + 1).unwrap(); + } + s.push_str("int main(){int t=0;"); + for i in 0..1200 { + write!(s, "t+=g{i}(t);").unwrap(); + } + s.push_str("printf(\"%d\\n\",t);return 0;}\n"); + std::fs::write(&src, s).unwrap(); + } + let macho = tmp.join("bigcpp"); + cc(&["-g", "-O0", "-o", macho.to_str().unwrap(), src.to_str().unwrap()]); + + // A locally-built (non-restricted) parent that execs dsymutil and reports + // how the child died via its own exit code: 0 = clean, 1 = killed by signal. + let spawner_src = tmp.join("spawner.c"); + std::fs::write( + &spawner_src, + r#" +#include +#include +#include +#include +int main(int argc, char** argv){ + pid_t pid = fork(); + if(pid==0){ execl(argv[1],"dsymutil","-f",argv[2],"-o",argv[3],(char*)0); _exit(127); } + int st=0; waitpid(pid,&st,0); + if(WIFSIGNALED(st)){ fprintf(stderr,"dsymutil killed by signal %d\n", WTERMSIG(st)); return 1; } + fprintf(stderr,"dsymutil exited code %d\n", WEXITSTATUS(st)); return 0; +} +"#, + ) + .unwrap(); + let spawner = tmp.join("spawner"); + cc(&["-O0", "-o", spawner.to_str().unwrap(), spawner_src.to_str().unwrap()]); + + let out_dwarf = tmp.join("out.dwarf"); + let profile = tmp.join("profile.json.gz"); + + // samply record --save-only -o -- + let status = Command::new(samply) + .args(["record", "--save-only", "-o"]) + .arg(&profile) + .arg("--") + .arg(&spawner) + .arg(&dsymutil) + .arg(&macho) + .arg(&out_dwarf) + .status() + .expect("failed to run samply"); + + // The spawner exits 0 iff dsymutil completed normally. Under the bug it exits + // 1 because dsymutil was SIGKILLed by samply. + let killed = !status.success(); + let produced_output = Path::new(&out_dwarf).exists(); + + // Clean up only after we've inspected the results (out.dwarf lives in `tmp`). + let _ = std::fs::remove_dir_all(&tmp); + + assert!( + !killed, + "dsymutil was killed when run under `samply record` \ + (spawner exit = {:?}). samply must not SIGKILL build subprocesses it \ + injects into.", + status.code() + ); + assert!(produced_output, "dsymutil did not produce its output"); +}