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

const-eval: make misalignment a hard error #115524

Merged
merged 2 commits into from
Oct 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
14 changes: 3 additions & 11 deletions compiler/rustc_const_eval/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
use crate::const_eval::CheckAlignment;
use crate::errors::ConstEvalError;

use either::{Left, Right};

use rustc_hir::def::DefKind;
Expand All @@ -15,7 +12,9 @@ use rustc_span::source_map::Span;
use rustc_target::abi::{self, Abi};

use super::{CanAccessStatics, CompileTimeEvalContext, CompileTimeInterpreter};
use crate::const_eval::CheckAlignment;
use crate::errors;
use crate::errors::ConstEvalError;
use crate::interpret::eval_nullary_intrinsic;
use crate::interpret::{
intern_const_alloc_recursive, CtfeValidationMode, GlobalId, Immediate, InternKind, InterpCx,
Expand Down Expand Up @@ -290,14 +289,7 @@ pub fn eval_to_allocation_raw_provider<'tcx>(
key.param_env,
// Statics (and promoteds inside statics) may access other statics, because unlike consts
// they do not have to behave "as if" they were evaluated at runtime.
CompileTimeInterpreter::new(
CanAccessStatics::from(is_static),
if tcx.sess.opts.unstable_opts.extra_const_ub_checks {
CheckAlignment::Error
} else {
CheckAlignment::FutureIncompat
},
),
CompileTimeInterpreter::new(CanAccessStatics::from(is_static), CheckAlignment::Error),
);

let res = ecx.load_mir(cid.instance.def, cid.promoted);
Expand Down
55 changes: 5 additions & 50 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use rustc_hir::def::DefKind;
use rustc_hir::{LangItem, CRATE_HIR_ID};
use rustc_hir::LangItem;
use rustc_middle::mir;
use rustc_middle::mir::interpret::PointerArithmetic;
use rustc_middle::ty::layout::{FnAbiOf, TyAndLayout};
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::lint::builtin::INVALID_ALIGNMENT;
use std::borrow::Borrow;
use std::hash::Hash;
use std::ops::ControlFlow;
Expand All @@ -21,11 +20,11 @@ use rustc_target::abi::{Align, Size};
use rustc_target::spec::abi::Abi as CallAbi;

use crate::errors::{LongRunning, LongRunningWarn};
use crate::fluent_generated as fluent;
use crate::interpret::{
self, compile_time_machine, AllocId, ConstAllocation, FnArg, FnVal, Frame, ImmTy, InterpCx,
InterpResult, OpTy, PlaceTy, Pointer, Scalar,
};
use crate::{errors, fluent_generated as fluent};

use super::error::*;

Expand Down Expand Up @@ -65,22 +64,11 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> {

#[derive(Copy, Clone)]
pub enum CheckAlignment {
/// Ignore alignment when following relocations.
/// Ignore all alignment requirements.
/// This is mainly used in interning.
No,
/// Hard error when dereferencing a misaligned pointer.
Error,
/// Emit a future incompat lint when dereferencing a misaligned pointer.
FutureIncompat,
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
}

impl CheckAlignment {
pub fn should_check(&self) -> bool {
match self {
CheckAlignment::No => false,
CheckAlignment::Error | CheckAlignment::FutureIncompat => true,
}
}
}

#[derive(Copy, Clone, PartialEq)]
Expand Down Expand Up @@ -358,48 +346,15 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
const PANIC_ON_ALLOC_FAIL: bool = false; // will be raised as a proper error

#[inline(always)]
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> CheckAlignment {
ecx.machine.check_alignment
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool {
matches!(ecx.machine.check_alignment, CheckAlignment::Error)
}

#[inline(always)]
fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>, layout: TyAndLayout<'tcx>) -> bool {
ecx.tcx.sess.opts.unstable_opts.extra_const_ub_checks || layout.abi.is_uninhabited()
}

fn alignment_check_failed(
ecx: &InterpCx<'mir, 'tcx, Self>,
has: Align,
required: Align,
check: CheckAlignment,
) -> InterpResult<'tcx, ()> {
let err = err_ub!(AlignmentCheckFailed { has, required }).into();
match check {
CheckAlignment::Error => Err(err),
CheckAlignment::No => span_bug!(
ecx.cur_span(),
"`alignment_check_failed` called when no alignment check requested"
),
CheckAlignment::FutureIncompat => {
let (_, backtrace) = err.into_parts();
backtrace.print_backtrace();
let (span, frames) = super::get_span_and_frames(&ecx);

ecx.tcx.emit_spanned_lint(
INVALID_ALIGNMENT,
ecx.stack().iter().find_map(|frame| frame.lint_root()).unwrap_or(CRATE_HIR_ID),
span,
errors::AlignmentCheckFailed {
has: has.bytes(),
required: required.bytes(),
frames,
},
);
Ok(())
}
}
}

fn load_mir(
ecx: &InterpCx<'mir, 'tcx, Self>,
instance: ty::InstanceDef<'tcx>,
Expand Down
13 changes: 2 additions & 11 deletions compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,9 @@ use rustc_middle::mir;
use rustc_middle::ty::layout::TyAndLayout;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::def_id::DefId;
use rustc_target::abi::{Align, Size};
use rustc_target::abi::Size;
use rustc_target::spec::abi::Abi as CallAbi;

use crate::const_eval::CheckAlignment;

use super::{
AllocBytes, AllocId, AllocRange, Allocation, ConstAllocation, FnArg, Frame, ImmTy, InterpCx,
InterpResult, MPlaceTy, MemoryKind, OpTy, PlaceTy, Pointer, Provenance,
Expand Down Expand Up @@ -134,21 +132,14 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
const POST_MONO_CHECKS: bool = true;

/// Whether memory accesses should be alignment-checked.
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> CheckAlignment;
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool;

/// Whether, when checking alignment, we should look at the actual address and thus support
/// custom alignment logic based on whatever the integer address happens to be.
///
/// If this returns true, Provenance::OFFSET_IS_ADDR must be true.
fn use_addr_for_alignment_check(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool;

fn alignment_check_failed(
ecx: &InterpCx<'mir, 'tcx, Self>,
has: Align,
required: Align,
check: CheckAlignment,
) -> InterpResult<'tcx, ()>;

/// Whether to enforce the validity invariant for a specific layout.
fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>, layout: TyAndLayout<'tcx>) -> bool;

Expand Down
49 changes: 18 additions & 31 deletions compiler/rustc_const_eval/src/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use rustc_middle::mir::display_allocation;
use rustc_middle::ty::{self, Instance, ParamEnv, Ty, TyCtxt};
use rustc_target::abi::{Align, HasDataLayout, Size};

use crate::const_eval::CheckAlignment;
use crate::fluent_generated as fluent;

use super::{
Expand Down Expand Up @@ -373,8 +372,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.check_and_deref_ptr(
ptr,
size,
align,
M::enforce_alignment(self),
M::enforce_alignment(self).then_some(align),
CheckInAllocMsg::MemoryAccessTest,
|alloc_id, offset, prov| {
let (size, align) = self
Expand All @@ -395,17 +393,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
align: Align,
msg: CheckInAllocMsg,
) -> InterpResult<'tcx> {
self.check_and_deref_ptr(
ptr,
size,
align,
CheckAlignment::Error,
msg,
|alloc_id, _, _| {
let (size, align) = self.get_live_alloc_size_and_align(alloc_id, msg)?;
Ok((size, align, ()))
},
)?;
self.check_and_deref_ptr(ptr, size, Some(align), msg, |alloc_id, _, _| {
let (size, align) = self.get_live_alloc_size_and_align(alloc_id, msg)?;
Ok((size, align, ()))
})?;
Ok(())
}

Expand All @@ -419,8 +410,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
&self,
ptr: Pointer<Option<M::Provenance>>,
size: Size,
align: Align,
check: CheckAlignment,
align: Option<Align>,
msg: CheckInAllocMsg,
alloc_size: impl FnOnce(
AllocId,
Expand All @@ -436,8 +426,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
throw_ub!(DanglingIntPointer(addr, msg));
}
// Must be aligned.
if check.should_check() {
self.check_offset_align(addr, align, check)?;
if let Some(align) = align {
self.check_offset_align(addr, align)?;
}
None
}
Expand All @@ -460,16 +450,16 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
// Test align. Check this last; if both bounds and alignment are violated
// we want the error to be about the bounds.
if check.should_check() {
if let Some(align) = align {
if M::use_addr_for_alignment_check(self) {
// `use_addr_for_alignment_check` can only be true if `OFFSET_IS_ADDR` is true.
self.check_offset_align(ptr.addr().bytes(), align, check)?;
self.check_offset_align(ptr.addr().bytes(), align)?;
} else {
// Check allocation alignment and offset alignment.
if alloc_align.bytes() < align.bytes() {
M::alignment_check_failed(self, alloc_align, align, check)?;
throw_ub!(AlignmentCheckFailed { has: alloc_align, required: align });
}
self.check_offset_align(offset.bytes(), align, check)?;
self.check_offset_align(offset.bytes(), align)?;
}
}

Expand All @@ -480,18 +470,16 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
})
}

fn check_offset_align(
&self,
offset: u64,
align: Align,
check: CheckAlignment,
) -> InterpResult<'tcx> {
fn check_offset_align(&self, offset: u64, align: Align) -> InterpResult<'tcx> {
if offset % align.bytes() == 0 {
Ok(())
} else {
// The biggest power of two through which `offset` is divisible.
let offset_pow2 = 1 << offset.trailing_zeros();
M::alignment_check_failed(self, Align::from_bytes(offset_pow2).unwrap(), align, check)
throw_ub!(AlignmentCheckFailed {
has: Align::from_bytes(offset_pow2).unwrap(),
required: align
});
}
}
}
Expand Down Expand Up @@ -609,8 +597,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let ptr_and_alloc = self.check_and_deref_ptr(
ptr,
size,
align,
M::enforce_alignment(self),
M::enforce_alignment(self).then_some(align),
CheckInAllocMsg::MemoryAccessTest,
|alloc_id, offset, prov| {
let alloc = self.get_alloc_raw(alloc_id)?;
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,7 @@ where
.size_and_align_of_mplace(&mplace)?
.unwrap_or((mplace.layout.size, mplace.layout.align.abi));
// Due to packed places, only `mplace.align` matters.
let align =
if M::enforce_alignment(self).should_check() { mplace.align } else { Align::ONE };
let align = if M::enforce_alignment(self) { mplace.align } else { Align::ONE };
self.check_ptr_access_align(mplace.ptr(), size, align, CheckInAllocMsg::DerefTest)?;
Ok(())
}
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,11 @@ fn register_builtins(store: &mut LintStore) {
"replaced with another group of lints, see RFC \
<https://rust-lang.github.io/rfcs/2145-type-privacy.html> for more information",
);
store.register_removed(
"invalid_alignment",
"converted into hard error, see PR #104616 \
<https://github.com/rust-lang/rust/pull/104616> for more information",
);
}

