From 64cc5973c451e92144f20e7a70e04e6de2ef6883 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Sun, 6 Sep 2020 17:56:41 -0700 Subject: [PATCH] Add more tests, overhaul CI, and fix plug soundness hole in `bytemuck::offset_of!` (#38) * Ensure functions that take generic don't call them * Plug soundness hole in offset_of with safe_packed_borrows lint * Add some more tests I noticed were missing (still many missing) * Make CI way more thorough * Fix running MSRV tests with unsupported `--feature`s * Disable derive tests in sanitizers, and fix miri ci --- .github/workflows/rust.yml | 113 +++++++++++++++++++++++++++++-------- src/lib.rs | 6 +- src/offset_of.rs | 89 ++++++++++++++++++++++------- tests/cast_slice_tests.rs | 75 ++++++++++++++++++++++++ 4 files changed, 235 insertions(+), 48 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 43d467c..4bdb9df 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -4,35 +4,100 @@ on: push: {} pull_request: {} +env: + RUST_BACKTRACE: 1 + jobs: - build_test_no_features: - runs-on: ubuntu-latest + test: + name: Test Rust ${{ matrix.rust }} on ${{ matrix.os }} + runs-on: ${{ matrix.os }} strategy: matrix: - rust: - # 1.34 doesn't support the alloc feature! (1.36 or later) - - { toolchain: 1.34.0, extra_args: "" } - # Stable and later support all of our features. - - { toolchain: stable, extra_args: "" } - - { toolchain: stable, extra_args: "--all-features" } - - { toolchain: beta, extra_args: "" } - - { toolchain: beta, extra_args: "--all-features" } - - { toolchain: nightly, extra_args: "" } - - { toolchain: nightly, extra_args: "--all-features" } + # it's a little tempting to use `matrix` to do a cartesian product with + # our `--feature` config here, but doing so will be very slow, as the + # free tier only supports up to 20 job runners at a time. + include: + # versions (all on linux-x86_64) + - { rust: 1.34.0, os: ubuntu-latest } + - { rust: stable, os: ubuntu-latest } + - { rust: beta, os: ubuntu-latest } + - { rust: nightly, os: ubuntu-latest } + # non-linux platforms (ones which don't require `cross`) + - { rust: stable, os: macos-latest } + - { rust: stable, os: windows-latest } + - { rust: stable-x86_64-gnu, os: windows-latest } + - { rust: stable-i686-msvc, os: windows-latest } + - { rust: stable-i686-gnu, os: windows-latest } steps: - uses: hecrj/setup-rust-action@v1 with: - rust-version: ${{ matrix.rust.toolchain }} - - uses: actions/checkout@v1 - - uses: actions-rs/cargo@v1 + rust-version: ${{ matrix.rust }} + - uses: actions/checkout@v2 + - run: cargo test --verbose + - run: cargo test --verbose --no-default-features + - run: cargo test --verbose --all-features + if: matrix.rust != '1.34.0' + - run: cargo test --verbose --manifest-path=derive/Cargo.toml --all-features + if: matrix.rust != '1.34.0' + + cross-test: + name: Test on ${{ matrix.target }} with cross + runs-on: ubuntu-latest + strategy: + matrix: + # note: the mips targets are here so that we have big-endian coverage (both 32bit and 64bit) + target: [i686-unknown-linux-gnu, mips-unknown-linux-gnu, mips64-unknown-linux-gnuabi64] + steps: + - uses: hecrj/setup-rust-action@v1 + - uses: actions/checkout@v2 + - run: cargo install cross + - run: cross test --verbose --target=${{ matrix.target }} --no-default-features + - run: cross test --verbose --target=${{ matrix.target }} --all-features + - run: cross test --verbose --target=${{ matrix.target }} --manifest-path=derive/Cargo.toml --all-features + + miri-test: + name: Test with miri + runs-on: ubuntu-latest + steps: + - uses: hecrj/setup-rust-action@v1 with: - toolchain: ${{ matrix.rust.toolchain }} - command: test - args: ${{ matrix.rust.extra_args }} - - name: Switch to Derive Folder - run: cd derive - - uses: actions-rs/cargo@v1 + rust-version: nightly + components: miri + - uses: actions/checkout@v2 + - run: cargo miri test --verbose --no-default-features + - run: cargo miri test --verbose --all-features + - run: cargo miri test --verbose --manifest-path=derive/Cargo.toml --all-features + + sanitizer-test: + name: Test with -Zsanitizer=${{ matrix.sanitizer }} + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + sanitizer: [address, memory, leak] + steps: + - uses: actions/checkout@v2 + - uses: hecrj/setup-rust-action@v1 with: - toolchain: ${{ matrix.rust.toolchain }} - command: test - args: ${{ matrix.rust.extra_args }} + rust-version: nightly + components: rust-src + - name: Test with sanitizer + env: + RUSTFLAGS: -Zsanitizer=${{ matrix.sanitizer }} + RUSTDOCFLAGS: -Zsanitizer=${{ matrix.sanitizer }} + ASAN_OPTIONS: detect_stack_use_after_return=1 + # Asan's leak detection occasionally complains about some small leaks if + # backtraces are captured. + RUST_BACKTRACE: 0 + # We don't run `derive`'s unit tests here the way we do elsewhere (for + # example, in `miri-test` above), as at the moment we can't easily build + # the `proc_macro` runtime with sanitizers on. IIUC doing this would + # require a custom rustc build, and... lol nope. + # + # This would mean only part of the code running under the sanitizer would + # actually include the sanitizer's checks, which is a recipe for false + # positives, so we just skip it, the generated code is what we care + # about anyway. + run: | + cargo test -Zbuild-std --verbose --target=x86_64-unknown-linux-gnu --no-default-features + cargo test -Zbuild-std --verbose --target=x86_64-unknown-linux-gnu --all-features diff --git a/src/lib.rs b/src/lib.rs index c0fa320..ccbdbda 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -278,7 +278,7 @@ pub fn cast_ref(a: &A) -> &B { } } -/// Cast `&[T]` into `&[U]`. +/// Cast `&[A]` into `&[B]`. /// /// ## Panics /// @@ -376,7 +376,7 @@ pub fn try_cast_mut(a: &mut A) -> Result<&mut B, PodCastError> { } } -/// Try to convert `&[T]` into `&[U]` (possibly with a change in length). +/// Try to convert `&[A]` into `&[B]` (possibly with a change in length). /// /// * `input.as_ptr() as usize == output.as_ptr() as usize` /// * `input.len() * size_of::() == output.len() * size_of::()` @@ -411,7 +411,7 @@ pub fn try_cast_slice(a: &[A]) -> Result<&[B], PodCastError> { } } -/// Try to convert `&mut [T]` into `&mut [U]` (possibly with a change in +/// Try to convert `&mut [A]` into `&mut [B]` (possibly with a change in /// length). /// /// As [`try_cast_slice`], but `&mut`. diff --git a/src/offset_of.rs b/src/offset_of.rs index fdaba20..604379a 100644 --- a/src/offset_of.rs +++ b/src/offset_of.rs @@ -17,16 +17,6 @@ /// type used is `repr(Rust)` then they're *not* fixed across compilations or /// compilers. /// -/// **CAUTION:** It is **unsound** to use this macro with a `repr(packed)` type. -/// Currently this will give a warning, and in the future it will become a hard -/// error. -/// * See [rust-lang/rust#27060](https://github.com/rust-lang/rust/issues/27060) -/// for more info and for status updates. -/// * Once this issue is resolved, a future version of this crate will use -/// `raw_ref` to correct the issue. For the duration of the `1.x` version of -/// this crate it will be an on-by-default cargo feature (to maintain minimum -/// rust version support). -/// /// ## Examples /// /// ### 3-arg Usage @@ -64,20 +54,77 @@ /// assert_eq!(offset_of!(Vertex, loc), 0); /// assert_eq!(offset_of!(Vertex, color), 12); /// ``` +/// +/// # Usage with `#[repr(packed)]` structs +/// +/// Attempting to compute the offset of a `#[repr(packed)]` struct with +/// `bytemuck::offset_of!` requires an `unsafe` block. We hope to relax this in +/// the future, but currently it is required to work around a soundness hole in +/// Rust (See [rust-lang/rust#27060]). +/// +/// [rust-lang/rust#27060]: https://github.com/rust-lang/rust/issues/27060 +/// +///

+/// Warning: This is only true for versions of bytemuck > 1.4.0. +/// Previous versions of +/// bytemuck::offset_of! +/// will only emit a warning when used on the field of a packed struct in safe code, +/// which can lead to unsoundness. +///

+/// +/// For example, the following will fail to compile: +/// +/// ```compile_fail +/// #[repr(C, packed)] +/// #[derive(Default)] +/// struct Example { +/// field: u32, +/// } +/// // Doesn't compile: +/// let _offset = bytemuck::offset_of!(Example, field); +/// ``` +/// +/// While the error message this generates will mention the +/// `safe_packed_borrows` lint, the macro will still fail to compile even if +/// that lint is `#[allow]`ed: +/// +/// ```compile_fail +/// # #[repr(C, packed)] #[derive(Default)] struct Example { field: u32 } +/// // Still doesn't compile: +/// #[allow(safe_packed_borrows)] { +/// let _offset = bytemuck::offset_of!(Example, field); +/// } +/// ``` +/// +/// This *can* be worked around by using `unsafe`, but it is only sound to do so +/// if you can guarantee that taking a reference to the field is sound. +/// +/// In practice, this means it only works for fields of align(1) types, or if +/// you know the field's offset in advance (defeating the point of `offset_of`) +/// and can prove that the struct's alignment and the field's offset are enough +/// to prove the field's alignment. +/// +/// Once the `raw_ref` macros are available, a future version of this crate will +/// use them to lift the limitations of packed structs. For the duration of the +/// `1.x` version of this crate that will be behind an on-by-default cargo +/// feature (to maintain minimum rust version support). #[macro_export] macro_rules! offset_of { ($instance:expr, $Type:path, $field:tt) => {{ - // This helps us guard against field access going through a Deref impl. - #[allow(clippy::unneeded_field_pattern)] - let $Type { $field: _, .. }; - let reference: &$Type = &$instance; - let address = reference as *const _ as usize; - let field_pointer = &reference.$field as *const _ as usize; - // These asserts/unwraps are compiled away at release, and defend against - // the case where somehow a deref impl is still invoked. - let result = field_pointer.checked_sub(address).unwrap(); - assert!(result <= $crate::__core::mem::size_of::<$Type>()); - result + #[forbid(safe_packed_borrows)] + { + // This helps us guard against field access going through a Deref impl. + #[allow(clippy::unneeded_field_pattern)] + let $Type { $field: _, .. }; + let reference: &$Type = &$instance; + let address = reference as *const _ as usize; + let field_pointer = &reference.$field as *const _ as usize; + // These asserts/unwraps are compiled away at release, and defend against + // the case where somehow a deref impl is still invoked. + let result = field_pointer.checked_sub(address).unwrap(); + assert!(result <= $crate::__core::mem::size_of::<$Type>()); + result + } }}; ($Type:path, $field:tt) => {{ $crate::offset_of!(<$Type as Default>::default(), $Type, $field) diff --git a/tests/cast_slice_tests.rs b/tests/cast_slice_tests.rs index 1177a7f..9eb08c5 100644 --- a/tests/cast_slice_tests.rs +++ b/tests/cast_slice_tests.rs @@ -88,3 +88,78 @@ fn test_types() { let _: Result<&[i32], PodCastError> = try_cast_slice(&[1.0_f32]); let _: Result<&mut [i32], PodCastError> = try_cast_slice_mut(&mut [1.0_f32]); } + +#[test] +fn test_bytes_of() { + assert_eq!(bytes_of(&0xaabbccdd_u32), &0xaabbccdd_u32.to_ne_bytes()); + assert_eq!(bytes_of_mut(&mut 0xaabbccdd_u32), &mut 0xaabbccdd_u32.to_ne_bytes()); + let mut a = 0xaabbccdd_u32; + let a_addr = &a as *const _ as usize; + // ensure addresses match. + assert_eq!(bytes_of(&a).as_ptr() as usize, a_addr); + assert_eq!(bytes_of_mut(&mut a).as_ptr() as usize, a_addr); +} + +#[test] +fn test_try_from_bytes() { + let u32s = [0xaabbccdd, 0x11223344_u32]; + let bytes = bytemuck::cast_slice::(&u32s); + assert_eq!(try_from_bytes::(&bytes[..4]), Ok(&u32s[0])); + assert_eq!(try_from_bytes::(&bytes[..5]), Err(PodCastError::SizeMismatch)); + assert_eq!(try_from_bytes::(&bytes[..3]), Err(PodCastError::SizeMismatch)); + assert_eq!(try_from_bytes::(&bytes[1..5]), Err(PodCastError::TargetAlignmentGreaterAndInputNotAligned)); +} + +#[test] +fn test_try_from_bytes_mut() { + let mut abcd = 0xaabbccdd; + let mut u32s = [abcd, 0x11223344_u32]; + let bytes = bytemuck::cast_slice_mut::(&mut u32s); + assert_eq!(try_from_bytes_mut::(&mut bytes[..4]), Ok(&mut abcd)); + assert_eq!(try_from_bytes_mut::(&mut bytes[..4]), Ok(&mut abcd)); + assert_eq!(try_from_bytes_mut::(&mut bytes[..5]), Err(PodCastError::SizeMismatch)); + assert_eq!(try_from_bytes_mut::(&mut bytes[..3]), Err(PodCastError::SizeMismatch)); + assert_eq!(try_from_bytes::(&mut bytes[1..5]), Err(PodCastError::TargetAlignmentGreaterAndInputNotAligned)); +} + +#[test] +fn test_from_bytes() { + let abcd = 0xaabbccdd_u32; + let aligned_bytes = bytemuck::bytes_of(&abcd); + assert_eq!(from_bytes::(aligned_bytes), &abcd); + assert!(core::ptr::eq(from_bytes(aligned_bytes), &abcd)); +} + +#[test] +fn test_from_bytes_mut() { + let mut a = 0xaabbccdd_u32; + let a_addr = &a as *const _ as usize; + let aligned_bytes = bytemuck::bytes_of_mut(&mut a); + assert_eq!(*from_bytes_mut::(aligned_bytes), 0xaabbccdd_u32); + assert_eq!(from_bytes_mut::(aligned_bytes) as *const u32 as usize, a_addr); +} + +// like #[should_panic], but can be a part of another test, instead of requiring +// it to be it's own test. +macro_rules! should_panic { + ($ex:expr) => { + assert!( + std::panic::catch_unwind(|| { let _ = $ex; }).is_err(), + concat!("should have panicked: `", stringify!($ex), "`") + ); + }; +} + +#[test] +fn test_panics() { + should_panic!(cast_slice::(&[1u8, 2u8])); + should_panic!(cast_slice_mut::(&mut [1u8, 2u8])); + should_panic!(from_bytes::(&[1u8, 2])); + should_panic!(from_bytes::(&[1u8, 2, 3, 4, 5])); + should_panic!(from_bytes_mut::(&mut [1u8, 2])); + should_panic!(from_bytes_mut::(&mut [1u8, 2, 3, 4, 5])); + // use cast_slice on some u32s to get some align>=4 bytes, so we can know + // we'll give from_bytes unaligned ones. + let aligned_bytes = bytemuck::cast_slice::(&[0, 0]); + should_panic!(from_bytes::(&aligned_bytes[1..5])); +}