Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request to change ClearOnDrop to hold ManuallyDrop<P> rather than P. #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

emilHof
Copy link

@emilHof emilHof commented Dec 2, 2022

While I do not necessarily expect this to be merged, as this is maybe not the most urgent change, it does reduce the potential for UB in a couple of places.

The main reason for this change is the following original implementation:

pub fn into_uncleared_place(c: Self) -> P {
    unsafe {
        let place = ptr::read(&c._place);
        mem::forget(c);
        place
    }
}

When place is read into through a *const P it creates a Unique retag of the underlying value, which gets invalidated by the consuming call of mem::forget(c) as this causes a new Unique retag. While this is not inherently UB there it does violate some safety invariants that make unsafe code more predictable.

By changing ClearOnDrop to hold ManuallyDrop<P> rather than just P, we avoid having to forget c and thus need not invalided our borrow tag.

There since now we do not forget our ClearOnDrop, if we do not want our value to be cleared we zero out the ManuallyDrop<P> as follows:

pub fn into_uncleared_place(mut c: Self) -> P {
    unsafe {
        let place = ptr::read(&c._place);
        ptr::write_bytes(
            &mut c._place as *mut _ as *mut u8,
            0,
            mem::size_of::<ManuallyDrop<P>>(),
        );
        ManuallyDrop::into_inner(place)
    }
}

This case is then checked for in the drop implementation as such:

fn drop(&mut self) {
    let ptr = &mut self._place as *mut _ as *mut u8;
    unsafe {
        if (0..mem::size_of::<ManuallyDrop<P>>() as isize)
            .fold(0, |acc, i| acc + *ptr.offset(i) as i32)
            != 0
        {
            self.clear();
            ManuallyDrop::drop(&mut self._place);
        }
    }
}

These are the results of running cargo bench on the ub-drop-clear branch:

clear_on_drop_small     time:   [1.0799 ns 1.0837 ns 1.0877 ns]
                        change: [+0.6411% +0.9332% +1.2594%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high severe

clear_on_drop_medium    time:   [7.2033 ns 7.2378 ns 7.2762 ns]
                        change: [+1.2775% +1.7232% +2.2066%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe

clear_on_drop_large     time:   [196.54 ns 197.07 ns 197.66 ns]
                        change: [+0.6144% +1.0976% +1.5578%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 7 outliers among 100 measurements (7.00%)
  5 (5.00%) high mild
  2 (2.00%) high severe

I totally understand if you do not see this as a necessary addition, I had fun working on it anyways! :D

@emilHof emilHof changed the title Request to change ClearOnDrop to hold ManuallyDrop<P> rather than P. Request to change ClearOnDrop to hold ManuallyDrop<P> rather than P. Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant