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

MIR: Do we allow accesing a moved place? #112564

Closed
cbeuw opened this issue Jun 12, 2023 · 3 comments · Fixed by #113569
Closed

MIR: Do we allow accesing a moved place? #112564

cbeuw opened this issue Jun 12, 2023 · 3 comments · Fixed by #113569
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-opsem Relevant to the opsem team

Comments

@cbeuw
Copy link
Contributor

cbeuw commented Jun 12, 2023

fn14() dereferences a pointer to a place that has been moved to another function. Currently, Miri thinks this is fine, but this causes the compiled code to produce different results under different optimisation levels.

#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;

pub fn dump_var(val0: u32) {
    println!("{val0}");
}

pub struct Adt52 {
    fld1: (u32, usize, u16),
}

#[custom_mir(dialect = "runtime", phase = "initial")]
fn fn14() {
    mir! {
    let fld1: (u32, usize, u16);
    let non_copy: Adt52;
    let p: *const u32;
    let i: u32;
    let unit: ();
    {
        fld1 = (0, 0_usize, 0);
        non_copy = Adt52 {fld1};
        p = core::ptr::addr_of!(non_copy.fld1.0);
        Call(unit, bb13, fn15(Move(non_copy)))
    }
    bb13 = {
        i = *p;
        Call(unit, bb18, dump_var(i))
    }
    bb18 = {
        Return()
    }

    }
}
pub fn fn15(mut x: Adt52) {
    x.fld1 = (1, 0, 0);
}
pub fn main() {
    fn14();
}
% rustc -Zmir-opt-level=2 -Copt-level=3 repro.rs && ./repro
0
% rustc -Zmir-opt-level=1 -Copt-level=3 repro.rs && ./repro
1
% ../miri/miri run ./repro.rs
0

At the moment, this is only reproducible with custom MIR, because when we build MIR from surface Rust we always create a temporary, so you cannot create a pointer to anything that will be directly moved into another function.

fn fn14() {
    let fld1 = (0, 0, 0);

    let mut non_copy = Adt52 {fld1};              // _2 = Adt52 { fld1: _1 };
    let p = core::ptr::addr_of!(non_copy.fld1.0); // _3 = &raw const ((_2.0: (u32, usize, u16)).0: u32);
    fn15(non_copy);                               // _5 = move _2;
                                                  // _4 = fn15(move _5) -> bb1;

    let i = unsafe { *p };
    dump_var(i);
}

So either we should call this UB or codegen shouldn't rely on the assumption that a moved MIR place will never be used later.

cc @RalfJung

@clubby789 clubby789 added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Jun 12, 2023
@jyn514 jyn514 added the T-opsem Relevant to the opsem team label Jun 12, 2023
@cbeuw
Copy link
Contributor Author

cbeuw commented Jun 12, 2023

@JakobDegen mentioned this optimisation before rust-lang/unsafe-code-guidelines#188 (comment)

@cbeuw
Copy link
Contributor Author

cbeuw commented Jun 13, 2023

This code writes to a moved place instead. Both LLVM and Cranelift output 0, where as Miri output 1. For the same reason (function arguments are always a MIR temporary and no move-propagation), this currently cannot be reproduced using surface Rust.

#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;

pub struct Inner {
    tup: (usize, i32, f64),
}
pub struct Outer {
    inner: Inner,
}

#[custom_mir(dialect = "runtime", phase = "initial")]
pub fn start() {
    mir! {
    let non_copy: Outer;
    let fld_ptr: *mut (usize, i32, f64);
    let unit: ();
    {
        non_copy.inner.tup = (0, 1, 0.);
        let fld_ptr = core::ptr::addr_of_mut!(non_copy.inner.tup);
        Call(unit, ret, write(fld_ptr, Move(non_copy.inner)))
    }
    ret = {
        Return()
    }
    }
}

pub unsafe fn write(mut ptr_to_non_copy: *mut (usize, i32, f64), mut non_copy: Inner) {
    (*ptr_to_non_copy).1 = 0;
    println!("{}", non_copy.tup.1);
}

pub fn main() {
    unsafe { start() };
}

@cbeuw cbeuw changed the title MIR: Do we allow reading from a moved place? MIR: Do we allow accesing a moved place? Jun 13, 2023
@RalfJung
Copy link
Member

I think this falls in the scope of #71117 -- we don't currently have any proper spec for these move arguments. Your 2nd argument uses move on a place that is not just a local (non_copy.inner) which IMO shouldn't even be allowed.

@bors bors closed this as completed in 136dab6 Jul 12, 2023
RalfJung pushed a commit to RalfJung/miri that referenced this issue Jul 12, 2023
miri: protect Move() function arguments during the call

This gives `Move` operands a meaning specific to function calls:
- for the duration of the call, the place the operand comes from is protected, making all read and write accesses insta-UB.
- the contents of that place are reset to `Uninit`, so looking at them again after the function returns, we cannot observe their contents

Turns out we can replace the existing "retag return place" hack with the exact same sort of protection on the return place, which is nicely symmetric.

Fixes rust-lang/rust#112564
Fixes rust-lang#2927

This starts with a Miri rustc-push, since we'd otherwise conflict with a PR that recently landed in Miri.
(The "miri tree borrows" commit is an unrelated cleanup I noticed while doing the PR. I can remove it if you prefer.)
r? `@oli-obk`
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this issue Aug 29, 2024
miri: protect Move() function arguments during the call

This gives `Move` operands a meaning specific to function calls:
- for the duration of the call, the place the operand comes from is protected, making all read and write accesses insta-UB.
- the contents of that place are reset to `Uninit`, so looking at them again after the function returns, we cannot observe their contents

Turns out we can replace the existing "retag return place" hack with the exact same sort of protection on the return place, which is nicely symmetric.

Fixes rust-lang/rust#112564
Fixes rust-lang/miri#2927

This starts with a Miri rustc-push, since we'd otherwise conflict with a PR that recently landed in Miri.
(The "miri tree borrows" commit is an unrelated cleanup I noticed while doing the PR. I can remove it if you prefer.)
r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants