UB inside Weak::as_ptr

[vorner]

I tried this code:

use std::sync::Weak;

fn main() {
    println!("{:?}", Weak::into_raw(Weak::<usize>::new()));
}

When I run it under miri, I get this:

error: Undefined Behavior: inbounds test failed: 0xffffffffffffffff is not a valid pointer
    --> /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:2367:45
     |
2367 |     unsafe { data_offset_align(align_of_val(&*ptr)) }
     |                                             ^^^^^ inbounds test failed: 0xffffffffffffffff is not a valid pointer
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

     = note: inside `alloc::sync::data_offset::<usize>` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:2367:45
     = note: inside `std::sync::Weak::<usize>::as_ptr` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:1575:26
     = note: inside `std::sync::Weak::<usize>::into_raw` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:1609:22
note: inside `main` at src/main.rs:4:22
    --> src/main.rs:4:22
     |
4    |     println!("{:?}", Weak::into_raw(Weak::<usize>::new()));
     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
     = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:125:18
     = note: inside closure at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:66:18
     = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:259:13
     = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:379:40
     = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:343:19
     = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:396:14
     = note: inside `std::rt::lang_start_internal` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:51:25
     = note: inside `std::rt::lang_start::<()>` at /home/vorner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:65:5

Creating a reference from such pointer looks suspicious indeed. I could send a pull request that adds a condition there, but I want to ask first if this is being done because „we are std, we can“, or if this is something that really should be fixed.

Meta

rustc --version --verbose:

rustc 1.50.0-nightly (f74583445 2020-12-18)
binary: rustc
commit-hash: f74583445702e2e27ec4415376f2c540a83d7ded
commit-date: 2020-12-18
host: x86_64-unknown-linux-gnu
release: 1.50.0-nightly

(but I've looked at the git sources and the thing is still there)

[camelid]

I think it should only be UB to dereference the pointer you get from into_raw, so I'm not sure why Miri thinks there's UB happening. Just in case, @rustbot prioritize

[vorner]

Because that pointer is being turned into a reference to feed to align_of_val internally. And references are supposed to be always valid.

[SNCPlay42]

Looks like #73845 fixed this for Rc but missed it in Arc.

[vitalyd]

Yes, seems this needs to use align_of_val_raw.

[camelid]

I renamed the issue to reflect that the bug is actually in Weak::as_ptr (which Weak::into_raw calls).

[RalfJung]

Cc @CAD97

[RalfJung]

Looks like #73845 fixed this for Rc but missed it in Arc.

Assuming you use the default flags for Miri, that change should not be required to make Miri pass, so I'd be surprised if doing the same change for Rc would make any difference here. If it does, that seems like a bug in Miri that I should investigate.

[RalfJung]

Ah, but the &*ptr in data_offset is indeed a problem (the PR was mostly about &raw so I got side-tracked; the &raw is what should not have any effect here).

Miri ensures that references are at least dereferencable, so yeah that would explain the problem here. Good catch!

[CAD97]

Yep, I apparently missed this somehow in #73845. Fixing the indicated line to use align_of_val_raw should be enough to fix it; I'm preparing the patch currently.

@rustbot claim

[RalfJung]

Hm, Miri still complains, even after the fix landed... it seems Miri doesn't like align_of_val_raw being called with dangling pointers. I'll have to look into this.

[CAD97]

it seems Miri doesn't like align_of_val_raw being called with dangling pointers.

IIRC, when I added align_of_val_raw, I changed the type of the intrinsic to take a raw pointer rather than a reference, but miri wasn't touched, so that would make sense miri's still expecting a proper reference.

[apiraino]

Patch arrived really fast, we are assigning "post-mortem" a priority of P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.