Skip to content

Commit

Permalink
Auto merge of #2985 - RalfJung:retag-fields, r=saethlin
Browse files Browse the repository at this point in the history 
make full field retagging the default

The 'scalar' field retagging mode is clearly a hack -- it mirrors details of the codegen backend and how various structs are represented in LLVM. This means whether code has UB or not depends on surprising aspects, such as whether a struct has 2 or 3 (non-zero-sized) fields. Now that both hashbrown and scopeguard have released fixes to be compatible with field retagging, I think it is time to enable full field retagging by default.

`@saethlin` do you have an idea of how much fallout enabling full field retagging by default will cause? Do you have objections to enabling it by default?

Fixes #2528
  • Loading branch information
@bors
bors committed Jul 21, 2023
2 parents 43caac7 + ae3b961 commit cd96466
Show file tree
Hide file tree
Showing 18 changed files with 140 additions and 89 deletions.
14 changes: 5 additions & 9 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,15 +407,11 @@ to Miri failing to detect cases of undefined behavior in a program.
application instead of raising an error within the context of Miri (and halting
execution). Note that code might not expect these operations to ever panic, so
this flag can lead to strange (mis)behavior.
* `-Zmiri-retag-fields` changes Stacked Borrows retagging to recurse into *all* fields.
This means that references in fields of structs/enums/tuples/arrays/... are retagged,
and in particular, they are protected when passed as function arguments.
(The default is to recurse only in cases where rustc would actually emit a `noalias` attribute.)
* `-Zmiri-retag-fields=<all|none|scalar>` controls when Stacked Borrows retagging recurses into
fields. `all` means it always recurses (like `-Zmiri-retag-fields`), `none` means it never
recurses, `scalar` (the default) means it only recurses for types where we would also emit
`noalias` annotations in the generated LLVM IR (types passed as individual scalars or pairs of
scalars). Setting this to `none` is **unsound**.
* `-Zmiri-retag-fields[=<all|none|scalar>]` controls when Stacked Borrows retagging recurses into
fields. `all` means it always recurses (the default, and equivalent to `-Zmiri-retag-fields`
without an explicit value), `none` means it never recurses, `scalar` means it only recurses for
types where we would also emit `noalias` annotations in the generated LLVM IR (types passed as
individual scalars or pairs of scalars). Setting this to `none` is **unsound**.
* `-Zmiri-tag-gc=<blocks>` configures how often the pointer tag garbage collector runs. The default
is to search for and remove unreachable tags once every `10000` basic blocks. Setting this to
`0` disables the garbage collector, which causes some programs to have explosive memory usage
Expand Down
65 changes: 45 additions & 20 deletions src/borrow_tracker/stacked_borrows/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,19 @@ use rustc_span::{Span, SpanData};
use rustc_target::abi::Size;

use crate::borrow_tracker::{
stacked_borrows::{err_sb_ub, Permission},
AccessKind, GlobalStateInner, ProtectorKind,
stacked_borrows::Permission, AccessKind, GlobalStateInner, ProtectorKind,
};
use crate::*;

/// Error reporting
fn err_sb_ub<'tcx>(
msg: String,
help: Vec<String>,
history: Option<TagHistory>,
) -> InterpError<'tcx> {
err_machine_stop!(TerminationInfo::StackedBorrowsUb { msg, help, history })
}

