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

Listing 19-03 is potentially UB under Stacked Borrows #3014

Open
2 tasks done
nico-abram opened this issue Jan 21, 2022 · 3 comments
Open
2 tasks done

Listing 19-03 is potentially UB under Stacked Borrows #3014

nico-abram opened this issue Jan 21, 2022 · 3 comments

Comments

@nico-abram
Copy link

  • 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

nico-abram added a commit to nico-abram/book that referenced this issue Jan 21, 2022
@carols10cents carols10cents added this to the ch19 milestone Jan 22, 2022
@steffahn
Copy link
Member

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

@kellda
Copy link

kellda commented Aug 27, 2023

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
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants