Add more tests, overhaul CI, and fix plug soundness hole in bytemuck::offset_of! (#38)

* Ensure functions that take generic <A, B> don't call them <T, U>

* 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

@@ -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

src/lib.rs

@@ -278,7 +278,7 @@ pub fn cast_ref<A: Pod, B: Pod>(a: &A) -> &B {
   }
 }
 
-/// Cast `&[T]` into `&[U]`.
+/// Cast `&[A]` into `&[B]`.
 ///
 /// ## Panics
 ///
@@ -376,7 +376,7 @@ pub fn try_cast_mut<A: Pod, B: Pod>(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::<A>() == output.len() * size_of::<B>()`
@@ -411,7 +411,7 @@ pub fn try_cast_slice<A: Pod, B: Pod>(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`.

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,9 +54,65 @@
 /// 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
+///
+/// <p style="background:rgba(255,181,77,0.16);padding:0.75em;">
+/// <strong>Warning:</strong> This is only true for versions of bytemuck > 1.4.0.
+/// Previous versions of
+/// <code style="background:rgba(41,24,0,0.1);">bytemuck::offset_of!</code>
+/// will only emit a warning when used on the field of a packed struct in safe code,
+/// which can lead to unsoundness.
+/// </p>
+///
+/// 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) => {{
+    #[forbid(safe_packed_borrows)]
+    {
       // This helps us guard against field access going through a Deref impl.
       #[allow(clippy::unneeded_field_pattern)]
       let $Type { $field: _, .. };
@@ -78,6 +124,7 @@ macro_rules! offset_of {
       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)

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::<u32, u8>(&u32s);
+  assert_eq!(try_from_bytes::<u32>(&bytes[..4]), Ok(&u32s[0]));
+  assert_eq!(try_from_bytes::<u32>(&bytes[..5]), Err(PodCastError::SizeMismatch));
+  assert_eq!(try_from_bytes::<u32>(&bytes[..3]), Err(PodCastError::SizeMismatch));
+  assert_eq!(try_from_bytes::<u32>(&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::<u32, u8>(&mut u32s);
+  assert_eq!(try_from_bytes_mut::<u32>(&mut bytes[..4]), Ok(&mut abcd));
+  assert_eq!(try_from_bytes_mut::<u32>(&mut bytes[..4]), Ok(&mut abcd));
+  assert_eq!(try_from_bytes_mut::<u32>(&mut bytes[..5]), Err(PodCastError::SizeMismatch));
+  assert_eq!(try_from_bytes_mut::<u32>(&mut bytes[..3]), Err(PodCastError::SizeMismatch));
+  assert_eq!(try_from_bytes::<u32>(&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::<u32>(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::<u32>(aligned_bytes), 0xaabbccdd_u32);
+  assert_eq!(from_bytes_mut::<u32>(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::<u8, u32>(&[1u8, 2u8]));
+  should_panic!(cast_slice_mut::<u8, u32>(&mut [1u8, 2u8]));
+  should_panic!(from_bytes::<u32>(&[1u8, 2]));
+  should_panic!(from_bytes::<u32>(&[1u8, 2, 3, 4, 5]));
+  should_panic!(from_bytes_mut::<u32>(&mut [1u8, 2]));
+  should_panic!(from_bytes_mut::<u32>(&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::<u32, u8>(&[0, 0]);
+  should_panic!(from_bytes::<u32>(&aligned_bytes[1..5]));
+}