Rc: remove unused allocation and fix segfault in Weak::new() #51901

SimonSapin · 2018-06-29T14:19:17Z

Same as #50357 did for Arc.

Fixes #48493

rust-highfive · 2018-06-29T14:19:21Z

r? @withoutboats

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

SimonSapin · 2018-06-29T14:19:27Z

r? @RalfJung

bors · 2018-06-29T20:27:01Z

☔ The latest upstream changes (presumably #51569) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin · 2018-06-29T20:44:28Z

Rebased.

RalfJung · 2018-06-29T23:34:51Z

src/liballoc/sync.rs

        }
    }
 }

+// Specialization because `NonNull::<T>::dangling()` only exits for `T: Sized`


Why not allow NonNull::<T>::dangling() for T: ?Sized instead?

It wouldn’t compile because usize can only be cast to thin pointers because we wouldn’t know what to use for the vtable pointer (for trait objects, for example).

Oh, wait, I see now... this panics when actually called with something unsized? Is that really the desired behavior, for std::rc::Weak::<str>::new() to panic?

Ah, Weak::new requires T: Sized.

This seems rather hacky to have to write dangling_ptr here but stub it out. At the very least, there should be a comment saying why this unimplemented!() can never be hit. Actually, shouldn't it be unreachable!() instead? unimplemented!() to mean reads like "TODO: implement this".

Also, there are unsizing coercions for Weak. Can't I use them to create a dangling Weak<[u8; 1]>, then coerce that to Weak<[u8]> and then run into problems because is_dangling will now return false?

Sorry, I definitely should have commented this more clearly.

Yes, the idea was that Weak::new only exists for T: Sized. And yes, this is broken because I didn’t think of unsizing coecion. Good catch, thanks.

SimonSapin · 2018-06-30T10:51:29Z

As pointed out by @RalfJung in inline comments, my attempt to making dangling pointers be aligned instead of having address 1 is broken. Even though Weak::<T>::new requires T: Sized, that pointer can then be "unsized" and coerced to a DST pointer.

The "obvious" fix would be to use unsafe { align_of_val(&*ptr) }. I expect this would work in practice since the unsizing coercion creates a valid fat pointer payload (vtable pointer or slice length) next to the dangling pointer. But is dereferencing a dangling pointer to create a &_ reference still UB, even if we don’t actually access memory through that pointer? Note that Box deallocation already does this: https://github.com/rust-lang/rust/blob/1.27.0/src/liballoc/alloc.rs#L166-L167

With no way to find out the "real" alignment of a DST, I’m afraid we have no reliable way to check whether a pointer came from NonNull::dangling (before unsizing). So the only way for Weak::new to create a dangling pointer would be to use a single arbitrary address such as 1, like #50357 did. How problematic are unaligned pointers? CC @gankro and @pnkfelix for #41064 and #42794.

RalfJung · 2018-06-30T11:53:29Z

I expect this would work in practice since the unsizing coercion creates a valid fat pointer payload (vtable pointer or slice length) next to the dangling pointer.

Notice that this is already happening right now when using unsizing coercions on Arc's Weak.

I think we could declare that unsizing coercions effectively operate on raw pointers, similar to the &s.field as *const _ situation where the intermediate reference doesn't have to be fully valid (e.g., it could be unaligned). However, that still leaves the issue of using align_of_val with "bad" data. Notice that this is not the first time this comes up; at the Berlin all-hands we talked about how Arc::drop_slow effectively calls Layout::for_value with dropped data. I feel like we should have variants of these methods which take raw pointers, so that we do not have to create/use references to call them.

SimonSapin · 2018-06-30T14:17:06Z

Is the "payload" of fat raw pointers guaranteed to be valid, or is something like mem::zeroed::<*mut SomeTrait>() (creating a null vtable pointer) acceptable?

RalfJung · 2018-07-03T17:14:13Z

Good question. My inclination is that the vtable must always be a valid vtable, and the size of a raw slice must always be an integer (i.e., it must not be undef). I consider these to be "bit-validity" requirements similar to "references must not be NULL".

For example, I could imagine us doing enum layout optimizations based on the assumption that the vtable field is not NULL, and I think we could do these optimizations even for raw pointers. However, there may be crazy things people are doing that would be broken by this. ;)

Gankra · 2018-07-03T23:28:57Z

this is an under-defined part of the language

SimonSapin · 2018-07-04T15:05:03Z

Alright, I’ve changed is_dangling to use align_of_val on the basis that:

In this code, the pointer is in a private field and for DSTs always created with a valid payload
The Box destructor does the same.

Adding a raw-pointer variation of align_of_val can still be done later.

alexcrichton · 2018-07-06T16:51:22Z

r=me, but mind also adding some tests for DST coercions like casting a Weak<()> to Weak<Any> and cloning/dropping it?

SimonSapin · 2018-07-06T18:46:26Z

Done.

@bors r=alexcrichton

bors · 2018-07-06T18:46:27Z

