Skip to content

Commit 71bf986

Browse files
authored
Rollup merge of #71655 - RalfJung:const-pattern-soundness, r=oli-obk
Miri: better document and fix dynamic const pattern soundness checks 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
2 parents 58d955e + 07772fc commit 71bf986

11 files changed

+250
-24
lines changed

src/librustc_mir/const_eval/eval_queries.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ fn validate_and_turn_into_const<'tcx>(
193193
mplace.into(),
194194
path,
195195
&mut ref_tracking,
196-
/*may_ref_to_static*/ is_static,
196+
/*may_ref_to_static*/ ecx.memory.extra.can_access_statics,
197197
)?;
198198
}
199199
}

src/librustc_mir/const_eval/machine.rs

+10-1
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,12 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> {
9999

100100
#[derive(Copy, Clone, Debug)]
101101
pub struct MemoryExtra {
102-
/// Whether this machine may read from statics
102+
/// We need to make sure consts never point to anything mutable, even recursively. That is
103+
/// relied on for pattern matching on consts with references.
104+
/// To achieve this, two pieces have to work together:
105+
/// * Interning makes everything outside of statics immutable.
106+
/// * Pointers to allocations inside of statics can never leak outside, to a non-static global.
107+
/// This boolean here controls the second part.
103108
pub(super) can_access_statics: bool,
104109
}
105110

@@ -337,6 +342,10 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
337342
} else if static_def_id.is_some() {
338343
// Machine configuration does not allow us to read statics
339344
// (e.g., `const` initializer).
345+
// See const_eval::machine::MemoryExtra::can_access_statics for why
346+
// this check is so important: if we could read statics, we could read pointers
347+
// to mutable allocations *inside* statics. These allocations are not themselves
348+
// statics, so pointers to them can get around the check in `validity.rs`.
340349
Err(ConstEvalErrKind::ConstAccessesStatic.into())
341350
} else {
342351
// Immutable global, this read is fine.

src/librustc_mir/interpret/memory.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
453453
// thing here: one maps to `GlobalAlloc::Static`, this is the "lazy" ID,
454454
// and the other one is maps to `GlobalAlloc::Memory`, this is returned by
455455
// `const_eval_raw` and it is the "resolved" ID.
456-
// The resolved ID is never used by the interpreted progrma, it is hidden.
456+
// The resolved ID is never used by the interpreted program, it is hidden.
457+
// This is relied upon for soundness of const-patterns; a pointer to the resolved
458+
// ID would "sidestep" the checks that make sure consts do not point to statics!
457459
// The `GlobalAlloc::Memory` branch here is still reachable though; when a static
458460
// contains a reference to memory that was created during its evaluation (i.e., not
459461
// to another static), those inner references only exist in "resolved" form.

src/librustc_mir/interpret/validity.rs

+15-7
Original file line numberDiff line numberDiff line change
@@ -404,19 +404,27 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
404404
// Skip validation entirely for some external statics
405405
let alloc_kind = self.ecx.tcx.alloc_map.lock().get(ptr.alloc_id);
406406
if let Some(GlobalAlloc::Static(did)) = alloc_kind {
407-
// `extern static` cannot be validated as they have no body.
408-
// FIXME: Statics from other crates are also skipped.
409-
// They might be checked at a different type, but for now we
410-
// want to avoid recursing too deeply. This is not sound!
411-
if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
412-
return Ok(());
413-
}
407+
// See const_eval::machine::MemoryExtra::can_access_statics for why
408+
// this check is so important.
409+
// This check is reachable when the const just referenced the static,
410+
// but never read it (so we never entered `before_access_global`).
411+
// We also need to do it here instead of going on to avoid running
412+
// into the `before_access_global` check during validation.
414413
if !self.may_ref_to_static && self.ecx.tcx.is_static(did) {
415414
throw_validation_failure!(
416415
format_args!("a {} pointing to a static variable", kind),
417416
self.path
418417
);
419418
}
419+
// `extern static` cannot be validated as they have no body.
420+
// FIXME: Statics from other crates are also skipped.
421+
// They might be checked at a different type, but for now we
422+
// want to avoid recursing too deeply. We might miss const-invalid data,
423+
// but things are still sound otherwise (in particular re: consts
424+
// referring to statics).
425+
if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
426+
return Ok(());
427+
}
420428
}
421429
}
422430
// Proceed recursively even for ZST, no reason to skip them!
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
pub static mut ZERO: [u8; 1] = [0];
2+
pub static ZERO_REF: &[u8; 1] = unsafe { &ZERO };
3+
pub static mut OPT_ZERO: Option<u8> = Some(0);

src/test/ui/consts/miri_unleashed/const_refers_to_static.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
use std::sync::atomic::AtomicUsize;
88
use std::sync::atomic::Ordering;
99

10-
// These tests only cause an error when *using* the const.
10+
// These fail during CTFE (as they read a static), so they only cause an error
11+
// when *using* the const.
1112

1213
const MUTATE_INTERIOR_MUT: usize = {
1314
static FOO: AtomicUsize = AtomicUsize::new(0);

src/test/ui/consts/miri_unleashed/const_refers_to_static.stderr

+8-8
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,47 @@
11
warning: skipping const checks
2-
--> $DIR/const_refers_to_static.rs:14:5
2+
--> $DIR/const_refers_to_static.rs:15:5
33
|
44
LL | FOO.fetch_add(1, Ordering::Relaxed)
55
| ^^^
66

77
warning: skipping const checks
8-
--> $DIR/const_refers_to_static.rs:14:5
8+
--> $DIR/const_refers_to_static.rs:15:5
99
|
1010
LL | FOO.fetch_add(1, Ordering::Relaxed)
1111
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1212

1313
warning: skipping const checks
14-
--> $DIR/const_refers_to_static.rs:21:17
14+
--> $DIR/const_refers_to_static.rs:22:17
1515
|
1616
LL | unsafe { *(&FOO as *const _ as *const usize) }
1717
| ^^^
1818

1919
warning: skipping const checks
20-
--> $DIR/const_refers_to_static.rs:26:32
20+
--> $DIR/const_refers_to_static.rs:27:32
2121
|
2222
LL | const READ_MUT: u32 = unsafe { MUTABLE };
2323
| ^^^^^^^
2424

2525
warning: skipping const checks
26-
--> $DIR/const_refers_to_static.rs:26:32
26+
--> $DIR/const_refers_to_static.rs:27:32
2727
|
2828
LL | const READ_MUT: u32 = unsafe { MUTABLE };
2929
| ^^^^^^^
3030

3131
error[E0080]: erroneous constant used
32-
--> $DIR/const_refers_to_static.rs:31:5
32+
--> $DIR/const_refers_to_static.rs:32:5
3333
|
3434
LL | MUTATE_INTERIOR_MUT;
3535
| ^^^^^^^^^^^^^^^^^^^ referenced constant has errors
3636

3737
error[E0080]: erroneous constant used
38-
--> $DIR/const_refers_to_static.rs:33:5
38+
--> $DIR/const_refers_to_static.rs:34:5
3939
|
4040
LL | READ_INTERIOR_MUT;
4141
| ^^^^^^^^^^^^^^^^^ referenced constant has errors
4242

4343
error[E0080]: erroneous constant used
44-
--> $DIR/const_refers_to_static.rs:35:5
44+
--> $DIR/const_refers_to_static.rs:36:5
4545
|
4646
LL | READ_MUT;
4747
| ^^^^^^^^ referenced constant has errors

src/test/ui/consts/miri_unleashed/const_refers_to_static2.rs

+6-1
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,21 @@
66
use std::sync::atomic::AtomicUsize;
77
use std::sync::atomic::Ordering;
88

9-
// These tests cause immediate error when *defining* the const.
9+
// These only fail during validation (they do not use but just create a reference to a static),
10+
// so they cause an immediate error when *defining* the const.
1011

1112
const REF_INTERIOR_MUT: &usize = { //~ ERROR undefined behavior to use this value
13+
//~| NOTE encountered a reference pointing to a static variable
14+
//~| NOTE
1215
static FOO: AtomicUsize = AtomicUsize::new(0);
1316
unsafe { &*(&FOO as *const _ as *const usize) }
1417
//~^ WARN skipping const checks
1518
};
1619

1720
// ok some day perhaps
1821
const READ_IMMUT: &usize = { //~ ERROR it is undefined behavior to use this value
22+
//~| NOTE encountered a reference pointing to a static variable
23+
//~| NOTE
1924
static FOO: usize = 0;
2025
&FOO
2126
//~^ WARN skipping const checks

src/test/ui/consts/miri_unleashed/const_refers_to_static2.stderr

+8-4
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,21 @@
11
warning: skipping const checks
2-
--> $DIR/const_refers_to_static2.rs:13:18
2+
--> $DIR/const_refers_to_static2.rs:16:18
33
|
44
LL | unsafe { &*(&FOO as *const _ as *const usize) }
55
| ^^^
66

77
warning: skipping const checks
8-
--> $DIR/const_refers_to_static2.rs:20:6
8+
--> $DIR/const_refers_to_static2.rs:25:6
99
|
1010
LL | &FOO
1111
| ^^^
1212

1313
error[E0080]: it is undefined behavior to use this value
14-
--> $DIR/const_refers_to_static2.rs:11:1
14+
--> $DIR/const_refers_to_static2.rs:12:1
1515
|
1616
LL | / const REF_INTERIOR_MUT: &usize = {
17+
LL | |
18+
LL | |
1719
LL | | static FOO: AtomicUsize = AtomicUsize::new(0);
1820
LL | | unsafe { &*(&FOO as *const _ as *const usize) }
1921
LL | |
@@ -23,9 +25,11 @@ LL | | };
2325
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
2426

2527
error[E0080]: it is undefined behavior to use this value
26-
--> $DIR/const_refers_to_static2.rs:18:1
28+
--> $DIR/const_refers_to_static2.rs:21:1
2729
|
2830
LL | / const READ_IMMUT: &usize = {
31+
LL | |
32+
LL | |
2933
LL | | static FOO: usize = 0;
3034
LL | | &FOO
3135
LL | |
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// compile-flags: -Zunleash-the-miri-inside-of-you -Zdeduplicate-diagnostics
2+
// aux-build:static_cross_crate.rs
3+
#![allow(const_err)]
4+
5+
#![feature(exclusive_range_pattern, half_open_range_patterns, const_if_match, const_panic)]
6+
7+
extern crate static_cross_crate;
8+
9+
// Sneaky: reference to a mutable static.
10+
// Allowing this would be a disaster for pattern matching, we could violate exhaustiveness checking!
11+
const SLICE_MUT: &[u8; 1] = { //~ ERROR undefined behavior to use this value
12+
//~| NOTE encountered a reference pointing to a static variable
13+
//~| NOTE
14+
unsafe { &static_cross_crate::ZERO }
15+
//~^ WARN skipping const checks
16+
};
17+
18+
const U8_MUT: &u8 = { //~ ERROR undefined behavior to use this value
19+
//~| NOTE encountered a reference pointing to a static variable
20+
//~| NOTE
21+
unsafe { &static_cross_crate::ZERO[0] }
22+
//~^ WARN skipping const checks
23+
};
24+
25+
// Also test indirection that reads from other static. This causes a const_err.
26+
#[warn(const_err)] //~ NOTE
27+
const U8_MUT2: &u8 = { //~ NOTE
28+
unsafe { &(*static_cross_crate::ZERO_REF)[0] }
29+
//~^ WARN skipping const checks
30+
//~| WARN [const_err]
31+
//~| NOTE constant accesses static
32+
};
33+
#[warn(const_err)] //~ NOTE
34+
const U8_MUT3: &u8 = { //~ NOTE
35+
unsafe { match static_cross_crate::OPT_ZERO { Some(ref u) => u, None => panic!() } }
36+
//~^ WARN skipping const checks
37+
//~| WARN [const_err]
38+
//~| NOTE constant accesses static
39+
};
40+
41+
pub fn test(x: &[u8; 1]) -> bool {
42+
match x {
43+
SLICE_MUT => true,
44+
//~^ ERROR could not evaluate constant pattern
45+
&[1..] => false,
46+
}
47+
}
48+
49+
pub fn test2(x: &u8) -> bool {
50+
match x {
51+
U8_MUT => true,
52+
//~^ ERROR could not evaluate constant pattern
53+
&(1..) => false,
54+
}
55+
}
56+
57+
// We need to use these *in a pattern* to trigger the failure... likely because
58+
// the errors above otherwise stop compilation too early?
59+
pub fn test3(x: &u8) -> bool {
60+
match x {
61+
U8_MUT2 => true,
62+
//~^ ERROR could not evaluate constant pattern
63+
&(1..) => false,
64+
}
65+
}
66+
pub fn test4(x: &u8) -> bool {
67+
match x {
68+
U8_MUT3 => true,
69+
//~^ ERROR could not evaluate constant pattern
70+
&(1..) => false,
71+
}
72+
}
73+
74+
fn main() {
75+
unsafe {
76+
static_cross_crate::ZERO[0] = 1;
77+
}
78+
// Now the pattern is not exhaustive any more!
79+
test(&[0]);
80+
test2(&0);
81+
}

0 commit comments

Comments
 (0)