diff --git a/src/lib.rs b/src/lib.rs index f7b1bba..6a45c5b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -492,6 +492,9 @@ impl Drain<'_, T, N> { let vec = unsafe { self.vec.as_mut() }; let range_start = vec.len(); let range_end = self.tail_start; + // Whether the buffer lives on the heap does not change while filling, so + // read it once, before `range_slice` is created. + let on_heap = vec.len.on_heap(); let range_slice = unsafe { core::slice::from_raw_parts_mut( vec.as_mut_ptr().add(range_start), @@ -499,10 +502,20 @@ impl Drain<'_, T, N> { ) }; + let mut len = range_start; for place in range_slice { if let Some(new_item) = replace_with.next() { unsafe { core::ptr::write(place, new_item) }; - vec.set_len(vec.len() + 1); + len += 1; + // Update the length by writing the `len` field directly rather + // than calling `vec.set_len(..)`. Calling a `&mut self` method + // reborrows the whole `SmallVec`; when the buffer is inline it + // aliases `range_slice`, so that reborrow would invalidate + // `range_slice` (and the `place` reference derived from it) under + // Stacked Borrows, causing undefined behavior on the next + // iteration. This mirrors the standard library's `Drain::fill`, + // which bumps `vec.len` directly for the same reason. + vec.len = TaggedLen::new(len, on_heap); } else { return false; } @@ -524,8 +537,14 @@ impl Drain<'_, T, N> { let new_tail_start = self.tail_start + additional; unsafe { - let src = vec.as_ptr().add(self.tail_start); - let dst = vec.as_mut_ptr().add(new_tail_start); + // Derive both the source and destination from a single mutable base + // pointer. Taking a `*const` via `vec.as_ptr()` and then reborrowing + // the whole `SmallVec` again via `vec.as_mut_ptr()` would invalidate + // the former under Stacked Borrows when the buffer is inline (the + // buffer aliases the `SmallVec` that `as_mut_ptr` reborrows). + let base = vec.as_mut_ptr(); + let src = base.add(self.tail_start); + let dst = base.add(new_tail_start); core::ptr::copy(src, dst, self.tail_len); } self.tail_start = new_tail_start; diff --git a/src/tests.rs b/src/tests.rs index cfd801b..be374fd 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -165,6 +165,51 @@ fn splice() { assert_eq!(u, [0, 1, 2]); } +// Splicing an *inline* `SmallVec` used to trigger undefined behavior (a Stacked +// Borrows aliasing violation): `Drain::fill` and `Drain::move_tail` held a +// reference/pointer into the inline buffer while reborrowing the whole +// `SmallVec` (via `set_len`/`as_mut_ptr`), which aliases that buffer. All of +// these cases keep the vector inline (`N` is large), so they exercise the fix. +// Run under `cargo miri test` to detect a regression. +#[test] +fn splice_inline() { + // more replacements than removed -> fill + move_tail + fill + let mut v: SmallVec = smallvec![1, 2, 3, 4, 5]; + let u: SmallVec = v.splice(1..3, [10, 11, 12]).collect(); + assert!(!v.spilled()); + assert_eq!(u, [2, 3]); + assert_eq!(v, [1, 10, 11, 12, 4, 5]); + + // equal count -> fill only + let mut v: SmallVec = smallvec![1, 2, 3, 4, 5]; + let u: SmallVec = v.splice(1..3, [20, 21]).collect(); + assert!(!v.spilled()); + assert_eq!(u, [2, 3]); + assert_eq!(v, [1, 20, 21, 4, 5]); + + // fewer replacements -> partial fill, tail moved back on drop + let mut v: SmallVec = smallvec![1, 2, 3, 4, 5, 6]; + let u: SmallVec = v.splice(1..4, [30]).collect(); + assert!(!v.spilled()); + assert_eq!(u, [2, 3, 4]); + assert_eq!(v, [1, 30, 5, 6]); + + // empty tail, growing -> move_tail with tail_len == 0 + let mut v: SmallVec = smallvec![1, 2, 3]; + let u: SmallVec = v.splice(2..3, [40, 41, 42, 43]).collect(); + assert!(!v.spilled()); + assert_eq!(u, [3]); + assert_eq!(v, [1, 2, 40, 41, 42, 43]); + + // loose size_hint -> "collect the remainder" branch of Splice::drop + let mut v: SmallVec = smallvec![1, 2, 3, 4, 5]; + let repl = (100..110).filter(|x| x % 2 == 0); + let u: SmallVec = v.splice(1..3, repl).collect(); + assert!(!v.spilled()); + assert_eq!(u, [2, 3]); + assert_eq!(v, [1, 100, 102, 104, 106, 108, 4, 5]); +} + #[test] fn into_iter() { let mut v: SmallVec = SmallVec::new();