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

Consider the cargo workspace when checking if a frame is local #2024

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Mar 13, 2022

DefId::is_local returns a result which is technically correct, but doesn't match the user's intuition when running integration tests or doctests. This incorporates the workspace crates mentioned in cargo metadata into the check for whether a frame is local to match user intuition.

For example, here is the backtrace you get from MIRIFLAGS=-Zmiri-tag-raw-pointers cargo miri test in bytes 1.1.0:

   --> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:131:14
    |
131 |     unsafe { &mut *ptr::slice_from_raw_parts_mut(data, len) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to reborrow for Unique at alloc67158, but parent tag <untagged> 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 `std::slice::from_raw_parts_mut::<u8>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:131:14
    = note: inside `bytes::bytes::rebuild_boxed_slice` at /tmp/bytes-1.1.0/src/bytes.rs:938:19
    = note: inside closure at /tmp/bytes-1.1.0/src/bytes.rs:904:18
    = note: inside `<std::sync::atomic::AtomicPtr<()> as bytes::loom::sync::atomic::AtomicMut<()>>::with_mut::<[closure@bytes::bytes::promotable_even_drop::{closure#0}], ()>` at /tmp/bytes-1.1.0/src/loom.rs:17:17
    = note: inside `bytes::bytes::promotable_even_drop` at /tmp/bytes-1.1.0/src/bytes.rs:895:5
    = note: inside `<bytes::Bytes as std::ops::Drop>::drop` at /tmp/bytes-1.1.0/src/bytes.rs:515:18
    = note: inside `std::ptr::drop_in_place::<bytes::Bytes> - shim(Some(bytes::Bytes))` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:188:1
note: inside `copy_to_bytes_less` at tests/test_buf.rs:112:1
   --> tests/test_buf.rs:112:1
    |
112 | }
    | ^
note: inside closure at tests/test_buf.rs:106:1
   --> tests/test_buf.rs:106:1
    |
105 |   #[test]
    |   ------- in this procedural macro expansion
106 | / fn copy_to_bytes_less() {
107 | |     let mut buf = &b"hello world"[..];
108 | |
109 | |     let bytes = buf.copy_to_bytes(5);
110 | |     assert_eq!(bytes, &b"hello"[..]);
111 | |     assert_eq!(buf, &b" world"[..])
112 | | }
    | |_^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

We get these because the integration tests are occurring in a crate called test, not the actual bytes crate. With this PR, we get this:

    = note: inside `std::slice::from_raw_parts_mut::<u8>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:131:14
note: inside `bytes::bytes::rebuild_boxed_slice` at /tmp/bytes-1.1.0/src/bytes.rs:938:19
   --> /tmp/bytes-1.1.0/src/bytes.rs:938:19
    |
938 |     Box::from_raw(slice::from_raw_parts_mut(buf, cap))
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at /tmp/bytes-1.1.0/src/bytes.rs:904:18
   --> /tmp/bytes-1.1.0/src/bytes.rs:904:18
    |
904 |             drop(rebuild_boxed_slice(buf, ptr, len));
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<std::sync::atomic::AtomicPtr<()> as bytes::loom::sync::atomic::AtomicMut<()>>::with_mut::<[closure@bytes::bytes::promotable_even_drop::{closure#0}], ()>` at /tmp/bytes-1.1.0/src/loom.rs:17:17
   --> /tmp/bytes-1.1.0/src/loom.rs:17:17
    |
17  |                 f(self.get_mut())
    |                 ^^^^^^^^^^^^^^^^^
note: inside `bytes::bytes::promotable_even_drop` at /tmp/bytes-1.1.0/src/bytes.rs:895:5
   --> /tmp/bytes-1.1.0/src/bytes.rs:895:5
    |
895 | /     data.with_mut(|shared| {
896 | |         let shared = *shared;
897 | |         let kind = shared as usize & KIND_MASK;
898 | |
...   |
905 | |         }
906 | |     });
    | |______^
note: inside `<bytes::Bytes as std::ops::Drop>::drop` at /tmp/bytes-1.1.0/src/bytes.rs:515:18
   --> /tmp/bytes-1.1.0/src/bytes.rs:515:18
    |
515 |         unsafe { (self.vtable.drop)(&mut self.data, self.ptr, self.len) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: inside `std::ptr::drop_in_place::<bytes::Bytes> - shim(Some(bytes::Bytes))` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:188:1
note: inside `copy_to_bytes_less` at tests/test_buf.rs:112:1
   --> tests/test_buf.rs:112:1
    |
112 | }
    | ^
note: inside closure at tests/test_buf.rs:106:1
   --> tests/test_buf.rs:106:1
    |
105 |   #[test]
    |   ------- in this procedural macro expansion
106 | / fn copy_to_bytes_less() {
107 | |     let mut buf = &b"hello world"[..];
108 | |
109 | |     let bytes = buf.copy_to_bytes(5);
110 | |     assert_eq!(bytes, &b"hello"[..]);
111 | |     assert_eq!(buf, &b" world"[..])
112 | | }
    | |_^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

Note that this kind of inflation is rather rare to see. Most backtraces change not at all or only a tiny bit.

I originally implemented this to support another improvement to Miri diagnostics, but I think this is hairy enough to deserve its own PR, if somewhat poorly-motivated.

src/machine.rs Outdated Show resolved Hide resolved
cargo-miri/bin.rs Outdated Show resolved Hide resolved
cargo-miri/bin.rs Outdated Show resolved Hide resolved
cargo-miri/bin.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Mar 17, 2022

☔ The latest upstream changes (presumably #2029) made this pull request unmergeable. Please resolve the merge conflicts.

src/helpers.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
@saethlin saethlin force-pushed the better-local-check branch 2 times, most recently from 8943a1a to f19d6f4 Compare March 18, 2022 19:36
README.md Show resolved Hide resolved
@RalfJung
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 18, 2022

📌 Commit 65125df has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Mar 18, 2022

⌛ Testing commit 65125df with merge 5d72cd9...

@bors
Copy link
Contributor

bors commented Mar 18, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 5d72cd9 to master...

@bors bors merged commit 5d72cd9 into rust-lang:master Mar 18, 2022
@saethlin saethlin deleted the better-local-check branch March 19, 2022 00:09
bors added a commit that referenced this pull request May 14, 2022
Print spans where tags are created and invalidated

5225225 called this "automatic tag tracking" and I think that may be a reasonable description, but I would like to kill tag tracking as a primary use of Miri if possible. Tag tracking isn't always possible; for example if the UB is only detected with isolation off and the failing tag is made unstable by removing isolation. (also it's bad UX to run the tool twice)

This is just one of the things we can do with #2024

The memory usage of this is _shockingly_ low, I think because the memory usage of Miri is driven by allocations where each byte ends up with its own very large stack. The memory usage in this change is linear with the number of tags, not tags * bytes. If memory usage gets out of control we can cap the number of events we save per allocation, from experience we tend to only use the most recent few in diagnostics but of course there's no guarantee of that so if we can manage to keep everything that would be best.

In many cases now I can tell exactly what these codebases are doing wrong just from the new outputs here, which I think is extremely cool.

New helps generated with plain old `cargo miri test` on `rust-argon2` v1.0.0:
```
test argon2::tests::single_thread_verification_multi_lane_hash ... error: Undefined Behavior: trying to reborrow <1485898> for Unique permission at alloc110523[0x0], but that tag does not exist in the borrow stack for this location
   --> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/mem/manually_drop.rs:89:9
    |
89  |         slot.value
    |         ^^^^^^^^^^
    |         |
    |         trying to reborrow <1485898> for Unique permission at alloc110523[0x0], but that tag does not exist in the borrow stack for this location
    |         this error occurs as part of a reborrow at alloc110523[0x0..0x20]
    |
    = 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
help: <1485898> was created by a retag at offsets [0x0..0x20]
   --> src/memory.rs:42:13
    |
42  |             vec.push(unsafe { &mut (*ptr) });
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <1485898> was later invalidated at offsets [0x0..0x20]
   --> src/memory.rs:42:31
    |
42  |             vec.push(unsafe { &mut (*ptr) });
    |                               ^^^^^^^^^^^
```

And with `-Zmiri-tag-raw-pointers` on `slab` v0.4.5
```
error: Undefined Behavior: trying to reborrow <2915> for Unique permission at alloc1418[0x0], but that tag does not exist in the borrow stack for this location
   --> /tmp/slab-0.4.5/src/lib.rs:835:16
    |
835 |         match (&mut *ptr1, &mut *ptr2) {
    |                ^^^^^^^^^^
    |                |
    |                trying to reborrow <2915> for Unique permission at alloc1418[0x0], but that tag does not exist in the borrow stack for this location
    |                this error occurs as part of a reborrow at alloc1418[0x0..0x10]
    |
    = 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
help: <2915> was created by a retag at offsets [0x0..0x10]
   --> /tmp/slab-0.4.5/src/lib.rs:833:20
    |
833 |         let ptr1 = self.entries.get_unchecked_mut(key1) as *mut Entry<T>;
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <2915> was later invalidated at offsets [0x0..0x20]
   --> /tmp/slab-0.4.5/src/lib.rs:834:20
    |
834 |         let ptr2 = self.entries.get_unchecked_mut(key2) as *mut Entry<T>;
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

And without raw pointer tagging, `cargo miri test` on `half` v1.8.2
```
error: Undefined Behavior: trying to reborrow <untagged> for Unique permission at alloc1340[0x0], but that tag only grants SharedReadOnly permission for this location
   --> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:141:9
    |
141 |         &mut *ptr::slice_from_raw_parts_mut(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to reborrow <untagged> for Unique permission at alloc1340[0x0], but that tag only grants SharedReadOnly permission for this location
    |         this error occurs as part of a reborrow at alloc1340[0x0..0x6]
    |
    = 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
help: tag was most recently created at offsets [0x0..0x6]
   --> /tmp/half-1.8.2/src/slice.rs:309:22
    |
309 |         let length = self.len();
    |                      ^^^^^^^^^^
help: this tag was also created here at offsets [0x0..0x6]
   --> /tmp/half-1.8.2/src/slice.rs:308:23
    |
308 |         let pointer = self.as_ptr() as *mut u16;
    |                       ^^^^^^^^^^^^^
```
The second suggestion is close to guesswork, but from experience it tends to be correct (as in, it tends to locate the pointer the user wanted) more often that it doesn't.
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

Successfully merging this pull request may close these issues.

3 participants