google / google/zerocopy

Derive macros are unsound due to types being mentioned twice

Open
#2,762 6 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
2.6k
Forks
179
Avg merge
1d 19h
Merged PRs (30d)
29

Description

This is a different unsoundness than https://github.com/google/zerocopy/issues/388.

The derive macros expand to code that mentions the field types, in order to validate that they implement the expected traits. This doesn't work if mentioning the same type name twice produces a different type. This can happen if the type is defined as a macro invocation that's nondeterministic.

The code below causes a segmentation fault. (Also [in a zip file](https://github.com/user-attachments/files/23456453/repro.zip) with all files necessary to reproduce, for your convenience.)

dep/src/lib.rs:
```rust
use proc_macro::TokenStream;
use std::sync::atomic::{AtomicI32, Ordering::SeqCst};

static COUNTER: AtomicI32 = AtomicI32::new(0);

#[proc_macro]
pub fn make_type(_: TokenStream) -> TokenStream {
let index = COUNTER.fetch_add(1, SeqCst);
match index {
0..=2 => "i32",
3 => "&'static i32",
_ => panic!(),
}
.parse()
.unwrap()
}
```

src/main.rs:
```rust
use dep::make_type;
use zerocopy::FromBytes;

#[derive(FromBytes)]
struct Thing {
field: make_type!(),
}

fn main() {
let thing: Thing = Thing::read_from_bytes(&1usize.to_ne_bytes()).unwrap();
let field: &'static i32 = thing.field;
println!("{field}");
}
```

Macro expansion

```rust
#![feature(prelude_import)]
#[macro_use]
extern crate std;
#[prelude_import]
use std::prelude::rust_2024::*;
use dep::make_type;
use zerocopy::FromBytes;

struct Thing {
field: &'static i32,
}
#[allow(deprecated)]
#[automatically_derived]
unsafe impl ::zerocopy::TryFromBytes for Thing<> where
i32: ::zerocopy::TryFromBytes {
fn only_derive_is_allowed_to_implement_this_trait() {}
fn is_bit_valid<___ZerocopyAliasing>(_candidate:
::zerocopy::Maybe)
-> ::zerocopy::util::macro_util::core_reexport::primitive::bool where
___ZerocopyAliasing: ::zerocopy::pointer::invariant::Reference {
if false {
fn assert_is_from_bytes() where T: ::zerocopy::FromBytes,
T: ?::zerocopy::util::macro_util::core_reexport::marker::Sized {}
assert_is_from_bytes::();
}
true
}
}
#[allow(deprecated)]
#[automatically_derived]
unsafe impl ::zerocopy::FromZeros for Thing<> where i32: ::zerocopy::FromZeros
{
fn only_derive_is_allowed_to_implement_this_trait() {}
}
#[allow(deprecated)]
#[automatically_derived]
unsafe impl ::zerocopy::FromBytes for Thing<> where i32: ::zerocopy::FromBytes
{
fn only_derive_is_allowed_to_implement_this_trait() {}
}

fn main() {
let thing: Thing = Thing::read_from_bytes(&1usize.to_ne_bytes()).unwrap();
let field: &'static i32 = thing.field;
{ ::std::io::_print(format_args!("{0}\n", field)); };
}
```

The `make_type!()` macro expands to `i32` three times, and `&'static i32` once. Turns out that the order of macro expansion ends up causing the `field` field to be defined as a `&'static i32`, but zerocopy instead checks whether `i32` implements `FromBytes`. The zerocopy derive macro then implements the `FromBytes` on the `Thing` type, which can then be exploited to cause UB.

The [same issue](https://github.com/Lokathor/bytemuck/issues/335) exists in bytemuck.

For the issue about this as a systematic problem, see https://github.com/rust-lang/rust/issues/148793.

See also https://github.com/rust-lang/rust/issues/147103, for a similar issue in std, although that case didn't cause unsoundness.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.