Tracking issue for enabling field retagging by default

[RalfJung]

What has to happen for us to enable -Zmiri-retag-fields by default? LLVM already does optimizations that can only be explained on the Rust level if we do field retagging. This is the last case I am aware of of Rust code that has LLVM UB but is not considered UB by Miri. (There are other differences between Miri and the Reference, but none of those are LLVM UB -- all that extra UB is wiggle room for future optimizations, but not exploited yet.)

@saethlin has been doing some experiments here I think, but I am no aware of the current status. As a start, we should probably get miri-test-libstd to pass with field retagging.

Related issues:

• VecDeque's Drain::drop writes to memory that a shared reference points to rust#60076
• Scoped threads violate 'dereferenceable for function call' requirement of references rust#101983
• Release of hashbrown that includes Fix issue with field retagging in scopeguard hashbrown#359
• Release of scopeguard that includes Use ManuallyDrop instead of mem::forget in into_inner bluss/scopeguard#25

[saethlin]

has been doing some experiments here I think

I updated https://miri.saethlin.dev/ub to use field retagging a week or two ago. Generally, it looks like the ecosystem impact is minimal.

[RalfJung]

So for now the primary blocker is rust-lang/rust#60076.

[RalfJung]

miri-test-libstd now runs the libcore tests with -Zmiri-retag-fields.

liballoc fails due to rust-lang/rust#60076:

test collections::vec_deque::tests::test_drain ... error: Undefined Behavior: not granting access to tag <42280440> because that would remove [SharedReadOnly for <42659664>] which is protected because it is an argument of call 10005308
    --> alloc_miri_test/../library/alloc/src/collections/vec_deque/mod.rs:273:13
     |
273  |             ptr::copy(self.ptr().add(src), self.ptr().add(dst), len);
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <42280440> because that would remove [SharedReadOnly for <42659664>] which is protected because it is an argument of call 10005308
     |
     = 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: <42280440> was created here, as the base tag for alloc16971938
    --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/alloc.rs:95:14
     |
95   |     unsafe { __rust_alloc(layout.size(), layout.align()) }
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <42659664> is this argument
    --> alloc_miri_test/../library/alloc/src/collections/vec_deque/spec_extend.rs:17:39
     |
17   |     default fn spec_extend(&mut self, mut iter: I) {
     |                                       ^^^^^^^^

[RalfJung]

Interestingly a scope thread doctest also fails:

  0.000006   error: Undefined Behavior: not granting access to tag <39241> because that would remove [SharedReadOnly for <9450>] which is protected because it is an argument of call 2615
  0.000007       --> /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/vec/mod.rs:1764:17
  0.000006        |
  0.000005   1764 |     pub fn push(&mut self, value: T) {
  0.000006        |                 ^^^^^^^^^ not granting access to tag <39241> because that would remove [SharedReadOnly for <9450>] which is protected because it is an argument of call 2615
  0.000006        |
  0.000006        = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
  0.000006        = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
  0.000006   help: <39241> was created by a SharedReadWrite retag at offsets [0x0..0x18]
  0.000006       --> ../library/std/src/thread/scoped.rs:106:1
  0.000006        |
  0.000006   25   | a.push(4);
  0.000006        | ^^^^^^^^^
  0.000006   help: <9450> is this argument
  0.000006       --> /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/core/src/ops/function.rs:248:5
  0.000006        |
  0.000006   248  |     extern "rust-call" fn call_once(self, args: Args) -> Self::Output;
  0.000006        |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  0.000005        = note: BACKTRACE:
  0.000006        = note: inside `std::vec::Vec::<usize>::push` at /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/alloc/src/vec/mod.rs:1764:17
  0.000006   note: inside `main::_doctest_main____library_std_src_thread_scoped_rs_84_0` at ../library/std/src/thread/scoped.rs:25:1
  0.000006       --> ../library/std/src/thread/scoped.rs:106:1
  0.000006        |
  0.000006   25   | a.push(4);
  0.000006        | ^^^^^^^^^
  0.000006   note: inside `main` at ../library/std/src/thread/scoped.rs:27:3
  0.000006       --> ../library/std/src/thread/scoped.rs:108:3
  0.000006        |
  0.000006   27   | } _doctest_main____library_std_src_thread_scoped_rs_84_0() }
  0.000006        |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  0.000006   note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/core/src/ops/function.rs:248:5
  0.000005       --> /home/runner/work/miri-test-libstd/miri-test-libstd/rust-src-patched/library/core/src/ops/function.rs:248:5
  0.000006        |
  0.000006   248  |     extern "rust-call" fn call_once(self, args: Args) -> Self::Output;
  0.000006        |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The failing push is this one. The "is this argument" looks rather strange...

