From d54fe5353dea98cc6d9a73c9708fda6d9c2b5aca Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Fri, 11 Jul 2025 14:33:04 -0400 Subject: [PATCH] Remove some unsafe code; fix a soundness hole Replace a number of lines of unsafe code with safe equivalents, some using `zerocopy`. In one instance, fix a soundness hole (#720). Closes #720 --- Cargo.lock | 22 ++++++++++++++++++++ Cargo.toml | 1 + crates/as-if-std/Cargo.toml | 1 + src/print/fuchsia.rs | 17 +++++++-------- src/symbolize/dbghelp.rs | 32 ++++++++++++++++++++++++++--- src/symbolize/gimli/libs_illumos.rs | 3 +-- 6 files changed, 61 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2730a4b1..c41b87fb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -29,6 +29,7 @@ dependencies = [ "rustc-demangle", "ruzstd", "windows-targets", + "zerocopy", ] [[package]] @@ -47,6 +48,7 @@ dependencies = [ "ruzstd", "serde", "windows-targets", + "zerocopy", ] [[package]] @@ -267,3 +269,23 @@ name = "windows_x86_64_msvc" version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" + +[[package]] +name = "zerocopy" +version = "0.8.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1039dd0d3c310cf05de012d8a39ff557cb0d23087fd44cad61df08fc31907a2f" +dependencies = [ + "zerocopy-derive", +] + +[[package]] +name = "zerocopy-derive" +version = "0.8.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ecf5b4cc5364572d7f4c329661bcc82724222973f2cab6f050a4e5c22f75181" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] diff --git a/Cargo.toml b/Cargo.toml index 7e1233c9..18679443 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,7 @@ exclude = [ [dependencies] cfg-if = "1.0" rustc-demangle = "0.1.24" +zerocopy = "0.8.26" # Optionally enable the ability to serialize a `Backtrace`, controlled through # the `serialize-serde` feature below. diff --git a/crates/as-if-std/Cargo.toml b/crates/as-if-std/Cargo.toml index 8d6d6816..ace51a71 100644 --- a/crates/as-if-std/Cargo.toml +++ b/crates/as-if-std/Cargo.toml @@ -15,6 +15,7 @@ bench = false cfg-if = "1.0" rustc-demangle = "0.1.21" libc = { version = "0.2.156", default-features = false } +zerocopy = "0.8.26" [target.'cfg(not(all(windows, target_env = "msvc", not(target_vendor = "uwp"))))'.dependencies] miniz_oxide = { version = "0.8", optional = true, default-features = false } diff --git a/src/print/fuchsia.rs b/src/print/fuchsia.rs index 01821fd9..d11a5aa0 100644 --- a/src/print/fuchsia.rs +++ b/src/print/fuchsia.rs @@ -1,7 +1,7 @@ use core::fmt::{self, Write}; -use core::mem::{size_of, transmute}; use core::slice::from_raw_parts; use libc::c_char; +use zerocopy::FromBytes; unsafe extern "C" { // dl_iterate_phdr takes a callback that will receive a dl_phdr_info pointer @@ -181,15 +181,12 @@ fn take_bytes_align4<'a>(num: usize, bytes: &mut &'a [u8]) -> Option<&'a [u8]> { // architectures correctness). The values in the Elf_Nhdr fields might // be nonsense but this function ensures no such thing. fn take_nhdr<'a>(bytes: &mut &'a [u8]) -> Option<&'a Elf_Nhdr> { - if size_of::() > bytes.len() { - return None; - } - // This is safe as long as there is enough space and we just confirmed that - // in the if statement above so this should not be unsafe. - let out = unsafe { transmute::<*const u8, &'a Elf_Nhdr>(bytes.as_ptr()) }; - // Note that sice_of::() is always 4-byte aligned. - *bytes = &bytes[size_of::()..]; - Some(out) + let (hdr, suffix) = <[u32; 3]>::ref_from_prefix(*bytes).ok()?; + *bytes = suffix; + + let hdr: *const [u32; 3] = hdr; + // SAFETY: `[u32; 3]` and `Elf_Nhdr` have the same layout. + Some(unsafe { &*hdr.cast::() }) } impl<'a> Iterator for NoteIter<'a> { diff --git a/src/symbolize/dbghelp.rs b/src/symbolize/dbghelp.rs index d3b688f8..0beb724a 100644 --- a/src/symbolize/dbghelp.rs +++ b/src/symbolize/dbghelp.rs @@ -222,9 +222,35 @@ unsafe fn do_resolve( get_line_from_addr: impl FnOnce(&mut IMAGEHLP_LINEW64) -> BOOL, cb: &mut dyn FnMut(&super::Symbol), ) { - const SIZE: usize = 2 * MAX_SYM_NAME as usize + mem::size_of::(); - let mut data = Aligned8([0u8; SIZE]); - let info = unsafe { &mut *data.0.as_mut_ptr().cast::() }; + const TRAILER_SIZE: usize = 2 * MAX_SYM_NAME as usize; + + #[repr(C)] + struct Data { + symbol_infow: SYMBOL_INFOW, + __max_sym_name: [u8; TRAILER_SIZE], + } + + let mut data = Aligned8(Data { + symbol_infow: SYMBOL_INFOW { + SizeOfStruct: 0, + TypeIndex: 0, + Reserved: [0, 0], + Index: 0, + Size: 0, + ModBase: 0, + Flags: 0, + Value: 0, + Address: 0, + Register: 0, + Scope: 0, + Tag: 0, + NameLen: 0, + MaxNameLen: 0, + Name: [0], + }, + __max_sym_name: [0u8; TRAILER_SIZE], + }); + let info = &mut data.0.symbol_infow; info.MaxNameLen = MAX_SYM_NAME as u32; // the struct size in C. the value is different to // `size_of::() - MAX_SYM_NAME + 1` (== 81) diff --git a/src/symbolize/gimli/libs_illumos.rs b/src/symbolize/gimli/libs_illumos.rs index 025eb250..0836a1d5 100644 --- a/src/symbolize/gimli/libs_illumos.rs +++ b/src/symbolize/gimli/libs_illumos.rs @@ -4,7 +4,6 @@ use super::{Library, LibrarySegment}; use alloc::borrow::ToOwned; use alloc::vec::Vec; use core::ffi::CStr; -use core::mem; use object::NativeEndian; #[cfg(target_pointer_width = "64")] @@ -38,8 +37,8 @@ pub(super) fn native_libraries() -> Vec { let mut libs = Vec::new(); // Request the current link map from the runtime linker: + let mut map: *const LinkMap = core::ptr::null(); let map = unsafe { - let mut map: *const LinkMap = mem::zeroed(); if dlinfo( RTLD_SELF, RTLD_DI_LINKMAP,