`bool` and `char`'s `Bytes` implementations cause undefined behavior.

[zachs18]

The documentation for secrets::traits::Bytes states:

Any type that implements Bytes must not exhibit undefined behavior when its underlying bits are set to any arbitrary bit pattern.

Currently, bool and char (the primitive types) have implementations for Bytes (https://github.com/stouset/secrets/blob/master/src/traits.rs#L69 ), but these types can not be set to arbitrary bit patterns (specifically, bool must have the bit pattern 0x00 or 0x01, and char must have a bit pattern in the range 0x0000_0000..=0x0000_D7FF or the range 0x0000_E000..=0x0010_FFFF)

The following example program exhibits undefined behavior due to this (run it in debug mode and in release mode and you'll most likely see different results):

fn main() {
    let b: char = secrets::traits::Bytes::uninitialized();
    match b {
        // Note that these two patterns together include all valid char values
        '\x00'..='\u{10fffe}' => dbg!("char1"),
        '\x01'..='\u{10ffff}' => dbg!("char2"), // This prints in release mode on my machine (secrets 1.2.0, rustc 1.61.0)
        _ => dbg!("huh?"), // This prints in debug mode on my machine
    };
}

[197g]

Note that more of the implementation is unsound:

• tuples have padding, which violates the requirements for as_bytes(&self) that is accessible via the AsContiguousBytes provided default implementations for [T]::as_bytes{,mut} as well as ConstantEq::constant_eq.
• the wording and signature of Zeroable::transfer make no sense in combination. Mutable references implies that non-overlapping, which makes me suspicious the uniqueness requirement for creating mutable references may be violated somewhere in the crate
• the implementation of Bytes::uninitialized always writes exactly one byte, regardless of the underlying type. This does not in fact initialize anything in particular large structs will be mostly uninitialized, just imagine a [u8; 1 << 14].

Maybe file a RUSTSEC issue.

[stouset]

Reopening so I can address the remaining unsoundness issues brought up.

[stouset]

tuples have padding, which violates the requirements for as_bytes(&self) that is accessible via the AsContiguousBytes provided default implementations for [T]::as_bytes{,mut} as well as ConstantEq::constant_eq.

This is a good catch. I will remove the impl from tuple types.

the wording and signature of Zeroable::transfer make no sense in combination. Mutable references implies that non-overlapping, which makes me suspicious the uniqueness requirement for creating mutable references may be violated somewhere in the crate

Can you elaborate here? What part of the wording do you have an issue with?

Yes, the two mutable references need to be non-overlapping. But I don't believe we return mutable references from anything other than places where we allocate a new block of memory ourselves, or derived from an existing mutable reference. If you find a place where we can return overlapping mutable references from anywhere, that would definitely be a soundness bug.

• the implementation of Bytes::uninitialized always writes exactly one byte, regardless of the underlying type. This does not in fact initialize anything in particular large structs will be mostly uninitialized, just imagine a [u8; 1 << 14].

I had to do a double-take looking at the code, but this is not correct. From ptr::write_bytes:

Sets count * size_of::<T>() bytes of memory starting at dst to val.

We aren't writing one byte, we're writing one count of size_of:<T>() bytes. size_of::<[u8; 1 << 14]> is 16,384 bytes, so we write 1 * 16,384 bytes of memory into it.