Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MaybeUninit impls are unsound #299

Closed
joshlf opened this issue Aug 29, 2023 · 0 comments · Fixed by #308
Closed

MaybeUninit impls are unsound #299

joshlf opened this issue Aug 29, 2023 · 0 comments · Fixed by #308
Labels
blocking-next-release This issue should be resolved before we release on crates.io compatibility-breaking Changes that are (likely to be) breaking

Comments

@joshlf
Copy link
Member

joshlf commented Aug 29, 2023 •
edited
Loading

Since FromZeroes and FromBytes ban UnsafeCells, and since rust-lang/unsafe-code-guidelines#455 implies that there's no way to "disable" interior mutability, our impls of FromZeroes and FromBytes for MaybeUninit<T> with no bounds on T are unsound. See also this discussion.

One option we may want to consider is introducing a NoCell trait that is a super-trait of all of our traits (other than Unaligned) so that we can do e.g. impl<T: NoCell> FromBytes for MaybeUninit<T>.

A note on semver: When releasing this, it's doubtful that anyone is relying on this behavior. We may want to just release 0.7.X (whatever the next patch version is at the time of releasing) and yank the previous 0.7.Y versions.
@joshlf joshlf added compatibility-breaking Changes that are (likely to be) breaking blocking-next-release This issue should be resolved before we release on crates.io labels Aug 29, 2023
joshlf added a commit that referenced this issue Sep 2, 2023
@joshlf
Restrict MaybeUninit trait impls to fix soundness
7ed3777 
Previously, we implemented `FromZeroes` and `FromBytes` for
`MaybeUninit<T>` with no bound on `T`. This resulted in a soundness hole
in which `T` - and thus `MaybeUninit<T>` - could contain an
`UnsafeCell`, which is a violation of the contracts of `FromZeroes` and
`FromBytes`.

This is a breaking change, but it's very unlikely to be one that code is
currently relying on, especially given that the 0.7.x release train was
published very recently. Thus, in this commit, we publish 0.7.3, and we
will yank 0.7.{0,1,2} as soon as 0.7.3 is published.

Fixes #299
@joshlf joshlf mentioned this issue Sep 2, 2023
Merged
joshlf added a commit that referenced this issue Sep 2, 2023
@joshlf
Restrict MaybeUninit trait impls to fix soundness (#308)
62f76d2 
Previously, we implemented `FromZeroes` and `FromBytes` for
`MaybeUninit<T>` with no bound on `T`. This resulted in a soundness hole
in which `T` - and thus `MaybeUninit<T>` - could contain an
`UnsafeCell`, which is a violation of the contracts of `FromZeroes` and
`FromBytes`.

This is a breaking change, but it's very unlikely to be one that code is
currently relying on, especially given that the 0.7.x release train was
published very recently. Thus, in this commit, we publish 0.7.3, and we
will yank 0.7.{0,1,2} as soon as 0.7.3 is published.

Fixes #299
@joshlf joshlf mentioned this issue Sep 2, 2023
Closed
@joshlf joshlf mentioned this issue Sep 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
blocking-next-release This issue should be resolved before we release on crates.io compatibility-breaking Changes that are (likely to be) breaking
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Restrict MaybeUninit trait impls to fix soundness google/zerocopy
1 participant
@joshlf