google / zerocopy

https://discord.gg/MAvWH2R6zk
Apache License 2.0
1.14k stars 87 forks source link

Remove panic opportunities #1661

Open joshlf opened 3 days ago

joshlf commented 3 days ago

These are the result of auditing (as of 000318497d86599e24cecb66b945c86a53af3163) 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

https://github.com/google/zerocopy/blob/000318497d86599e24cecb66b945c86a53af3163/src/layout.rs#L444-L445

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

PointerMetadata::size_for_metadata

https://github.com/google/zerocopy/blob/000318497d86599e24cecb66b945c86a53af3163/src/lib.rs#L719-L721

TryFromBytes::is_bit_valid

https://github.com/google/zerocopy/blob/000318497d86599e24cecb66b945c86a53af3163/src/lib.rs#L1243-L1251

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

https://github.com/google/zerocopy/blob/000318497d86599e24cecb66b945c86a53af3163/src/util/mod.rs#L623-L624

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

jswrenn commented 2 days ago

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 commented 2 days ago

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