[RalfJung]

Ah, I think I know what happens... the spawned threads actually keep running after scope returns (we are not using the OS join but custom synchronization to join all the threads). One argument to that thread is the closure, which contains as a field a reference to a. Hence the protector requires the reference to stay alive for as long as this main function runs. Which may continue running until after scope returns.

However when we create a mutable reference to a for the push, that protected reference gets invalidated. 💥

[RalfJung]

Blocked on rust-lang/rust#101983

[RalfJung]

Now that rust-lang/rust#101983 is resolved -- should we enable field retagging by default? @saethlin probably has the best idea of how bad the fallout from what will be.

We will certainly have a flag to opt-out of this.

[saethlin]

"shockingly not very bad" is my extremely qualitative current answer. Give me about 7 hours and I'll have a better answer. (I realize that if I had started this when I first read your comment I'd probably have the answer by now, ah well)

[saethlin]

There should probably be a release for hashbrown and a patch to plus release of scopeguard based on rust-lang/hashbrown#359

[saethlin]

Here is what I currently think is a reasonably complete list of crates which report UB with field retagging on, but do not report UB with it off (sorted by recent downloads): https://gist.github.com/saethlin/7337b32a82d352659ed758a0ecded69d

There are 312 crates in that list, of the ~4400 crates which report UB. I think that number is high enough to be concerning but not a blocker, and I would like to know what it looks like with field retagging for newtypes only (which I believe is what rustc implements?).

I'll admit my methodology here leaves something to be desired, I'm in the middle of migrating my whole setup to use nextest (finally).

[RalfJung]

I would like to know what it looks like with field retagging for newtypes only (which I believe is what rustc implements?).

I think it's not just newtypes, ScalarPair layout can also have per-field attributes.

[RalfJung]

patch to plus release of scopeguard

Are you sure about the patch? scopeguard already does not use mem::forget.
A release seems needed though, v1.1.0 still uses mem::forget.

[saethlin]

I was probably just being lazy, I have gotten too used to some maintainers like dtolnay aggressively releasing every PR.

[RalfJung]

miri-test-libstd now enables full field retagging on all its tests, and things are passing. :)

In terms of the wider ecosystem, #2613 adds an option to do field retagging only for cases where we would emit noalias in LLVM. So to ease the migration and to focus on the more pressing cases first (where the corresponding LLVM IR has UB), maybe we want to make that the default first, some time soon-ish?

[saethlin]

Yes. I'm ~vacation at the moment but I'll punch in that crate list above with the new option and we will see what comes up. But honestly I can't imagine what I would come back with that I would think is a compelling argument to not make this new option the default.

[saethlin]

The list above contains 311 crates. Of those, only 70 report UB with -Zmiri-retag-fields=scalar: https://gist.github.com/saethlin/b8a0d25ca25ed85a534dca5e5756c974#file-miri-retag-fields-scalar

From #2613

I don't think this actually helps to make many things sound that are unsound under full field retagging -- but it might still be useful for some experimentation.

So my initial impression is that this is not accurate, but it could be if the are one or two dependencies causing the UB reports with full field retagging.

[RalfJung]

I'll need to take a look at those cases -- it might also be that they just happen to use the structs containing references in ways that they don't have ScalarPair ABI, but users could pick other generic type instances that do lead to a ScalarPair and thus still cause the UB. (IOW, it might remove some UB, but not actually help with soundness.)