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.

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

Remove panic opportunities #1661

Open
Tracked by #671
joshlf opened this issue Sep 15, 2024 · 2 comments
Open
Tracked by #671

Remove panic opportunities #1661

joshlf opened this issue Sep 15, 2024 · 2 comments

Comments

@joshlf
Copy link
Member

joshlf commented Sep 15, 2024

These are the result of auditing (as of 0003184) for panic opportunities. Some have been left off which are either unavoidable, in progress of being removed (#1658), or downstream of ones listed here (namely, downstream of is_bit_valid).

validate_cast_and_convert_metadata

zerocopy/src/layout.rs

Lines 444 to 445 in 0003184

/// `validate_cast_and_convert_metadata` will panic if `self` describes a
/// DST whose trailing slice element is zero-sized.

We should be able to make this work via a post-monomorphization error instead, and thus avoid a panic opportunity.

PointerMetadata::size_for_metadata

zerocopy/src/lib.rs

Lines 719 to 721 in 0003184

/// If `Self = ()`, `layout` must describe a sized type. If `Self = usize`,
/// `layout` must describe a slice DST. Otherwise, `size_for_metadata` may
/// panic.

TryFromBytes::is_bit_valid

zerocopy/src/lib.rs

Lines 1243 to 1251 in 0003184

/// `is_bit_valid` may panic. Callers are responsible for ensuring that any
/// `unsafe` code remains sound even in the face of `is_bit_valid`
/// panicking. (We support user-defined validation routines; so long as
/// these routines are not required to be `unsafe`, there is no way to
/// ensure that these do not generate panics.)
///
/// Besides user-defined validation routines panicking, `is_bit_valid` will
/// either panic or fail to compile if called on a pointer with [`Shared`]
/// aliasing when `Self: !Immutable`.

Now that const eval semantics are more nailed down, we can probably stop hedging that this might panic and just guarantee a post-monomorphization error.

Note that many panics are downstream of is_bit_valid. If we tackle this, we should make sure to remove panic documentation from all downstream functions.

round_down_to_next_multiple_of_alignment

zerocopy/src/util/mod.rs

Lines 623 to 624 in 0003184

/// May panic if `align` is not a power of two. Even if it doesn't panic in this
/// case, it will produce nonsense results.

We could benefit from a power-of-two witness type.

@joshlf joshlf changed the title In validate_cast_and_convert_metadata, use post-monomorphization error to ban ZSTs instead of panicking Remove panic opportunities Sep 15, 2024
@joshlf joshlf mentioned this issue Sep 15, 2024
82 tasks
@joshlf joshlf added blocking-next-release This issue should be resolved before we release on crates.io and removed blocking-next-release This issue should be resolved before we release on crates.io labels Sep 15, 2024
@jswrenn
Copy link
Collaborator

jswrenn commented Sep 16, 2024

validate_cast_and_convert_metadata

I don't think we can remove this one. validate_cast_and_convert_metadata's sole callsite is in Ptr::try_cast_into. That method consumes a meta: Option<U::PointerMetadata> argument. This meta is then combined U::LAYOUT to create layout: DstLayout, on which validate_cast_and_convert_metadata is called.

As a rule, we can only convert a panic to PME in scenarios where the PME condition is observable in a const context. This is a little too dynamic.

@joshlf
Copy link
Member Author

joshlf commented Sep 16, 2024

Once our MSRV is high enough, maybe we can pass the Layout as a KnownLayout bound instead of as a value?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants