6

Enable permissive provenance by default by RalfJung · Pull Request #2275 · rust-...

 2 years ago
source link: https://github.com/rust-lang/miri/pull/2275
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Member

@RalfJung RalfJung commented 11 days ago

edited

This completes the plan laid out in #2133:

  • We use permissive provenance with wildcard pointers by default.
  • We print a warning on int2ptr casts. -Zmiri-permissive-provenance suppresses the warning; -Zmiri-strict-provenance turns it into a hard error.
  • Raw pointer tagging is now always enabled, so we remove the -Zmiri-tag-raw-pointers flag and the code for untagged pointers. (Passing the flag still works, for compatibility -- but we just ignore it, with a warning.)

We also fix an intptrcast issue:

  • Only live allocations are considered when computing the AllocId from an address.

So, finally, Miri has a good story for ptr2int2ptr roundtrips and no weird false negatives when doing raw pointer stuff with Stacked Borrows. :-) tada Thanks a lot to everyone who helped with this, in particular @carbotaniuman who convinced me this is even possible.

Fixes #2133
Fixes #1866
Fixes #1993

jonhoo, taiki-e, and ChayimFriedman2 reacted with hooray emojimadsmtm reacted with heart emoji All reactions

RalfJung

changed the title Enabled permissive provenance by default

Enable permissive provenance by default

11 days ago

// #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]

// static_assert_size!(Pointer<Option<Tag>>, 24);

//static_assert_size!(Pointer<Option<Tag>>, 24);

Member

Author

@RalfJung RalfJung 11 days ago

I am quite bummed by this... Tag now has two NonZeroU64, so we could use one to indicate Wildcard and one to indicate None, but it seems rustc is not smart enough for that. :(

I wonder if it's worth implementing the packing by hand, or if having the extra 8 bytes is just not such a big deal...

All reactions

Contributor

@oli-obk oli-obk left a comment

I'm a bit surprised about all the behaviour changes. I would have expected this to just change the defaults. I guess the subsequent cleanups are the cause here?

I wish we had a good way to run perf. Maybe we could figure out a way to run a separate perfbot suite via the same infrastructure... I'll check that out

All reactions

src/intptrcast.rs

Outdated Show resolved

Member

Author

RalfJung commented 11 days ago

edited

I'm a bit surprised about all the behaviour changes. I would have expected this to just change the defaults.

The change to check liveness of the allocation in intptrcast.rs is something I just discovered while working on this. I can factor it into a separate PR if you want.

The change I just added to make allocations adjacent again could be made dependent on the provenance mode on master already, I guess. It doesn't work with legacy provenance though. I can factor it into a separate PR if you want.

And that should be all the behavior changes I think? The remaining output changes come from the fact that we now enable raw pointer tagging by default. This also means we have to adjust some tests that inadvertantly relied on raw pointers being untagged. A bunch of tests were using int2ptr casts unnecessarily so I made them not do that.

And finally I killed the 'untagged' support from Stacked Borrows since I cannot wait to see it go away. :)

Contributor

oli-obk commented 11 days ago

The remaining output changes come from the fact that we now enable raw pointer tagging by default.

yea I got that part.

the rest seems ok to include here, I was just surprised, as the PR title and description did not make me think of anything beyond the test changes due to changes in defaults.

Member

Author

RalfJung commented 11 days ago

That test failure is very strange, I cannot reproduce it locally

Testing `cargo miri test` (no isolation, no doctests)...
Test stdout or stderr did not match reference!
--- BEGIN test stdout ---
running 2 tests
..
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
imported main
running 8 tests
..i--- END test stdout ---
--- BEGIN test stderr ---
error: Undefined Behavior: 0x93e046 is not a valid pointer
--> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.2/src/macos.rs:24:32
|
24|            let ret = unsafe { func(chunk.as_mut_ptr(), chunk.len()) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^0x93e046 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 `getrandom::imp::getrandom_inner` at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.2/src/macos.rs:24:32
= note: inside `getrandom::getrandom` at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.2/src/lib.rs:235:5
= note: inside `<rand::prelude::SmallRng as rand::SeedableRng>::from_entropy` at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/rand_core-0.6.2/src/lib.rs:380:27
note: inside `entropy_rng` at tests/test.rs:33:19
--> tests/test.rs:33:19
|
33|    let mut rng = SmallRng::from_entropy();
| ^^^^^^^^^^^^^^^^^^^^^^^^

Member

Author

RalfJung commented 11 days ago

edited

Ah, got it.

MIRIFLAGS="-Zmiri-permissive-provenance -Zmiri-seed=B" cargo miri test --target aarch64-apple-darwin rng --test test

Seems to need the "adjacent allocation" change to reproduce. Interesting...

Member

Author

RalfJung commented 11 days ago

The function pointer lacks provenance. Maybe I didn't think this part about adjacent allocations through carefully enough... I think I'll back that part out again.

Member

Author

RalfJung commented 10 days ago

Ah bummer, getrandom 1 still needs permissive provenance on macOS...

Contributor

saethlin commented 10 days ago

edited

When running cargo miri test, I think the ptr-int cast warnings are eaten by the test runner for lib/bin tests. For example, running the tests of string_cache:

     Running tests/small-stack.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/small_stack-936c74f9283f43cc)
error: Undefined Behavior: trying to reborrow <wildcard> for SharedReadOnly permission at alloc4271[0x0], but no exposed tags have suitable permission in the borrow stack for this location
   --> /root/build/src/atom.rs:233:25
    |
233 |             if unsafe { &*entry }.ref_count.fetch_sub(1, SeqCst) == 1 {
    |                         ^^^^^^^
    |                         |
    |                         trying to reborrow <wildcard> for SharedReadOnly permission at alloc4271[0x0], but no exposed tags have suitable permission in the borrow stack for this location
    |                         this error occurs as part of a reborrow at alloc4271[0x0..0x8]
    |
    = 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
            
    = note: inside `<string_cache::Atom<string_cache::EmptyStaticAtomSet> as std::ops::Drop>::drop` at /root/build/src/atom.rs:233:25
    = note: inside `std::ptr::drop_in_place::<string_cache::Atom<string_cache::EmptyStaticAtomSet>> - shim(Some(string_cache::Atom<string_cache::EmptyStaticAtomSet>))` at /root/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:487:1
note: inside closure at tests/small-stack.rs:13:9
   --> tests/small-stack.rs:13:9
    |
13  |         })
    |         ^

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

error: aborting due to previous error

   Doc-tests string_cache

Amusingly, one of the doctests runs into the same sort of issue, but in that case the ptr-int cast warning is displayed before the UB error.

EDIT: Hm actually I have no idea what's going on here, this could just be a me problem.

use std::sync::atomic::{AtomicBool, Ordering};

static FIRST_WARNING: AtomicBool = AtomicBool::new(true);

