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.