tesselode / kira

Library for expressive game audio.
https://crates.io/crates/kira
Apache License 2.0
866 stars 45 forks source link

Fix reference invalidation under stacked borrows in `arena::iter::IterMut` #106

Closed Imberflur closed 2 weeks ago

Imberflur commented 4 weeks ago

Hi! I was exploring the one unsafe line in this crate and found a small aspect to fix.

I added this test:

fn iter_mut_use_after_next() {
    let mut arena = Arena::new(2);
    let _ = arena.insert(1).unwrap();
    let _ = arena.insert(2).unwrap();
    let mut iter = arena.iter_mut();
    let first = iter.next().unwrap();
    let _second = iter.next().unwrap();
    *first.1 = 3;
}

Running with miri cargo +nightly miri test iter_mut_use_after_next shows:

test arena::test::iter_mut_use_after_next ... error: Undefined Behavior: attempting a write access using <512353> at alloc206762[0x18], but that tag does not exist in the borrow stack for this location
   --> crates/kira/src/arena/test.rs:278:2
    |
278 |     *first.1 = 3;
    |     ^^^^^^^^^^^^
    |     |
    |     attempting a write access using <512353> at alloc206762[0x18], but that tag does not exist in the borrow stack for this location
    |     this error occurs as part of an access at alloc206762[0x18..0x1c]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <512353> was created by a Unique retag at offsets [0x18..0x1c]
   --> crates/kira/src/arena/test.rs:275:14
    |
275 |     let first = iter.next().unwrap();
    |                 ^^^^^^^^^^^^^^^^^^^^
help: <512353> was later invalidated at offsets [0x0..0x30] by a Unique retag
   --> crates/kira/src/arena/iter.rs:77:36
    |
77  |             let slot = &mut self.arena.slots[index as usize];
    |                                             ^^^^^^^^^^^^^^^^
    = note: BACKTRACE (of the first span) on thread `arena::test::it`:
    = note: inside `arena::test::iter_mut_use_after_next` at crates/kira/src/arena/test.rs:278:2: 278:14

Notably things are fine with tree borrows MIRIFLAGS="-Zmiri-tree-borrows" cargo +nightly miri test iter_mut_use_after_next:

test arena::test::iter_mut_use_after_next ... ok

However, it is preferable to make things work with both models since the final model will likely be some mixture of these two.

The issue in stacked borrows is that creating the mutable reference to the slice (inside Vec's index impl) essentially asserts that it has unique access to the whole contents of the slice which invalidates any previously derived references.

We can avoid this by getting a raw pointer when creating the IterMut and exclusively using that pointer to derive references to the individual items. Raw pointer ergonomics aren't the best, but at least not having a reference to the Vec also makes it more difficult to accidentally introduce other code that uses the Vec in ways that would invalidate the references.

Imberflur commented 2 weeks ago

Changes look good to me.

Imberflur commented 2 weeks ago

included in 6ad44db793ccb12516c67bb054ca4d545df26cac