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

Confusing diagnostics when the return place and argument overlap in a Call terminator #2851

Closed
cbeuw opened this issue Apr 24, 2023 · 3 comments

Comments

@cbeuw
Copy link
Contributor

cbeuw commented Apr 24, 2023

This program might have UB as the return place and argument operand alias when calling fn2. I'm not sure if there's any conclusion from rust-lang/rust#71117, also the docs only say that the return place cannot alias if arguments are move, but everything's Copy here so maybe this should be fine?

#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;
#[custom_mir(dialect = "runtime", phase = "optimized")]
pub fn fn1(mut x: (i32, bool)) {
    mir! ({
        x.1 = false;
        x.0 = 12;

        Call(x.1, bb1, fn2(x))
    }
    bb1 = {
        Return()
    })
}

pub fn fn2(mut _1: (i32, bool)) -> bool {
    true
}

pub fn main() {
    fn1((1, false));
}

In any case, Miri's error message doesn't really tell you the root cause. If aliasing return place and arguments are to be forbidden, perhaps this should be explicitly checked?

This is with Stacked Borrows:

error: Undefined Behavior: not granting access to tag <3303> because that would remove [Unique for <3304>] which is strongly protected because it is an argument of call 865
  --> repro.rs:10:9
   |
10 |         Call(x.1, bb1, fn2(x))
   |         ^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <3303> because that would remove [Unique for <3304>] which is strongly protected because it is an argument of call 865
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows 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
help: <3303> was created here, as the base tag for alloc1665
  --> repro.rs:7:9
   |
7  |         x.1 = false;
   |         ^^^^^^^^^^^
help: <3304> is this argument
  --> repro.rs:17:1
   |
17 | / pub fn fn2(mut _1: (i32, bool)) -> bool {
18 | |     true
19 | | }
   | |_^
   = note: BACKTRACE (of the first span):
   = note: inside `fn1` at repro.rs:10:9: 10:31
note: inside `main`
  --> repro.rs:21:5
   |
21 |     fn1((1, false));
   |     ^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

This is with -Zmiri-tree-borrows:

error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
  --> repro.rs:10:9
   |
10 |         Call(x.1, bb1, fn2(x))
   |         ^^^^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
   |
   = 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: BACKTRACE:
   = note: inside `fn1` at repro.rs:10:9: 10:31
note: inside `main`
  --> repro.rs:21:5
   |
21 |     fn1((1, false));
   |     ^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error
@RalfJung
Copy link
Member

Our diagnostics code was never tested on this since you need custom MIR to trigger it (AFAIK).

@RalfJung
Copy link
Member

RalfJung commented Apr 24, 2023

Regarding whether this should be UB, currently we do retag_return_place after pushing the stack frame but before putting in the arguments, so no access to the return place is allowed while evaluating arguments. The semantics of this are completely up in the air, see rust-lang/rust#71117.

It's pretty unclear what, operationally, it should even mean to only do this for Copy arguments.

@RalfJung
Copy link
Member

RalfJung commented Sep 5, 2023

The original example currently doesn't UB any more (since we copy the argument before protecting the return place). Similar diagnostics issues remain; I opened #3051 for that.

@RalfJung RalfJung closed this as completed Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants