Rollup merge of #130885 - RalfJung:interp-error-discard, r=oli-obk

panic when an interpreter error gets unintentionally discarded

One important invariant of Miri is that when an interpreter error is raised (*in particular* a UB error), those must not be discarded: it's not okay to just check `foo().is_err()` and then continue executing.

This seems to catch new contributors by surprise fairly regularly, so this PR tries to make it so that *if* this ever happens, we get a panic rather than a silent missed UB bug. The interpreter error type now contains a "guard" that panics on drop, and that is explicitly passed to `mem::forget` when an error is deliberately discarded.

Fixes rust-lang#3855

Author: workingjubilee

src/alloc_addresses/mod.rs

@@ -200,7 +200,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 AllocKind::Dead => unreachable!(),
             };
             // Ensure this pointer's provenance is exposed, so that it can be used by FFI code.
-            return Ok(base_ptr.expose_provenance().try_into().unwrap());
+            return interp_ok(base_ptr.expose_provenance().try_into().unwrap());
         }
         // We are not in native lib mode, so we control the addresses ourselves.
         if let Some((reuse_addr, clock)) =
@@ -209,7 +209,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
             if let Some(clock) = clock {
                 ecx.acquire_clock(&clock);
             }
-            Ok(reuse_addr)
+            interp_ok(reuse_addr)
         } else {
             // We have to pick a fresh address.
             // Leave some space to the previous allocation, to give it some chance to be less aligned.
@@ -234,7 +234,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 throw_exhaust!(AddressSpaceFull);
             }
 
-            Ok(base_addr)
+            interp_ok(base_addr)
         }
     }
 
@@ -248,7 +248,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let global_state = &mut *global_state;
 
         match global_state.base_addr.get(&alloc_id) {
-            Some(&addr) => Ok(addr),
+            Some(&addr) => interp_ok(addr),
             None => {
                 // First time we're looking for the absolute address of this allocation.
                 let base_addr =
@@ -274,7 +274,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 };
                 global_state.int_to_ptr_map.insert(pos, (base_addr, alloc_id));
 
-                Ok(base_addr)
+                interp_ok(base_addr)
             }
         }
     }
@@ -287,20 +287,20 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let global_state = ecx.machine.alloc_addresses.get_mut();
         // In strict mode, we don't need this, so we can save some cycles by not tracking it.
         if global_state.provenance_mode == ProvenanceMode::Strict {
-            return Ok(());
+            return interp_ok(());
         }
         // Exposing a dead alloc is a no-op, because it's not possible to get a dead allocation
         // via int2ptr.
         if !ecx.is_alloc_live(alloc_id) {
-            return Ok(());
+            return interp_ok(());
         }
         trace!("Exposing allocation id {alloc_id:?}");
         let global_state = ecx.machine.alloc_addresses.get_mut();
         global_state.exposed.insert(alloc_id);
         if ecx.machine.borrow_tracker.is_some() {
             ecx.expose_tag(alloc_id, tag)?;
         }
-        Ok(())
+        interp_ok(())
     }
 
     fn ptr_from_addr_cast(&self, addr: u64) -> InterpResult<'tcx, Pointer> {
@@ -337,7 +337,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // cast is fairly irrelevant. Instead we generate this as a "wildcard" pointer, such that
         // *every time the pointer is used*, we do an `AllocId` lookup to find the (exposed)
         // allocation it might be referencing.
-        Ok(Pointer::new(Some(Provenance::Wildcard), Size::from_bytes(addr)))
+        interp_ok(Pointer::new(Some(Provenance::Wildcard), Size::from_bytes(addr)))
     }
 
     /// Convert a relative (tcx) pointer to a Miri pointer.
@@ -359,7 +359,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             Size::from_bytes(base_addr),
         );
         // Add offset with the right kind of pointer-overflowing arithmetic.
-        Ok(base_ptr.wrapping_offset(offset, ecx))
+        interp_ok(base_ptr.wrapping_offset(offset, ecx))
     }
 
     // This returns some prepared `MiriAllocBytes`, either because `addr_from_alloc_id` reserved
@@ -390,9 +390,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             assert_eq!(prepared_alloc_bytes.len(), bytes.len());
             // Copy allocation contents into prepared memory.
             prepared_alloc_bytes.copy_from_slice(bytes);
-            Ok(prepared_alloc_bytes)
+            interp_ok(prepared_alloc_bytes)
         } else {
-            Ok(MiriAllocBytes::from_bytes(std::borrow::Cow::Borrowed(bytes), align))
+            interp_ok(MiriAllocBytes::from_bytes(std::borrow::Cow::Borrowed(bytes), align))
         }
     }
 

src/borrow_tracker/mod.rs

@@ -322,7 +322,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         match method {
             BorrowTrackerMethod::StackedBorrows => {
                 this.tcx.tcx.dcx().warn("Stacked Borrows does not support named pointers; `miri_pointer_name` is a no-op");
-                Ok(())
+                interp_ok(())
             }
             BorrowTrackerMethod::TreeBorrows =>
                 this.tb_give_pointer_debug_name(ptr, nth_parent, name),
@@ -333,7 +333,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let this = self.eval_context_mut();
         let Some(borrow_tracker) = &this.machine.borrow_tracker else {
             eprintln!("attempted to print borrow state, but no borrow state is being tracked");
-            return Ok(());
+            return interp_ok(());
         };
         let method = borrow_tracker.borrow().borrow_tracker_method;
         match method {
@@ -376,7 +376,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             }
         }
         borrow_tracker.borrow_mut().end_call(&frame.extra);
-        Ok(())
+        interp_ok(())
     }
 }
 
@@ -489,7 +489,7 @@ impl AllocState {
         alloc_id: AllocId, // diagnostics
     ) -> InterpResult<'tcx> {
         match self {
-            AllocState::StackedBorrows(_sb) => Ok(()),
+            AllocState::StackedBorrows(_sb) => interp_ok(()),
             AllocState::TreeBorrows(tb) =>
                 tb.borrow_mut().release_protector(machine, global, tag, alloc_id),
         }

src/borrow_tracker/stacked_borrows/mod.rs

@@ -230,7 +230,7 @@ impl<'tcx> Stack {
         }
 
         if !item.protected() {
-            return Ok(());
+            return interp_ok(());
         }
 
         // We store tags twice, once in global.protected_tags and once in each call frame.
@@ -252,10 +252,10 @@ impl<'tcx> Stack {
             let allowed = matches!(cause, ItemInvalidationCause::Dealloc)
                 && matches!(protector_kind, ProtectorKind::WeakProtector);
             if !allowed {
-                return Err(dcx.protector_error(item, protector_kind).into());
+                return Err(dcx.protector_error(item, protector_kind)).into();
             }
         }
-        Ok(())
+        interp_ok(())
     }
 
     /// Test if a memory `access` using pointer tagged `tag` is granted.
@@ -295,7 +295,7 @@ impl<'tcx> Stack {
             self.pop_items_after(first_incompatible_idx, |item| {
                 Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?;
                 dcx.log_invalidation(item.tag());
-                Ok(())
+                interp_ok(())
             })?;
         } else {
             // On a read, *disable* all `Unique` above the granting item.  This ensures U2 for read accesses.
@@ -316,7 +316,7 @@ impl<'tcx> Stack {
             self.disable_uniques_starting_at(first_incompatible_idx, |item| {
                 Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?;
                 dcx.log_invalidation(item.tag());
-                Ok(())
+                interp_ok(())
             })?;
         }
 
@@ -345,7 +345,7 @@ impl<'tcx> Stack {
         }
 
         // Done.
-        Ok(())
+        interp_ok(())
     }
 
     /// Deallocate a location: Like a write access, but also there must be no
@@ -367,7 +367,7 @@ impl<'tcx> Stack {
             Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Dealloc)?;
         }
 
-        Ok(())
+        interp_ok(())
     }
 
     /// Derive a new pointer from one with the given tag.
@@ -418,7 +418,7 @@ impl<'tcx> Stack {
                     "reborrow: forgetting stack entirely due to SharedReadWrite reborrow from wildcard or unknown"
                 );
                 self.set_unknown_bottom(global.next_ptr_tag);
-                return Ok(());
+                return interp_ok(());
             };
 
             // SharedReadWrite can coexist with "existing loans", meaning they don't act like a write
@@ -431,7 +431,7 @@ impl<'tcx> Stack {
         // Put the new item there.
         trace!("reborrow: adding item {:?}", new);
         self.insert(new_idx, new);
-        Ok(())
+        interp_ok(())
     }
 }
 // # Stacked Borrows Core End
@@ -491,7 +491,7 @@ impl<'tcx> Stacks {
             f(stack, &mut dcx, &mut self.exposed_tags)?;
             dcx_builder = dcx.unbuild();
         }
-        Ok(())
+        interp_ok(())
     }
 }
 
@@ -576,7 +576,7 @@ impl Stacks {
         self.for_each(alloc_range(Size::ZERO, size), dcx, |stack, dcx, exposed_tags| {
             stack.dealloc(tag, &state, dcx, exposed_tags)
         })?;
-        Ok(())
+        interp_ok(())
     }
 }
 
@@ -623,7 +623,7 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> {
             drop(global); // don't hold that reference any longer than we have to
 
             let Some((alloc_id, base_offset, orig_tag)) = loc else {
-                return Ok(())
+                return interp_ok(())
             };
 
             let (_size, _align, alloc_kind) = this.get_alloc_info(alloc_id);
@@ -655,7 +655,7 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> {
                     // No stacked borrows on these allocations.
                 }
             }
-            Ok(())
+            interp_ok(())
         };
 
         if size == Size::ZERO {
@@ -676,12 +676,12 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> {
             {
                 log_creation(this, Some((alloc_id, base_offset, orig_tag)))?;
                 // Still give it the new provenance, it got retagged after all.
-                return Ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }));
+                return interp_ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }));
             } else {
                 // This pointer doesn't come with an AllocId. :shrug:
                 log_creation(this, None)?;
                 // Provenance unchanged.
-                return Ok(place.ptr().provenance);
+                return interp_ok(place.ptr().provenance);
             }
         }
 
@@ -800,12 +800,12 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> {
                             )?;
                         }
                     }
-                    Ok(())
+                    interp_ok(())
                 })?;
             }
         }
 
-        Ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }))
+        interp_ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }))
     }
 
     fn sb_retag_place(
@@ -832,7 +832,7 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> {
                     *shown = true;
                     this.emit_diagnostic(NonHaltingDiagnostic::ExternTypeReborrow);
                 });
-                return Ok(place.clone());
+                return interp_ok(place.clone());
             }
         };
 
@@ -845,7 +845,7 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> {
         // Adjust place.
         // (If the closure gets called, that means the old provenance was `Some`, and hence the new
         // one must also be `Some`.)
-        Ok(place.clone().map_provenance(|_| new_prov.unwrap()))
+        interp_ok(place.clone().map_provenance(|_| new_prov.unwrap()))
     }
 
     /// Retags an individual pointer, returning the retagged version.
@@ -859,7 +859,7 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> {
         let this = self.eval_context_mut();
         let place = this.ref_to_mplace(val)?;
         let new_place = this.sb_retag_place(&place, new_perm, info)?;
-        Ok(ImmTy::from_immediate(new_place.to_ref(this), val.layout))
+        interp_ok(ImmTy::from_immediate(new_place.to_ref(this), val.layout))
     }
 }
 
@@ -917,7 +917,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     in_field: self.in_field,
                 })?;
                 self.ecx.write_immediate(*val, place)?;
-                Ok(())
+                interp_ok(())
             }
         }
         impl<'ecx, 'tcx> ValueVisitor<'tcx, MiriMachine<'tcx>> for RetagVisitor<'ecx, 'tcx> {
@@ -935,7 +935,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     let new_perm = NewPermission::from_box_ty(place.layout.ty, self.kind, self.ecx);
                     self.retag_ptr_inplace(place, new_perm)?;
                 }
-                Ok(())
+                interp_ok(())
             }
 
             fn visit_value(&mut self, place: &PlaceTy<'tcx>) -> InterpResult<'tcx> {
@@ -944,7 +944,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // This optimization is crucial for ZSTs, because they can contain way more fields
                 // than we can ever visit.
                 if place.layout.is_sized() && place.layout.size < self.ecx.pointer_size() {
-                    return Ok(());
+                    return interp_ok(());
                 }
 
                 // Check the type of this value to see what to do with it (retag, or recurse).
@@ -983,7 +983,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     }
                 }
 
-                Ok(())
+                interp_ok(())
             }
         }
     }
@@ -1028,7 +1028,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // No stacked borrows on these allocations.
             }
         }
-        Ok(())
+        interp_ok(())
     }
 
     fn print_stacks(&mut self, alloc_id: AllocId) -> InterpResult<'tcx> {
@@ -1046,6 +1046,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             }
             println!(" ]");
         }
-        Ok(())
+        interp_ok(())
     }
 }