Correctness of "untouched" mutable borrow of a value in post-drop state

Consider this module:

mod foo {
    use std::{mem, ptr};

    pub struct Foo<'a, T>(&'a mut T);

    impl<'a, T> Foo<'a, T> {
        pub fn new(slot: &'a mut mem::MaybeUninit<T>, val: T) -> Self {
            Foo(slot.write(val))
        }
    }

    impl<T> Drop for Foo<'_, T> {
        fn drop(&mut self) {
            unsafe { ptr::drop_in_place(self.0) }
        }
    }
}

Is this module safe to use?

Now notice, every field of a struct must be valid even after this struct's Drop::drop call, so the mutable borrow from the sample should be "invalid", as it may point to an invalid (freed) value of a type.

However, someone argued to me that drop_in_place simply calls every needed Drop::drop, which just leaves the pointee in post-drop state, and it is allowed to have a mutable borrow to this kind of value as long as it is unused, because while Drop::drop is executed, its &mut self argument lives. But you may remember that Drop::drop does not drop fields of the struct, so the argument makes another jump and says that because mutable borrow of every field is valid, could/should indicate that &mut self is valid too. Packed structs may challenge that assumption, as the field is moved.

So is this code valid after all? Also, if sample have used ManuallyDrop, i believe there would be no such questions as these, but that would be the "recovery" option.

EDIT
If the sample is safe tho, should it be mentioned in unsafe guidelines or smth? Or should it be not? I am afraid it can introduce an unnecessary rules bloat for you heads to keep in mind, while this can be mitigated via ManuallyDrop. On the other hand i am afraid of code being wrong just for some meta reasons. Anyway should i make a pr when i see this in someones code? Should i prefer to use ManuallyDrop there?

2 Likes

I'm tending slightly towards saying it's safe, after all dropping a &mut T is a no-op, so the field isn't touched anymore after the Drop implementation is called; and no-longer-used &mut T values can even e. g. still exist in a variable on the stack after they're no longer valid.

I may have seen a previous discussion of a similar question before, I'll look if I can find that. Edit: Nevermind, I either misremembered or cannot find the right place.

2 Likes

I've discussed the issue with Ralf on Zulip. This was in the context of ManuallyDrop::drop() being sound to call within Drop::drop(). For reference, here was the relevant code:

pub struct P<T: ?Sized> {
    ptr: ManuallyDrop<Box<T>>,
}

/* ... */

impl<T: ?Sized> Drop for P<T> {
    fn drop(&mut self) {
        ensure_sufficient_stack(|| unsafe { ManuallyDrop::drop(&mut self.ptr) });
    }
}

Here, we capture &mut self in a FnOnce closure, then invalidate it with ManuallyDrop::drop().

My discussion with @quinedot regarding ManuallyDrop, SB, and validity may also be relevant here.

I think perhaps we can declare invalid pointees to be "deprecated"? Like it will work in practice, but it should be avoided and it may be unstable.

I assume ManuallyDrop::drop() is valid to call within Drop::drop().

Indeed, but I'm pretty sure this is part of a larger pattern: the targets of mutable references don't have to be valid if they're never touched again. Therefore, you can invalidate the &mut self in Drop::drop(), since the remaining drop_in_place() glue accesses self only by pointer (except for fields with their own Drop impls). In this sense, ManuallyDrop isn't really special: it's invalid to touch a ManuallyDrop<T> that has been ManuallyDrop::drop()ped if T has special validity requirements. For instance, a ManuallyDrop<Box<T>> cannot be touched after being dropped, since a Box must always point to allocated memory (see rust-lang/unsafe-code-guidelines#245).

I think to deal with this situation currently i would recommend to add miri to the project's CI, which would fail if this practice is decided to be banned. I assume it is relatively easy implement this check for MIRI.

That's certainly not sufficient, because Miri has false negatives (i.e., if it detects UB then it's surely UB, but if it doesn't detect anything, the code may still be UB).


