mirror of
https://github.com/bevyengine/bevy.git
synced 2026-07-06 02:23:17 -04:00
85a87c4603
# Objective - `MovingPtr::assign_to` is currently unsound if the destructor of `*dst` panics because it will leave it populated with the old value whose destructor panicked. When the owner of `*dst` then drops it it will result in a double drop. - Fix this unsoundness, fixes #23500 ## Solution - Use a drop guard to ensure that no matter what happens when dropping `*dst`, it will be overwritten with the value from `self.` This has the same semantics as normal assignments except it will prevent generating multiple `memcpy`s. - ~~Rewrite the method to use a normal assignment to the reference so that the language itself takes care of this issue.~~ This ended up making two `memcpy`s ## Testing - A regression test has been added
53 lines
1.6 KiB
Rust
53 lines
1.6 KiB
Rust
//! Tests for [`bevy_ptr::MovingPtr`] with panic behavior.
|
|
|
|
#![allow(unsafe_code, reason = "unsafe is needed to use bevy_ptr::MovingPtr")]
|
|
|
|
use bevy_ptr::MovingPtr;
|
|
|
|
use core::cell::Cell;
|
|
use core::mem::MaybeUninit;
|
|
use core::panic::AssertUnwindSafe;
|
|
|
|
use std::panic::catch_unwind;
|
|
|
|
#[test]
|
|
fn moving_ptr_assign_drop_is_unwind_safe() {
|
|
struct IncAndPanicOnDrop<'a>(&'a Cell<u32>);
|
|
impl<'a> Drop for IncAndPanicOnDrop<'a> {
|
|
fn drop(&mut self) {
|
|
self.0.set(self.0.get() + 1);
|
|
|
|
// Panic, but avoid double-panics to avoid aborts.
|
|
if !std::thread::panicking() {
|
|
panic!();
|
|
}
|
|
}
|
|
}
|
|
|
|
let drops1 = Cell::new(0);
|
|
let drops2 = Cell::new(0);
|
|
|
|
let mut value1 = MaybeUninit::new(IncAndPanicOnDrop(&drops1));
|
|
let mut value2 = IncAndPanicOnDrop(&drops2);
|
|
|
|
_ = catch_unwind(AssertUnwindSafe(|| {
|
|
// SAFETY:
|
|
// - value1 is initialized
|
|
// - we're not using value1 after this point.
|
|
let moving_ptr = unsafe { MovingPtr::from_value(&mut value1) };
|
|
|
|
// This should drop value2 and overwrite it with value1 no matter what happen.
|
|
// If the overwrite doesn't happen then it is unsound and the second pair of asserts will fail.
|
|
moving_ptr.assign_to(&mut value2);
|
|
}));
|
|
|
|
assert_eq!(drops1.get(), 0);
|
|
assert_eq!(drops2.get(), 1);
|
|
|
|
// Now drop value2, which should now hold value1. We expect this to drop value1 and increase drops1.
|
|
_ = catch_unwind(AssertUnwindSafe(|| drop(value2)));
|
|
|
|
assert_eq!(drops1.get(), 1);
|
|
assert_eq!(drops2.get(), 1);
|
|
}
|