From 556beacb0e0023bc507090a918af173f43c8a447 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Tue, 10 Sep 2024 13:42:43 +0200 Subject: [PATCH 1/4] Validate AlignedVec::resize safety requirements This is a necessary check for soundness, as demonstrated by the test which can SIGSEGV without the check. Before the check, an overflow in the underlying buffer calculation can create incoherent state where the vector believes in an impossibly large buffer of the item type which is not actually backed by a correctly sized buffer of chunks. --- src/align.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/align.rs b/src/align.rs index a19d1ab83..103e783c0 100644 --- a/src/align.rs +++ b/src/align.rs @@ -141,6 +141,15 @@ pub struct AlignedVec { } impl AlignedVec { + // Note that in Rust, no single allocation can exceed `isize::MAX` __bytes_. + const MAX_LEN: usize = { + if core::mem::size_of::() == 0 { + usize::MAX + } else { + (isize::MAX as usize) / core::mem::size_of::() + } + }; + /// Must check in all constructors. const fn check_byte_chunk_type_is_aligned() { assert!(mem::size_of::() == mem::align_of::()); @@ -183,6 +192,14 @@ impl AlignedVec { } pub fn resize(&mut self, new_len: usize, value: T) { + // In addition to the obvious effect, this verifies the wrapping behavior of the + // `new_bytes` calculation. That can not overflow as the length limit does not overflow + // when multiplied with the size of `T`. Note that we one can still pass ludicrous + // requested buffer lengths, just not unsound ones. + assert!( + new_len <= Self::MAX_LEN, + "Resizing would overflow the underlying aligned buffer" + ); let old_len = self.len(); // Resize the underlying vector to have enough chunks for the new length. @@ -267,3 +284,17 @@ unsafe impl AsMutPtr for AlignedVec { self.len() } } + +#[test] +#[should_panic] +fn align_vec_fails() { + let mut v = AlignedVec::>::new(); + // This resize must fail. Otherwise, the code below creates a very small actual allocation, and + // consequently a slice reference that points to memory outside the buffer. + v.resize(isize::MAX as usize + 2, 0u16); + // Note that in Rust, no single allocation can exceed `isize::MAX` __bytes_. _Meaning it is + // impossible to soundly create a slice of `u16` with `isize::MAX` elements. If we got to this + // point, everything is broken already. The indexing will the probably also wrap and appear to + // work. + assert_eq!(v.as_slice()[isize::MAX as usize], 0); +} From 7275739e5e6e6707ae96701f62705bb77d746743 Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Wed, 18 Sep 2024 11:44:32 +0200 Subject: [PATCH 2/4] Address stylistic review comments --- src/align.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/align.rs b/src/align.rs index 103e783c0..fe8fd0e9a 100644 --- a/src/align.rs +++ b/src/align.rs @@ -141,12 +141,12 @@ pub struct AlignedVec { } impl AlignedVec { - // Note that in Rust, no single allocation can exceed `isize::MAX` __bytes_. + // Note that in Rust, no single allocation can exceed `isize::MAX` _bytes_. const MAX_LEN: usize = { - if core::mem::size_of::() == 0 { + if mem::size_of::() == 0 { usize::MAX } else { - (isize::MAX as usize) / core::mem::size_of::() + (isize::MAX as usize) / mem::size_of::() } }; @@ -292,7 +292,7 @@ fn align_vec_fails() { // This resize must fail. Otherwise, the code below creates a very small actual allocation, and // consequently a slice reference that points to memory outside the buffer. v.resize(isize::MAX as usize + 2, 0u16); - // Note that in Rust, no single allocation can exceed `isize::MAX` __bytes_. _Meaning it is + // Note that in Rust, no single allocation can exceed `isize::MAX` _bytes_. Meaning it is // impossible to soundly create a slice of `u16` with `isize::MAX` elements. If we got to this // point, everything is broken already. The indexing will the probably also wrap and appear to // work. From c26713193653120a02670562de4df102767c4d9f Mon Sep 17 00:00:00 2001 From: Andreas Molzer Date: Thu, 19 Sep 2024 12:02:37 +0200 Subject: [PATCH 3/4] Rewrite AligvedVec::resize check to checked_mul --- src/align.rs | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/src/align.rs b/src/align.rs index fe8fd0e9a..71b174166 100644 --- a/src/align.rs +++ b/src/align.rs @@ -141,15 +141,6 @@ pub struct AlignedVec { } impl AlignedVec { - // Note that in Rust, no single allocation can exceed `isize::MAX` _bytes_. - const MAX_LEN: usize = { - if mem::size_of::() == 0 { - usize::MAX - } else { - (isize::MAX as usize) / mem::size_of::() - } - }; - /// Must check in all constructors. const fn check_byte_chunk_type_is_aligned() { assert!(mem::size_of::() == mem::align_of::()); @@ -192,26 +183,25 @@ impl AlignedVec { } pub fn resize(&mut self, new_len: usize, value: T) { - // In addition to the obvious effect, this verifies the wrapping behavior of the - // `new_bytes` calculation. That can not overflow as the length limit does not overflow - // when multiplied with the size of `T`. Note that we one can still pass ludicrous - // requested buffer lengths, just not unsound ones. - assert!( - new_len <= Self::MAX_LEN, - "Resizing would overflow the underlying aligned buffer" - ); let old_len = self.len(); // Resize the underlying vector to have enough chunks for the new length. - // - // NOTE: We don't need to `drop` any elements if the `Vec` is truncated since `T: Copy`. - let new_bytes = mem::size_of::() * new_len; + // SAFETY: The `new_bytes` calculation must not overflow, ensuring a mathematical match + // with the underlying `inner` buffer size. NOTE: one can still pass ludicrous requested + // buffer lengths, just not unsound ones. + let Some(new_bytes) = mem::size_of::().checked_mul(new_len) else { + panic!("Resizing would overflow the underlying aligned buffer"); + }; + let chunk_size = mem::size_of::(); let new_chunks = if (new_bytes % chunk_size) == 0 { new_bytes / chunk_size } else { + // NOTE: can not overflow. This case only occurs on `chunk_size >= 2`. (new_bytes / chunk_size) + 1 }; + + // NOTE: We don't need to `drop` any elements if the `Vec` is truncated since `T: Copy`. self.inner.resize_with(new_chunks, MaybeUninit::uninit); // If we grew the vector, initialize the new elements past `len`. From 9464e27c09e159c09d215eef87a7a8f80a970c89 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Mon, 23 Sep 2024 02:02:27 -0700 Subject: [PATCH 4/4] `fn AlignedVec::resize`: Use `.expect` of `let else panic!`. --- src/align.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/align.rs b/src/align.rs index 71b174166..01bc7f68e 100644 --- a/src/align.rs +++ b/src/align.rs @@ -186,12 +186,12 @@ impl AlignedVec { let old_len = self.len(); // Resize the underlying vector to have enough chunks for the new length. - // SAFETY: The `new_bytes` calculation must not overflow, ensuring a mathematical match - // with the underlying `inner` buffer size. NOTE: one can still pass ludicrous requested - // buffer lengths, just not unsound ones. - let Some(new_bytes) = mem::size_of::().checked_mul(new_len) else { - panic!("Resizing would overflow the underlying aligned buffer"); - }; + // SAFETY: The `new_bytes` calculation must not overflow, + // ensuring a mathematical match with the underlying `inner` buffer size. + // NOTE: one can still pass ludicrous requested buffer lengths, just not unsound ones. + let new_bytes = mem::size_of::() + .checked_mul(new_len) + .expect("Resizing would overflow the underlying aligned buffer"); let chunk_size = mem::size_of::(); let new_chunks = if (new_bytes % chunk_size) == 0 {