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

RFC: Add std::mem::zero #2291

Closed
wants to merge 3 commits into from
Closed

RFC: Add std::mem::zero #2291

wants to merge 3 commits into from

Conversation

nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Jan 14, 2018

Rendered

This is a functionality that's already possible in stable Rust, but I think it's well worth adding to libcore and transitively to libstd.

I would also like to be the one to implement this feature.

@SimonSapin
Copy link
Contributor

Note that intrinsics::write_bytes is available on stable Rust at ptr::write_bytes, so this three-line function can be defined outside of the standard library.

@nvzqz
Copy link
Contributor Author

nvzqz commented Jan 14, 2018

This is added functionality that I think complements mem::zeroed nicely. I'm aware this is available in stable Rust. There are plenty of small functions in the standard library, though. I wrote this to gauge community opinion on whether this seems like a useful addition (to me it is).

@iitalics
Copy link

Seems an opportunity for lots of UB

pub unsafe fn zero<T: ?Sized>(val: &mut T) {
let len = mem::size_of_val(val);
let ptr = val as *mut T as *mut u8;
intrinsics::write_bytes(ptr, 0, len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nvzqz It'd be nice to at least change this to ptr::write_bytes.

@Mark-Simulacrum Mark-Simulacrum added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jan 15, 2018
@mgattozzi
Copy link
Contributor

@iitalics in the cases of mem::zeroed and mem::unitialized UB is quite possible thus the functions are marked as unsafe and the documentation with them describes how to use it without causing UB. Adding another function like this really isn't a problem in that sense as long as it is marked as unsafe and it is documented on how to use it safely. These functions have a place when doing things with FFI or other low level operations. This is just an extension of that.

@mark-i-m
Copy link
Member

will work with both slices and trait objects

Is this intended as a good thing? I might be wrong, but it seems like zeroing a trait object is always undefined behavior, right? Why would you want to zero a vtable?

@eddyb
Copy link
Member

eddyb commented Jan 15, 2018

Why would you want to zero a vtable?

The vtable pointer isn't in the trait object data though, it's in the fat pointer, next to the data pointer.

@mark-i-m
Copy link
Member

So zeroing a trait object does not mean this?

let x: &mut Any;
unsafe { mem::zero(x) };

@eddyb
Copy link
Member

eddyb commented Jan 15, 2018

@mark-i-m that's distinct from mem::zero(&mut x) (which would null the data and vtable pointers).

@notriddle
Copy link
Contributor

notriddle commented Jan 17, 2018

Can a post-monomorphization lint check if mem::zero is being run on a &mut N where N: NonZero<T> (including, but not limited to, zeroing out a reference)?

Being able to do that seems like a good reason to include in in the standard library. Because that seems super easy to get wrong.

@bstrie
Copy link
Contributor

bstrie commented Jan 17, 2018

Even if this can be expressed outside of the stdlib, one of the primary features of the stdlib is to give some assurance that unsafe functions have been considered and vetted. When it comes to unsafe utility functions I am very much in favor of stdlib maximalism--as long as someone finds it useful, that is.

That said, this function as proposed doesn't actually guarantee that the data is zeroed, correct? If not, then cryptography implementations will find this function lacking. Is that worth supporting?

@eddyb
Copy link
Member

eddyb commented Jan 17, 2018

We could have a volatile_zero function for the crypto usecase.

@scottmcm
Copy link
Member

one of the primary features of the stdlib is to give some assurance that unsafe functions have been considered and vetted

I'm not convinced by that for unsafe functions. I find the checking of the safe APIs that use unsafe internally incredibly valuable (like Ralf's work on Mutex), but the value seems much lower for an unsafe function that fairly-simply wraps another unsafe function.

@nvzqz
Copy link
Contributor Author

nvzqz commented Jan 18, 2018

If the function is already unsafe, does it make sense to require a mutable reference instead of a raw pointer? And if using a raw pointer makes more sense, should this (or these) function(s) be in std::ptr instead?

@kennytm
Copy link
Member

kennytm commented Jan 18, 2018

The &mut &mut T problem can be delegated to clippy: if we write let x = zeroed() or zero(&mut x) where x has a non-zeroable type (&T, &mut T, Box<T>), issue a warning. I don't think this needs to be in rustc.

Or you could create an auto trait...

auto trait MaybeZeroSafe {}
impl<T: Zeroable> !MaybeZeroSafe for NonZero<T> {}
impl<T: ?Sized> !MaybeZeroSafe for &T {}
impl<T: ?Sized> !MaybeZeroSafe for &mut T {}
impl !MaybeZeroSafe for /*** all kinds of function pointers ***/ {}

unsafe fn zero<T: ?Sized + MaybeZeroSafe>(x: &mut T) {}

... this feels extremely heavy-handed though.

@bstrie
Copy link
Contributor

bstrie commented Jan 20, 2018

one of the primary features of the stdlib is to give some assurance that unsafe functions have been considered and vetted

I'm not convinced by that for unsafe functions. I find the checking of the safe APIs that use unsafe internally incredibly valuable

@scottmcm I consider these the same concern though: unsafe code is unsafe code. The difference is just that a safe API has zero invariants that callers must uphold, and an unsafe API has one or more invariants that callers must uphold; if the unsafe code inside an unsafe function is incorrectly implemented, there may secretly exist undocumented invariants that can result in surprising cases of memory unsafety. Just because a function is marked as unsafe, it doesn't mean that it can't still be inherently safer than other functions marked unsafe. :)

@Restioson
Copy link

unsafe API has one or more invariants that callers must uphold

While this is true, unsafe code should still try to be as safe as possible. In this case, making it take *mut T stops it from violating the guarantee that &mut T must point to valid data for a type.

@SimonSapin
Copy link
Contributor

The libs team discussed this today and we felt that this is too niche to be part of the standard library:

@rfcbot fcp close

Especially given that:

  • This is literally a three-line function that can be defined outside of the standard library (std::ptr::write_bytes is stable, and now also as a <*mut T>::write_bytes method)
  • That function is still an unsafe fn, so it doesn’t remove much safety hazard from callers

With manual inlining, the example in the RFC simplifies to one line:

fn clear_bytes(slice: &mut [u8]) {
    unsafe { slice.as_mut_ptr().write_bytes(0, slice.len()) }
}

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 8, 2018

Team member @SimonSapin has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Aug 8, 2018
@Restioson
Copy link

Personally, I want this. I found myself causing a stack overflow with

*ptr = mem::zeroed::<VeryLargeT>();

Naturally, I next looked for mem::zero(ptr); and found it not to exist. I think that it does complement mem::zeroed, although it is quite niche. In the case that it is not included in core, where should it go?

@dtolnay
Copy link
Member

dtolnay commented Aug 8, 2018

@Restioson with write_bytes I would write this as:

ptr.write_bytes(0u8, 1);

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 9, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Aug 9, 2018
@Restioson
Copy link

@dtolnay I did settle for that in the end, but it feels like something that "you shouldn't have to define yourself" although you can.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Aug 19, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 19, 2018

The final comment period, with a disposition to close, as per the review above, is now complete.

By the power vested in me by Rust, I hereby close this RFC.

@rfcbot rfcbot added closed This FCP has been closed (as opposed to postponed) and removed disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Aug 19, 2018
@rfcbot rfcbot closed this Aug 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed This FCP has been closed (as opposed to postponed) finished-final-comment-period The final comment period is finished for this RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.