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

emilio · 2019-11-05T18:29:30Z

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 · 2019-11-05T18:29:39Z

cc @SimonSapin

rust-highfive · 2019-11-05T18:29:49Z

r? @alexcrichton

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

alexcrichton · 2019-11-05T19:31:00Z

r? @SimonSapin

ollie27 · 2019-11-05T20:41:16Z

src/liballoc/boxed.rs

+    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);


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.

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 · 2019-11-05T21:34:59Z

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

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

emilio · 2019-11-05T23:14:20Z

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 · 2019-11-05T23:45:59Z

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 · 2019-11-06T01:10:21Z

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

facepalm, indeed

JohnCSimon · 2019-11-09T08:14:44Z

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 · 2019-11-09T11:26:20Z

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

JohnCSimon · 2019-11-16T23:50:12Z

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

JohnCSimon · 2019-11-24T02:08:25Z

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

SimonSapin

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.

emilio · 2019-11-26T20:01:48Z

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

SimonSapin · 2019-11-26T21:03:04Z

Alright!

@bors r+

bors · 2019-11-26T21:03:05Z

📌 Commit b12e142 has been approved by SimonSapin

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

@ghost

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

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

b12e142

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

rust-highfive assigned alexcrichton Nov 5, 2019

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

rust-highfive assigned SimonSapin and unassigned alexcrichton Nov 5, 2019

ollie27 reviewed Nov 5, 2019

View reviewed changes

Centril changed the title ~~alloc: Add new_zeroed() versions like new_uninit().~~ [WIP] alloc: Add new_zeroed() versions like new_uninit(). Nov 5, 2019

SimonSapin approved these changes Nov 26, 2019

View reviewed changes

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

SimonSapin mentioned this pull request Nov 26, 2019

Tracking issue for uninitialized constructors for Box, Rc, Arc #63291

Closed

emilio changed the title ~~[WIP] alloc: Add new_zeroed() versions like new_uninit().~~ alloc: Add new_zeroed() versions like new_uninit(). Nov 26, 2019

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 mentioned this pull request Nov 26, 2019

Rollup of 14 pull requests #66794

Merged

bors merged commit b12e142 into rust-lang:master Nov 27, 2019

workingjubilee mentioned this pull request Aug 22, 2024

Tracking Issue for new_zeroed_alloc #129396

Open

2 tasks

Uh oh!

Conversation

emilio commented Nov 5, 2019

Uh oh!

emilio commented Nov 5, 2019

Uh oh!

rust-highfive commented Nov 5, 2019

Uh oh!

alexcrichton commented Nov 5, 2019

Uh oh!

ollie27 Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

emilio Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

Centril commented Nov 5, 2019

Uh oh!

emilio commented Nov 5, 2019

Uh oh!

SimonSapin commented Nov 5, 2019

Uh oh!

emilio commented Nov 6, 2019

Uh oh!

JohnCSimon commented Nov 9, 2019

Uh oh!

emilio commented Nov 9, 2019

Uh oh!

JohnCSimon commented Nov 16, 2019

Uh oh!

JohnCSimon commented Nov 24, 2019

Uh oh!

SimonSapin left a comment

Choose a reason for hiding this comment

Uh oh!

emilio commented Nov 26, 2019

Uh oh!

SimonSapin commented Nov 26, 2019

Uh oh!

bors commented Nov 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants