Remove panic opportunities

[joshlf]

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.

[jswrenn]

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]

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

[clundin25]

Hello! I am hoping you can help me on this topic.

Problem

I'm working on migrating from zerocopy v0.6.6 to v0.8.6. This is an embedded RISCV codebase, that is strictly no-panic but I see the same behavior on my x86_64 Linux workstation.

I am blocked on validate_cast_and_convert_metadata. The compiler is not able to optimize this method out and thus retains a panic in the binary.

I'm hoping you might have some suggestions on how to tackle this.

Examples

Here is a stripped down example of my code. This will introduce validate_cast_and_convert_metadata and a panic into the final binary.

use core::mem::size_of;
use zerocopy::FromBytes;

fn main() {
    let q = [0x1, 0x1, 0x1, 0x1, 0x2, 0x2, 0x2, 0x2, 0xA];
    let count = q.len() / size_of::<u32>();
    let (word_buf, _suffix) = <[u32]>::ref_from_prefix_with_elems(&q, count).unwrap();

    let mut p = [0x1, 0x1, 0x1, 0x1, 0x2, 0x2, 0x2, 0x2, 0xA];
    let p_count = p.len() / size_of::<u32>();
    let (mut writeable_word_buf, _suffix) = <[u32]>::mut_from_prefix_with_elems(&mut p, p_count).unwrap();
}

This example will optimize validate_cast_and_convert_metadata out of the binary.

use core::mem::size_of;
use zerocopy::FromBytes;

fn main() {
    let q = [0x1, 0x1, 0x1, 0x1, 0x2, 0x2, 0x2, 0x2, 0xA];
    let count = q.len() / size_of::<u32>();
    let (word_buf, _suffix) = <[u32]>::ref_from_prefix_with_elems(&q, count).unwrap();
}

Here are the profile options:

[profile.release]
panic = "abort"
lto = true
opt-level = "s"
codegen-units = 1

Workarounds

So far I have identified two workarounds with significant drawbacks.

Deserialize one u32 at a time for the immutable code path

I can deserialize one u32 at a time using [read_from_bytes](https://docs.rs/zerocopy/latest/zerocopy/trait.FromBytes.html#method.read_from_bytes).

This has two major drawbacks:

• Performance takes a massive hit.
• I have to use a copy. Using the ref variant will pull in validate_cast_and_convert_metadata and it's panic into the binary.

Only use the mutable API

I can refactor the code to only use the mutable mut_from_prefix_with_elems variant. This avoid introducing validate_cast_and_convert_metadata to the binary.

The major drawback here is that byte slices that were once immutable are now mutable across several layers of code, which could lead to future bugs.

I don't think this workaround will get much buy in.

Sources

I've posted an example repo on my GitHub profile. You can compile with cargo build --release and inspect the assembly to confirm the above behavior.

Additionally I can share the production changes, since it's an open source project.

[clundin25]

This patch proposed by @jhand2 solves my problem. clundin25@1068bb7

I'm going to spend some more time to verify it, but I wanted to share it to get your thoughts on this.

Would you consider adding this patch to 0.8?

[clundin25]

#1997 Resolves the issue I was facing.

[joshlf]

#1997 has been published in 0.8.8.