Other proc macros can break the soundness of our custom derives

[joshlf]

This issue tracks soundness holes in our custom derives introduced by other proc macros. Tasks:

• Enumerate all soundness holes
• Confirm that the list is complete (prove there are no other soundness holes)
• Enumerate things which feel like they could be soundness holes, but aren't
  • Write tests that confirm we're robust against these
• Fix the following known soundness holes:
  • Proc macro attributes which run after our custom derives can modify the type definition "out from under us" (proof of concept). Possible solutions:
    • Update the reference to guarantee the order of evaluation of attributes, and then fail compilation if our custom derives are not in the position which is evaluated last. (Currently, order of evaluation is lexical, but it's not guaranteed to remain this way.)
    • If attributes are stripped out after evaluation so that any subsequently-executed custom derives don't see them in their input token stream, we could simply fail compilation if we see any unrecognized attributes. This would not require the order of evaluation to be guaranteed in order to guarantee soundness, but it would mean that future changes to the order of evaluation could break zerocopy, creating a stability hazard.
• Confirm that the following hypothesized soundness holes are real:
  • #[cfg_attr(foo, repr(...)] might cause our derive to either fail to notice a repr or notice a repr which is removed in some compilations
  • A future language addition could introduce a built-in attribute which we don't recognize, and which changes the layout or bit validity of a type (one possible solution to this would just be to fail if we see unrecognized attributes).
  • A future language addition could allow custom derives to modify the types they annotate, making them equivalent to proc macro attributes in terms of ability to mess with our custom derives.
• Write tests to confirm that the following aren't soundness holes:
  • A user could import an attribute macro named repr, and trick our custom derives into thinking it's the "real" repr attribute. This seems not to be a hole for two reasons:
    • When you write #[repr(...)], rustc currently complains that the name is ambiguous, and fails compilation. We need to confirm that this behavior is guaranteed.
    • If we are able to require that our derive is evaluated after all attributes are evaluated and we are guaranteed that all already-executed attributes will not be present in the token stream which is fed to our custom derive, then a repr attribute which is actually a proc macro will be evaluated first, and the token stream passed to our custom derive will not include it - we'll only see "real" repr attributes.

Misc Notes

• It might be the case that attribute evaluation order has to be guaranteed (whether that was intended in the past or not) because it's observable. In particular, the widely-used technique of a custom derive defining its own attributes (e.g., #[serde(...)]) seems to depend on attribute evaluation order.

[djkoloski]

I verified that attribute macros can change the definition of an item after derives on it have already run:

use transform::insert_field;

trait A {}
trait B {}

#[derive(Debug)]
#[insert_field(baz = "u32")]
struct Foo {
    bar: i32,
}

fn main() {
    println!("{:?}", Foo {
        bar: 10,
        baz: 100,
    });
}

prints:

Foo { bar: 10 }

Which demonstrates that even though Foo had an extra baz field inserted via attribute proc-macro, the item gets passed to the Debug derive before that modification takes place. Expanding the source with cargo expand also demonstrates the issue. For completeness, the source of the insert_field proc-macro is:

use proc_macro2::{Ident, Span};
use quote::quote;
use syn::{parse_macro_input, DeriveInput, Meta, Lit, Expr, Visibility, token::Colon, Data, Fields, Field, parse_quote};

#[proc_macro_attribute]
pub fn insert_field(
    attr: proc_macro::TokenStream,
    item: proc_macro::TokenStream,
) -> proc_macro::TokenStream {
    let meta = parse_macro_input!(attr as Meta);
    let Meta::NameValue(name_value) = meta else { panic!("expected name-value") };
    let name = name_value.path.get_ident().expect("name to be an identifier");
    let Expr::Lit(lit) = &name_value.value else { panic!("expected literal") };
    let Lit::Str(ty_name) = &lit.lit else { panic!("expected string") };
    let ident = Ident::new(&ty_name.value(), Span::call_site());

    let mut item = parse_macro_input!(item as DeriveInput);
    let Data::Struct(struct_) = &mut item.data else { panic!("expected struct") };
    let Fields::Named(fields) = &mut struct_.fields else { panic!("expected fields") };
    fields.named.push(Field {
        attrs: Vec::new(),
        vis: Visibility::Inherited,
        mutability: syn::FieldMutability::None,
        ident: Some(name.clone()),
        colon_token: Some(Colon { spans: [Span::call_site()]}),
        ty: parse_quote!(#ident),
    });

    let output = quote! {
        #item
    };

    output.into()
}

[jswrenn]

The status quo is that proc macros evaluate from the outside in. We should confirm this is specified, and do what we can to mitigate it.

We could defend against @djkoloski's example by also emitting code that destructures the annotated type, thus ensuring that there would be a compile error if the definition changed.

However, imagine a proc-macro attribute that only removed (or tampered) with #[repr(C)] from annotated definitions, but left fields unchanged. For this, the only mitigation I can see is forbidding the presence of unknown attributes.

[joshlf]

IIUC, guaranteeing evaluation order should be enough to mitigate the "unknown attribute" problem: We just ensure that we're placed in a location that evaluates after any attribute macros.

That still leaves open the question of shadowing attributes by name - e.g., introducing a proc macro attribute called repr that our custom derives mistakenly think is the built-in repr attribute.

Another thought: Does the token stream emitted by a proc macro attribute include the proc macro attribute annotation? If not, we should expect that any proc macro attributes which execute before us will no longer be present in the token stream that we see. This should mean that a proc macro attribute which shadows repr would be removed by the time it gets to us, and we'd only see "real" repr attributes.

[jswrenn]

I've confirmed that it's not possible to shadow repr attributes. Doing so produces an error:

`repr` is ambiguous
ambiguous because of a name conflict with a builtin attribute
use `crate::repr` to refer to this attribute macro unambiguously

[joshlf]

Phew

[joshlf]

cc @reinerp

[joshlf]

A user ran into this: #1497 (comment)

[ianthetechie]

Said user jumping in since it appears I found the wrong thread ;)

To make things more concrete, here's an MRE repo which illustrates one of the holes when interacting with the bitfield-struct crate.

[joshlf]

Said user jumping in since it appears I found the wrong thread ;)

To make things more concrete, here's an MRE repo which illustrates one of the holes when interacting with the bitfield-struct crate.

It looks like your code has compilation errors even if I remove all the zerocopy bits:

use bitfield_struct::bitfield;

// Enum that's using an integer representation,
// but does not cover the full range.
// Thus, it must be TryFromBytes.

#[repr(u8)]
pub enum IntBackedEnum {
    VariantA = 0,
    VariantB = 1,
    VariantC = 26,
}

// The trouble seems to come when we further use bitfield-struct
// to shove this into a packed field.

#[bitfield(u8)]
struct BitfieldWithEnum {
    // We're bit packing and only care about 6 bits!
    #[bits(6)]
    enum_value: IntBackedEnum,
    #[bits(2)]
    other_field: u8,
}

// And here's a second example of breakage.
// If you derive TryFromBytes before the bitfield macro,
// compilation fails.
// It works when you flip the order (bitfield macro first).

#[bitfield(u8)]
struct BitfieldWithInteger {
    // We're bit packing and only care about 6 bits!
    #[bits(6)]
    enum_value: u8,
    #[bits(2)]
    other_field: u8,
}

error[E0599]: no variant or associated item named `from_bits` found for enum `IntBackedEnum` in the current scope
  --> src/lib.rs:17:1
   |
8  | pub enum IntBackedEnum {
   | ---------------------- variant or associated item `from_bits` not found for this enum
...
17 | #[bitfield(u8)]
   | ^^^^^^^^^^^^^^^ variant or associated item not found in `IntBackedEnum`
   |
   = note: this error originates in the attribute macro `bitfield` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no variant or associated item named `into_bits` found for enum `IntBackedEnum` in the current scope
  --> src/lib.rs:17:1
   |
8  | pub enum IntBackedEnum {
   | ---------------------- variant or associated item `into_bits` not found for this enum
...
17 | #[bitfield(u8)]
   | ^^^^^^^^^^^^^^^ variant or associated item not found in `IntBackedEnum`
   |
   = note: this error originates in the attribute macro `bitfield` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: `IntBackedEnum` doesn't implement `Debug`
  --> src/lib.rs:17:1
   |
17 | #[bitfield(u8)]
   | ^^^^^^^^^^^^^^^ `IntBackedEnum` cannot be formatted using `{:?}`
   |
   = help: the trait `Debug` is not implemented for `IntBackedEnum`
   = note: add `#[derive(Debug)]` to `IntBackedEnum` or manually `impl Debug for IntBackedEnum`
   = note: required for the cast from `&IntBackedEnum` to `&dyn Debug`
   = note: this error originates in the attribute macro `bitfield` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `IntBackedEnum` with `#[derive(Debug)]`
   |
8  + #[derive(Debug)]
9  | pub enum IntBackedEnum {
   |

Some errors have detailed explanations: E0277, E0599.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `zerocopy-388-mre` (lib) due to 5 previous errors

[ianthetechie]

It looks like your code has compilation errors even if I remove all the zerocopy bits:

d'oh! I feel kind of silly ;) My example included a custom enum which needs to have some magic methods for the bitfield to work.

I have just pushed a correction that will not compile as-is, but will compile if you flip the order of the bitfield and derive macros. (Though now there's another problem: that the other crate doesn't support "failable" conversion, but that's out of scope here).

So, in the end, it looks like there is only one issue that I'm highlighing in the MRE: that the ordering can affect whether the generated code is valid.

[joshlf]

Ah okay, yeah unfortunately that is the limitation we're aware of. We should probably reach our derives to detect unrecognized attributes and bail with an error.