fn register_internals(store: &mut LintStore) {
Expand Down
40 changes: 0 additions & 40 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -986,45 +986,6 @@ declare_lint! {
"detects trivial casts of numeric types which could be removed"
}

declare_lint! {
/// The `invalid_alignment` lint detects dereferences of misaligned pointers during
/// constant evaluation.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![feature(const_mut_refs)]
/// const FOO: () = unsafe {
/// let x = &[0_u8; 4];
/// let y = x.as_ptr().cast::<u32>();
/// let mut z = 123;
/// y.copy_to_nonoverlapping(&mut z, 1); // the address of a `u8` array is unknown
/// // and thus we don't know if it is aligned enough for copying a `u32`.
/// };
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// The compiler allowed dereferencing raw pointers irrespective of alignment
/// during const eval due to the const evaluator at the time not making it easy
/// or cheap to check. Now that it is both, this is not accepted anymore.
///
/// Since it was undefined behaviour to begin with, this breakage does not violate
/// Rust's stability guarantees. Using undefined behaviour can cause arbitrary
/// behaviour, including failure to build.
///
/// [future-incompatible]: ../index.md#future-incompatible-lints
pub INVALID_ALIGNMENT,
Deny,
"raw pointers must be aligned before dereferencing",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
reference: "issue #68585 <https://github.com/rust-lang/rust/issues/104616>",
};
}

declare_lint! {
/// The `exported_private_dependencies` lint detects private dependencies
/// that are exposed in a public interface.
Expand Down Expand Up @@ -3378,7 +3339,6 @@ declare_lint_pass! {
INDIRECT_STRUCTURAL_MATCH,
INEFFECTIVE_UNSTABLE_TRAIT_IMPL,
INLINE_NO_SANITIZE,
INVALID_ALIGNMENT,
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
INVALID_DOC_ATTRIBUTES,
INVALID_MACRO_EXPORT_ARGUMENTS,
INVALID_TYPE_PARAM_DEFAULT,
Expand Down
Loading
Loading