📌 Commit 96d1f27 has been approved by alexcrichton

bors · 2018-07-06T22:09:10Z

⌛ Testing commit 96d1f27 with merge 3d553bd...

Rc: remove unused allocation and fix segfault in Weak::new() Same as #50357 did for `Arc`. Fixes #48493

bors · 2018-07-06T23:36:10Z

💔 Test failed - status-travis

rust-highfive · 2018-07-06T23:36:12Z

The job dist-various-1 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:08:56] [RUSTC-TIMING] alloc test:false 2.530
[01:08:56] error: Could not compile `alloc`.
[01:08:56] 
[01:08:56] Caused by:
[01:08:56]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name alloc liballoc/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=29f0a4c2b0209285 -C extra-filename=-29f0a4c2b0209285 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/deps --target thumbv6m-none-eabi -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/release/deps --extern compiler_builtins=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/deps/libcompiler_builtins-0eff4e3891d39af0.rlib --extern core=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/deps/libcore-02bed33325735a38.rlib -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/build/compiler_builtins-f4ddcda5e74d05f6/out` (exit code: 101)
[01:08:56] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "thumbv6m-none-eabi" "-j" "4" "--release" "--locked" "--color" "always" "--features" "c mem" "-p" "alloc" "-p" "compiler_builtins" "-p" "std_unicode" "--manifest-path" "/checkout/src/rustc/compiler_builtins_shim/Cargo.toml" "--message-format" "json"
[01:08:56] expected success, got: exit code: 101
[01:08:56] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1117:9
[01:08:56] travis_fold:end:stage2-std

[01:08:56] travis_time:end:stage2-std:start=1530920024822083168,finish=1530920062386157973,duration=37564074805

---
travis_time:end:24028b2d:start=1530920064608513636,finish=1530920064615654578,duration=7140942
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:04d8da70
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1a6fcaf8
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · 2018-07-06T23:36:17Z

The job dist-various-1 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:08:56] [RUSTC-TIMING] alloc test:false 2.530
[01:08:56] error: Could not compile `alloc`.
[01:08:56] 
[01:08:56] Caused by:
[01:08:56]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name alloc liballoc/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=29f0a4c2b0209285 -C extra-filename=-29f0a4c2b0209285 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/deps --target thumbv6m-none-eabi -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/release/deps --extern compiler_builtins=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/deps/libcompiler_builtins-0eff4e3891d39af0.rlib --extern core=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/deps/libcore-02bed33325735a38.rlib -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/thumbv6m-none-eabi/release/build/compiler_builtins-f4ddcda5e74d05f6/out` (exit code: 101)
[01:08:56] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "thumbv6m-none-eabi" "-j" "4" "--release" "--locked" "--color" "always" "--features" "c mem" "-p" "alloc" "-p" "compiler_builtins" "-p" "std_unicode" "--manifest-path" "/checkout/src/rustc/compiler_builtins_shim/Cargo.toml" "--message-format" "json"
[01:08:56] expected success, got: exit code: 101
[01:08:56] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1117:9
[01:08:56] travis_fold:end:stage2-std

[01:08:56] travis_time:end:stage2-std:start=1530920024822083168,finish=1530920062386157973,duration=37564074805

---
travis_time:end:24028b2d:start=1530920064608513636,finish=1530920064615654578,duration=7140942
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:04d8da70
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1a6fcaf8
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Same as #50357

SimonSapin · 2018-07-06T23:46:23Z

Fixed the error that happened with targets that don’t have atomic instructions (and so no Arc).

@bors r=alexcrichton

bors · 2018-07-06T23:46:24Z

📌 Commit 67202b8 has been approved by alexcrichton

…hton Rc: remove unused allocation and fix segfault in Weak::new() Same as rust-lang#50357 did for `Arc`. Fixes rust-lang#48493

@ghost

Rollup of 9 pull requests Successful merges: - #51901 (Rc: remove unused allocation and fix segfault in Weak::new()) - #52058 (Use of unimplemented!() causing ICE with NLL) - #52067 (Visit the mir basic blocks in reverse-postfix order) - #52083 (Dont run ast borrowck on mir mode) - #52099 (fix typo in stable `--edition` error message) - #52103 (Stabilize rc_downcast) - #52104 (Remove unnecessary feature gate.) - #52117 (Dedupe filetime) - #52120 (ARM: expose the "mclass" target feature) Failed merges: r? @ghost

bors · 2018-07-07T05:54:33Z

⌛ Testing commit 67202b8 with merge 4f0ca92...

Correct outdated documentation for rc::Weak This was overlooked in ~~rust-lang#50357~~ rust-lang#51901

rust-highfive assigned withoutboats Jun 29, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 29, 2018

This was referenced Jun 29, 2018

Remove use of uninitialized() in Weak::new() #51851

Closed

Arc: remove unused allocation from Weak::new() #50357

Merged

