From 4b5efec7910158bc10c43a3015f69bfb299b832b Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Sat, 15 Oct 2022 02:27:31 +0000 Subject: [PATCH] Fix unsoundness in FromBytes::new_box_slice_zeroed Previously, `FromBytes::new_box_slice_zeroed` called `Layout::from_size_align_unchecked` with arguments that could overflow `isize`, which would violate its safety preconditions. In this change, we use the safe variant, which returns a `Result` which we can `.expect()`. Though I haven't benchmarked it, this likely has little impact on performance over the optimal code since optimal code would still need to perform the same bounds check that `from_size_align` is performing. --- Cargo.toml | 1 + src/lib.rs | 51 +++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 76aea47fcb..223653bb3d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,4 +32,5 @@ default-features = false [dev-dependencies] rand = "0.6" +rustversion = "1.0" trybuild = "1.0" diff --git a/src/lib.rs b/src/lib.rs index a30c82c470..7fb32c409e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -358,17 +358,14 @@ pub unsafe trait FromBytes { { // TODO(#2): Use `Layout::repeat` when `alloc_layout_extra` is // stabilized. - // - // This will intentionally panic if it overflows. + let layout = Layout::from_size_align( + mem::size_of::() + .checked_mul(len) + .expect("mem::size_of::() * len overflows `usize`"), + mem::align_of::(), + ) + .expect("total allocation size overflows `isize`"); unsafe { - // SAFETY: `from_size_align_unchecked` is sound because - // slice_len_bytes is guaranteed to be properly aligned (we just - // multiplied it by `size_of::()`, which is guaranteed to be - // aligned). - let layout = Layout::from_size_align_unchecked( - mem::size_of::().checked_mul(len).unwrap(), - mem::align_of::(), - ); if layout.size() != 0 { let ptr = alloc::alloc::alloc_zeroed(layout) as *mut Self; if ptr.is_null() { @@ -2204,6 +2201,40 @@ mod alloc_support { let s: Box<[()]> = <()>::new_box_slice_zeroed(0); assert_eq!(s.len(), 0); } + + #[test] + #[should_panic(expected = "mem::size_of::() * len overflows `usize`")] + fn test_new_box_slice_zeroed_panics_mul_overflow() { + let _ = u16::new_box_slice_zeroed(usize::MAX); + } + + // This test fails on stable <= 1.64.0, but succeeds on 1.65.0-beta.2 + // (2022-09-13). In particular, on stable <= 1.64.0, + // `new_box_slice_zeroed` attempts to perform the allocation (which of + // course fails). `Layout::from_size_align` should be returning an error + // due to `isize` overflow, but it doesn't. I (joshlf) haven't + // investigated enough to confirm, but my guess is that this was a bug + // that was fixed recently. + // + // Triggering the bug requires calling `FromBytes::new_box_slice_zeroed` + // with an allocation which overflows `isize`, and all that happens is + // that the program panics due to a failed allocation. Even on 32-bit + // platforms, this requires a 2GB allocation. On 64-bit platforms, this + // requires a 2^63-byte allocation. In both cases, even a slightly + // smaller allocation that didn't trigger this bug would likely + // (absolutely certainly in the case of 64-bit platforms) fail to + // allocate in exactly the same way regardless. + // + // Given how minor the impact is, and given that the bug seems fixed in + // 1.65.0, I've chosen to just release the code as-is and disable the + // test on buggy toolchains. Once our MSRV is at or above 1.65.0, we can + // remove this conditional compilation (and this comment) entirely. + #[rustversion::since(1.65.0)] + #[test] + #[should_panic(expected = "total allocation size overflows `isize`: LayoutError")] + fn test_new_box_slice_zeroed_panics_isize_overflow() { + let _ = u16::new_box_slice_zeroed((isize::MAX as usize / mem::size_of::()) + 1); + } } }