From 6ab8f3d13d130c2b10002b5c534b08d10c7779f8 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Sat, 27 Jul 2024 14:33:41 -0700 Subject: [PATCH 1/5] tell miri to stop fake-inlining test fn use a few less sus casts for skip_inner_frames and use inline(never) to appease miri. miri now tries to pretend this frame may differ. --- tests/skip_inner_frames.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/skip_inner_frames.rs b/tests/skip_inner_frames.rs index 4a370fbc1..bce8d7fa5 100644 --- a/tests/skip_inner_frames.rs +++ b/tests/skip_inner_frames.rs @@ -1,4 +1,5 @@ use backtrace::Backtrace; +use core::ffi::c_void; // This test only works on platforms which have a working `symbol_address` // function for frames which reports the starting address of a symbol. As a @@ -12,6 +13,7 @@ const ENABLED: bool = cfg!(all( )); #[test] +#[inline(never)] fn backtrace_new_unresolved_should_start_with_call_site_trace() { if !ENABLED { return; @@ -22,13 +24,14 @@ fn backtrace_new_unresolved_should_start_with_call_site_trace() { assert!(!b.frames().is_empty()); - let this_ip = backtrace_new_unresolved_should_start_with_call_site_trace as usize; - println!("this_ip: {:?}", this_ip as *const usize); - let frame_ip = b.frames().first().unwrap().symbol_address() as usize; + let this_ip = backtrace_new_unresolved_should_start_with_call_site_trace as *mut c_void; + println!("this_ip: {:p}", this_ip); + let frame_ip = b.frames().first().unwrap().symbol_address(); assert_eq!(this_ip, frame_ip); } #[test] +#[inline(never)] fn backtrace_new_should_start_with_call_site_trace() { if !ENABLED { return; @@ -38,7 +41,7 @@ fn backtrace_new_should_start_with_call_site_trace() { assert!(!b.frames().is_empty()); - let this_ip = backtrace_new_should_start_with_call_site_trace as usize; - let frame_ip = b.frames().first().unwrap().symbol_address() as usize; + let this_ip = backtrace_new_should_start_with_call_site_trace as *mut c_void; + let frame_ip = b.frames().first().unwrap().symbol_address(); assert_eq!(this_ip, frame_ip); } From 02d1221e26802c0eda1c2e96288b6634aa6663dc Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Sun, 14 Jul 2024 20:29:45 -0700 Subject: [PATCH 2/5] Format trace in test --- tests/concurrent-panics.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/concurrent-panics.rs b/tests/concurrent-panics.rs index a44a26771..350c247db 100644 --- a/tests/concurrent-panics.rs +++ b/tests/concurrent-panics.rs @@ -45,9 +45,9 @@ fn parent() { fn child() { let done = Arc::new(AtomicBool::new(false)); let done2 = done.clone(); - let a = thread::spawn(move || { - while !done2.load(SeqCst) { - format!("{:?}", backtrace::Backtrace::new()); + let a = thread::spawn(move || loop { + if done2.load(SeqCst) { + break format!("{:?}", backtrace::Backtrace::new()); } }); From db9f52851bb159cd89801f0e0f8bedb51c7cc14b Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Sat, 27 Jul 2024 15:09:34 -0700 Subject: [PATCH 3/5] `addr_of_mut!(STATIC_MUT)` is safe on nightly stable doesn't know yet, so just allow the lint. --- src/dbghelp.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/dbghelp.rs b/src/dbghelp.rs index 9f94f2142..82b81e1dc 100644 --- a/src/dbghelp.rs +++ b/src/dbghelp.rs @@ -114,9 +114,8 @@ macro_rules! dbghelp { })* pub fn dbghelp(&self) -> *mut Dbghelp { - unsafe { - ptr::addr_of_mut!(DBGHELP) - } + #[allow(unused_unsafe)] + unsafe { ptr::addr_of_mut!(DBGHELP) } } } ) From bc6ba8e4d2cd3ca2008a6107848d6183427dae97 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Sat, 27 Jul 2024 15:20:09 -0700 Subject: [PATCH 4/5] fix sus provenance in tests/smoke.rs for miri --- tests/smoke.rs | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/tests/smoke.rs b/tests/smoke.rs index 9c87f530e..cbb93347f 100644 --- a/tests/smoke.rs +++ b/tests/smoke.rs @@ -1,8 +1,9 @@ use backtrace::Frame; +use core::ffi::c_void; use std::ptr; use std::thread; -fn get_actual_fn_pointer(fp: usize) -> usize { +fn get_actual_fn_pointer(fp: *mut c_void) -> *mut c_void { // On AIX, the function name references a function descriptor. // A function descriptor consists of (See https://reviews.llvm.org/D62532) // * The address of the entry point of the function. @@ -15,17 +16,18 @@ fn get_actual_fn_pointer(fp: usize) -> usize { // https://www.ibm.com/docs/en/aix/7.2?topic=program-understanding-programming-toc if cfg!(target_os = "aix") { unsafe { - let actual_fn_entry = *(fp as *const usize); + let actual_fn_entry = *(fp as *const *mut c_void); actual_fn_entry } } else { - fp + fp as *mut c_void } } #[test] // FIXME: shouldn't ignore this test on i686-msvc, unsure why it's failing #[cfg_attr(all(target_arch = "x86", target_env = "msvc"), ignore)] +#[inline(never)] #[rustfmt::skip] // we care about line numbers here fn smoke_test_frames() { frame_1(line!()); @@ -42,10 +44,10 @@ fn smoke_test_frames() { // Various platforms have various bits of weirdness about their // backtraces. To find a good starting spot let's search through the // frames - let target = get_actual_fn_pointer(frame_4 as usize); + let target = get_actual_fn_pointer(frame_4 as *mut c_void); let offset = v .iter() - .map(|frame| frame.symbol_address() as usize) + .map(|frame| frame.symbol_address()) .enumerate() .filter_map(|(i, sym)| { if sym >= target { @@ -61,7 +63,7 @@ fn smoke_test_frames() { assert_frame( frames.next().unwrap(), - get_actual_fn_pointer(frame_4 as usize), + get_actual_fn_pointer(frame_4 as *mut c_void) as usize, "frame_4", "tests/smoke.rs", start_line + 6, @@ -69,7 +71,7 @@ fn smoke_test_frames() { ); assert_frame( frames.next().unwrap(), - get_actual_fn_pointer(frame_3 as usize), + get_actual_fn_pointer(frame_3 as *mut c_void) as usize, "frame_3", "tests/smoke.rs", start_line + 3, @@ -77,7 +79,7 @@ fn smoke_test_frames() { ); assert_frame( frames.next().unwrap(), - get_actual_fn_pointer(frame_2 as usize), + get_actual_fn_pointer(frame_2 as *mut c_void) as usize, "frame_2", "tests/smoke.rs", start_line + 2, @@ -85,7 +87,7 @@ fn smoke_test_frames() { ); assert_frame( frames.next().unwrap(), - get_actual_fn_pointer(frame_1 as usize), + get_actual_fn_pointer(frame_1 as *mut c_void) as usize, "frame_1", "tests/smoke.rs", start_line + 1, @@ -93,7 +95,7 @@ fn smoke_test_frames() { ); assert_frame( frames.next().unwrap(), - get_actual_fn_pointer(smoke_test_frames as usize), + get_actual_fn_pointer(smoke_test_frames as *mut c_void) as usize, "smoke_test_frames", "", 0, @@ -249,11 +251,11 @@ fn sp_smoke_test() { return; #[inline(never)] - fn recursive_stack_references(refs: &mut Vec) { + fn recursive_stack_references(refs: &mut Vec<*mut c_void>) { assert!(refs.len() < 5); - let x = refs.len(); - refs.push(ptr::addr_of!(x) as usize); + let mut x = refs.len(); + refs.push(ptr::addr_of_mut!(x).cast()); if refs.len() < 5 { recursive_stack_references(refs); @@ -271,7 +273,7 @@ fn sp_smoke_test() { // mangled names. fn make_trace_closure<'a>( - refs: &'a mut Vec, + refs: &'a mut Vec<*mut c_void>, ) -> impl FnMut(&backtrace::Frame) -> bool + 'a { let mut child_sp = None; let mut child_ref = None; @@ -289,9 +291,9 @@ fn sp_smoke_test() { }) }); - let sp = frame.sp() as usize; - eprintln!("sp = {:p}", sp as *const u8); - if sp == 0 { + let sp = frame.sp(); + eprintln!("sp = {sp:p}"); + if sp as usize == 0 { // If the SP is null, then we don't have an implementation for // getting the SP on this target. Just keep walking the stack, // but don't make our assertions about the on-stack pointers and @@ -306,8 +308,8 @@ fn sp_smoke_test() { if is_recursive_stack_references { let r = refs.pop().unwrap(); - eprintln!("ref = {:p}", r as *const u8); - if sp != 0 { + eprintln!("ref = {:p}", r); + if sp as usize != 0 { assert!(r > sp); if let Some(child_ref) = child_ref { assert!(sp >= child_ref); From dfa7d9b3617565cda8b733f1dd69bde9cd676b95 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Sun, 14 Jul 2024 18:56:38 -0700 Subject: [PATCH 5/5] Introduce TracePtr for Send and Syncness This allows us to both - retain provenance throughout - describe why these are Send and Sync - progressively reduce errors by reducing the conflation of pointers --- src/capture.rs | 125 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 110 insertions(+), 15 deletions(-) diff --git a/src/capture.rs b/src/capture.rs index f7d4b4fea..f2154995a 100644 --- a/src/capture.rs +++ b/src/capture.rs @@ -2,7 +2,7 @@ use crate::resolve; use crate::PrintFmt; use crate::{resolve_frame, trace, BacktraceFmt, Symbol, SymbolName}; -use std::ffi::c_void; +use core::ffi::c_void; use std::fmt; use std::path::{Path, PathBuf}; use std::prelude::v1::*; @@ -29,6 +29,101 @@ pub struct Backtrace { frames: Vec, } +#[derive(Clone, Copy)] +struct TracePtr(*mut c_void); +/// SAFETY: These pointers are always valid within a process and are not used for mutation. +unsafe impl Send for TracePtr {} +/// SAFETY: These pointers are always valid within a process and are not used for mutation. +unsafe impl Sync for TracePtr {} + +impl TracePtr { + fn into_void(self) -> *mut c_void { + self.0 + } + #[cfg(feature = "serde")] + fn from_addr(addr: usize) -> Self { + TracePtr(addr as *mut c_void) + } +} + +#[cfg(feature = "serde")] +impl<'de> Deserialize<'de> for TracePtr { + #[inline] + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + struct PrimitiveVisitor; + + impl<'de> serde::de::Visitor<'de> for PrimitiveVisitor { + type Value = TracePtr; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("usize") + } + + #[inline] + fn visit_u8(self, v: u8) -> Result + where + E: serde::de::Error, + { + Ok(TracePtr(v as usize as *mut c_void)) + } + + #[inline] + fn visit_u16(self, v: u16) -> Result + where + E: serde::de::Error, + { + Ok(TracePtr(v as usize as *mut c_void)) + } + + #[inline] + fn visit_u32(self, v: u32) -> Result + where + E: serde::de::Error, + { + if usize::BITS >= 32 { + Ok(TracePtr(v as usize as *mut c_void)) + } else { + Err(E::invalid_type( + serde::de::Unexpected::Unsigned(v as _), + &self, + )) + } + } + + #[inline] + fn visit_u64(self, v: u64) -> Result + where + E: serde::de::Error, + { + if usize::BITS >= 64 { + Ok(TracePtr(v as usize as *mut c_void)) + } else { + Err(E::invalid_type( + serde::de::Unexpected::Unsigned(v as _), + &self, + )) + } + } + } + + deserializer.deserialize_u64(PrimitiveVisitor) + } +} + +#[cfg(feature = "serde")] +impl Serialize for TracePtr { + #[inline] + fn serialize(&self, serializer: S) -> Result + where + S: serde::ser::Serializer, + { + serializer.serialize_u64(self.0 as usize as u64) + } +} + fn _assert_send_sync() { fn _assert() {} _assert::(); @@ -54,9 +149,9 @@ enum Frame { Raw(crate::Frame), #[cfg(feature = "serde")] Deserialized { - ip: usize, - symbol_address: usize, - module_base_address: Option, + ip: TracePtr, + symbol_address: TracePtr, + module_base_address: Option, }, } @@ -65,7 +160,7 @@ impl Frame { match *self { Frame::Raw(ref f) => f.ip(), #[cfg(feature = "serde")] - Frame::Deserialized { ip, .. } => ip as *mut c_void, + Frame::Deserialized { ip, .. } => ip.into_void(), } } @@ -73,7 +168,7 @@ impl Frame { match *self { Frame::Raw(ref f) => f.symbol_address(), #[cfg(feature = "serde")] - Frame::Deserialized { symbol_address, .. } => symbol_address as *mut c_void, + Frame::Deserialized { symbol_address, .. } => symbol_address.into_void(), } } @@ -84,7 +179,7 @@ impl Frame { Frame::Deserialized { module_base_address, .. - } => module_base_address.map(|addr| addr as *mut c_void), + } => module_base_address.map(|addr| addr.into_void()), } } @@ -94,7 +189,7 @@ impl Frame { let sym = |symbol: &Symbol| { symbols.push(BacktraceSymbol { name: symbol.name().map(|m| m.as_bytes().to_vec()), - addr: symbol.addr().map(|a| a as usize), + addr: symbol.addr().map(TracePtr), filename: symbol.filename().map(|m| m.to_owned()), lineno: symbol.lineno(), colno: symbol.colno(), @@ -104,7 +199,7 @@ impl Frame { Frame::Raw(ref f) => resolve_frame(f, sym), #[cfg(feature = "serde")] Frame::Deserialized { ip, .. } => { - resolve(ip as *mut c_void, sym); + resolve(ip.into_void(), sym); } } symbols @@ -124,7 +219,7 @@ impl Frame { #[cfg_attr(feature = "serde", derive(Deserialize, Serialize))] pub struct BacktraceSymbol { name: Option>, - addr: Option, + addr: Option, filename: Option, lineno: Option, colno: Option, @@ -347,7 +442,7 @@ impl BacktraceSymbol { /// This function requires the `std` feature of the `backtrace` crate to be /// enabled, and the `std` feature is enabled by default. pub fn addr(&self) -> Option<*mut c_void> { - self.addr.map(|s| s as *mut c_void) + self.addr.map(|s| s.into_void()) } /// Same as `Symbol::filename` @@ -468,7 +563,7 @@ mod serde_impls { SerializedFrame { ip: frame.ip() as usize, symbol_address: frame.symbol_address() as usize, - module_base_address: frame.module_base_address().map(|addr| addr as usize), + module_base_address: frame.module_base_address().map(|sym_a| sym_a as usize), symbols: symbols.clone(), } .serialize(s) @@ -483,9 +578,9 @@ mod serde_impls { let frame: SerializedFrame = SerializedFrame::deserialize(d)?; Ok(BacktraceFrame { frame: Frame::Deserialized { - ip: frame.ip, - symbol_address: frame.symbol_address, - module_base_address: frame.module_base_address, + ip: TracePtr::from_addr(frame.ip), + symbol_address: TracePtr::from_addr(frame.symbol_address), + module_base_address: frame.module_base_address.map(TracePtr::from_addr), }, symbols: frame.symbols, })