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

Unhelphful stacked borrows diagnostic span #2536

Closed
SparkyPotato opened this issue Sep 8, 2022 · 3 comments · Fixed by #2537
Closed

Unhelphful stacked borrows diagnostic span #2536

SparkyPotato opened this issue Sep 8, 2022 · 3 comments · Fixed by #2537
Labels
A-diagnostics errors and warnings emitted by miri

Comments

@SparkyPotato
Copy link

SparkyPotato commented Sep 8, 2022

Running https://gist.github.com/SparkyPotato/a32487ed62d8c28acbac2d4df9129af7 in miri, it errors with:

error: Undefined Behavior: attempting a write access using <3540> at alloc1752[0x18], but that tag does not exist in the borrow stack for this location
   --> src/main.rs:152:3
    |
152 |         *a = 123;
    |         ^^^^^^^^
    |         |
    |         attempting a write access using <3540> at alloc1752[0x18], but that tag does not exist in the borrow stack for this location
    |         this error occurs as part of an access at alloc1752[0x18..0x1c]
    |
    = 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: <3540> was created by a SharedReadWrite retag at offsets [0x18..0x1c]
   --> src/main.rs:130:31
    |
130 |                 Ok(NonNull::new_unchecked(from_raw_parts_mut(target, layout.size())))
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <3540> was later invalidated at offsets [0x18..0x100018] by a Unique FnEntry retag
   --> src/main.rs:150:11
    |
150 |         let b = arena.allocate(Layout::new::<u32>()).unwrap().as_ptr() as *mut u32;
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: backtrace:
    = note: inside `main` at src/main.rs:152:3

However, the stacked borrow violation is in lines 123 and 126, where data: [u8] is autorefed to a &mut [u8] for [u8]::as_mut_ptr.

While the span is technically correct, it doesn't help when trying to track where the reference actually came from.

@oli-obk oli-obk added the A-diagnostics errors and warnings emitted by miri label Sep 8, 2022
@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2022

However, the stacked borrow violation is in lines 123 and 126, where data: [u8] is autorefed to a &mut [u8] for [u8]::as_mut_ptr.

Are you saying there already is UB earlier in the program, before line 152 is reached?

While the span is technically correct, it doesn't help when trying to track where the reference actually came from.

Ah, that's very different from "incorrect". :) Could you elaborate a bit more why you think that other span is more relevant than the one where 3540 is created and invalidated, and what lines 123 and 126 even have to do with tag 3540?

@RalfJung RalfJung changed the title Incorrect stacked borrows diagnostic span Unhelphful stacked borrows diagnostic span Sep 8, 2022
@SparkyPotato
Copy link
Author

Lines 123 and 126 call .as_mut_ptr() on the field .data of a DST, which is an [u8]. This causes the [u8] to be autorefed to an &mut [u8], which then has .as_mut_ptr() called. The pointer is then offset and returned as an allocation (the NonNull highlighted in the diagnostic).

Since the pointer was derived from an &mut [u8] of the entire memory block, I believe it is a stacked borrows violation when two allocations are created, even though they do not overlap. Miri doesn't highlight the place where the &mut was formed (lines 123 and 126), making it hard to figure out why it thinks the second pointer invalidates [0x18..0x100018] (the entire memory block).

Replacing the .as_mut_ptr() to addr_of_mut!(...).cast() fixes the error.

@saethlin
Copy link
Member

saethlin commented Sep 8, 2022

This is an order of operations issue. We find the project-local current span, then consider if the retag we're executing is a FnEntry and reach up to the parent for a span in the caller. But in this case, we are already in the caller because the span of the FnEntry retag is in core.

I'll cook up a little test for this, and I think just adding a bool to CurrentSpan will let us know if we should reach up to the parent or not.

@bors bors closed this as completed in 3e91125 Sep 24, 2022
RalfJung pushed a commit to RalfJung/rust that referenced this issue Oct 4, 2022
Don't back up past the caller when looking for an FnEntry span

Fixes rust-lang/miri#2536

This adds a fix for the logic as well as a regression test. In the new test `tests/fail/stacked_borrows/fnentry_invalidation2.rs`, before this PR, we display this diagnostic:
```
help: <3278> was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag
  --> tests/fail/stacked_borrows/fnentry_invalidation2.rs:13:5
   |
13 |     inner(&mut t);
   |     ^^^^^^^^^^^^^
```
Which is very misleading. It is not this call itself, but what happens within the call that invalidates the tag we want. With this PR, we get:
```
help: <2798> was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag inside this call
  --> tests/fail/stacked_borrows/fnentry_invalidation2.rs:20:13
   |
20 |     let _ = t.sli.as_mut_ptr();
   |             ^^^^^^^^^^^^^^^^^^
```
Which is much better.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics errors and warnings emitted by miri
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants