From 08da9ae13f18037512ae86f3a66e5983a82648dd Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Mon, 4 May 2026 12:09:12 -0400 Subject: [PATCH 1/2] Isolated Processes 1: Refactor thread_info for ease of redirecting Currently, thread_info is a bit of a mess and is difficult to cleanly redirect into an interface. This cleans it up and makes it easier to move around. It also fixes a piece of unsound code (mod.rs line 165) where we are passing an immutable pointer to an API that says it will modify the result. --- src/linux/thread_info/aarch64.rs | 41 ++----- src/linux/thread_info/arm.rs | 25 ++-- src/linux/thread_info/mod.rs | 198 +++++++++++-------------------- src/linux/thread_info/x86.rs | 137 ++++++++++----------- 4 files changed, 149 insertions(+), 252 deletions(-) diff --git a/src/linux/thread_info/aarch64.rs b/src/linux/thread_info/aarch64.rs index 681cc56d..152ba6d9 100644 --- a/src/linux/thread_info/aarch64.rs +++ b/src/linux/thread_info/aarch64.rs @@ -1,7 +1,6 @@ use { - super::{CommonThreadInfo, NT_Elf, Pid, ThreadInfoError}, + super::{Pid, PtraceRequestType, ThreadInfoError}, crate::minidump_cpu::{FP_REG_COUNT, GP_REG_COUNT, RawContextCPU}, - nix::sys::ptrace, }; /// https://github.com/rust-lang/libc/pull/2719 @@ -25,47 +24,29 @@ pub struct ThreadInfoAarch64 { pub fpregs: user_fpsimd_struct, } -impl CommonThreadInfo for ThreadInfoAarch64 {} - impl ThreadInfoAarch64 { pub fn get_instruction_pointer(&self) -> usize { self.regs.pc as usize } - // nix currently doesn't support PTRACE_GETREGSET, so we have to do it ourselves fn getregset(pid: Pid) -> Result { - Self::ptrace_get_data_via_io( - 0x4204 as ptrace::RequestType, // PTRACE_GETREGSET - Some(NT_Elf::NT_PRSTATUS), - nix::unistd::Pid::from_raw(pid), - ) + const NT_PRSTATUS: usize = 1; + super::ptrace_getregset(NT_PRSTATUS, pid) } fn getregs(pid: Pid) -> Result { - // TODO: nix restricts PTRACE_GETREGS to arm android for some reason - Self::ptrace_get_data( - 12 as ptrace::RequestType, // PTRACE_GETREGS - None, - nix::unistd::Pid::from_raw(pid), - ) + const PTRACE_GETREGS: PtraceRequestType = 12; + unsafe { super::ptrace_getregs::(PTRACE_GETREGS, pid) } } - // nix currently doesn't support PTRACE_GETREGSET, so we have to do it ourselves fn getfpregset(pid: Pid) -> Result { - Self::ptrace_get_data_via_io( - 0x4204 as ptrace::RequestType, // PTRACE_GETREGSET - Some(NT_Elf::NT_PRFPREGSET), - nix::unistd::Pid::from_raw(pid), - ) + const NT_PRFPREGSET: usize = 2; + super::ptrace_getregset(NT_PRFPREGSET, pid) } - // nix currently doesn't support PTRACE_GETFPREGS, so we have to do it ourselves fn getfpregs(pid: Pid) -> Result { - Self::ptrace_get_data( - 14 as ptrace::RequestType, // PTRACE_GETFPREGS - None, - nix::unistd::Pid::from_raw(pid), - ) + const PTRACE_GETFPREGS: PtraceRequestType = 14; + unsafe { super::ptrace_getregs::(PTRACE_GETFPREGS, pid) } } pub fn fill_cpu_context(&self, out: &mut RawContextCPU) { @@ -85,8 +66,8 @@ impl ThreadInfoAarch64 { out.float_regs[..FP_REG_COUNT].copy_from_slice(&self.fpregs.vregs[..FP_REG_COUNT]); } - pub fn create_impl(_pid: Pid, tid: Pid) -> Result { - let (ppid, tgid) = Self::get_ppid_and_tgid(tid)?; + pub fn create(_pid: Pid, tid: Pid) -> Result { + let (ppid, tgid) = super::get_ppid_and_tgid(tid)?; let regs = Self::getregset(tid).or_else(|_| Self::getregs(tid))?; let fpregs = Self::getfpregset(tid).or_else(|_| Self::getfpregs(tid))?; diff --git a/src/linux/thread_info/arm.rs b/src/linux/thread_info/arm.rs index b5f12636..03637840 100644 --- a/src/linux/thread_info/arm.rs +++ b/src/linux/thread_info/arm.rs @@ -1,7 +1,6 @@ use { - super::{CommonThreadInfo, NT_Elf, Pid, ThreadInfoError}, + super::{Pid, PtraceRequestType, ThreadInfoError}, crate::minidump_cpu::RawContextCPU, - nix::sys::ptrace, }; type Result = std::result::Result; @@ -30,25 +29,15 @@ pub struct ThreadInfoArm { pub fpregs: user_fpregs_struct, } -impl CommonThreadInfo for ThreadInfoArm {} - impl ThreadInfoArm { - // nix currently doesn't support PTRACE_GETFPREGS, so we have to do it ourselves fn getfpregs(pid: Pid) -> Result { - Self::ptrace_get_data_via_io( - 0x4204 as ptrace::RequestType, // PTRACE_GETREGSET - Some(NT_Elf::NT_ARM_VFP), - nix::unistd::Pid::from_raw(pid), - ) + const NT_ARM_VFP: usize = 0x400; + super::ptrace_getregset(NT_ARM_VFP, pid) } - // nix currently doesn't support PTRACE_GETREGS, so we have to do it ourselves fn getregs(pid: Pid) -> Result { - Self::ptrace_get_data::( - ptrace::Request::PTRACE_GETREGS as ptrace::RequestType, - None, - nix::unistd::Pid::from_raw(pid), - ) + const PTRACE_GETREGS: PtraceRequestType = 12; + unsafe { super::ptrace_getregs::(PTRACE_GETREGS, pid) } } pub fn get_instruction_pointer(&self) -> usize { @@ -65,8 +54,8 @@ impl ThreadInfoArm { out.float_save.regs = self.fpregs.fpregs; } - pub fn create_impl(_pid: Pid, tid: Pid) -> Result { - let (ppid, tgid) = Self::get_ppid_and_tgid(tid)?; + pub fn create(_pid: Pid, tid: Pid) -> Result { + let (ppid, tgid) = super::get_ppid_and_tgid(tid)?; let regs = Self::getregs(tid)?; let fpregs = Self::getfpregs(tid).unwrap_or(Default::default()); diff --git a/src/linux/thread_info/mod.rs b/src/linux/thread_info/mod.rs index 06c281db..b623cfa0 100644 --- a/src/linux/thread_info/mod.rs +++ b/src/linux/thread_info/mod.rs @@ -1,13 +1,17 @@ use { super::{Pid, serializers::*}, crate::serializers::*, - nix::{errno::Errno, sys::ptrace}, + nix::errno::Errno, std::{ + ffi::c_void, io::{self, BufRead}, path, }, }; +#[cfg(target_arch = "x86_64")] +pub use x86::copy_u32_registers; + type Result = std::result::Result; #[derive(thiserror::Error, Debug, serde::Serialize)] @@ -30,7 +34,7 @@ pub enum ThreadInfoError { ), #[error("nix::ptrace() error")] PtraceError( - #[from] + #[source] #[serde(serialize_with = "serialize_nix_error")] nix::Error, ), @@ -51,141 +55,79 @@ cfg_if::cfg_if! { } } -#[derive(Debug)] -#[allow(non_camel_case_types, dead_code)] -enum NT_Elf { - NT_NONE = 0, - NT_PRSTATUS = 1, - NT_PRFPREGSET = 2, - //NT_PRPSINFO = 3, - //NT_TASKSTRUCT = 4, - //NT_AUXV = 6, - NT_ARM_VFP = 0x400, // ARM VFP/NEON registers -} - -#[cfg(target_arch = "x86_64")] -#[inline] -pub fn copy_u32_registers(dst: &mut [u128], src: &[u32]) { - // SAFETY: We are copying a block of memory from ptrace as u32s to the u128 - // format of minidump-common - unsafe { - let dst: &mut [u8] = - std::slice::from_raw_parts_mut(dst.as_mut_ptr().cast(), dst.len() * 16); - let src: &[u8] = std::slice::from_raw_parts(src.as_ptr().cast(), src.len() * 4); +#[cfg(target_env = "gnu")] +type PtraceRequestType = core::ffi::c_uint; - let to_copy = std::cmp::min(dst.len(), src.len()); - dst[..to_copy].copy_from_slice(&src[..to_copy]); - } -} +#[cfg(not(target_env = "gnu"))] +type PtraceRequestType = core::ffi::c_int; -trait CommonThreadInfo { - fn get_ppid_and_tgid(tid: Pid) -> Result<(Pid, Pid)> { - let mut ppid = -1; - let mut tgid = -1; +fn get_ppid_and_tgid(tid: Pid) -> Result<(Pid, Pid)> { + let mut ppid = -1; + let mut tgid = -1; - let status_path = path::PathBuf::from(format!("/proc/{tid}/status")); - let status_file = std::fs::File::open(status_path)?; - for line in io::BufReader::new(status_file).lines() { - let l = line?; - let start = l - .get(0..6) - .ok_or_else(|| ThreadInfoError::InvalidProcStatusFile(tid, l.clone()))?; - match start { - "Tgid:\t" => { - tgid = l - .get(6..) - .ok_or_else(|| ThreadInfoError::InvalidProcStatusFile(tid, l.clone()))? - .parse::()?; - } - "PPid:\t" => { - ppid = l - .get(6..) - .ok_or_else(|| ThreadInfoError::InvalidProcStatusFile(tid, l.clone()))? - .parse::()?; - } - _ => continue, + let status_path = path::PathBuf::from(format!("/proc/{tid}/status")); + let status_file = std::fs::File::open(status_path)?; + for line in io::BufReader::new(status_file).lines() { + let l = line?; + let start = l + .get(0..6) + .ok_or_else(|| ThreadInfoError::InvalidProcStatusFile(tid, l.clone()))?; + match start { + "Tgid:\t" => { + tgid = l + .get(6..) + .ok_or_else(|| ThreadInfoError::InvalidProcStatusFile(tid, l.clone()))? + .parse::()?; } + "PPid:\t" => { + ppid = l + .get(6..) + .ok_or_else(|| ThreadInfoError::InvalidProcStatusFile(tid, l.clone()))? + .parse::()?; + } + _ => continue, } - if ppid == -1 || tgid == -1 { - return Err(ThreadInfoError::InvalidPid( - format!("/proc/{tid}/status"), - ppid, - tgid, - )); - } - Ok((ppid, tgid)) } - - /// SLIGHTLY MODIFIED COPY FROM CRATE nix - /// Function for ptrace requests that return values from the data field. - /// Some ptrace get requests populate structs or larger elements than `c_long` - /// and therefore use the data field to return values. This function handles these - /// requests. - fn ptrace_get_data( - request: ptrace::RequestType, - flag: Option, - pid: nix::unistd::Pid, - ) -> Result { - let mut data = std::mem::MaybeUninit::uninit(); - let res = unsafe { - libc::ptrace( - request, - libc::pid_t::from(pid), - flag.unwrap_or(NT_Elf::NT_NONE), - data.as_mut_ptr(), - ) - }; - Errno::result(res)?; - Ok(unsafe { data.assume_init() }) + if ppid == -1 || tgid == -1 { + return Err(ThreadInfoError::InvalidPid( + format!("/proc/{tid}/status"), + ppid, + tgid, + )); } + Ok((ppid, tgid)) +} - /// SLIGHTLY MODIFIED COPY FROM CRATE nix - /// Function for ptrace requests that return values from the data field. - /// Some ptrace get requests populate structs or larger elements than `c_long` - /// and therefore use the data field to return values. This function handles these - /// requests. - fn ptrace_get_data_via_io( - request: ptrace::RequestType, - flag: Option, - pid: nix::unistd::Pid, - ) -> Result { - let mut data = std::mem::MaybeUninit::::uninit(); - let io = libc::iovec { - iov_base: data.as_mut_ptr().cast(), - iov_len: std::mem::size_of::(), - }; - let res = unsafe { - libc::ptrace( - request, - libc::pid_t::from(pid), - flag.unwrap_or(NT_Elf::NT_NONE), - &io as *const _, - ) - }; - Errno::result(res)?; - Ok(unsafe { data.assume_init() }) - } +/// Safety: RequestType and T must agree on the size of the returned type +unsafe fn ptrace_getregs(request: PtraceRequestType, pid: libc::pid_t) -> Result { + let mut output = std::mem::MaybeUninit::uninit(); - /// COPY FROM CRATE nix BECAUSE ITS NOT PUBLIC - #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] - fn ptrace_peek( - request: ptrace::RequestType, - pid: nix::unistd::Pid, - addr: ptrace::AddressType, - data: *mut libc::c_void, - ) -> nix::Result { - let ret = unsafe { - Errno::clear(); - libc::ptrace(request, libc::pid_t::from(pid), addr, data) - }; - match Errno::result(ret) { - Ok(..) | Err(Errno::UnknownErrno) => Ok(ret), - err @ Err(..) => err, - } - } + // Since ptrace() is vararg, best to explicitly state arg types + let addr: *mut c_void = std::ptr::null_mut(); + let data: *mut c_void = (&raw mut output).cast(); + let res = unsafe { libc::ptrace(request, pid, addr, data) }; + Errno::result(res).map_err(ThreadInfoError::PtraceError)?; + Ok(unsafe { output.assume_init() }) } -impl ThreadInfo { - pub fn create(pid: Pid, tid: Pid) -> std::result::Result { - Self::create_impl(pid, tid) + +fn ptrace_getregset(regset_type: usize, pid: libc::pid_t) -> Result { + let mut output = std::mem::MaybeUninit::::uninit(); + let mut io = libc::iovec { + iov_base: output.as_mut_ptr().cast(), + iov_len: std::mem::size_of::(), + }; + + // Since ptrace() is vararg, best to explicitly state arg types + let addr: *mut c_void = regset_type as *mut c_void; + let data: *mut c_void = (&raw mut io).cast(); + let res = unsafe { libc::ptrace(libc::PTRACE_GETREGSET, pid, addr, data) }; + Errno::result(res).map_err(ThreadInfoError::PtraceError)?; + + // PTRACE_GETREGSET returns the number of bytes actually read in iov_len. Need to ensure + // all bytes of T are actually initialized + if io.iov_len != std::mem::size_of::() { + return Err(ThreadInfoError::PtraceError(Errno::EINVAL)); } + + Ok(unsafe { output.assume_init() }) } diff --git a/src/linux/thread_info/x86.rs b/src/linux/thread_info/x86.rs index 6e9d8b17..f76cadbf 100644 --- a/src/linux/thread_info/x86.rs +++ b/src/linux/thread_info/x86.rs @@ -1,8 +1,8 @@ use { - super::{CommonThreadInfo, NT_Elf, Pid, ThreadInfoError}, + super::{Pid, PtraceRequestType, ThreadInfoError}, crate::{minidump_cpu::RawContextCPU, minidump_format::format}, - core::mem::size_of_val, - nix::sys::ptrace, + core::mem, + nix::errno::Errno, scroll::Pwrite, }; @@ -13,6 +13,12 @@ use libc::{user, user_fpregs_struct, user_regs_struct}; type Result = std::result::Result; +#[cfg(target_arch = "x86")] +type RegType = u32; + +#[cfg(target_arch = "x86_64")] +type RegType = u64; + // Not defined by libc on Android #[cfg(all(target_os = "android", target_arch = "x86"))] #[allow(non_camel_case_types)] @@ -101,107 +107,59 @@ pub struct ThreadInfoX86 { pub ppid: Pid, // parent process pub regs: user_regs_struct, pub fpregs: user_fpregs_struct, - #[cfg(target_arch = "x86_64")] - pub dregs: [libc::c_ulonglong; NUM_DEBUG_REGISTERS], - #[cfg(target_arch = "x86")] - pub dregs: [libc::c_int; NUM_DEBUG_REGISTERS], + pub dregs: [RegType; NUM_DEBUG_REGISTERS], #[cfg(target_arch = "x86")] pub fpxregs: user_fpxregs_struct, } -impl CommonThreadInfo for ThreadInfoX86 {} - impl ThreadInfoX86 { - // nix currently doesn't support PTRACE_GETREGSET, so we have to do it ourselves fn getregset(pid: Pid) -> Result { - Self::ptrace_get_data_via_io( - 0x4204 as ptrace::RequestType, // PTRACE_GETREGSET - Some(NT_Elf::NT_PRSTATUS), - nix::unistd::Pid::from_raw(pid), - ) + const NT_PRSTATUS: usize = 1; + super::ptrace_getregset(NT_PRSTATUS, pid) } pub fn getregs(pid: Pid) -> Result { - // TODO: nix restricts PTRACE_GETREGS to arm android for some reason - Self::ptrace_get_data( - 12 as ptrace::RequestType, // PTRACE_GETREGS - None, - nix::unistd::Pid::from_raw(pid), - ) + const PTRACE_GETREGS: PtraceRequestType = 12; + unsafe { super::ptrace_getregs::(PTRACE_GETREGS, pid) } } - // nix currently doesn't support PTRACE_GETREGSET, so we have to do it ourselves fn getfpregset(pid: Pid) -> Result { - Self::ptrace_get_data_via_io( - 0x4204 as ptrace::RequestType, // PTRACE_GETREGSET - Some(NT_Elf::NT_PRFPREGSET), - nix::unistd::Pid::from_raw(pid), - ) + const NT_PRFPREGSET: usize = 2; + super::ptrace_getregset(NT_PRFPREGSET, pid) } - // nix currently doesn't support PTRACE_GETFPREGS, so we have to do it ourselves fn getfpregs(pid: Pid) -> Result { - Self::ptrace_get_data( - 14 as ptrace::RequestType, // PTRACE_GETFPREGS - None, - nix::unistd::Pid::from_raw(pid), - ) + const PTRACE_GETFPREGS: PtraceRequestType = 14; + unsafe { super::ptrace_getregs::(PTRACE_GETFPREGS, pid) } } - // nix currently doesn't support PTRACE_GETFPXREGS, so we have to do it ourselves #[cfg(target_arch = "x86")] fn getfpxregs(pid: Pid) -> Result { - Self::ptrace_get_data( - 18 as ptrace::RequestType, // PTRACE_GETFPXREGS - None, - nix::unistd::Pid::from_raw(pid), - ) - } - - fn peek_user(pid: Pid, addr: ptrace::AddressType) -> nix::Result { - Self::ptrace_peek( - ptrace::Request::PTRACE_PEEKUSER as ptrace::RequestType, - nix::unistd::Pid::from_raw(pid), - addr, - std::ptr::null_mut(), - ) + const PTRACE_GETFPXREGS: PtraceRequestType = 18; + unsafe { super::ptrace_getregs::(PTRACE_GETFPXREGS, pid) } } - pub fn create_impl(_pid: Pid, tid: Pid) -> Result { - let (ppid, tgid) = Self::get_ppid_and_tgid(tid)?; + pub fn create(_pid: Pid, tid: Pid) -> Result { + let (ppid, tgid) = super::get_ppid_and_tgid(tid)?; let regs = Self::getregset(tid).or_else(|_| Self::getregs(tid))?; let fpregs = Self::getfpregset(tid).or_else(|_| Self::getfpregs(tid))?; + #[cfg(target_arch = "x86")] - let fpxregs: user_fpxregs_struct; - #[cfg(target_arch = "x86")] - { + let fpxregs = { if cfg!(target_feature = "fxsr") { - fpxregs = Self::getfpxregs(tid)?; + Self::getfpxregs(tid)? } else { - fpxregs = unsafe { std::mem::zeroed() }; + unsafe { mem::zeroed() } } - } + }; - #[cfg(target_arch = "x86_64")] - let mut dregs: [libc::c_ulonglong; NUM_DEBUG_REGISTERS] = [0; NUM_DEBUG_REGISTERS]; - #[cfg(target_arch = "x86")] - let mut dregs: [libc::c_int; NUM_DEBUG_REGISTERS] = [0; NUM_DEBUG_REGISTERS]; + let mut dregs: [RegType; NUM_DEBUG_REGISTERS] = [0; NUM_DEBUG_REGISTERS]; - let debug_offset = std::mem::offset_of!(user, u_debugreg); - let elem_offset = size_of_val(&dregs[0]); + let debug_offset = mem::offset_of!(user, u_debugreg); for (idx, dreg) in dregs.iter_mut().enumerate() { - let chunk = Self::peek_user( - tid, - (debug_offset + idx * elem_offset) as ptrace::AddressType, - )?; - #[cfg(target_arch = "x86_64")] - { - *dreg = chunk as u64; // libc / ptrace is very messy wrt int types used... - } - #[cfg(target_arch = "x86")] - { - *dreg = chunk as i32; // libc / ptrace is very messy wrt int types used... - } + let chunk = ptrace_peekuser(tid, debug_offset + idx * mem::size_of::())?; + + *dreg = RegType::from_ne_bytes(chunk[0..mem::size_of::()].try_into().unwrap()); } #[cfg(target_arch = "x86_64")] @@ -294,8 +252,8 @@ impl ThreadInfoX86 { ..Default::default() }; - super::copy_u32_registers(&mut float_save.float_registers, &fs.st_space); - super::copy_u32_registers(&mut float_save.xmm_registers, &fs.xmm_space); + copy_u32_registers(&mut float_save.float_registers, &fs.st_space); + copy_u32_registers(&mut float_save.xmm_registers, &fs.xmm_space); out.float_save .pwrite_with(float_save, 0, scroll::Endian::Little) @@ -394,3 +352,30 @@ impl ThreadInfoX86 { } } } + +fn ptrace_peekuser(pid: libc::pid_t, addr: usize) -> Result<[u8; mem::size_of::()]> { + // Since ptrace() is vararg, best to explicitly state arg types + let addr: *mut libc::c_void = addr as *mut libc::c_void; + let data: *mut libc::c_void = core::ptr::null_mut(); + Errno::set_raw(0); + let rv = unsafe { libc::ptrace(libc::PTRACE_PEEKUSER, pid, addr, data) }; + if rv == -1 && Errno::last_raw() != 0 { + Err(ThreadInfoError::PtraceError(Errno::last())) + } else { + Ok(rv.to_ne_bytes()) + } +} + +#[cfg(target_arch = "x86_64")] +pub fn copy_u32_registers(dst: &mut [u128], src: &[u32]) { + // SAFETY: We are copying a block of memory from ptrace as u32s to the u128 + // format of minidump-common + unsafe { + let dst: &mut [u8] = + std::slice::from_raw_parts_mut(dst.as_mut_ptr().cast(), dst.len() * 16); + let src: &[u8] = std::slice::from_raw_parts(src.as_ptr().cast(), src.len() * 4); + + let to_copy = std::cmp::min(dst.len(), src.len()); + dst[..to_copy].copy_from_slice(&src[..to_copy]); + } +} From d19f007bfd852adbe5bc8327c6510f681f418d20 Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Fri, 8 May 2026 11:32:06 -0400 Subject: [PATCH 2/2] Address review feedback from Gabriele --- src/linux/thread_info/x86.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/linux/thread_info/x86.rs b/src/linux/thread_info/x86.rs index f76cadbf..155ca430 100644 --- a/src/linux/thread_info/x86.rs +++ b/src/linux/thread_info/x86.rs @@ -368,14 +368,13 @@ fn ptrace_peekuser(pid: libc::pid_t, addr: usize) -> Result<[u8; mem::size_of::< #[cfg(target_arch = "x86_64")] pub fn copy_u32_registers(dst: &mut [u128], src: &[u32]) { - // SAFETY: We are copying a block of memory from ptrace as u32s to the u128 - // format of minidump-common + assert_eq!(mem::size_of_val(src), mem::size_of_val(dst)); + // SAFETY: All bit patterns are valid for both types unsafe { - let dst: &mut [u8] = - std::slice::from_raw_parts_mut(dst.as_mut_ptr().cast(), dst.len() * 16); - let src: &[u8] = std::slice::from_raw_parts(src.as_ptr().cast(), src.len() * 4); - - let to_copy = std::cmp::min(dst.len(), src.len()); - dst[..to_copy].copy_from_slice(&src[..to_copy]); + std::ptr::copy_nonoverlapping( + src.as_ptr().cast::(), + dst.as_mut_ptr().cast::(), + mem::size_of_val(src), + ); } }