if FIRST_WARNING.swap(false, Ordering::Relaxed) {

register_diagnostic(NonHaltingDiagnostic::Int2Ptr);

Contributor

@saethlin saethlin 10 days ago

    --> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.4.3/src/tendril.rs:1073:9
     |
1073 |         (self.ptr.get().get() & !1) as *mut Header<A>
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pointer-to-integer cast
     |
     = help: this program is using integer-to-pointer casts or (equivalently) `from_exposed_addr`,

The diagnostic says pointer-to-integer, but the span is an int-to-pointer cast. Similarly, this diagnostic is called Int2Ptr but it is in the function called ptr_from_addr_cast. Which direction are we trying to highlight?

All reactions

Member

Author

@RalfJung RalfJung 10 days ago

oops int2ptr is right

All reactions

Member

Author

RalfJung commented 10 days ago

I have seen the warning in cargo miri test. In fact this reproduces inside test-cargo-miri: cargo miri test --target aarch64-apple-darwin gives

     Running tests/test.rs (target/miri/aarch64-apple-darwin/debug/deps/test-8bd6ea9154753acd)

running 8 tests
test cargo_env ... ok
test do_panic - should panic ... ok
test does_not_work_on_miri ... ignored
test entropy_rng ... warning: pointer-to-integer cast
  --> /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.1.16/src/util_libc.rs:98:22
   |
98 |         NonNull::new(addr as *mut _)
   |                      ^^^^^^^^^^^^^^ pointer-to-integer cast
   |
   = help: this program is using integer-to-pointer casts or (equivalently) `from_exposed_addr`,
   = help: which means that Miri might miss pointer bugs in this program
   = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.from_exposed_addr.html for more details on that operation
   = help: to ensure that Miri does not miss bugs in your program, use `with_addr` (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance) instead
   = help: you can then pass the `-Zmiri-strict-provenance` flag to Miri, to ensure you are not relying on `from_exposed_addr` semantics
   = help: alternatively, the `-Zmiri-permissive-provenance` flag disables this warning
           
   = note: inside `getrandom::util_libc::Weak::ptr` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.1.16/src/util_libc.rs:98:22
   = note: inside `getrandom::imp::getrandom_inner` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.1.16/src/macos.rs:18:25
   = note: inside `getrandom::getrandom` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.1.16/src/lib.rs:291:5
note: inside `entropy_rng` at tests/test.rs:29:5
  --> tests/test.rs:29:5
   |
29 |     getrandom_1::getrandom(&mut data).unwrap();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at tests/test.rs:26:1
  --> tests/test.rs:26:1
   |
25 |   #[test]
   |   ------- in this procedural macro expansion
26 | / fn entropy_rng() {
27 | |     // Test `getrandom` directly (in multiple different versions).
28 | |     let mut data = vec![0; 16];
29 | |     getrandom_1::getrandom(&mut data).unwrap();
...  |
42 | |     let _val = rng.gen::<i128>();
43 | | }
   | |_^
   = note: this warning originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

ok
test fail_index_check - should panic ... ok

Member

Author

RalfJung commented 10 days ago

One thing I was wondering about: currently we only print the warning on the first cast.
We might instead keep a HashSet of Spans that we have reported and print the warning the first time it is triggered for each unique span? (possibly with some kind of limit, like stopping to warn after 10 or 100 warnings or so)

Contributor

saethlin commented 10 days ago

That may be useful to have behind a flag.

What I am thinking of for diagnostics, is that when we encounter a problem we should be able to report what Miri knows about what offsets and with what permissions have been exposed in an allocation. I'd have to actually get to implementing this to know how useful it is, but for example in string_cache we have an invalid use of Wildcard for SRO at offset 0, which makes me think we just didn't expose that address. Most of the other errors I'm seeing are for SRW.

Member

Author

RalfJung commented 10 days ago

That may be useful to have behind a flag.

So many flags.^^ For now I'd like to have a reasonable default.
And I feel reporting not only the first hit might be useful...

for example in string_cache we have an invalid use of Wildcard for SRO at offset 0, which makes me think we just didn't expose that address. Most of the other errors I'm seeing are for SRW.

Provenances (AllocId + SbTag) are exposed, not addresses. And if the AllocId had not been exposed we would not even have gotten to the Stacked Borrows part.

But likely the tag that was exposed is already invalid, or never was valid for this location.

Contributor

saethlin commented 10 days ago

In real code, especially from a test suite, you can expect 1-2 backtraces to fit on a screen. I think if Miri emits 10, people will rapidly decide to always silence them, just like how people learn to always disable isolation.

Contributor

saethlin commented 10 days ago

Many of the int2ptr warnings I've seen are for once_cell. I should submit my patch, but also this warning is pretty far off the mark. Perhaps this is another good place to use the is_local logic, if you're looking to filter? A user probably cares more about the warning when it's for something they wrote, rather than a dependency.

Member

Author

RalfJung commented 10 days ago

In real code, especially from a test suite, you can expect 1-2 backtraces to fit on a screen. I think if Miri emits 10, people will rapidly decide to always silence them, just like how people learn to always disable isolation.

If they are not interested in removing these casts, that's fine.
But if they are interested in removing them, I would be quite annoyed if Miri told me the first cast, I fix that, the I rerun and learn about the 2nd cast, fix that, rinse, repeat.

Many of the int2ptr warnings I've seen are for once_cell. I should submit my patch, but also this warning is pretty far off the mark. Perhaps this is another good place to use the is_local logic, if you're looking to filter? A user probably cares more about the warning when it's for something they wrote, rather than a dependency.

Well, I do want to make sure people are aware that such a precision-losing cast happens -- even if they cannot do anything about it, IMO it is important that they can know that Miri might miss some bugs here. This warning is load-bearing, in a sense.

We could prune the backtrace but I doubt that would help much...

Member

Author

RalfJung commented 10 days ago

I'm going to land this for now, we can always fine-tune it later.
Thanks all for the helpful feedback. :)

@bors r+

saethlin reacted with thumbs up emoji All reactions

Contributor

bors commented 10 days ago

pushpin Commit c1eddbc has been approved by RalfJung

Contributor

bors commented 10 days ago

hourglass Testing commit c1eddbc with merge 7fafbde...

Contributor

bors commented 10 days ago

sunny Test successful - checks-actions
Approved by: RalfJung
Pushing 7fafbde to master...

bors

merged commit 7fafbde into

rust-lang:master

10 days ago

8 checks passed

RalfJung

deleted the permissive-provenance-for-all branch

10 days ago


About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK