Listing 19-03 is potentially UB under Stacked Borrows

[nico-abram]

• I have checked the latest main branch to see if this has already been fixed
• I have searched existing issues and pull requests for duplicates

URL to the section(s) of the book with this problem: https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html#dereferencing-a-raw-pointer

Description of the problem: Running the listing

fn main() {
    let mut num = 5;

    let r1 = &num as *const i32;
    let r2 = &mut num as *mut i32;

    unsafe {
        println!("r1 is: {}", *r1);
        println!("r2 is: {}", *r2);
    }
}

Under MIRI with MIRIFLAGS="-Zmiri-symbolic-alignment-check -Zmiri-check-number-validity -Zmiri-tag-raw-pointers" results in the following:

error: Undefined Behavior: trying to reborrow for SharedReadOnly at alloc766, but parent tag <1658> does not have an appropriate item in the borrow stack
 --> src\main.rs:9:31
  |
9 |         println!("r1 is: {}", *r1);
  |                               ^^^ trying to reborrow for SharedReadOnly at alloc766, but parent tag <1658> does not have an appropriate item in the borrow stack
  |
  = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
  = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

  = note: inside `main` at src\main.rs:9:31
  = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:227:5
  = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys_common\backtrace.rs:123:18
  = note: inside closure at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:145: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 D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\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 D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:406:40
  = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:370:19
  = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:133:14
  = note: inside closure at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:128:48
  = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:406:40
  = note: inside `std::panicking::r#try::<isize, [closure@std::rt::lang_start_internal::{closure#2}]>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:370:19
  = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:133:14
  = note: inside `std::rt::lang_start_internal` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:128:20
  = note: inside `std::rt::lang_start::<()>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:144:17
  = note: this error originates in the macro `$crate::format_args_nl` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

Suggested fix:
I'm not sure. The following fixes that specific problem:

fn main() {
    let mut num = 5;

    let r2 = &mut num as *mut i32;
    let r1 = unsafe { &*r2 as *const i32 };

    unsafe {
        println!("r1 is: {}", *r1);
        println!("r2 is: {}", *r2);
    }
}

But I'm not sure if it preserves the original meaning

Possibly related: rust-lang/unsafe-code-guidelines#133

[steffahn]

You don’t need any flags anymore. I believe since tag-raw-pointers is now on by default AFAIR.

[kellda]

Hi, I just found this issue.

I think a better fix would be to use (and explain) std::ptr::addr_of[_mut]!. See this playground for an example. Note that the original issue also reproduce using Tools > Miri on the playground.

The problem here is that the temporary &mut invalidates the reference used to create r1, and as such also invalidates the r1 pointer. Deriving r2 from r1 or using std::ptr::addr_of_mut avoid this problem.

I also think that this is a great opportunity to warn and/or illustrate what can go wrong with unsafe, and maybe introduce miri. IMO, the best fix would be to leave the listing as is, explain and illustrate with miri why it's wrong, then provide a correct version.

[chriskrycho]

Oooof. We definitely don’t want unsound code in our example of unsafe. 😂 I’ve flagged this one for our 2024 revisions and we'll see if we can get it resolved!

[chriskrycho]

I just spent some time poking at this and thinking about it, and a couple things stood out to me:

1. As far as I can see, this is a case where the code as written should never be ill-defined… but it’s definitely a hair’s breadth away from it. That said, in the land of compiler optimizations there be many 🐉s.

2. The Tree Borrows formulation agrees with my analysis!

   $ MIRIFLAGS="-Zmiri-tree-borrows" cargo +nightly miri run
       Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s
        Running `/Users/chris/.rustup/toolchains/nightly-aarch64-apple-darwin/bin/cargo-miri runner target/miri/aarch64-apple-darwin/debug/unsafe-example`
   r1 is: 5
   r2 is: 5

   (This shows quite the meaningful difference between Stacked and Tree Borrows!)

To @kellda’s suggestion—I like that thought, as it is safer and Miri is happy with it (playground). However, since https://github.com/rust-lang/rust#127679 stabilized and should be in 1.82.0, out in about two weeks, iff we change it then we should use &raw const and &raw mut instead of addr_of! and addr_of_mut!:

fn main() {
    let mut num = 5;

    let r1 = &raw const num;
    let r2 = &raw mut num;

    unsafe {
        println!("r1 is: {}", *r1);
        println!("r2 is: {}", *r2);
    }
}

That works exactly as expected, including being safe on both Stacked and Tree Borrows—which, you know, it should; that’s one of the main points of the feature! The trick is thinking about how to rework this section to be correct. I do like the idea of getting a reference to Miri here, too. I’m going to see if I can figure out a good way to do both.

All of that said, I welcome input from someone who is actually an expert in this space, seeing as this was the first time I had ever actually used Miri rather than simply reading about it. 😅