Manishearth/triomphe

`Arc::<MaybeUninit<T>>::write` is UB

WaffleLapkin opened this issue · 1 comments

write is defined like this:

impl<T> Arc<MaybeUninit<T>> {
    pub fn write(&mut self, val: T) -> &mut T {
        unsafe {
            let ptr = (*self.ptr()).data.as_mut_ptr();
            ptr.write(val);
            &mut *ptr
        }
    }
}

It writes to the inner value of the Arc even though unique access to Arc does not guarantee unique access to the inner value (b/c of clones). Here is a simple test case that fails under miri:

let mut uninit = Arc::new_uninit();
let clone = uninit.clone();

let x: &MaybeUninit<String> = &*clone;

// This write invalidates `x` reference
uninit.write(String::from("nonononono"));

// Read invalidated reference to trigger UB
let _ = &*x;
error: Undefined Behavior: trying to reborrow <205779> for SharedReadOnly permission at alloc89625[0x8], but that tag does not exist in the borrow stack for this location
   --> src/lib.rs:107:5
    |
107 |     &*_x;
    |     ^^^^
    |     |
    |     trying to reborrow <205779> for SharedReadOnly permission at alloc89625[0x8], but that tag does not exist in the borrow stack for this location
    |     this error occurs as part of a reborrow at alloc89625[0x8..0x20]
    |
    = 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: <205779> was created by a retag at offsets [0x8..0x20]
   --> src/lib.rs:103:36
    |
103 |     let _x: &MaybeUninit<String> = &*clone;
    |                                    ^^^^^^^
help: <205779> was later invalidated at offsets [0x8..0x20]
   --> src/arc.rs:247:23
    |
247 |             let ptr = (*self.ptr()).data.as_mut_ptr();
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: inside `ub` at src/lib.rs:107:5
note: inside closure at src/lib.rs:97:1
   --> src/lib.rs:97:1
    |
96  |   #[test]
    |   ------- in this procedural macro expansion
97  | / fn ub() {
98  | |     use core::mem::MaybeUninit;
99  | |
100 | |     let mut uninit = Arc::new_uninit();
...   |
107 | |     &*_x;
108 | | }
    | |_^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

Good catch, thanks! That's an older API that should have just been removed, yeah, deprecating it and making it panicky is fine for now.