`bevy_ptr::MovingPtr::assign_to` can cause double drops / use after free

[KyleDavidE]

Bevy version and features

bevy_ptr@0.18.1

Info

bevy_ptr::MovingPtr::assign_to doesn't continue to assign even if the drop glue panic like *dst = self.read() would. Thus if the panic is caught, you can access the partially dropped dst.

Not of immediate concern because it doesn't seem like bevy_ptr::MovingPtr::assign_to is actually used anywhere

repo code

use bevy_ptr::move_as_ptr;
use std::{
    panic::{AssertUnwindSafe, catch_unwind, resume_unwind},
    sync::atomic::{AtomicBool, Ordering},
};

#[derive(Debug)]
struct PanicOnDropOnce;

impl Drop for PanicOnDropOnce {
    fn drop(&mut self) {
        static DID: AtomicBool = AtomicBool::new(false);
        if !DID.fetch_or(true, Ordering::Relaxed) {
            // using resume unwind here to avoid calling panic hook to keep stderr clean
            resume_unwind(Box::new("intentional"))
        }
    }
}

fn main() {
    let mut dst = (String::from("dst"), PanicOnDropOnce);

    catch_unwind(AssertUnwindSafe(|| {
        let moving_ptr = (String::from("moving"), PanicOnDropOnce);
        move_as_ptr!(moving_ptr);

        moving_ptr.assign_to(&mut dst);
    }))
    .expect_err("Should have unwound");

    dbg!(dst);
}

expected

[src\main.rs:31:5] dst = (
    "moving",
    PanicOnDropOnce,
)

actual

UB (exact output varies run to run)

Additional information

Miri output

