Auto merge of rust-lang#2985 - RalfJung:retag-fields, r=saethlin

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 rust-lang/miri#2528

Author: bors

src/tools/miri/README.md

@@ -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

src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs

@@ -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,
@@ -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!(
@@ -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()),
         }
     }
 }
@@ -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 }
     }
@@ -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,
@@ -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)),
@@ -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)),
         )
     }
@@ -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)),
         )
     }
@@ -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, .. })
@@ -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()))),
                 ),
         }
@@ -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)),
         )
     }
@@ -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
     }
 }

src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs

@@ -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};
@@ -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;
 
@@ -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:
@@ -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();
 
@@ -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),
@@ -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),
@@ -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),
@@ -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.
@@ -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| {
@@ -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(
@@ -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.
@@ -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(())
             }
@@ -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> {
@@ -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
@@ -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;
                         }
                     }
                 }
@@ -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(())