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

-Zmiri-strict-provenance reports UB when using the address-exposing Strict Provenance APIs correctly #2134

Closed
saethlin opened this issue May 20, 2022 · 5 comments
Labels
A-intptrcast Area: affects int2ptr and ptr2int casts C-support Category: Not necessarily a bug, but someone asking for support

Comments

@saethlin
Copy link
Member

#![feature(strict_provenance)]
fn main() {
    let x = 0u8;
    let original_ptr = &x as *const u8;
    let addr = original_ptr.expose_addr();
    let new_ptr: *const u8 = core::ptr::from_exposed_addr(addr);
    unsafe {
        dbg!(*new_ptr);
    }
}
error: Undefined Behavior: dereferencing pointer failed: 0x27c02 is not a valid pointer
 --> src/main.rs:8:9
  |
8 |         dbg!(*new_ptr);
  |         ^^^^^^^^^^^^^^ dereferencing pointer failed: 0x27c02 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 `main` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/macros.rs:315:13
  = note: this error originates in the macro `dbg` (in Nightly builds, run with -Z macro-backtrace for more info)

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

error: aborting due to previous error

I understand why this happens, but this is if nothing else incredibly surprising for users. The intuition is that the Strict Provenance mode in Miri should be compatible with the Strict Provenance APIs in the standard library. Could we make that happen? Or will this just be closed by the permissive provenance work?

@RalfJung
Copy link
Member

RalfJung commented May 20, 2022

Quoting the from_exposed_addr docs:

Using this method means that code is not following strict provenance rules.

So I think this is intended behavior.

However, after #2059 landed I want to add a warning (or maybe even an error?) so Miri in strict provenance mode says, the first time from_exposed_addr or an int2ptr cast is used, that this operation is not supported. This will require using some other operation for ptr::invalid though.

@RalfJung
Copy link
Member

Also see #2133 which I literally wrote in parallel with you writing this. ;)

@Gankra
Copy link
Contributor

Gankra commented May 20, 2022

Is there a way to invoke miri such that the distinction between these two APIs is actually useful? i.e. such that it attempts to be strict but incorporates exposing and from_exposed_addr as an operation? I forget, is angelic nondet a nightmare to actually support in an interpretter?

@RalfJung
Copy link
Member

Not yet, but see #2133 for the plan to make that happen. :D My plan is to make that the default, and make -Zmiri-strict-provenance a flag to basically promise that one doesn't use the exposed stuff.

Yes, angelic is a nightmare, so this mode will miss some bugs where you use a from_exposed_addr-ptr in multiple mutually incompatible ways. That's why we also have -Zmiri-strict-provenance which will not miss bugs but will also not support the angelic stuff.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue May 21, 2022
make ptr::invalid not the same as a regular int2ptr cast

In Miri, we would like to distinguish `ptr::invalid` from `ptr::from_exposed_provenance`, so that we can provide better diagnostics issues like rust-lang/miri#2134, and so that we can detect the UB in programs like
```rust
fn main() {
    let x = 0u8;
    let original_ptr = &x as *const u8;
    let addr = original_ptr.expose_addr();
    let new_ptr: *const u8 = core::ptr::invalid(addr);
    unsafe {
        dbg!(*new_ptr);
    }
}
```

To achieve that, the two functions need to have different implementations. Currently, both are just `as` casts. We *could* add an intrinsic for this, but it turns out `transmute` already has the right behavior, at least as far as Miri is concerned. So I propose we just use that.

Cc `@Gankra`
@RalfJung RalfJung added C-support Category: Not necessarily a bug, but someone asking for support A-intptrcast Area: affects int2ptr and ptr2int casts labels Jun 5, 2022
@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2022

I will close this in favor of #2133. Currently this is intended behavior, and eventually the plan is that Miri would already error in the from_exposed_addr call and say that this is not supported with strict provenance enabled.

@RalfJung RalfJung closed this as not planned Won't fix, can't repro, duplicate, stale Jun 5, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
make ptr::invalid not the same as a regular int2ptr cast

In Miri, we would like to distinguish `ptr::invalid` from `ptr::from_exposed_provenance`, so that we can provide better diagnostics issues like rust-lang/miri#2134, and so that we can detect the UB in programs like
```rust
fn main() {
    let x = 0u8;
    let original_ptr = &x as *const u8;
    let addr = original_ptr.expose_addr();
    let new_ptr: *const u8 = core::ptr::invalid(addr);
    unsafe {
        dbg!(*new_ptr);
    }
}
```

To achieve that, the two functions need to have different implementations. Currently, both are just `as` casts. We *could* add an intrinsic for this, but it turns out `transmute` already has the right behavior, at least as far as Miri is concerned. So I propose we just use that.

Cc `@Gankra`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intptrcast Area: affects int2ptr and ptr2int casts C-support Category: Not necessarily a bug, but someone asking for support
Projects
None yet
Development

No branches or pull requests

3 participants