Make SizeEq safe, introduce CastExact #2872
Conversation
Summary of ChangesHello @joshlf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the safety and soundness of pointer casting operations by introducing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step forward for the library's safety by making the SizeEq trait safe to implement. The introduction of the CastExact trait to enforce size-preserving casts is a solid approach. The changes are consistently applied throughout the codebase.
I've found a couple of places where new unsafe impls for CastExact are missing their safety comments. It's important to document the justification for these unsafe blocks to ensure long-term maintainability and correctness. Please see the specific comments for details.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## G3eff65bc88b62c899bbd054028b3ff9306fe2167 #2872 +/- ##
=============================================================================
+ Coverage 91.90% 91.91% +0.01%
=============================================================================
Files 20 20
Lines 5878 5886 +8
=============================================================================
+ Hits 5402 5410 +8
Misses 476 476 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joshlf
force-pushed
the
G57ec07c3841271440bbaf40cab04b942cbdbddb9
branch
from
a6e9a9b to
a972b24
Compare
| // TODO: Update this comment to not rely on `SizeEq` implying size equality | ||
| // (but instead rely on *runtime execution* of `SizeEq::CastFrom::project` | ||
| // guaranteeing size equality). |
There was a problem hiding this comment.
@jswrenn Calling out this subtlety. We should have made sure to update all of our safety comments this way before – when we introduced the PME for metadata updating in the layout module. But now that SizeEq is safe, and CastExact technically allows impls which diverge at runtime, we need to be extra careful.
| // SAFETY: FIXME(#1818, #2701): THIS IS UNSOUND. However, given how it's | ||
| // used today, it can't cause UB. In particular, the static assertions below | ||
| // only check that source alignment is not less than destination alignment, | ||
| // but not that they are equal. This means that a pointer cast could result | ||
| // in a referent with less trailing padding. This technically violates the | ||
| // safety invariant of `CastExact`, but can only result in UB if the padding | ||
| // bytes are read, which they never are. Obviously we should still fix this. |
| /// Projects a [`PtrInner`] from `Src` to `Dst`. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller may assume that the resulting `PtrInner` addresses a | ||
| /// subset of the bytes of `src`'s referent. | ||
| #[must_use] | ||
| #[inline(always)] | ||
| fn project(src: PtrInner<'_, Src>) -> PtrInner<'_, Dst> { |
There was a problem hiding this comment.
NOTE: Moving this here so we can reference SizeEq::CastFrom::project in safety comments. This allows us to just assume that project is a valid PtrInner -> PtrInner conversion and not have to prove anything about PtrInner's invariants (see the safety comment on TryTransmuteFromPtr in transmute.rs).
joshlf
force-pushed
the
G57ec07c3841271440bbaf40cab04b942cbdbddb9
branch
2 times, most recently
from
420424b to
a0789ec
Compare
joshlf
changed the base branch from
main
to
G3cc35009cf6e17a935df0ae0c3c37dcc389e2f9f
joshlf
force-pushed
the
G57ec07c3841271440bbaf40cab04b942cbdbddb9
branch
from
a0789ec to
8d0245a
Compare
joshlf
force-pushed
the
G57ec07c3841271440bbaf40cab04b942cbdbddb9
branch
from
8d0245a to
796deda
Compare
joshlf
changed the base branch from
main
to
G3eff65bc88b62c899bbd054028b3ff9306fe2167
gherrit-pr-id: G3eff65bc88b62c899bbd054028b3ff9306fe2167
Introduce `CastExact: Cast`, which denotes that a `Cast` exactly preserves the set of referent bytes. Add this bound to `SizeEq::CastFrom`, allowing `SizeEq` to be safe to implement. gherrit-pr-id: G57ec07c3841271440bbaf40cab04b942cbdbddb9
joshlf
force-pushed
the
G3eff65bc88b62c899bbd054028b3ff9306fe2167
branch
from
f7e10fa to
59f812e
Compare
joshlf
force-pushed
the
G57ec07c3841271440bbaf40cab04b942cbdbddb9
branch
from
796deda to
4ef287c
Compare
2 participants
Introduce
CastExact: Cast, which denotes that aCastexactlypreserves the set of referent bytes. Add this bound to
SizeEq::CastFrom, allowingSizeEqto be safe to implement.Latest Update: v19 — Compare vs v18
📚 Full Patch History
Links show the diff between the row version and the column version.