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

alloc: Add new_zeroed() versions like new_uninit(). #66128

Merged
merged 1 commit into from
Nov 27, 2019

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Nov 5, 2019

MaybeUninit has both uninit() and zeroed(), it seems reasonable to have the same
surface on Box/Rc/Arc.

Needs tests.

cc #63291

MaybeUninit has both uninit() and zeroed(), it seems reasonable to have the same
surface on Box/Rc/Arc.

Needs tests.
@emilio
Copy link
Contributor Author

emilio commented Nov 5, 2019

cc @SimonSapin

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2019
@alexcrichton
Copy link
Member

r? @SimonSapin

pub fn new_zeroed() -> Box<mem::MaybeUninit<T>> {
unsafe {
let mut uninit = Self::new_uninit();
ptr::write_bytes::<T>(uninit.as_mut_ptr(), 0, 1);
Copy link
Member

Choose a reason for hiding this comment

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

How about using alloc_zeroed when creating the Box so you don't need to manually zero the memory? It might make sense to do the same for Rc and Arc as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For Rc / Arc it was a bit harder because the refcount is part of the allocation, so you don't really want to do that. You could, but then you have to duplicate a lot more code, which is not great.

For Box, I guess I can indeed do that without too much effort, though it's still more code and not 100% sure if worth it.

@Centril
Copy link
Contributor

Centril commented Nov 5, 2019

it seems reasonable to have the same
surface on Box/Rc/Arc.

I get the consistency argument, but do you have concrete use cases?

@Centril Centril changed the title alloc: Add new_zeroed() versions like new_uninit(). [WIP] alloc: Add new_zeroed() versions like new_uninit(). Nov 5, 2019
@emilio
Copy link
Contributor Author

emilio commented Nov 5, 2019

I get the consistency argument, but do you have concrete use cases?

It was mostly for consistency, but I happen to, kinda!

The reason I thought of this is because I was replacing this piece of firefox code full of undefined behavior.

What it does is allocating a bunch of C++ structs in an Arc<> using mem::zeroed(), and then calls into ffi to in-place construct them.

These C++ structs do contain various things which have NonNull / non-zero discriminants, etc. But it also contained various things for which zero is a valid bit pattern, and there happened to be C++ constructors that weren't properly initializing them, relying on the zeroing.

Arguably that was quite a bit footgunny, and that's fixed now using our equivalent to Arc::new_uninit() (we have an Arc version without weak refs), but the obvious solution to make the previous code UB-free would've been Arc::new_zeroed().

Our internal Arc didn't have this, but if there were more similar use-cases in our code-base I would've opted for adding an equivalent of this.

The other reason to add this is that writing it on your own is slightly footgunny / really easy to get wrong. See the discussion today here: https://mozilla.logbot.info/servo/20191105#c16736990-c16737025. The initial code by Simon created a reference to an uninitialized value, which is UB as I understand it. Plus it has the one-less-star-zeroes-the-pointer issue.

@SimonSapin
Copy link
Contributor

created a reference to an uninitialized value, which is UB as I understand it

(That version created a &mut MaybeUninit<T>, which is fine.)

@emilio
Copy link
Contributor Author

emilio commented Nov 6, 2019

(That version created a &mut MaybeUninit, which is fine.)

facepalm, indeed

@JohnCSimon
Copy link
Member

Ping from triage:
@SimonSapin Can you please review this PR? or does @emilio need to make changes to the PR? If so please change the tag to S-waiting-on-author?

Thanks!

@emilio
Copy link
Contributor Author

emilio commented Nov 9, 2019

I'm happy to add tests if people are fine with the addition.

@JohnCSimon
Copy link
Member

Ping from triage:
@SimonSapin Can you provide additional info and review this PR?
cc: @emilio
Thanks.

@JohnCSimon
Copy link
Member

Pinging again from triage:
@SimonSapin Can you provide additional info and review this PR?
cc: @emilio
Thanks.

Copy link
Contributor

@SimonSapin SimonSapin left a comment

Choose a reason for hiding this comment

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

r=me

@emilio Do you still have changes you want to make? If not please remove [WIP] from the PR title and let’s land this.

@SimonSapin SimonSapin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2019
@emilio emilio changed the title [WIP] alloc: Add new_zeroed() versions like new_uninit(). alloc: Add new_zeroed() versions like new_uninit(). Nov 26, 2019
@emilio
Copy link
Contributor Author

emilio commented Nov 26, 2019

Ah, I wanted to add tests, but I forgot that doctests do actually run, so this should be good to go.

@SimonSapin
Copy link
Contributor

Alright!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 26, 2019

📌 Commit b12e142 has been approved by SimonSapin

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 26, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Nov 26, 2019
alloc: Add new_zeroed() versions like new_uninit().

MaybeUninit has both uninit() and zeroed(), it seems reasonable to have the same
surface on Box/Rc/Arc.

Needs tests.

cc rust-lang#63291
bors added a commit that referenced this pull request Nov 27, 2019
Rollup of 14 pull requests

Successful merges:

 - #66128 (alloc: Add new_zeroed() versions like new_uninit().)
 - #66661 (Add riscv64gc-unknown-linux-gnu target)
 - #66663 (Miri: print leak report even without tracing)
 - #66711 (Add hardware floating point features to aarch64-pc-windows-msvc)
 - #66713 (introduce a target to build the kernel of the unikernel HermitCore)
 - #66717 (tidy: Accommodate rustfmt's preferred layout of stability attributes)
 - #66719 (Store pointer width as u32 on Config)
 - #66720 (Move ErrorReported to rustc_errors)
 - #66737 (Error codes cleanup)
 - #66754 (Various tweaks to diagnostic output)
 - #66763 (Minor edit for documentation-tests.md that increases clarity)
 - #66779 (follow the same function order in the trait)
 - #66786 (Add wildcard test for const_if_match)
 - #66788 (Allow `Unreachable` terminators through `min_const_fn` checks)

Failed merges:

r? @ghost
@bors bors merged commit b12e142 into rust-lang:master Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants