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

Experiment idea: -Zmiri-unique-library-types to control treating Box and Vec as Uniq retaggers #2728

Closed
CAD97 opened this issue Dec 13, 2022 · 4 comments

Comments

@CAD97
Copy link

CAD97 commented Dec 13, 2022

Currently, rustc tags Box as noalias, so the default behavior of Miri is to treat moving Box as a Uniq SB retag, invalidating any pointers to its interior.

[Raw]Vec is built on the same internal ptr::Unique. It has been considered if treating Vec as Uniq as well, though the fallout from doing so is very likely (and iirc, there was an experiment showing it to be) to make more programs UB than with Box.

Would it be possible/reasonable to expose a miri flag to treat Vec's pointer as unique? This wouldn't have any impact without also setting -Zmiri-retag-fields=all since Vec is larger than a scalar pair, but I'd like to be able to put Miri into super pedantic mode when testing my own unsafe code which should ideally be valid even under this extremely strict interpretation.

In short, I would like a (combination of) flags that allows the following snippet to be diagnosed as UB. By my understanding of the current SB rules, under the strictest form (always doing field retagging, ptr::Unique is unique), it is. Even if this is stricter than we end up actually adopting, I think having the ability to diagnose this as potential UB is a useful tool in making the decision, since it allows us to quantify how much crater-visible miri-testable code (doesn't have other issues and) is using Vec as a stand-in for RFC3336<Pin<Vec<_>>>.

let mut v = vec![0];
let p = addr_of!(v[0]);
let v = v;
&*p;

The flag could also allow more crates to benefit from Miri testing coverage with field retagging even if they're implicitly relying on -Zbox-noalias=no, if it allows opting out of Uniq retagging on Box (though obviously doing so would currently be unsound).

@RalfJung
Copy link
Member

Would it be possible/reasonable to expose a miri flag to treat Vec's pointer as unique?

Not currently. Stacked Borrows does not support noalias for data of unknown size, as would be required for Unique semantics.

We're currently working on a next aliasing model, which should support this, and then we can consider making Unique actually unique.

@RalfJung
Copy link
Member

The flag could also allow more crates to benefit from Miri testing coverage with field retagging even if they're implicitly relying on -Zbox-noalias=no

If such crates actually start to be a thing we should consider removing the flag from rustc, IMO. Certainly I don't want Miri to support such an ecosystem split.

@CAD97
Copy link
Author

CAD97 commented Dec 13, 2022

To be clear, I don't expect any crates to actually be using -Zbox-noalias=no. However, any crate providing self-referential types where the payload is just some &impl Sized (e.g. ouroboros or yoke1) is relying on UB as diagnosed by Miri (and avoided by -Zbox-noalias=no) until such a time as they can use an RFC#3336-like type. That's what I was attempting to say via "implicitly relying;" not actually using, but relying on UB removed by that flag being nonproblematic at the time being.

Footnotes

  1. Since yoke only allows shared access to the cart pointee, the noalias doesn't AIUI actually cause LLVM-level UB for Freeze types. It potentially does again if the pointee is !Freeze, though. Ouroboros allows mutably borrowing from the cart pointee, so potentially allows for LLVM-level UB when doing so even for Freeze types. Both are theoretically compatible with an RFC#3336 "dormant" cart owner, but aren't capable of using such until the language provides for it.

@RalfJung
Copy link
Member

This exists now in Tree Borrows, kind of: -Zmiri-unique-is-unique. It doesn't really have the semantics we want though. But that's better tracked at rust-lang/unsafe-code-guidelines#384.

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

No branches or pull requests

2 participants