[src\main.rs:31:5] dst = (
error: Undefined Behavior: pointer not dereferenceable: alloc259 has been freed, so this pointer is dangling
    --> C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\vec\mod.rs:1833:13
     |
1833 |             &*core::intrinsics::aggregate_raw_ptr::<*const [T], _, _>(self.as_ptr(), self.len)
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
     |
     = 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
help: alloc259 was allocated here:
    --> src\main.rs:21:20
     |
  21 |     let mut dst = (String::from("dst"), PanicOnDropOnce);
     |                    ^^^^^^^^^^^^^^^^^^^
help: alloc259 was deallocated here:
    --> src\main.rs:27:9
     |
  27 |         moving_ptr.assign_to(&mut dst);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: stack backtrace:
             0: std::vec::Vec::<u8>::as_slice
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\vec\mod.rs:1833:13: 1833:95
             1: std::string::String::as_str
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\string.rs:1055:43: 1055:62
             2: <std::string::String as std::ops::Deref>::deref
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\string.rs:2823:9: 2823:22
             3: <std::string::String as std::fmt::Debug>::fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\string.rs:2720:26: 2720:32
             4: <&std::string::String as std::fmt::Debug>::fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\mod.rs:2866:62: 2866:82
             5: std::fmt::DebugTuple::<'_, '_>::field::{closure#0}
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\builders.rs:330:29: 330:41
             6: std::fmt::DebugTuple::<'_, '_>::field_with::<{closure@std::fmt::DebugTuple<'_, '_>::field::{closure#0}}>::{closure#0}
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\builders.rs:350:17: 350:39
             7: std::result::Result::<(), std::fmt::Error>::and_then::<(), {closure@std::fmt::DebugTuple<'_, '_>::field_with<{closure@std::fmt::DebugTuple<'_, '_>::field::{closure#0}}>::{closure#0}}>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\result.rs:1493:22: 1493:27
             8: std::fmt::DebugTuple::<'_, '_>::field_with::<{closure@std::fmt::DebugTuple<'_, '_>::field::{closure#0}}>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\builders.rs:342:23: 357:11
             9: std::fmt::DebugTuple::<'_, '_>::field
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\builders.rs:330:9: 330:42
             10: <(std::string::String, PanicOnDropOnce) as std::fmt::Debug>::fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\mod.rs:3087:25: 3087:46
             11: <&(std::string::String, PanicOnDropOnce) as std::fmt::Debug>::fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\mod.rs:2866:62: 2866:82
             12: <&dyn std::fmt::Debug as std::fmt::Debug>::fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\mod.rs:2866:62: 2866:82
             13: core::fmt::rt::Argument::<'_>::fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\rt.rs:152:76: 152:95
             14: std::fmt::write
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\fmt\mod.rs:1735:17: 1735:83
             15: std::io::default_write_fmt::<std::io::StderrLock<'_>>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\io\mod.rs:639:11: 639:40
             16: <std::io::StderrLock<'_> as std::io::Write>::write_fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\io\mod.rs:1994:13: 1994:42
             17: <&std::io::Stderr as std::io::Write>::write_fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\io\stdio.rs:1066:9: 1066:36
             18: <std::io::Stderr as std::io::Write>::write_fmt
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\io\stdio.rs:1040:9: 1040:33
             19: std::io::stdio::print_to::<std::io::Stderr>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\io\stdio.rs:1164:21: 1164:47
             20: std::io::_eprint
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\io\stdio.rs:1286:5: 1286:37
             21: main
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\macros.rs:180:9: 180:60
             22: <fn() as std::ops::FnOnce<()>>::call_once - shim(fn())
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:250:5: 250:71
             23: std::sys::backtrace::__rust_begin_short_backtrace::<fn(), ()>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\backtrace.rs:166:18: 166:21
             24: std::rt::lang_start::<()>::{closure#0}
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:206:18: 206:75
             25: std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:287:13: 287:31
             26: std::panicking::catch_unwind::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:581:40: 581:43
             27: std::panicking::catch_unwind::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:544:19: 544:88
             28: std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:359:14: 359:40
             29: std::rt::lang_start_internal::{closure#0}
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:175:24: 175:49
             30: std::panicking::catch_unwind::do_call::<{closure@std::rt::lang_start_internal::{closure#0}}, isize>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:581:40: 581:43
             31: std::panicking::catch_unwind::<isize, {closure@std::rt::lang_start_internal::{closure#0}}>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:544:19: 544:88
             32: std::panic::catch_unwind::<{closure@std::rt::lang_start_internal::{closure#0}}, isize>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:359:14: 359:40
             33: std::rt::lang_start_internal
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:171:5: 193:7
             34: std::rt::lang_start::<()>
                 at C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:205:5: 210:6

error: aborting due to 1 previous error

error: process didn't exit successfully: `C:\Users\kylee\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin\cargo-miri.exe runner target\miri\x86_64-pc-windows-msvc\debug\bevy-ptr-ub.exe` (exit code: 1)

[KyleDavidE]

I realized you don't actually need the catch unwind to observe this. This setup will double drop the string "dst".

use bevy_ptr::move_as_ptr;
use std::sync::atomic::{AtomicBool, Ordering};

#[derive(Debug)]
struct PanicOnDropOnce;

impl Drop for PanicOnDropOnce {
    fn drop(&mut self) {
        static DID: AtomicBool = AtomicBool::new(false);
        if !DID.fetch_or(true, Ordering::Relaxed) {
            panic!("intentional")
        }
    }
}

fn main() {
    let mut dst = (String::from("dst"), PanicOnDropOnce);

    let moving_ptr = (String::from("moving"), PanicOnDropOnce);
    move_as_ptr!(moving_ptr);
    moving_ptr.assign_to(&mut dst);
}

[alice-i-cecile]

I'm inclined to just cut this: we should not have unsoundness, and frankly we should very much minimize the extent of dead unsafe code we expose as well.

[KyleDavidE]

Fair enough. Since it is unused the best option would probably be to just replace it with *dst = self.read() if you want to avoid api changes and get this fixed asap. I wouldn't be surprised if that optimized to the correct thing anyways.