Fix Stacked Borrows UB when splicing an inline SmallVec - #449
Conversation
`SmallVec::splice` could trigger undefined behavior (an aliasing violation detected by Miri under Stacked Borrows) when the vector is stored inline. `Drain::fill` built a `&mut [T]` (`range_slice`) pointing into the inline buffer and then called `vec.set_len(..)` inside the fill loop. Because the inline buffer lives *inside* the `SmallVec`, calling a `&mut self` method reborrows the whole struct and so invalidates `range_slice` (and the `place` reference derived from it), making the next loop iteration read through a dangling tag. `Drain::move_tail` had the same problem: it took a `*const` via `vec.as_ptr()` and then reborrowed the whole `SmallVec` again through `vec.as_mut_ptr()`, invalidating the first pointer before `ptr::copy` used it. Both are fixed the way the standard library's `Drain::fill` avoids the issue: update the length by writing the `len` field directly instead of calling a method, and derive the copy's source and destination from a single base pointer. The bug only manifested while inline, which is why the existing `splice` test (using `SmallVec<u8, 1>`, always spilled) never caught it. Adds `tests::splice_inline`, which fails under `cargo miri test` before this change and passes after.
|
AI contributions are not allowed in any @servo repository as indicated on the contributing guidelines https://book.servo.org/contributing/getting-started.html#ai-contributions this PR will subsequently be closed |
|
@alejandro-vaz What signal did you use to decide this was an AI contribution? |
|
@jdm em-dashes, perfect syntax, the generic summary-implementation notes, and a comment in #444 saying it was literally a Codex agent and if you look at the code, agents tend to add tests for everything, helper functions where there should be none, and increase LOC way too much for what was necessary look at each individual one all these contributions started appearing when I labeled issues as "good first issue" and "help wanted" which is amazing for attracting contributors and people with agents correct me if I'm wrong but this looks really AI-made |
SmallVec::splicetriggers undefined behavior (a Stacked Borrows aliasing violation, caught bycargo miri test) when the vector is stored inline:Drain::fillbuilds a&mut [T]into the inline buffer and then callsvec.set_len(..)inside the fill loop. Since the inline buffer lives inside theSmallVec, a&mut selfmethod reborrows the whole struct and invalidates that slice (and theplacereference derived from it), so the next loop iteration retags from a dead tag:Drain::move_tailhas the same issue: it takes a*constviavec.as_ptr()and then reborrows the wholeSmallVecagain throughvec.as_mut_ptr(), invalidating the first pointer beforeptr::copyuses it. Both only manifest while inline, which is why the existingsplicetest (SmallVec<u8, 1>, always spilled) never caught it.Fixed the way std's
Drain::fillavoids it: update the length by writing thelenfield directly instead of calling a method, and derive the copy's source and destination from a single base pointer. Addstests::splice_inline, which fails undercargo miri testbefore this change and passes after. The full suite stays green under both Stacked Borrows and Tree Borrows.One note for your judgement: this is a Stacked Borrows violation — it passes under Tree Borrows, which tracks that
set_lenonly writes thelenfield. Both models are still experimental, but this is UB under the model Miri runs by default and thatscripts/run_miri.shuses in CI, it's reachable from entirely safe code, and holding a reference into a struct-embedded buffer across a&mut selfreborrow is the pattern std deliberately avoids. The fix costs nothing either way.