Miri: better document and fix dynamic const pattern soundness checks #71655
Conversation
rust-highfive
added
the
S-waiting-on-review
@rpjohnst solved this puzzle: pattern-match compilation has no support for |
| const SLICE_MUT: &[u8; 1] = { //~ ERROR undefined behavior to use this value | ||
| //~| NOTE encountered a reference pointing to a static variable | ||
| //~| NOTE | ||
| unsafe { &static_cross_crate::ZERO } |
There was a problem hiding this comment.
won't this be worked around if you do &static_cross_crate::ZERO[0] here, and then pattern matching suddenly sees the mutable memory?
There was a problem hiding this comment.
Why would that happen? It's still a reference to a GlobalAlloc::Static.
I can add that testcase.
There was a problem hiding this comment.
You are right, that error does not get caught. But I do not understand why. The pointer should be the same, shouldn't it?
Are we somewhere accidentally "leaking" the internal pointer of the static, as opposed to the lazy one?
There was a problem hiding this comment.
Ah, never mind -- we have to actually use the constant to cause the error. (Not sure why though, we should be evaluating all consts whether they are used or not... probably because I did allow(const_err).)
There was a problem hiding this comment.
Are we somewhere accidentally "leaking" the internal pointer of the static, as opposed to the lazy one?
If that case isn't doing it, then using &&[u8] for the static and using &*static_cross_crate::ZERO will definitely do it, or by using an enum and getting a reference to a field. Any operation that needs to actually read from the static in order to produce the reference will yield the internal AllocId instead of the lazy one.
There was a problem hiding this comment.
Any operation that needs to actually read from the static in order to produce the reference will yield the internal AllocId instead of the lazy one.
I don't think that's true. The code in memory.rs is very careful to never leak the internal AllocId back to the interpreter.
Also, any code that actually reads from the static will fail the before_access_static dynamic check, and this is already covered.
|
@bors r+ |
|
📌 Commit 979bbf2 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- |
bors
added
S-waiting-on-author
|
All right, did that and also explained in @bors r=oli-obk |
|
📌 Commit 07772fc has been approved by |
bors
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
Rollup of 5 pull requests Successful merges: - rust-lang#71205 (rustc: fix check_attr() for methods, closures and foreign functions) - rust-lang#71540 (Suggest deref when coercing `ty::Ref` to `ty::RawPtr`) - rust-lang#71655 (Miri: better document and fix dynamic const pattern soundness checks) - rust-lang#71672 (document missing stable counterparts of intrinsics) - rust-lang#71692 (Add clarification on std::cfg macro docs v. #[cfg] attribute) Failed merges: r? @ghost
rust-lang/const-eval#42 got me thinking about soundness for consts being used in patterns, and I found a hole in our existing dynamic checks: a const referring to a mutable static in a different crate was not caught. This PR fixes that. It also adds some comments that explain which invariants are crucial for soundness of const-patterns.
Curiously, trying to weaponize this soundness hole failed: pattern matching compilation ICEd when encountering the cross-crate static, saying "expected allocation ID alloc0 to point to memory". I don't know why that would happen, statics should be entirely normal memory for pattern matching to access.
r? @oli-obk
Cc @rust-lang/wg-const-eval