google / google/zerocopy

Restrict support for `#[derive(IntoBytes)]` on unions, work to guarantee forwards-compatible soundness

Open
#1,792 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
2.6k
Forks
179
Avg merge
1d 19h
Merged PRs (30d)
29

Description

*Do you need `IntoBytes` support on unions? Let us know at #1802.*

## Progress

- [x] Put `#[derive(IntoBytes)]` on unions behind a `--cfg`
- #1804
- [x] Update 0.8 release notes to mention #1802
- [x] (Optionally) Update `IntoBytes` docs to mention #1802
- [ ] Get Rust to guarantee union bit validity or safety; either:
- [ ] Get Rust to guarantee union bit validity, or
- [ ] Get Rust to guarantee that union values can't be uninitialized by safe code
- [ ] Get Rust to guarantee that constructing a union value that is not a combination of bit-valid sub-regions of its sub-fields will always require `unsafe` code
- https://github.com/rust-lang/unsafe-code-guidelines/issues/533
- [ ] Get Rust to guarantee that, in a typed copy of a union value, any bytes previously written are preserved so long as those bytes are not written to a byte offset at which there is padding in all fields of the union
- https://github.com/rust-lang/unsafe-code-guidelines/issues/533
- [ ] Update documentation on both `FromBytes` and `IntoBytes` to be clear about what safety invariants are added

## Details

Support for `#[derive(IntoBytes)]` on unions was added in https://github.com/google/zerocopy/commit/9c19cbe1cf5c2621ae9a63179664db3470146c10 (reviewed at [fxrev.dev/639087](https://fuchsia-review.googlesource.com/c/fuchsia/+/639087)). This support is sound on the assumption that if every field of a union is `IntoBytes` and there is no extra padding before or after any field, then no bit-valid instance of that union can have uninitialized bytes.

However, this assumes too much. It is currently [up in the air](https://github.com/rust-lang/unsafe-code-guidelines/issues/438) whether this actually holds. It's not clear from reading the history why we were okay adding this implementation in the first place, but it now seems like it may have been premature.

It may eventually become the case that this assumption is guaranteed by Rust, so this may eventually become a non-issue. However, for the time being, it's a violation of our soundness policy, which [promises to be sound on all future Rust compilers](https://github.com/google/zerocopy/blob/main/POLICIES.md#forwards-compatibility).

### Short-term mitigation

Unfortunately, existing users rely on `#[derive(IntoBytes)]` support on unions, so removing this support will break users. This is likely not a problem on today's Rust, so forcing users to migrate to something else might be too drastic of a solution. Instead, I propose that we discourage further use by gating `#[derive(IntoBytes)]` on unions behind a `--cfg`. The reason for a `--cfg` instead of a Cargo feature is that Cargo features are footguns in cases like this - it's easy for crate A to enable the feature and then for crate B, which depends on A, to accidentally rely on that feature despite not enabling that feature itself. For something with soundness and stability implications, that's risky. By contrast, a `--cfg` requires the top-level crate to enable it for all downstream crates.

### Long-term mitigation

There are two options for long-term mitigations.

#### Union validity

We can try to get Rust to promise what we need in order for `#[derive(IntoBytes)]` on unions to be guaranteed sound on all future compilers. In particular, this entails restricting union bit validity so that, if the byte at a given offset is initialized in every valid value of every field, then it must be initialized in the union as well.

#### Union safety

Alternatively, we can take a weaker approach that only requires a safety rather than bit validity constraint: https://github.com/google/zerocopy/issues/1792#issuecomment-2389405203

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.