From what I can tell, all authoritative sources on unsafe have always asserted that references must point to valid, initialized, usable values, and violating this hard requirement in any way instantly results in UB. I.e., the soundness of a reference isn't affected by whether it's used; its mere existence means that it must point to a valid value. Therefore, I would say that the code above is definitely not sound, strictly speaking.

I agree that this is unfortunate, but I think this is a subtlety that is necessary, i.e., the implementation above is naïve. A correct implementation would be:

mod foo {
    use core::ops::{Deref, DerefMut};
    use core::mem::MaybeUninit;

    pub struct Foo<'a, T>(&'a mut MaybeUninit<T>);

    impl<'a, T> Foo<'a, T> {
        pub fn new(slot: &'a mut MaybeUninit<T>, val: T) -> Self {
            slot.write(val);
            Foo(slot)
        }
    }

    impl<T> Drop for Foo<'_, T> {
        fn drop(&mut self) {
            unsafe {
                self.0.assume_init_drop();
            }
        }
    }
    
    impl<T> Deref for Foo<'_, T> {
        type Target = T;
        
        fn deref(&self) -> &Self::Target {
            unsafe {
                self.0.assume_init_ref()
            }
        }
    }
    
    impl<T> DerefMut for Foo<'_, T> {
        fn deref_mut(&mut self) -> &mut Self::Target {
            unsafe {
                self.0.assume_init_mut()
            }
        }
    }
}

I.e., wrap the &mut MaybeUninit itself, because a &mut MaybeUninit remains valid by definition, even if the underlying value is uninitialized/dropped. Then, implement Deref[Mut] for convenience, since the private field and the constructor ensure that the wrapped MaybeUninit is always initialized.

I have known it all already. All i am saying MIRI isn't an immutable abstract, i believe we can add validity of a borrow after drop check to disrupt projects doing this thing, if they run MIRI in their CI.

Stacked Borrows doesn't really have a concept of a value "existing" in the abstract. Instead, validity is always asserted when a value is touched in the MIR, e.g., when a pointer is retagged, or a bool variable is read from or written to. In Rust, this roughly corresponds to naming the variable in either a place or value context following any given (lexical) point. To illustrate (Rust Playground):

fn main() {
    let mut x = false;
    let ptr = &mut x as *mut _ as *mut u8;
    unsafe { *ptr = 2 };
    //println!("{x}");
}

This program runs in Miri without errors. But if we uncomment the println!(), x is used after being invalidated, and Miri accordingly emits an error:

error: Undefined Behavior: type validation failed: encountered 0x02, but expected a boolean
    --> /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2398:25
     |
2398 |         Display::fmt(if *self { "true" } else { "false" }, f)
     |                         ^^^^^ type validation failed: encountered 0x02, but expected a boolean
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
             
     = note: inside `<bool as std::fmt::Display>::fmt` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2398:25
     = note: inside `std::fmt::write` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:1198:17
     = note: inside `<std::io::StdoutLock as std::io::Write>::write_fmt` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/mod.rs:1672:15
     = note: inside `<&std::io::Stdout as std::io::Write>::write_fmt` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:711:9
     = note: inside `<std::io::Stdout as std::io::Write>::write_fmt` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:685:9
     = note: inside `std::io::stdio::print_to::<std::io::Stdout>` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1014:21
     = note: inside `std::io::_print` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1027:5
note: inside `main` at /home/lm978/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/macros.rs:106:9
    --> src/main.rs:5:5
     |
5    |     println!("{x}");
     |     ^^^^^^^^^^^^^^^
     = note: this error originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

This can be viewed as a natural extension of the borrow checker's behavior in safe code. We can invalidate a &mut reference by creating another, but it only becomes an error when we attempt to touch the first reference again (Rust Playground):

fn main() {
    let mut x = 42;
    let _r1 = &mut x;
    let _r2 = &mut x;
    // error[E0499]: cannot borrow `x` as mutable more than once at a time
    //println!("{_r1}");
}

Thus, I'd argue that the original code is sound, since nothing can touch self or self.0 after the drop_in_place(). No MaybeUninit workaround is necessary.