#[derive(Clone, Debug)]
pub struct AllocHistory {
id: AllocId,
Expand Down Expand Up @@ -61,12 +69,15 @@ struct Invalidation {
#[derive(Clone, Debug)]
enum InvalidationCause {
Access(AccessKind),
Retag(Permission, RetagCause),
Retag(Permission, RetagInfo),
}

impl Invalidation {
fn generate_diagnostic(&self) -> (String, SpanData) {
let message = if let InvalidationCause::Retag(_, RetagCause::FnEntry) = self.cause {
let message = if matches!(
self.cause,
InvalidationCause::Retag(_, RetagInfo { cause: RetagCause::FnEntry, .. })
) {
// For a FnEntry retag, our Span points at the caller.
// See `DiagnosticCx::log_invalidation`.
format!(
Expand All @@ -87,8 +98,8 @@ impl fmt::Display for InvalidationCause {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
InvalidationCause::Access(kind) => write!(f, "{kind}"),
InvalidationCause::Retag(perm, kind) =>
write!(f, "{perm:?} {retag}", retag = kind.summary()),
InvalidationCause::Retag(perm, info) =>
write!(f, "{perm:?} {retag}", retag = info.summary()),
}
}
}
Expand Down Expand Up @@ -129,13 +140,13 @@ impl<'ecx, 'mir, 'tcx> DiagnosticCxBuilder<'ecx, 'mir, 'tcx> {

pub fn retag(
machine: &'ecx MiriMachine<'mir, 'tcx>,
cause: RetagCause,
info: RetagInfo,
new_tag: BorTag,
orig_tag: ProvenanceExtra,
range: AllocRange,
) -> Self {
let operation =
Operation::Retag(RetagOp { cause, new_tag, orig_tag, range, permission: None });
Operation::Retag(RetagOp { info, new_tag, orig_tag, range, permission: None });

DiagnosticCxBuilder { machine, operation }
}
Expand Down Expand Up @@ -179,13 +190,19 @@ enum Operation {

#[derive(Debug, Clone)]
struct RetagOp {
cause: RetagCause,
info: RetagInfo,
new_tag: BorTag,
orig_tag: ProvenanceExtra,
range: AllocRange,
permission: Option<Permission>,
}

#[derive(Debug, Clone, Copy, PartialEq)]
pub struct RetagInfo {
pub cause: RetagCause,
pub in_field: bool,
}

#[derive(Debug, Clone, Copy, PartialEq)]
pub enum RetagCause {
Normal,
Expand Down Expand Up @@ -258,11 +275,11 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
pub fn log_invalidation(&mut self, tag: BorTag) {
let mut span = self.machine.current_span();
let (range, cause) = match &self.operation {
Operation::Retag(RetagOp { cause, range, permission, .. }) => {
if *cause == RetagCause::FnEntry {
Operation::Retag(RetagOp { info, range, permission, .. }) => {
if info.cause == RetagCause::FnEntry {
span = self.machine.caller_span();
}
(*range, InvalidationCause::Retag(permission.unwrap(), *cause))
(*range, InvalidationCause::Retag(permission.unwrap(), *info))
}
Operation::Access(AccessOp { kind, range, .. }) =>
(*range, InvalidationCause::Access(*kind)),
Expand Down Expand Up @@ -372,9 +389,13 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
self.history.id,
self.offset.bytes(),
);
let mut helps = vec![operation_summary(&op.info.summary(), self.history.id, op.range)];
if op.info.in_field {
helps.push(format!("errors for retagging in fields are fairly new; please reach out to us (e.g. at <https://rust-lang.zulipchat.com/#narrow/stream/269128-miri>) if you find this error troubling"));
}
err_sb_ub(
format!("{action}{}", error_cause(stack, op.orig_tag)),
Some(operation_summary(&op.cause.summary(), self.history.id, op.range)),
helps,
op.orig_tag.and_then(|orig_tag| self.get_logs_relevant_to(orig_tag, None)),
)
}
Expand All @@ -397,7 +418,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
);
err_sb_ub(
format!("{action}{}", error_cause(stack, op.tag)),
Some(operation_summary("an access", self.history.id, op.range)),
vec![operation_summary("an access", self.history.id, op.range)],
op.tag.and_then(|tag| self.get_logs_relevant_to(tag, None)),
)
}
Expand All @@ -423,7 +444,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
Operation::Dealloc(_) =>
err_sb_ub(
format!("deallocating while item {item:?} is {protected} by call {call_id:?}",),
None,
vec![],
None,
),
Operation::Retag(RetagOp { orig_tag: tag, .. })
Expand All @@ -432,7 +453,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
format!(
"not granting access to tag {tag:?} because that would remove {item:?} which is {protected} because it is an argument of call {call_id:?}",
),
None,
vec![],
tag.and_then(|tag| self.get_logs_relevant_to(tag, Some(item.tag()))),
),
}
Expand All @@ -450,7 +471,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
alloc_id = self.history.id,
cause = error_cause(stack, op.tag),
),
None,
vec![],
op.tag.and_then(|tag| self.get_logs_relevant_to(tag, None)),
)
}
Expand Down Expand Up @@ -496,14 +517,18 @@ fn error_cause(stack: &Stack, prov_extra: ProvenanceExtra) -> &'static str {
}
}

impl RetagCause {
impl RetagInfo {
fn summary(&self) -> String {
match self {
let mut s = match self.cause {
RetagCause::Normal => "retag",
RetagCause::FnEntry => "function-entry retag",
RetagCause::InPlaceFnPassing => "in-place function argument/return passing protection",
RetagCause::TwoPhase => "two-phase retag",
}
.to_string()
.to_string();
if self.in_field {
s.push_str(" (of a reference/box inside this compound value)");
}
s
}
}
56 changes: 30 additions & 26 deletions src/borrow_tracker/stacked_borrows/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ pub mod diagnostics;
mod item;
mod stack;

use log::trace;
use std::cmp;
use std::fmt::Write;
use std::mem;

use log::trace;

use rustc_data_structures::fx::FxHashSet;
use rustc_middle::mir::{Mutability, RetagKind};
Expand All @@ -19,12 +21,12 @@ use rustc_middle::ty::{
use rustc_target::abi::{Abi, Size};

use crate::borrow_tracker::{
stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder, TagHistory},
stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder},
AccessKind, GlobalStateInner, ProtectorKind, RetagFields,
};
use crate::*;

use diagnostics::RetagCause;
use diagnostics::{RetagCause, RetagInfo};
pub use item::{Item, Permission};
pub use stack::Stack;

Expand Down Expand Up @@ -168,15 +170,6 @@ impl NewPermission {
}
}

/// Error reporting
pub fn err_sb_ub<'tcx>(
msg: String,
help: Option<String>,
history: Option<TagHistory>,
) -> InterpError<'tcx> {
err_machine_stop!(TerminationInfo::StackedBorrowsUb { msg, help, history })
}

