[derive] Fix AsBytes for #[repr(C, packed(N))]

[maurer]

packed(N) does not gaurantee no padding, but it doesn't prevent it either. This was previously supported.

This regressed in #643 (left out of the commit message because of the requirement not to link to review sites).

[joshlf]

Woah, great catch, and thanks for such a thorough PR! We'll review this soon. Out of curiosity, how did you find this issue?

[maurer]

crosvm uses #[repr(C, packed(4))] in a correct but not load bearing way (compared to #[repr(C, packed)]) in one of their tests. This hit us when updating zerocopy.

[joshlf]

Okay cool, thanks!

[jswrenn]

What happened here is subtle, so I'm attempting to figure it out and put it in writing:

Parsing before #643

In 7b98c7f (the commit before #643), the regression test provided by @maurer is accepted by zerocopy:

#[derive(AsBytes)]
#[repr(C, packed(2))]
// The same caveats as for CPacked apply - we're assuming u64 is at least
// 4-byte aligned by default. Without packed(2), this should fail, as there
// would be padding between a/b assuming u64 is 4+ byte aligned.
struct CPacked2 {
    a: u16,
    b: u64,
}

static_assertions::assert_impl_all!(CPacked2: AsBytes);

This was accepted possibly accidentally. To see why, let's trace the expansion of derive(AsBytes). We start at derive_as_bytes:

zerocopy/zerocopy-derive/src/lib.rs

Lines 95 to 99 in 7b98c7f

	#[proc_macro_derive(AsBytes)]
	pub fn derive_as_bytes(ts: proc_macro::TokenStream) -> proc_macro::TokenStream {
	let ast = syn::parse_macro_input!(ts as DeriveInput);
	match &ast.data {
	Data::Struct(strct) => derive_as_bytes_struct(&ast, strct),

Then enter derive_as_bytes_struct:

zerocopy/zerocopy-derive/src/lib.rs

Lines 270 to 273 in 7b98c7f

	fn derive_as_bytes_struct(ast: &DeriveInput, strct: &DataStruct) -> proc_macro2::TokenStream {
	let reprs = try_or_print!(STRUCT_UNION_AS_BYTES_CFG.validate_reprs(ast));
	let is_transparent = reprs.contains(&StructRepr::Transparent);
	let is_packed = reprs.contains(&StructRepr::Packed);

The definition of STRUCT_UNION_ALLOWED_REPR_COMBINATIONS in STRUCT_UNION_AS_BYTES_CFG would suggest that packed(N) isn't permitted:

zerocopy/zerocopy-derive/src/lib.rs

Lines 128 to 133 in 7b98c7f

	const STRUCT_UNION_ALLOWED_REPR_COMBINATIONS: &[&[StructRepr]] = &[
	&[StructRepr::C],
	&[StructRepr::Transparent],
	&[StructRepr::Packed],
	&[StructRepr::C, StructRepr::Packed],
	];

So what happens when we call valid_reprs? The first thing is that we call reprs to parse the metas:

zerocopy/zerocopy-derive/src/repr.rs

Lines 51 to 52 in 7b98c7f

	pub fn validate_reprs(&self, input: &DeriveInput) -> Result<Vec<R>, Vec<Error>> {
	let mut metas_reprs = reprs(&input.attrs)?;

...which calls StructRepr::parse:

zerocopy/zerocopy-derive/src/repr.rs

Lines 131 to 136 in 7b98c7f

	fn parse(meta: &Meta) -> syn::Result<$repr_name> {
	match Repr::from_meta(meta)? {
	$(Repr::$repr_variant => Ok($repr_name::$repr_variant),)*
	Repr::Align(u) => Ok($repr_name::Align(u)),
	_ => Err(Error::new_spanned(meta, concat!("unsupported representation for deriving FromBytes, AsBytes, or Unaligned on ", $type_name)))
	}

...which calls Repr::from_meta:

zerocopy/zerocopy-derive/src/repr.rs

Lines 190 to 220 in 7b98c7f

	fn from_meta(meta: &Meta) -> Result<Repr, Error> {
	match meta {
	Meta::Path(path) => {
	let ident = path
	.get_ident()
	.ok_or_else(|| Error::new_spanned(meta, "unrecognized representation hint"))?;
	match format!("{}", ident).as_str() {
	"u8" => return Ok(Repr::U8),
	"u16" => return Ok(Repr::U16),
	"u32" => return Ok(Repr::U32),
	"u64" => return Ok(Repr::U64),
	"usize" => return Ok(Repr::Usize),
	"i8" => return Ok(Repr::I8),
	"i16" => return Ok(Repr::I16),
	"i32" => return Ok(Repr::I32),
	"i64" => return Ok(Repr::I64),
	"isize" => return Ok(Repr::Isize),
	"C" => return Ok(Repr::C),
	"transparent" => return Ok(Repr::Transparent),
	"packed" => return Ok(Repr::Packed),
	_ => {}
	}
	}
	Meta::List(list) => {
	return Ok(Repr::Align(list.parse_args::<LitInt>()?.base10_parse::<u64>()?))
	}
	_ => {}
	}
	Err(Error::new_spanned(meta, "unrecognized representation hint"))
	}

Here, we find the bug. If the meta has arguments, it is unconditionally parsed as Repr::Align(..):

zerocopy/zerocopy-derive/src/repr.rs

Lines 213 to 215 in 7b98c7f

	Meta::List(list) => {
	return Ok(Repr::Align(list.parse_args::<LitInt>()?.base10_parse::<u64>()?))
	}

validate_reprs then filters this out:

zerocopy/zerocopy-derive/src/repr.rs

Lines 68 to 71 in 7b98c7f

	metas_reprs.into_iter().filter(|(_, repr)| !repr.is_align()).for_each(|(meta, repr)| {
	metas.push(meta);
	reprs.push(repr)
	});

...so the apparent repr of CPacked2 becomes just repr(C). Since this is in the allowed_combinations of STRUCT_UNION_ALLOWED_REPR_COMBINATIONS, the repr is accepted.

Fortunately, this is not a soundness bug. If the example is modified to use repr(C, packed(4)) instead, an error is produced. This is because the expansion of AsBytes includes a const-eval padding check for padding:

zerocopy/zerocopy-derive/src/lib.rs

Lines 559 to 566 in 7b98c7f

	let padding_check_bound = padding_check.and_then(|check| (!field_types.is_empty()).then_some(check)).map(|check| {
	let fields = field_types.iter();
	let validator_macro = check.validator_macro_ident();
	parse_quote!(
	::zerocopy::macro_util::HasPadding<#type_ident, {::zerocopy::#validator_macro!(#type_ident, #(#fields),*)}>:
	::zerocopy::macro_util::ShouldBe<false>
	)
	});

Parsing after #643

In #643, from_repr is modified to parse packed(n) as Repr::PackedN(n), rather than Repr::Align(n). Since PackedN is not in the allowed combinations of STRUCT_UNION_ALLOWED_REPR_COMBINATIONS, the repr is rejected by validate_reprs.

[jswrenn]

This PR partially repairs a longstanding oddity in our repr parsing: the special treatment of Align. align does not appear in our list of allowed combinations:

zerocopy/zerocopy-derive/src/lib.rs

Lines 128 to 133 in 7b98c7f

	const STRUCT_UNION_ALLOWED_REPR_COMBINATIONS: &[&[StructRepr]] = &[
	&[StructRepr::C],
	&[StructRepr::Transparent],
	&[StructRepr::Packed],
	&[StructRepr::C, StructRepr::Packed],
	];

...nor the invocation of define_kind_specific_repr:

zerocopy/zerocopy-derive/src/repr.rs

Line 165 in 7b98c7f

define_kind_specific_repr!("a struct", StructRepr, C, Transparent, Packed);

Rather, it is defined implicitly during macro expansion:

zerocopy/zerocopy-derive/src/repr.rs

Lines 111 to 114 in 7b98c7f

	pub enum $repr_name {
	$($repr_variant,)*
	Align(u64),
	}

This is because, unlike the other modifiers, align always takes an argument. This PR makes define_kind_specific_repr a little less magical: It extends the macro with the ability to generate provided argument-taking modifiers.

However, the way it fixes the issue is still a little magical. Recall that validate_reprs filters out the Align repr (because it would be a pain to enumerate these in allowed_combinations):

zerocopy/zerocopy-derive/src/repr.rs

Lines 68 to 71 in 7b98c7f

	metas_reprs.into_iter().filter(|(_, repr)| !repr.is_align()).for_each(|(meta, repr)| {
	metas.push(meta);
	reprs.push(repr)
	});

This PR leverages that behavior by having PackedN(n).is_align() return true:

zerocopy/zerocopy-derive/src/repr.rs

Lines 117 to 122 in 8d3c3fc

	fn is_align(&self) -> bool {
	match self {
	$($repr_name::$repr_variant_aligned(_) => true,)*
	_ => false,
	}
	}

I'm not entirely comfortable with this. Not only is the name misleading now, but what happens when a new argument-consuming is added (particularly one that doesn't modify alignment)?

@joshlf, I think we need to reconsider our repr parsing framework, and reduce the the special handling of Align and PackedN. My understanding is we need this special handling because we can't write out argument-bearing modifiers in allowed_combinations. Perhaps we can abstract the argument component so we can specify either concrete or pattern arguments; e.g.:

// ditto for `StructRepr`, etc.
enum ReprKind<A: ReprArgument> {
    C,
    Transparent,
    Packed,
    PackedN(A::PackedN),
    Align(A::Align),
}

trait ReprArgument {
    type PackedN;
    type Align,
}

struct Pattern;
struct Concrete;

impl ReprArgument for Pattern {
    type PackedN = u64;
    type Align = u64;
}

impl ReprArgument for Concrete {
    type PackedN = RangeFull;
    type Align = RangeFull;
}

const STRUCT_UNION_ALLOWED_REPR_COMBINATIONS: &[&[StructRepr<Pattern>]] = &[
    &[StructRepr::C],
    &[StructRepr::Transparent],
    &[StructRepr::Packed],
    &[StructRepr::PackedN(..)],
    &[StructRepr::Align],
    &[StructRepr::C, StructRepr::Packed],
    &[StructRepr::C, StructRepr::Packed(..)],
    &[StructRepr::C, StructRepr::Align],
];

[joshlf]

@jswrenn and I discussed this offline. We're going to follow up with the changes he suggested, but in the meantime we'll merge this and publish a new version. Thanks again for catching this and for submitting a fix!

Discussed offline

[joshlf]

Published as 0.7.29.

[joshlf]

Yanked 0.7.27 and 0.7.28, both of which contain the regression.