SimonSapin force-pushed the weak-unboxing branch 2 times, most recently from 0e51dd4 to a309ea6 Compare June 29, 2018 20:44

RalfJung reviewed Jun 29, 2018

View reviewed changes

SimonSapin force-pushed the weak-unboxing branch from a309ea6 to 391c174 Compare July 4, 2018 15:01

SimonSapin changed the title ~~Rc: remove unused allocation from Weak::new()~~ Rc: remove unused allocation and fix segfault in Weak::new() Jul 4, 2018

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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2018

bors added a commit that referenced this pull request Jul 6, 2018

Auto merge of #51901 - rust-lang:weak-unboxing, r=alexcrichton

3d553bd

Rc: remove unused allocation and fix segfault in Weak::new() Same as #50357 did for `Arc`. Fixes #48493

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2018

SimonSapin added 5 commits July 7, 2018 01:41

Use an aligned dangling pointer in Weak::new, rather than address 1

6e2c49f

Rc: remove unused allocation from Weak::new()

41730b7

Same as #50357

Add a test for Weak::new() not crashing for uninhabited types

21526c5

Add some unit tests for dangling Weak references

5717d99

Fix is_dangling import when Arc is #[cfg]’ed out

67202b8

SimonSapin force-pushed the weak-unboxing branch from 96d1f27 to 67202b8 Compare July 6, 2018 23:45

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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2018

Mark-Simulacrum mentioned this pull request Jul 7, 2018

Rollup of 9 pull requests #52123

Merged

bors merged commit 67202b8 into master Jul 7, 2018

dtolnay deleted the weak-unboxing branch July 7, 2018 21:34

kennytm mentioned this pull request Jul 18, 2018

Document that Unique::empty() and NonNull::dangling() aren't sentinel values #52508

Merged

foriequal0 mentioned this pull request Jun 12, 2020

Support T: ?Sized in Weak::new() #50513

Closed

gpluscb mentioned this pull request Apr 15, 2021

Correct outdated documentation for rc::Weak #84220

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2021

Auto merge of rust-lang#84220 - gpluscb:weak_doc, r=jyn514

5e7beba

Correct outdated documentation for rc::Weak This was overlooked in ~~rust-lang#50357~~ rust-lang#51901

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rc: remove unused allocation and fix segfault in Weak::new() #51901

Rc: remove unused allocation and fix segfault in Weak::new() #51901

SimonSapin commented Jun 29, 2018

rust-highfive commented Jun 29, 2018

SimonSapin commented Jun 29, 2018

bors commented Jun 29, 2018

SimonSapin commented Jun 29, 2018

RalfJung Jun 29, 2018

SimonSapin Jun 29, 2018

RalfJung Jun 30, 2018

RalfJung Jun 30, 2018 •

edited

Loading

SimonSapin Jun 30, 2018

SimonSapin commented Jun 30, 2018

RalfJung commented Jun 30, 2018 •

edited

Loading

SimonSapin commented Jun 30, 2018

RalfJung commented Jul 3, 2018

Gankra commented Jul 3, 2018

SimonSapin commented Jul 4, 2018

alexcrichton commented Jul 6, 2018

SimonSapin commented Jul 6, 2018

bors commented Jul 6, 2018

bors commented Jul 6, 2018

bors commented Jul 6, 2018

rust-highfive commented Jul 6, 2018

rust-highfive commented Jul 6, 2018

SimonSapin commented Jul 6, 2018

bors commented Jul 6, 2018

bors commented Jul 7, 2018

Rc: remove unused allocation and fix segfault in Weak::new() #51901

Rc: remove unused allocation and fix segfault in Weak::new() #51901

Conversation

SimonSapin commented Jun 29, 2018

rust-highfive commented Jun 29, 2018

SimonSapin commented Jun 29, 2018

bors commented Jun 29, 2018

SimonSapin commented Jun 29, 2018

RalfJung Jun 29, 2018

Choose a reason for hiding this comment

SimonSapin Jun 29, 2018

Choose a reason for hiding this comment

RalfJung Jun 30, 2018

Choose a reason for hiding this comment

RalfJung Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

SimonSapin Jun 30, 2018

Choose a reason for hiding this comment

SimonSapin commented Jun 30, 2018

RalfJung commented Jun 30, 2018 • edited Loading

SimonSapin commented Jun 30, 2018

RalfJung commented Jul 3, 2018

Gankra commented Jul 3, 2018

SimonSapin commented Jul 4, 2018

alexcrichton commented Jul 6, 2018

SimonSapin commented Jul 6, 2018

bors commented Jul 6, 2018

bors commented Jul 6, 2018

bors commented Jul 6, 2018

rust-highfive commented Jul 6, 2018

rust-highfive commented Jul 6, 2018

SimonSapin commented Jul 6, 2018

bors commented Jul 6, 2018

bors commented Jul 7, 2018

RalfJung Jun 30, 2018 •

edited

Loading

RalfJung commented Jun 30, 2018 •

edited

Loading