Derive macros are unsound

[mahkoh]

I came across google/zerocopy#388 (comment) by chance. The same applies to this crate:

type S = &'static str;

#[derive(Copy, Clone, Debug, AnyBitPattern)]
#[insert_field(x = "S")]
struct X {
}

fn main() {
    let x: &X = bytemuck::cast_ref(&[0usize, 1usize]);
    println!("{}", x.x);
}

Segfaults. Solutions might involve generating a function that asserts (by compiling) that the types seen by the proc macro are the same as the types in the final type and that the final type contains no additional fields.

[Lokathor]

I'm not remembering what Derive proc macros are allowed to emit. Can they create code other code besides the trait impl they're for?

Otherwise, we might have to do the other option mentioned in the zerocopy thread and bail out on all unknown attributes. BUT that would be extremely uncool.

[zachs18]

Yes, derive macros can emit arbitrary items, essentially. (IIRC this is already used in bytemuck_derive, where some derive macros emit some const _: () = { some static assertion stuff }; in addition to the trait impl.)

That said, Is it documented anywhere that it is intentional for derive macros to see the "original" tokens when an item has been modified by an attribute macro? My first instinct would be to call it a Rust bug that the derive macro does not get the correct TokenStream that defines the item it is intended to be applied to.

[Lokathor]

i believe the exact ordering of the attributes matters, unfortunately

[zachs18]

use the_macros::*;

fn main() {
    {
        #[derive(PrintTokens)]
        #[add_fields({y: u32})]
        struct Foo {
            x: u32,
        }
        print_tokens()
    }

    {
        #[add_fields({y: u32})]
        #[derive(PrintTokens)]
        struct Foo {
            x: u32,
        }
        print_tokens()
    }
}

#[add_fields({ y: u32 })] struct Foo { x: u32, }
struct Foo { x : u32, y : u32 }

Okay, the original bug report made it sound like Rust never applied attribute macros before giving the item to derive macros, but yeah it does if it is before the derive (since it applies macros outside-in) and otherwise the attribute macro is still in the input. So erroring out if there are any unknown attributes in the input (the first case here) would be one way to make this sound, perhaps with an error message telling the user to move the attribute macro above the derive. (see reply)

the_macros source

# Cargo.toml
[package]
name = "the-macros"
version = "0.1.0"
edition = "2021"

[lib]
proc-macro = true

[dependencies]
proc-macro2 = "1.0.88"
quote = "1.0.37"
syn = { version = "2.0.82", features = ["full"] }

// lib.rs
use proc_macro::{TokenStream};
use proc_macro2::{Literal, Ident, Group, Punct, Span, Spacing, Delimiter};

#[proc_macro_derive(PrintTokens)]
pub fn derive_print_tokens(item: TokenStream) -> TokenStream {
    let literal = Literal::string(&item.to_string());
    quote::quote!{
        fn print_tokens() {
            println!("{}", #literal);
        }
    }.into()
}

#[proc_macro_attribute]
pub fn add_fields(attr: TokenStream, item: TokenStream) -> TokenStream {
    let syn::Item::Struct(mut item) = syn::parse::<syn::Item>(item).unwrap() else { panic!() };
    let new_fields = syn::parse::<syn::FieldsNamed>(attr).unwrap();
    let syn::Fields::Named(ref mut fields) = item.fields else { panic!() };
    fields.named.extend(new_fields.named);
    quote::quote! { #item }.into()
}

[zachs18]

So erroring out if there are any unknown attributes in the input (the first case here) would be one way to make this sound, perhaps with an error message telling the user to move the attribute macro above the derive.

Actually after some testing, this won't work in general. Even ignoring all the false positives it would have, there are also false negatives: there's no way in general to differentiate between our own helper attributes for other traits (like #[zeroable(bound = "")]) and actual unknown attribute proc-macros.

false negative example

use adversarial_macros::transparent; // inserts a `y: Infallible` field, e.g., or any 1-ZST with a nontrivial safety invariant
use bytemuck::{Zeroable};

#[derive(Clone, Copy, Zeroable, TransparentWrapper)]
#[transparent(u32)] // fine, made inert by `derive(TransparentWrapper)`
#[repr(transparent)]
struct Foo {
  x: u32
}

#[derive(Clone, Copy, Zeroable)]
#[transparent(u32)] // unsound false negative, adds a field after `Zeroable` derive macro runs
#[repr(transparent)]
struct Foo {
  x: u32
}

From inside the Zeroable derive-macro, we cannot know if derive(TransparentWrapper) is also being applied, so we don't know if #[transparent(u32)] is a helper attribute for derive(TransparentWrapper) or a call to the procmacro adversarial_macros::transparent.

Edit: I suppose we could get around this by only allowing the helper attributes for the specific trait being derived, which would require writing the derives separately with the helpers between them, actually no that won't work either, because the helper attributes are "inert", but are not actually stripped from the input for later derives, e.g.

example

use adversarial_macros::transparent;

#[derive(Debug, Clone, Copy)]
#[derive(TransparentWrapper)] // comment this out for a different error
#[transparent(u32)]
#[derive(Zeroable)]
#[repr(transparent)]
struct Foo {
    x: u32,
}

derive(Zeroable) still sees transparent(u32) in the input if it was used as an inert helper attribute in derive(TransparentWrapper) instead of being an "active" procmacro.

[zachs18]

Another wrinkle: For fields, we can check the types using some const _: () = { /* do some stuff */ };checks to make sure nothing changed the field types, but for enum discriminants, I don't know if there is any way to be sure that a procmacro did not change any variant discriminants, e.g.

#[derive(Zeroable)]
#[repr(u32)]
#[adversarial_macro] // changes discriminant so none are 0
enum Foo {
  X(i32) = 0,
}

Is there even any way to detect this at compile time?

Edit: For a fieldless enum you can const _: () = assert!(Foo::X as int_ty == 0);, but for fieldful enums I don't know if there is a way to do it at compile time.

[workingjubilee]

That said, Is it documented anywhere that it is intentional for derive macros to see the "original" tokens when an item has been modified by an attribute macro? My first instinct would be to call it a Rust bug that the derive macro does not get the correct TokenStream that defines the item it is intended to be applied to.

It's sort of the inverse: I don't believe we have fixed the order of macro evaluation for proc macros in a way that is particularly useful or provides any real guarantees to macro authors, in any direction.

[dilr]

Since we can't rely on being able to see the final tokens, can we just let the compiler do the checks for us? Consider the following examples:

#[derive(Zeroable)]
#[random_macro]
#[repr(C)]
struct Example1 {
  a : Type1,
  b: Type2.
}

#[derive(Zeroable)]
#[random_macro]
#[repr(u8)]
enum Example2 {
  Var1 = 0,
  Var2
}

We can't tell what random_macro will do to the Examples, but we can confirm afterwards that the Examples have (most of?) the properties we thought they had:

const _ : () = {
  // Check members of Example1
  let _ = |derived: Example1| {
    fn check_member<T: ::bytemuck::Zeroable>(member: T) {}
    match derived {
      Example1 { a, b } => {
        check_member::<Type1>(a);
        check_member::<Type2>(b);
      }
    }
  };

  // Check variants of Example2
  let _ = |derived: Example2| {
    match derived {
      Var1 => ()
      Var2 => ()
    }
  };

  // Ensure zero variant of Example2 is still zero
  assert_eq!(0, Example2::Var1 as u128);
}

The exhaustive nature of pattern matching means that we can confirm that we know about all members for Example1. Furthermore we know the types of each member and that such types all implement Zeroable. We can confirm that Example2 is an inhabited variant with all variants accounted for in the same manner. Additionally the assert_eq! statement confirms that the zero variant is still zero.

The main issue with this method is that there is no way (to my knowledge) to confirm the repr of a given struct or enum is what we think it is. The random_macro could have changed or removed the repr attributes. I think that this is mostly not a problem:

1. Currently, NoUninit requires repr(C) or repr(transparent). However, the reference gives the guarantee that fields of a Rust representation item will not overlap, and all other representations also have that guarantee. Thus, I think it is possible to check NoUninit on a struct by checking that the sum of the sizes of the fields is the size of the struct. This should ensure no padding is present regardless of the representation the struct has. I believe a similar strategy would work for TransparentWrapper.
2. Zeroable only makes sense on an enum if the enum has a dedicated discriminant field with value 0. This could be a problem, but the Example2::Var1 as u128 statement will not compile unless Example2 is of a representation which supports casting to a numeric value. By choosing the largest supported type, no truncation will occur. Even if the discriminant is allowed to be negative, the two's compliment semantics of numeric casts mean that the casted discriminant will still be some non-zero value.

There is at least one case where the representation does matter though. For non-fieldless enums with primitive representation

#[derive(Zeroable)]
#[random_macro]
#[repr(u8)]
enum Example3a {
  First{ a : Type4 } = 0,
  Second
}

Example3a::First as u128 fails to compile. But it would be possible to check if the discriminant of First was actually 0 using unsafe code if we could confirm that the representation is actually u8.

If we are willing to restrict the user slightly though, we can confirm our representation. First, note that the reference does guarantee that macro attributes evaluate outside in, and that fellow derive macros may not change the item they are deriving. Furthermore, according to my testing, there can only be one repr attribute attached to a struct. (though it seems like this might not have been the case on older compilers) With this in mind we might think we could ask the user to place the repr attribute directly below the derive macro if representation is important:

#[derive(Zeroable)]
#[repr(u8)]
#[random_macro]
enum Example3b {
  First{ a : Type4 } = 0,
  Second
}

However, random_macro could end up expanding its item to:

struct SwappedOut{}

enum Example3b {
  First{ a : Type4 } = 0,
  Second
}

Which (I believe) would cause the repr(u8) to apply to SwappedOut and not Example3b.

Therefore I think the only safe bet is to only allow repr between the derives and the struct definition. This includes rejecting unknown inner attributes for the item definition, but I don't think attributes on the fields would matter:

#[random_macro]
#[derive(Zeroable)]
#[repr(u8)]
enum Example3c {
  First{ a : Type4 } = 0,
  Second
}

In this case the derive macro would be privy to any changes made by random_macro, and so can know Example3c's representation. If we assume random_macro left Example3c and it's representation alone, we might emit the following checking code:

const _ : () = {
  let _ = |derived: Example3c| {
    fn check_member<T: ::bytemuck::Zeroable>(member: T) {}
    match derived {
      First { a } => {
        check_member::<Type4>(a);
      },
      Second => ()
    }
  }
  let test_case = Example3c::First { a : ::bytemuck::zeroed() };
  let discrim = unsafe { *(&raw test_case).cast::<u8>() };
  assert_eq!(discrim, 0);
}

We are able to construct a valid value of Example3c::First because we know all of its fields implement Zeroable, and then we can use the reference blessed method of accessing the integer discriminant value.

I haven't figured out how to do this with CheckedBitPattern though, since that trait doesn't provide a default value for each field as needed for the construction of variants. Furthermore, the restriction on where attributes are placed could break existing code. It would be nice if this enhanced checking could be turned on via a feature flag on a crate by crate basis, but unfortunately any crate enabling the check would currently implicitly enable the check for all other crates using bytemuck. A major semver bump would do the job more cleanly, but it would be a shame to have to do such a bump just for this.

However, if there was a semver bump, we could remove the transparent helper in @zachs18's example and introduce a wrapped helper which is meant to be attached to the field being wrapped:

#[derive(Clone, Copy, Zeroable)]
#[repr(transparent)]
struct Foo {
  #[wrapped]
  x: u32
}

At which point the wrapped macro would not be able to change the representation of Foo. The only other similar helper attribute that I see in the documentation is zeroable, which allows derive(Zeroable) to function only when each generic satisfies a custom set of bounds. However, generic parameters can also take attributes, so if the semver was increased, the zeroable helper attribute could be moved to be per generic parameter instead of being attached to the entire item. Additionally, a semver bump would allow some sort of default value producing function to be added to CheckedBitPattern, which would let the derive(CheckedBitPattern) macro generate all of the variants so that their discriminant values could be confirmed.

Alternatively all of this would be a lot easier if there was some safe way to get the discriminant value of non-fieldless primitive enums as integers. Especially if we could do so without needing to fully construct the associated variants.

[dilr]

Speaking of a unified way to access enum's discriminant as integers, there is an RFC pull request for that. (rust-lang/rfcs#3607) However, it currently seems to be somewhat stalled out.

If this was implemented though, I think that lack of knowledge about the representation would cease to be a problem.