google / google/zerocopy

Other proc macros can break the soundness of our custom derives

Open
#388 11 comments 1 reaction 0 assignees View on GitHub
bug compatibility-breaking
Dominant language
Rust
Stars
2.6k
Forks
179
Avg merge
1d 19h
Merged PRs (30d)
29

Description

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](https://github.com/google/zerocopy/issues/388#issuecomment-1737817682)). 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](https://github.com/rust-lang/rust/issues/63336), 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.

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.