Derive macros are unsound due to types being mentioned twice

[theemathas]

This is a different unsoundness than #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 with all files necessary to reproduce, for your convenience.)

dep/src/lib.rs:

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:

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

#![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<Self, ___ZerocopyAliasing>)
        -> ::zerocopy::util::macro_util::core_reexport::primitive::bool where     
        ___ZerocopyAliasing: ::zerocopy::pointer::invariant::Reference {
        if false {
            fn assert_is_from_bytes<T>() where T: ::zerocopy::FromBytes,
                T: ?::zerocopy::util::macro_util::core_reexport::marker::Sized {} 
            assert_is_from_bytes::<Self>();
        }
        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 exists in bytemuck.

For the issue about this as a systematic problem, see rust-lang/rust#148793.

See also rust-lang/rust#147103, for a similar issue in std, although that case didn't cause unsoundness.

[joshlf]

Wow, fantastic catch! What prompted you to think of this edge case?

Also, do you think it'd be sufficient for us to ban macro invocations in the type position? Or are there other non-macro ways of triggering the same behavior (or ways which don't look syntactically like macros but which secretly invoke macros)?

[theemathas]

You can also have attribute/derive macros on items defined inside expressions that are inside either const generics, array lengths, or enum discriminants.

There's also the potential issue of duplicating #[unsafe(no_mangle)] functions.

I came up with this edge case after Jules found the #[unsafe(no_mangle)] edge case in rust-lang/rust#147103, when working on how thread_local! parses attributes, presumably enumerating over all attributes looking for edge cases.

[theemathas]

Actually.... I'm not sure how panic::Location::caller and #[track_caller] work...

[joshlf]

Leaving a breadcrumb for myself: rust-lang/rust#148793 (comment) lists other ways that this could affect us. In general, the entirety of rust-lang/rust#148793 should be read before considering any solution.

[joshlf]

@jswrenn and I discussed this, and our tentative plan is:

• Ban all field types with const fn or macro invocations
• Permit users to opt-in on a per-type or per-field basis using an attribute like #[allow(zerocopy::const_fn_in_types)]

We'll need to figure out whether we want to release this as a breaking change in 0.8 or wait until 0.9.

[theemathas]

Just field types is not sufficient. Stuff also has to be prohibited in enum discriminants too.

(Macros can also appear inside where clauses, but I think that this can't cause issues for zerocopy. I'm not sure though.)