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

Ensure that evaluating or validating a constant never reads from a static #67337

Merged
merged 7 commits into from
Dec 24, 2019
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Dynamically prevent constants from accessing statics
  • Loading branch information
oli-obk committed Dec 23, 2019
commit ad6b9c79d63c3049f1ceb4a0ab0fa101c13de121
52 changes: 40 additions & 12 deletions src/librustc_mir/const_eval.rs
Original file line number Diff line number Diff line change
@@ -45,9 +45,15 @@ fn mk_eval_cx<'mir, 'tcx>(
tcx: TyCtxt<'tcx>,
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
span: Span,
param_env: ty::ParamEnv<'tcx>,
can_access_statics: bool,
) -> CompileTimeEvalContext<'mir, 'tcx> {
debug!("mk_eval_cx: {:?}", param_env);
InterpCx::new(tcx.at(span), param_env, CompileTimeInterpreter::new(), Default::default())
InterpCx::new(
tcx.at(span),
param_env,
CompileTimeInterpreter::new(),
MemoryExtra { can_access_statics },
)
}

fn op_to_const<'tcx>(
@@ -224,6 +230,12 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> {
pub(super) loop_detector: snapshot::InfiniteLoopDetector<'mir, 'tcx>,
}

#[derive(Copy, Clone, Debug)]
pub struct MemoryExtra {
/// Whether this machine may read from statics
can_access_statics: bool,
}

impl<'mir, 'tcx> CompileTimeInterpreter<'mir, 'tcx> {
fn new() -> Self {
CompileTimeInterpreter {
@@ -311,7 +323,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
type ExtraFnVal = !;

type FrameExtra = ();
type MemoryExtra = ();
type MemoryExtra = MemoryExtra;
type AllocExtra = ();

type MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>;
@@ -473,7 +485,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,

#[inline(always)]
fn init_allocation_extra<'b>(
_memory_extra: &(),
_memory_extra: &MemoryExtra,
_id: AllocId,
alloc: Cow<'b, Allocation>,
_kind: Option<MemoryKind<!>>,
@@ -484,7 +496,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,

#[inline(always)]
fn tag_static_base_pointer(
_memory_extra: &(),
_memory_extra: &MemoryExtra,
_id: AllocId,
) -> Self::PointerTag {
()
@@ -527,6 +539,19 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
fn stack_push(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
Ok(())
}

fn before_access_static(
memory_extra: &MemoryExtra,
_allocation: &Allocation,
) -> InterpResult<'tcx> {
if memory_extra.can_access_statics {
Ok(())
} else {
Err(ConstEvalError::NeedsRfc(
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
"constants accessing static items".to_string(),
).into())
}
}
}

/// Extracts a field of a (variant of a) const.
@@ -540,7 +565,7 @@ pub fn const_field<'tcx>(
value: &'tcx ty::Const<'tcx>,
) -> &'tcx ty::Const<'tcx> {
trace!("const_field: {:?}, {:?}", field, value);
let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env);
let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env, false);
// get the operand again
let op = ecx.eval_const_to_op(value, None).unwrap();
// downcast
@@ -560,7 +585,7 @@ pub fn const_caller_location<'tcx>(
(file, line, col): (Symbol, u32, u32),
) -> &'tcx ty::Const<'tcx> {
trace!("const_caller_location: {}:{}:{}", file, line, col);
let mut ecx = mk_eval_cx(tcx, DUMMY_SP, ty::ParamEnv::reveal_all());
let mut ecx = mk_eval_cx(tcx, DUMMY_SP, ty::ParamEnv::reveal_all(), false);

let loc_ty = tcx.caller_location_ty();
let loc_place = ecx.alloc_caller_location(file, line, col);
@@ -581,7 +606,7 @@ pub fn const_variant_index<'tcx>(
val: &'tcx ty::Const<'tcx>,
) -> VariantIdx {
trace!("const_variant_index: {:?}", val);
let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env);
let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env, false);
let op = ecx.eval_const_to_op(val, None).unwrap();
ecx.read_discriminant(op).unwrap().1
}
@@ -610,7 +635,9 @@ fn validate_and_turn_into_const<'tcx>(
key: ty::ParamEnvAnd<'tcx, GlobalId<'tcx>>,
) -> ::rustc::mir::interpret::ConstEvalResult<'tcx> {
let cid = key.value;
let ecx = mk_eval_cx(tcx, tcx.def_span(key.value.instance.def_id()), key.param_env);
let def_id = cid.instance.def.def_id();
let is_static = tcx.is_static(def_id);
let ecx = mk_eval_cx(tcx, tcx.def_span(key.value.instance.def_id()), key.param_env, is_static);
let val = (|| {
let mplace = ecx.raw_const_to_mplace(constant)?;
let mut ref_tracking = RefTracking::new(mplace);
@@ -624,8 +651,7 @@ fn validate_and_turn_into_const<'tcx>(
// Now that we validated, turn this into a proper constant.
// Statics/promoteds are always `ByRef`, for the rest `op_to_const` decides
// whether they become immediates.
let def_id = cid.instance.def.def_id();
if tcx.is_static(def_id) || cid.promoted.is_some() {
if is_static || cid.promoted.is_some() {
let ptr = mplace.ptr.to_ptr()?;
Ok(tcx.mk_const(ty::Const {
val: ty::ConstKind::Value(ConstValue::ByRef {
@@ -732,12 +758,14 @@ pub fn const_eval_raw_provider<'tcx>(
return Err(ErrorHandled::Reported);
}

let is_static = tcx.is_static(def_id);

let span = tcx.def_span(cid.instance.def_id());
let mut ecx = InterpCx::new(
tcx.at(span),
key.param_env,
CompileTimeInterpreter::new(),
Default::default()
MemoryExtra { can_access_statics: is_static },
);

let res = ecx.load_mir(cid.instance.def, cid.promoted);
@@ -751,7 +779,7 @@ pub fn const_eval_raw_provider<'tcx>(
}).map_err(|error| {
let err = error_to_const_error(&ecx, error);
// errors in statics are always emitted as fatal errors
if tcx.is_static(def_id) {
if is_static {
// Ensure that if the above error was either `TooGeneric` or `Reported`
// an error must be reported.
let v = err.report_as_error(ecx.tcx, "could not evaluate static initializer");
1 change: 0 additions & 1 deletion src/librustc_mir/interpret/intern.rs
Original file line number Diff line number Diff line change
@@ -20,7 +20,6 @@ pub trait CompileTimeMachine<'mir, 'tcx> = Machine<
PointerTag = (),
ExtraFnVal = !,
FrameExtra = (),
MemoryExtra = (),
AllocExtra = (),
MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>,
>;
5 changes: 4 additions & 1 deletion src/librustc_mir/interpret/machine.rs
Original file line number Diff line number Diff line change
@@ -212,7 +212,10 @@ pub trait Machine<'mir, 'tcx>: Sized {
}

/// Called before a `StaticKind::Static` value is accessed.
fn before_access_static(_allocation: &Allocation) -> InterpResult<'tcx> {
fn before_access_static(
_memory_extra: &Self::MemoryExtra,
_allocation: &Allocation,
) -> InterpResult<'tcx> {
Ok(())
}

7 changes: 4 additions & 3 deletions src/librustc_mir/interpret/memory.rs
Original file line number Diff line number Diff line change
@@ -116,15 +116,16 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> HasDataLayout for Memory<'mir, 'tcx, M>
// carefully copy only the reachable parts.
impl<'mir, 'tcx, M> Clone for Memory<'mir, 'tcx, M>
where
M: Machine<'mir, 'tcx, PointerTag = (), AllocExtra = (), MemoryExtra = ()>,
M: Machine<'mir, 'tcx, PointerTag = (), AllocExtra = ()>,
M::MemoryExtra: Copy,
M::MemoryMap: AllocMap<AllocId, (MemoryKind<M::MemoryKinds>, Allocation)>,
{
fn clone(&self) -> Self {
Memory {
alloc_map: self.alloc_map.clone(),
extra_fn_ptr_map: self.extra_fn_ptr_map.clone(),
dead_alloc_map: self.dead_alloc_map.clone(),
extra: (),
extra: self.extra,
tcx: self.tcx,
}
}
@@ -455,7 +456,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
let id = raw_const.alloc_id;
let allocation = tcx.alloc_map.lock().unwrap_memory(id);

M::before_access_static(allocation)?;
M::before_access_static(memory_extra, allocation)?;
Cow::Borrowed(allocation)
}
}
1 change: 1 addition & 0 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
@@ -224,6 +224,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine {
}

fn before_access_static(
_memory_extra: &(),
allocation: &Allocation<Self::PointerTag, Self::AllocExtra>,
) -> InterpResult<'tcx> {
// if the static allocation is mutable or if it has relocations (it may be legal to mutate
12 changes: 12 additions & 0 deletions src/test/ui/consts/const-points-to-static.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// compile-flags: -Zunleash-the-miri-inside-of-you

#![allow(dead_code)]

const TEST: &u8 = &MY_STATIC;
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
//~^ skipping const checks
//~| it is undefined behavior to use this value

static MY_STATIC: u8 = 4;

fn main() {
}
17 changes: 17 additions & 0 deletions src/test/ui/consts/const-points-to-static.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
warning: skipping const checks
--> $DIR/const-points-to-static.rs:5:20
|
LL | const TEST: &u8 = &MY_STATIC;
| ^^^^^^^^^

error[E0080]: it is undefined behavior to use this value
--> $DIR/const-points-to-static.rs:5:1
|
LL | const TEST: &u8 = &MY_STATIC;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ "constants accessing static items" needs an rfc before being allowed inside constants
|
= 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.

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.
3 changes: 1 addition & 2 deletions src/test/ui/consts/const-prop-read-static-in-const.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
// compile-flags: -Zunleash-the-miri-inside-of-you
// run-pass

#![allow(dead_code)]

const TEST: u8 = MY_STATIC;
const TEST: u8 = MY_STATIC; //~ ERROR any use of this value will cause an error
//~^ skipping const checks

static MY_STATIC: u8 = 4;
14 changes: 13 additions & 1 deletion src/test/ui/consts/const-prop-read-static-in-const.stderr
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
warning: skipping const checks
--> $DIR/const-prop-read-static-in-const.rs:6:18
--> $DIR/const-prop-read-static-in-const.rs:5:18
|
LL | const TEST: u8 = MY_STATIC;
| ^^^^^^^^^

error: any use of this value will cause an error
--> $DIR/const-prop-read-static-in-const.rs:5:18
|
LL | const TEST: u8 = MY_STATIC;
| -----------------^^^^^^^^^-
| |
| "constants accessing static items" needs an rfc before being allowed inside constants
|
= note: `#[deny(const_err)]` on by default

error: aborting due to previous error

38 changes: 38 additions & 0 deletions src/test/ui/consts/miri_unleashed/const_refers_to_static.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// compile-flags: -Zunleash-the-miri-inside-of-you
#![allow(const_err)]

#![feature(const_raw_ptr_deref)]

use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering;

const BOO: &usize = { //~ ERROR undefined behavior to use this value
static FOO: AtomicUsize = AtomicUsize::new(0);
unsafe { &*(&FOO as *const _ as *const usize) }
//~^ WARN skipping const checks
};

const FOO: usize = {
static FOO: AtomicUsize = AtomicUsize::new(0);
FOO.fetch_add(1, Ordering::Relaxed) // FIXME: this should error
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
//~^ WARN skipping const checks
//~| WARN skipping const checks
};

const BAR: usize = {
static FOO: AtomicUsize = AtomicUsize::new(0);
unsafe { *(&FOO as *const _ as *const usize) } // FIXME: this should error
//~^ WARN skipping const checks
};

static mut MUTABLE: u32 = 0;
const BAD: u32 = unsafe { MUTABLE }; // FIXME: this should error
//~^ WARN skipping const checks

// ok some day perhaps
const BOO_OK: &usize = { //~ ERROR it is undefined behavior to use this value
static FOO: usize = 0;
&FOO
//~^ WARN skipping const checks
};
fn main() {}
63 changes: 63 additions & 0 deletions src/test/ui/consts/miri_unleashed/const_refers_to_static.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
warning: skipping const checks
--> $DIR/const_refers_to_static.rs:11:18
|
LL | unsafe { &*(&FOO as *const _ as *const usize) }
| ^^^

warning: skipping const checks
--> $DIR/const_refers_to_static.rs:17:5
|
LL | FOO.fetch_add(1, Ordering::Relaxed) // FIXME: this should error
| ^^^

warning: skipping const checks
--> $DIR/const_refers_to_static.rs:17:5
|
LL | FOO.fetch_add(1, Ordering::Relaxed) // FIXME: this should error
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: skipping const checks
--> $DIR/const_refers_to_static.rs:24:17
|
LL | unsafe { *(&FOO as *const _ as *const usize) } // FIXME: this should error
| ^^^

warning: skipping const checks
--> $DIR/const_refers_to_static.rs:29:27
|
LL | const BAD: u32 = unsafe { MUTABLE }; // FIXME: this should error
| ^^^^^^^

warning: skipping const checks
--> $DIR/const_refers_to_static.rs:35:6
|
LL | &FOO
| ^^^

error[E0080]: it is undefined behavior to use this value
--> $DIR/const_refers_to_static.rs:9:1
|
LL | / const BOO: &usize = {
LL | | static FOO: AtomicUsize = AtomicUsize::new(0);
LL | | unsafe { &*(&FOO as *const _ as *const usize) }
LL | |
LL | | };
| |__^ "constants accessing static items" needs an rfc before being allowed inside constants
|
= 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.

error[E0080]: it is undefined behavior to use this value
--> $DIR/const_refers_to_static.rs:33:1
|
LL | / const BOO_OK: &usize = {
LL | | static FOO: usize = 0;
LL | | &FOO
LL | |
LL | | };
| |__^ "constants accessing static items" needs an rfc before being allowed inside constants
|
= 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.

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
1 change: 1 addition & 0 deletions src/test/ui/issues/issue-52060.rs
Original file line number Diff line number Diff line change
@@ -3,5 +3,6 @@
static A: &'static [u32] = &[1];
static B: [u32; 1] = [0; A.len()];
//~^ ERROR [E0013]
//~| ERROR evaluation of constant value failed

fn main() {}
11 changes: 9 additions & 2 deletions src/test/ui/issues/issue-52060.stderr
Original file line number Diff line number Diff line change
@@ -4,6 +4,13 @@ error[E0013]: constants cannot refer to statics, use a constant instead
LL | static B: [u32; 1] = [0; A.len()];
| ^

error: aborting due to previous error
error[E0080]: evaluation of constant value failed
--> $DIR/issue-52060.rs:4:26
|
LL | static B: [u32; 1] = [0; A.len()];
| ^ "constants accessing static items" needs an rfc before being allowed inside constants

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0013`.
Some errors have detailed explanations: E0013, E0080.
For more information about an error, try `rustc --explain E0013`.