SB: track whether a retag occurred nested inside a field

RalfJung · RalfJung · commit bd2e38e96190 · 2023-07-21T16:23:51.000+02:00
diff --git a/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/borrow_tracker/stacked_borrows/diagnostics.rs
@@ -61,12 +61,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 +90,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 +132,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 +182,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 +267,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)),
@@ -374,7 +383,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
         );
         err_sb_ub(
             format!("{action}{}", error_cause(stack, op.orig_tag)),
-            Some(operation_summary(&op.cause.summary(), self.history.id, op.range)),
+            Some(operation_summary(&op.info.summary(), self.history.id, op.range)),
             op.orig_tag.and_then(|orig_tag| self.get_logs_relevant_to(orig_tag, None)),
         )
     }
@@ -496,14 +505,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 type)");
+        }
+        s
     }
 }
diff --git a/src/borrow_tracker/stacked_borrows/mod.rs b/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};
@@ -24,7 +26,7 @@ use crate::borrow_tracker::{
 };
 use crate::*;
 
-use diagnostics::RetagCause;
+use diagnostics::{RetagCause, RetagInfo};
 pub use item::{Item, Permission};
 pub use stack::Stack;
 
@@ -623,7 +625,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 +672,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 +763,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 +806,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 +836,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 +854,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 +888,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 +908,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 +918,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 +950,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 +967,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 +991,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 +1020,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(())
diff --git a/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr b/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr
@@ -8,7 +8,7 @@ LL | |             )
    | |             ^
    | |             |
    | |_____________trying to retag from <TAG> for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
-   |               this error occurs as part of retag at ALLOC[0x0..0x10]
+   |               this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x0..0x10]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
diff --git a/tests/fail/both_borrows/pass_invalid_shr_option.stack.stderr b/tests/fail/both_borrows/pass_invalid_shr_option.stack.stderr
@@ -5,7 +5,7 @@ LL |     foo(some_xref);
    |         ^^^^^^^^^
    |         |
    |         trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
-   |         this error occurs as part of retag at ALLOC[0x0..0x4]
+   |         this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x0..0x4]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
diff --git a/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr b/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr
@@ -5,7 +5,7 @@ LL |     foo(pair_xref);
    |         ^^^^^^^^^
    |         |
    |         trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
-   |         this error occurs as part of retag at ALLOC[0x0..0x4]
+   |         this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x0..0x4]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
diff --git a/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr b/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr
@@ -5,7 +5,7 @@ LL |     ret
    |     ^^^
    |     |
    |     trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location
-   |     this error occurs as part of retag at ALLOC[0x4..0x8]
+   |     this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x4..0x8]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
diff --git a/tests/fail/both_borrows/return_invalid_shr_tuple.stack.stderr b/tests/fail/both_borrows/return_invalid_shr_tuple.stack.stderr
@@ -5,7 +5,7 @@ LL |     ret
    |     ^^^
    |     |
    |     trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location
-   |     this error occurs as part of retag at ALLOC[0x4..0x8]
+   |     this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x4..0x8]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
diff --git a/tests/fail/stacked_borrows/return_invalid_mut_option.stderr b/tests/fail/stacked_borrows/return_invalid_mut_option.stderr
@@ -5,7 +5,7 @@ LL |     ret
    |     ^^^
    |     |
    |     trying to retag from <TAG> for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location
-   |     this error occurs as part of retag at ALLOC[0x4..0x8]
+   |     this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x4..0x8]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
diff --git a/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr b/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr
@@ -5,7 +5,7 @@ LL |     ret
    |     ^^^
    |     |
    |     trying to retag from <TAG> for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location
-   |     this error occurs as part of retag at ALLOC[0x4..0x8]
+   |     this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x4..0x8]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@ LL \| \| )`
`8`	`8`	`\| \| ^`
`9`	`9`	`\| \| \|`
`10`	`10`	`\| \|_____________trying to retag from <TAG> for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location`
`11`		`- \| this error occurs as part of retag at ALLOC[0x0..0x10]`
	`11`	`+ \| this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x0..0x10]`
`12`	`12`	`\|`
`13`	`13`	`= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental`
`14`	`14`	`= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ LL \| foo(some_xref);`
`5`	`5`	`\| ^^^^^^^^^`
`6`	`6`	`\| \|`
`7`	`7`	`\| trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location`
`8`		`- \| this error occurs as part of retag at ALLOC[0x0..0x4]`
	`8`	`+ \| this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x0..0x4]`
`9`	`9`	`\|`
`10`	`10`	`= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental`
`11`	`11`	`= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ LL \| foo(pair_xref);`
`5`	`5`	`\| ^^^^^^^^^`
`6`	`6`	`\| \|`
`7`	`7`	`\| trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location`
`8`		`- \| this error occurs as part of retag at ALLOC[0x0..0x4]`
	`8`	`+ \| this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x0..0x4]`
`9`	`9`	`\|`
`10`	`10`	`= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental`
`11`	`11`	`= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ LL \| ret`
`5`	`5`	`\| ^^^`
`6`	`6`	`\| \|`
`7`	`7`	`\| trying to retag from <TAG> for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location`
`8`		`- \| this error occurs as part of retag at ALLOC[0x4..0x8]`
	`8`	`+ \| this error occurs as part of retag (of a reference/box inside this compound type) at ALLOC[0x4..0x8]`
`9`	`9`	`\|`
`10`	`10`	`= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental`
`11`	`11`	`= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information`