rkyv / rend

Cross-platform, endian-aware primitives for Rust
MIT License
31 stars 12 forks source link

CheckBytes for unaligned NonZero types is UB #13

Closed Awpteamoose closed 6 days ago

Awpteamoose commented 6 days ago

define_unaligned_nonzero invokes impl_nonzero, which implements bytecheck::CheckBytes, it has a comment stating

// SAFETY: `value` points to a `Self`, which has the same size
// as a `$prim` and is at least as aligned as one. Note that the
// bit pattern for 0 is always the same regardless of
// endianness.

But unaligned types always have an alignment of 1 and $prim (other than 8-bit ones) have different alignments. The actual implementation then casts the pointer and reads as if it were aligned, which goes kaboom in debug (sometimes) as it's UB. I was trying to use rkyv with unaligned feature and ran into this.

Awpteamoose commented 6 days ago

Using this as test:

#[cfg(feature = "bytecheck")]
#[test]
fn unsigned_non_zero_alignment() {
    unsafe {
        let src = std::alloc::alloc(std::alloc::Layout::from_size_align(std::mem::size_of::<NonZeroU32_ule>(), std::mem::align_of::<NonZeroU32_ule>()).unwrap()) as *mut NonZeroU32_ule;
        std::ptr::write(src, NonZeroU32_ule::new_unchecked(0x01020304));
        bytecheck::check_bytes::<NonZeroU32_ule, bytecheck::rancor::Failure>(src).unwrap();
    }
}

(had to comment out #![no_std] because of std::alloc for demonstrative purpose)

and running cargo +nightly miri test --features bytecheck

    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.01s
     Running unittests src\lib.rs (target\miri\x86_64-pc-windows-msvc\debug\deps\rend-4aee20f20e4d8083.exe)

running 14 tests
test tests::atomics_16 ... ok
test tests::atomics_32 ... ok
test tests::atomics_64 ... ok
test tests::floats ... ok
test tests::signed_integers ... ok
test tests::signed_non_zero ... ok
test tests::unsigned_integers ... ok
test tests::unsigned_non_zero ... ok
test unaligned::tests::floats ... ok
test unaligned::tests::signed_integers ... ok
test unaligned::tests::signed_non_zero ... ok
test unaligned::tests::unsigned_integers ... ok
test unaligned::tests::unsigned_non_zero ... ok
error: Undefined Behavior: accessing memory based on pointer with alignment 1, but alignment 4 is required
    --> C:\Users\GRD-User\.cargo\git\checkouts\bytecheck-f821dd2d145cbffe\1fb8574\bytecheck\src\lib.rs:1084:1
     |
1084 | impl_nonzero!(NonZeroU32, u32);
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ accessing memory based on pointer with alignment 1, but alignment 4 is required
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: BACKTRACE on thread `unaligned::tests::unsigned_non_zero_alignment`:
     = note: inside `<std::num::NonZero<u32> as bytecheck::CheckBytes<bytecheck::rancor::Strategy<(), bytecheck::rancor::Failure>>>::check_bytes` at C:\Users\GRD-User\.cargo\git\checkouts\bytecheck-f821dd2d145cbffe\1fb8574\bytecheck\src\lib.rs:1067:29: 1067:57
note: inside `<unaligned::NonZeroU32_ule as bytecheck::CheckBytes<bytecheck::rancor::Strategy<(), bytecheck::rancor::Failure>>>::check_bytes`
    --> src\common.rs:368:21
     |
368  |   ...   <$prim>::check_bytes(value.cast(), context)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
    ::: src\unaligned.rs:131:1
     |
131  | / define_unaligned_nonzeros! {
132  | |     NonZeroI16_ule NonZeroI16_ube: 2 NonZeroI16 as i16,
133  | |     NonZeroI32_ule NonZeroI32_ube: 2 NonZeroI32 as i32,
134  | |     NonZeroI64_ule NonZeroI64_ube: 4 NonZeroI64 as i64,
...    |
139  | |     NonZeroU128_ule NonZeroU128_ube: 16 NonZeroU128 a...
140  | | }
     | |_- in this macro invocation
     = note: inside `bytecheck::check_bytes_with_context::<unaligned::NonZeroU32_ule, (), bytecheck::rancor::Failure>` at C:\Users\GRD-User\.cargo\git\checkouts\bytecheck-f821dd2d145cbffe\1fb8574\bytecheck\src\lib.rs:347:14: 347:69
     = note: inside `bytecheck::check_bytes::<unaligned::NonZeroU32_ule, bytecheck::rancor::Failure>` at C:\Users\GRD-User\.cargo\git\checkouts\bytecheck-f821dd2d145cbffe\1fb8574\bytecheck\src\lib.rs:255:14: 255:54
note: inside `unaligned::tests::unsigned_non_zero_alignment`
    --> src\unaligned.rs:492:4
     |
492  | ...   bytecheck::check_bytes::<NonZeroU32_ule, bytecheck::rancor::Failure>(src).un...
     |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
    --> src\unaligned.rs:488:34
     |
487  |     #[test]
     |     ------- in this procedural macro expansion
488  |     fn unsigned_non_zero_alignment() {
     |                                     ^
     = note: this error originates in the macro `impl_nonzero` which comes from the expansion of the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

test unaligned::tests::unsigned_non_zero_alignment ... error: test failed, to rerun pass `--lib`
djkoloski commented 6 days ago

Thanks for the bug report, this one slipped through the cracks. Fixed by 57af743502de3c13842f5c73e4a4cd5fc4464fc8.

djkoloski commented 6 days ago

Now released in 0.5.1