Skip to content

Remove some unsafe code; fix a soundness hole #721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Remove some unsafe code; fix a soundness hole #721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions crates/as-if-std/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If kept, this could be restricted to target_os = "fuchsia".

Copy link
Member

@bjorn3 bjorn3 Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can you please check that zerocopy-derive doesn't end up in the lockfile? If it does end up in there, that will cause a tidy lint checking that the standard library doesn't depend on proc-macros to fail.

Edit: It does end up in the lockfile.


[target.'cfg(not(all(windows, target_env = "msvc", not(target_vendor = "uwp"))))'.dependencies]
miniz_oxide = { version = "0.8", optional = true, default-features = false }
Expand Down
17 changes: 7 additions & 10 deletions src/print/fuchsia.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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::<Elf_Nhdr>() > 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::<Elf_Nhdr>() is always 4-byte aligned.
*bytes = &bytes[size_of::<Elf_Nhdr>()..];
Some(out)
let (hdr, suffix) = <[u32; 3]>::ref_from_prefix(*bytes).ok()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not deeply familiar with the internals of zerocopy and its API surface, so I am afraid adding its API surface does not improve the ease of verifying the code correctness for me. I know you work very hard on it, but it is very general, whereas backtrace-rs only has one task. Well, two, as symbolization and recovering a trace are different, but it tends to collapse them, for better or worse.

Fixing this soundness problem here by expanding the trusted computing base (TCB) of backtrace means now someone has to verify zerocopy. It may improve things if the zerocopy inclusion was done systematically, but here it is only as a spot-fix, and does not address other locations in the code which could instead use zerocopy's APIs. I would like to separate out discussion of using a new dependency from any such fixes for any soundness problems, and certainly to separate soundness problems from any cleanup.

*bytes = suffix;

let hdr: *const [u32; 3] = hdr;
// SAFETY: `[u32; 3]` and `Elf_Nhdr` have the same layout and bit validity.
Some(unsafe { &*hdr.cast::<Elf_Nhdr>() })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fix anything, or only replace one unsafe usage with another?

If it fixes something, how simple would it be to fix that without the dependency?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fix anything

Yeah, it fixes a soundness hole in the original – in particular, that alignment isn't checked. This comment in the original is misleading:

    // Note that sice_of::<Elf_Nhdr>() is always 4-byte aligned.
    *bytes = &bytes[size_of::<Elf_Nhdr>()..];

While it's true that size_of::<Elf_Nhdr>() is a multiple of 4, there's no guarantee about the alignment of bytes itself, and so &bytes[size_of::<Elf_Nhdr>()..] is similarly not guaranteed to be aligned. As far as I can tell (from reading this git blame on this file), that's just an oversight that dates back to when this code was first written.

...or only replace one unsafe usage with another?

The unsafe usage in the new version is required so long as Elf_Nhdr doesn't implement some zerocopy traits. Implementing them would require a proc macro derive, which it sounds like isn't an option.

That said, the new unsafe usage is simpler than the old one - it only relies on [u32; 3] and Elf_Nhdr having the same layout and bit validity, while the preceding ref_from_prefix is doing the heavy lifting of making sure that the &[u8] to &[u32; 3] cast doesn't violate bit validity and performs runtime validation of size and alignment.

If it fixes something, how simple would it be to fix that without the dependency?

It wouldn't be hard in principle. The concern is more that it'd be brittle - it would be easy for a future developer who doesn't understand the subtleties we're discussing here to re-introduce a soundness bug. IMO the biggest clue to this is the "sice_of::<Elf_Nhdr>() is always 4-byte aligned" comment, which is patently incorrect and yet made it into the codebase and past code review. I won't call anyone out here by name, but if you git blame this file, you'll see that it was written by a very experienced Rust programmer (and one whom I personally hold in high regard) – if an experienced programmer could make this mistake, then presumably it's an easy and subtle mistake to make.

Copy link
Contributor

@philipc philipc Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the biggest clue to this is the "sice_of::<Elf_Nhdr>() is always 4-byte aligned" comment, which is patently incorrect and yet made it into the codebase and past code review

I don't think that comment is so badly wrong. It could be clearer, but I take that comment to mean that it preserves the 4-byte alignment required by ELF, which is what is important for that line.

The thing that is wrong is that the function doesn't check the 4-byte alignment to start with, and the comment prior to this function thinks that this requirement is optional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backtrace's maintenance is not idyllic. If it was anyone's first priority, maybe I would be paid to do it, yet I'm not. Even experienced programmers do not fire on all cylinders at all times. Certainly not all maintainers are willing to be insufferable exacting pedants, with borderline-antisocial disregard for whether someone needs the code to be merged for someone's job requirements, instead caring only about the code quality and correctness. As you seem to want the code to be reviewed by that kind of demon, here I am! But I have not retrospectively audited every last line of every last platform, to be sure. Improvements to their condition are certainly welcome.

Based on the conversation so far, it seems the soundness requirements have not been properly documented. Certainly, I am not yet convinced the current version of the code as present in this PR documents them. I would like to have the requirements properly documented first, so for all comments to be updated appropriately in this file, at the very least.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backtrace crate already indirectly depends on object, right? Maybe you could use the object::elf::NoteHeader32/64 type instead and then use object::pod::from_bytes() instead of the transmute?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems easy enough, see #725. object is already a direct dependency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool, TIL about object::pod.

}

impl<'a> Iterator for NoteIter<'a> {
Expand Down
32 changes: 29 additions & 3 deletions src/symbolize/dbghelp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::<SYMBOL_INFOW>();
let mut data = Aligned8([0u8; SIZE]);
let info = unsafe { &mut *data.0.as_mut_ptr().cast::<SYMBOL_INFOW>() };
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::<SYMBOL_INFOW>() - MAX_SYM_NAME + 1` (== 81)
Expand Down
3 changes: 1 addition & 2 deletions src/symbolize/gimli/libs_illumos.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -38,8 +37,8 @@ pub(super) fn native_libraries() -> Vec<Library> {
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();
Comment on lines +40 to -42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do find this an improvement, this is pure style. All changes like this, that only improve the readability, or otherwise "simply" replace unsafe-but-sound code with safe code, should not be in the same commit, or possibly even same PR, that says "fixes a soundness hole". I want the code in the commit that says "fixes a soundness hole" to be essential for fixing a soundness hole.

Consider it a parallel to my other "TCB"-related concerns, albeit distributed throughout history. Ignorable changes should be ignorable, and non-ignorable ones should be non-ignorable, throughout.

if dlinfo(
RTLD_SELF,
RTLD_DI_LINKMAP,
Expand Down
Loading