// # Stacked Borrows Core Begin

/// We need to make at least the following things true:
Expand Down Expand Up @@ -623,7 +616,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
size: Size,
new_perm: NewPermission,
new_tag: BorTag,
retag_cause: RetagCause, // What caused this retag, for diagnostics only
retag_info: RetagInfo, // diagnostics info about this retag
) -> InterpResult<'tcx, Option<AllocId>> {
let this = self.eval_context_mut();

Expand Down Expand Up @@ -670,7 +663,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
// FIXME: can this be done cleaner?
let dcx = DiagnosticCxBuilder::retag(
&this.machine,
retag_cause,
retag_info,
new_tag,
orig_tag,
alloc_range(base_offset, size),
Expand Down Expand Up @@ -761,7 +754,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
let global = machine.borrow_tracker.as_ref().unwrap().borrow();
let dcx = DiagnosticCxBuilder::retag(
machine,
retag_cause,
retag_info,
new_tag,
orig_tag,
alloc_range(base_offset, size),
Expand Down Expand Up @@ -804,7 +797,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
let global = this.machine.borrow_tracker.as_ref().unwrap().borrow();
let dcx = DiagnosticCxBuilder::retag(
&this.machine,
retag_cause,
retag_info,
new_tag,
orig_tag,
alloc_range(base_offset, size),
Expand Down Expand Up @@ -834,7 +827,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
&mut self,
val: &ImmTy<'tcx, Provenance>,
new_perm: NewPermission,
cause: RetagCause, // What caused this retag, for diagnostics only
info: RetagInfo, // diagnostics info about this retag
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
let this = self.eval_context_mut();
// We want a place for where the ptr *points to*, so we get one.
Expand All @@ -852,7 +845,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
let new_tag = this.machine.borrow_tracker.as_mut().unwrap().get_mut().new_ptr();

// Reborrow.
let alloc_id = this.sb_reborrow(&place, size, new_perm, new_tag, cause)?;
let alloc_id = this.sb_reborrow(&place, size, new_perm, new_tag, info)?;

// Adjust pointer.
let new_place = place.map_provenance(|p| {
Expand Down Expand Up @@ -886,12 +879,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
let this = self.eval_context_mut();
let new_perm = NewPermission::from_ref_ty(val.layout.ty, kind, this);
let retag_cause = match kind {
let cause = match kind {
RetagKind::TwoPhase { .. } => RetagCause::TwoPhase,
RetagKind::FnEntry => unreachable!(),
RetagKind::Raw | RetagKind::Default => RetagCause::Normal,
};
this.sb_retag_reference(val, new_perm, retag_cause)
this.sb_retag_reference(val, new_perm, RetagInfo { cause, in_field: false })
}

fn sb_retag_place_contents(
Expand All @@ -906,7 +899,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
RetagKind::FnEntry => RetagCause::FnEntry,
RetagKind::Default => RetagCause::Normal,
};
let mut visitor = RetagVisitor { ecx: this, kind, retag_cause, retag_fields };
let mut visitor =
RetagVisitor { ecx: this, kind, retag_cause, retag_fields, in_field: false };
return visitor.visit_value(place);

// The actual visitor.
Expand All @@ -915,17 +909,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
kind: RetagKind,
retag_cause: RetagCause,
retag_fields: RetagFields,
in_field: bool,
}
impl<'ecx, 'mir, 'tcx> RetagVisitor<'ecx, 'mir, 'tcx> {
#[inline(always)] // yes this helps in our benchmarks
fn retag_ptr_inplace(
&mut self,
place: &PlaceTy<'tcx, Provenance>,
new_perm: NewPermission,
retag_cause: RetagCause,
) -> InterpResult<'tcx> {
let val = self.ecx.read_immediate(&self.ecx.place_to_op(place)?)?;
let val = self.ecx.sb_retag_reference(&val, new_perm, retag_cause)?;
let val = self.ecx.sb_retag_reference(
&val,
new_perm,
RetagInfo { cause: self.retag_cause, in_field: self.in_field },
)?;
self.ecx.write_immediate(*val, place)?;
Ok(())
}
Expand All @@ -943,7 +941,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
fn visit_box(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
// Boxes get a weak protectors, since they may be deallocated.
let new_perm = NewPermission::from_box_ty(place.layout.ty, self.kind, self.ecx);
self.retag_ptr_inplace(place, new_perm, self.retag_cause)
self.retag_ptr_inplace(place, new_perm)
}

fn visit_value(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
Expand All @@ -960,7 +958,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
ty::Ref(..) => {
let new_perm =
NewPermission::from_ref_ty(place.layout.ty, self.kind, self.ecx);
self.retag_ptr_inplace(place, new_perm, self.retag_cause)?;
self.retag_ptr_inplace(place, new_perm)?;
}
ty::RawPtr(..) => {
// We do *not* want to recurse into raw pointers -- wide raw pointers have
Expand All @@ -984,7 +982,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
}
};
if recurse {
let in_field = mem::replace(&mut self.in_field, true); // remember and restore old value
self.walk_value(place)?;
self.in_field = in_field;
}
}
}
Expand All @@ -1011,7 +1011,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
access: Some(AccessKind::Write),
protector: Some(ProtectorKind::StrongProtector),
};
let _new_ptr = this.sb_retag_reference(&ptr, new_perm, RetagCause::InPlaceFnPassing)?;
let _new_ptr = this.sb_retag_reference(
&ptr,
new_perm,
RetagInfo { cause: RetagCause::InPlaceFnPassing, in_field: false },
)?;
// We just throw away `new_ptr`, so nobody can access this memory while it is protected.

Ok(())
Expand Down
Loading

0 comments on commit cd96466

Please sign in to comment.