adjust for symbolic vtables

RalfJung · RalfJung · commit 6d4eaab0da4a · 2022-07-18T10:41:16.000-04:00
diff --git a/src/intptrcast.rs b/src/intptrcast.rs
@@ -86,24 +86,31 @@ impl<'mir, 'tcx> GlobalStateInner {
         if global_state.exposed.contains(&alloc_id) {
             let (_size, _align, kind) = ecx.get_alloc_info(alloc_id);
             match kind {
-                AllocKind::LiveData | AllocKind::Function => return Some(alloc_id),
+                AllocKind::LiveData | AllocKind::Function | AllocKind::Vtable => {
+                    return Some(alloc_id);
+                }
                 AllocKind::Dead => {}
             }
         }
 
         None
     }
 
-    pub fn expose_ptr(ecx: &mut MiriEvalContext<'mir, 'tcx>, alloc_id: AllocId, sb: SbTag) {
+    pub fn expose_ptr(
+        ecx: &mut MiriEvalContext<'mir, 'tcx>,
+        alloc_id: AllocId,
+        sb: SbTag,
+    ) -> InterpResult<'tcx> {
         let global_state = ecx.machine.intptrcast.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 {
             trace!("Exposing allocation id {alloc_id:?}");
             global_state.exposed.insert(alloc_id);
             if ecx.machine.stacked_borrows.is_some() {
-                ecx.expose_tag(alloc_id, sb);
+                ecx.expose_tag(alloc_id, sb)?;
             }
         }
+        Ok(())
     }
 
     pub fn ptr_from_addr_transmute(
@@ -188,8 +195,8 @@ impl<'mir, 'tcx> GlobalStateInner {
 
                 // Remember next base address.  If this allocation is zero-sized, leave a gap
                 // of at least 1 to avoid two allocations having the same base address.
-                // (The logic in `alloc_id_from_addr` assumes unique addresses, and function
-                // pointers to different functions need to be distinguishable!)
+                // (The logic in `alloc_id_from_addr` assumes unique addresses, and different
+                // function/vtable pointers need to be distinguishable!)
                 global_state.next_base_addr = base_addr.checked_add(max(size.bytes(), 1)).unwrap();
                 // Given that `next_base_addr` increases in each allocation, pushing the
                 // corresponding tuple keeps `int_to_ptr_map` sorted
diff --git a/src/machine.rs b/src/machine.rs
@@ -620,12 +620,35 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
     ) -> InterpResult<'tcx, Pointer<Tag>> {
         let link_name = ecx.item_link_name(def_id);
         if let Some(&ptr) = ecx.machine.extern_statics.get(&link_name) {
+            // Various parts of the engine rely on `get_alloc_info` for size and alignment
+            // information. That uses the type information of this static.
+            // Make sure it matches the Miri allocation for this.
+            let Tag::Concrete { alloc_id, .. } = ptr.provenance else {
+                panic!("extern_statics cannot contain wildcards")
+            };
+            let (shim_size, shim_align, _kind) = ecx.get_alloc_info(alloc_id);
+            let extern_decl_layout =
+                ecx.tcx.layout_of(ty::ParamEnv::empty().and(ecx.tcx.type_of(def_id))).unwrap();
+            if extern_decl_layout.size != shim_size || extern_decl_layout.align.abi != shim_align {
+                throw_unsup_format!(
+                    "`extern` static `{name}` from crate `{krate}` has been declared \
+                    with a size of {decl_size} bytes and alignment of {decl_align} bytes, \
+                    but Miri emulates it via an extern static shim \
+                    with a size of {shim_size} bytes and alignment of {shim_align} byes",
+                    name = ecx.tcx.def_path_str(def_id),
+                    krate = ecx.tcx.crate_name(def_id.krate),
+                    decl_size = extern_decl_layout.size.bytes(),
+                    decl_align = extern_decl_layout.align.abi.bytes(),
+                    shim_size = shim_size.bytes(),
+                    shim_align = shim_align.bytes(),
+                )
+            }
             Ok(ptr)
         } else {
             throw_unsup_format!(
-                "`extern` static `{}` from crate `{}` is not supported by Miri",
-                ecx.tcx.def_path_str(def_id),
-                ecx.tcx.crate_name(def_id.krate),
+                "`extern` static `{name}` from crate `{krate}` is not supported by Miri",
+                name = ecx.tcx.def_path_str(def_id),
+                krate = ecx.tcx.crate_name(def_id.krate),
             )
         }
     }
@@ -692,7 +715,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
         if cfg!(debug_assertions) {
             // The machine promises to never call us on thread-local or extern statics.
             let alloc_id = ptr.provenance;
-            match ecx.tcx.get_global_alloc(alloc_id) {
+            match ecx.tcx.try_get_global_alloc(alloc_id) {
                 Some(GlobalAlloc::Static(def_id)) if ecx.tcx.is_thread_local_static(def_id) => {
                     panic!("tag_alloc_base_pointer called on thread-local static")
                 }
@@ -737,14 +760,14 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
     ) -> InterpResult<'tcx> {
         match ptr.provenance {
             Tag::Concrete { alloc_id, sb } => {
-                intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, sb);
+                intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, sb)
             }
             Tag::Wildcard => {
                 // No need to do anything for wildcard pointers as
                 // their provenances have already been previously exposed.
+                Ok(())
             }
         }
-        Ok(())
     }
 
     /// Convert a pointer with provenance into an allocation-offset pair,
diff --git a/src/shims/backtrace.rs b/src/shims/backtrace.rs
@@ -122,14 +122,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         let ptr = this.read_pointer(ptr)?;
-        // Take apart the pointer, we need its pieces.
+        // Take apart the pointer, we need its pieces. The offset encodes the span.
         let (alloc_id, offset, _tag) = this.ptr_get_alloc_id(ptr)?;
 
+        // This has to be an actual global fn ptr, not a dlsym function.
         let fn_instance =
-            if let Some(GlobalAlloc::Function(instance)) = this.tcx.get_global_alloc(alloc_id) {
+            if let Some(GlobalAlloc::Function(instance)) = this.tcx.try_get_global_alloc(alloc_id) {
                 instance
             } else {
-                throw_ub_format!("expected function pointer, found {:?}", ptr);
+                throw_ub_format!("expected static function pointer, found {:?}", ptr);
             };
 
         let lo =
diff --git a/src/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs
@@ -783,7 +783,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let log_creation = |this: &MiriEvalContext<'mir, 'tcx>,
                             current_span: &mut CurrentSpan<'_, 'mir, 'tcx>,
                             loc: Option<(AllocId, Size, SbTagExtra)>| // alloc_id, base_offset, orig_tag
-         -> InterpResult<'tcx> {
+        -> InterpResult<'tcx> {
             let global = this.machine.stacked_borrows.as_ref().unwrap().borrow();
             if global.tracked_pointer_tags.contains(&new_tag) {
                 register_diagnostic(NonHaltingDiagnostic::CreatedPointerTag(
@@ -794,29 +794,40 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, '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 Ok(());
             };
 
             // The SB history tracking needs a parent tag, so skip if we come from a wildcard.
             let SbTagExtra::Concrete(orig_tag) = orig_tag else {
                 // FIXME: should we log this?
-                return Ok(())
+                return Ok(());
             };
 
-            let extra = this.get_alloc_extra(alloc_id)?;
-            let mut stacked_borrows = extra
-                .stacked_borrows
-                .as_ref()
-                .expect("we should have Stacked Borrows data")
-                .borrow_mut();
-            stacked_borrows.history.log_creation(
-                Some(orig_tag),
-                new_tag,
-                alloc_range(base_offset, size),
-                current_span,
-            );
-            if protect {
-                stacked_borrows.history.log_protector(orig_tag, new_tag, current_span);
+            let (_size, _align, kind) = this.get_alloc_info(alloc_id);
+            match kind {
+                AllocKind::LiveData => {
+                    // This should have alloc_extra data, but `get_alloc_extra` can still fail
+                    // if converting this alloc_id from a global to a local one
+                    // uncovers a non-supported `extern static`.
+                    let extra = this.get_alloc_extra(alloc_id)?;
+                    let mut stacked_borrows = extra
+                        .stacked_borrows
+                        .as_ref()
+                        .expect("we should have Stacked Borrows data")
+                        .borrow_mut();
+                    stacked_borrows.history.log_creation(
+                        Some(orig_tag),
+                        new_tag,
+                        alloc_range(base_offset, size),
+                        current_span,
+                    );
+                    if protect {
+                        stacked_borrows.history.log_protector(orig_tag, new_tag, current_span);
+                    }
+                }
+                AllocKind::Function | AllocKind::Vtable | AllocKind::Dead => {
+                    // No stacked borrows on these allocations.
+                }
             }
             Ok(())
         };
@@ -1031,16 +1042,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         fn qualify(ty: ty::Ty<'_>, kind: RetagKind) -> Option<(RefKind, bool)> {
             match ty.kind() {
                 // References are simple.
-                ty::Ref(_, _, Mutability::Mut) =>
-                    Some((
-                        RefKind::Unique { two_phase: kind == RetagKind::TwoPhase },
-                        kind == RetagKind::FnEntry,
-                    )),
-                ty::Ref(_, _, Mutability::Not) =>
-                    Some((RefKind::Shared, kind == RetagKind::FnEntry)),
+                ty::Ref(_, _, Mutability::Mut) => Some((
+                    RefKind::Unique { two_phase: kind == RetagKind::TwoPhase },
+                    kind == RetagKind::FnEntry,
+                )),
+                ty::Ref(_, _, Mutability::Not) => {
+                    Some((RefKind::Shared, kind == RetagKind::FnEntry))
+                }
                 // Raw pointers need to be enabled.
-                ty::RawPtr(tym) if kind == RetagKind::Raw =>
-                    Some((RefKind::Raw { mutable: tym.mutbl == Mutability::Mut }, false)),
+                ty::RawPtr(tym) if kind == RetagKind::Raw => {
+                    Some((RefKind::Raw { mutable: tym.mutbl == Mutability::Mut }, false))
+                }
                 // Boxes are handled separately due to that allocator situation,
                 // see the visitor below.
                 _ => None,
@@ -1142,7 +1154,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     }
 
     /// Mark the given tag as exposed. It was found on a pointer with the given AllocId.
-    fn expose_tag(&mut self, alloc_id: AllocId, tag: SbTag) {
+    fn expose_tag(&mut self, alloc_id: AllocId, tag: SbTag) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
         // Function pointers and dead objects don't have an alloc_extra so we ignore them.
@@ -1151,14 +1163,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let (_size, _align, kind) = this.get_alloc_info(alloc_id);
         match kind {
             AllocKind::LiveData => {
-                // This should have alloc_extra data.
-                let alloc_extra = this.get_alloc_extra(alloc_id).unwrap();
+                // This should have alloc_extra data, but `get_alloc_extra` can still fail
+                // if converting this alloc_id from a global to a local one
+                // uncovers a non-supported `extern static`.
+                let alloc_extra = this.get_alloc_extra(alloc_id)?;
                 trace!("Stacked Borrows tag {tag:?} exposed in {alloc_id:?}");
                 alloc_extra.stacked_borrows.as_ref().unwrap().borrow_mut().exposed_tags.insert(tag);
             }
-            AllocKind::Function | AllocKind::Dead => {
+            AllocKind::Function | AllocKind::Vtable | AllocKind::Dead => {
                 // No stacked borrows on these allocations.
             }
         }
+        Ok(())
     }
 }
diff --git a/tests/fail/issue-miri-1112.rs b/tests/fail/issue-miri-1112.rs
@@ -28,7 +28,7 @@ impl FunnyPointer {
             data: data as *const _ as *const (),
             vtable: ptr as *const _ as *const (),
         };
-        let obj = std::mem::transmute::<FatPointer, *mut FunnyPointer>(obj); //~ ERROR: invalid drop function pointer in vtable
+        let obj = std::mem::transmute::<FatPointer, *mut FunnyPointer>(obj); //~ ERROR: expected a vtable pointer
         &*obj
     }
 }
diff --git a/tests/fail/issue-miri-1112.stderr b/tests/fail/issue-miri-1112.stderr
@@ -1,8 +1,8 @@
-error: Undefined Behavior: constructing invalid value: encountered invalid drop function pointer in vtable (function has incompatible signature)
+error: Undefined Behavior: constructing invalid value: encountered $HEX[ALLOC]<TAG>, but expected a vtable pointer
   --> $DIR/issue-miri-1112.rs:LL:CC
    |
 LL |         let obj = std::mem::transmute::<FatPointer, *mut FunnyPointer>(obj);
-   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered invalid drop function pointer in vtable (function has incompatible signature)
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered $HEX[ALLOC]<TAG>, but expected a vtable pointer
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
diff --git a/tests/fail/stacked_borrows/vtable.stderr b/tests/fail/stacked_borrows/vtable.stderr
diff --git a/tests/fail/validity/invalid_wide_raw.rs b/tests/fail/validity/invalid_wide_raw.rs
@@ -7,5 +7,5 @@ fn main() {
         #[allow(dead_code)]
         x: *mut dyn T,
     }
-    dbg!(S { x: unsafe { std::mem::transmute((0usize, 0usize)) } }); //~ ERROR: encountered dangling vtable pointer in wide pointer
+    dbg!(S { x: unsafe { std::mem::transmute((0usize, 0usize)) } }); //~ ERROR: encountered null pointer, but expected a vtable pointer
 }
diff --git a/tests/fail/validity/invalid_wide_raw.stderr b/tests/fail/validity/invalid_wide_raw.stderr
@@ -1,8 +1,8 @@
-error: Undefined Behavior: constructing invalid value: encountered dangling vtable pointer in wide pointer
+error: Undefined Behavior: constructing invalid value: encountered null pointer, but expected a vtable pointer
   --> $DIR/invalid_wide_raw.rs:LL:CC
    |
 LL |     dbg!(S { x: unsafe { std::mem::transmute((0usize, 0usize)) } });
-   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered dangling vtable pointer in wide pointer
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered null pointer, but expected a vtable pointer
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
diff --git a/tests/pass/issues/issue-miri-2123.rs b/tests/pass/issues/issue-miri-2123.rs
@@ -1,19 +1,21 @@
-//@error-pattern: vtable pointer does not have permission
-#![feature(ptr_metadata)]
+#![feature(ptr_metadata, layout_for_ptr)]
+
+use std::{mem, ptr};
 
 trait Foo {}
 
 impl Foo for u32 {}
 
 fn uwu(thin: *const (), meta: &'static ()) -> *const dyn Foo {
-    core::ptr::from_raw_parts(thin, unsafe { core::mem::transmute(meta) })
+    ptr::from_raw_parts(thin, unsafe { mem::transmute(meta) })
 }
 
 fn main() {
     unsafe {
         let orig = 1_u32;
         let x = &orig as &dyn Foo;
         let (ptr, meta) = (x as *const dyn Foo).to_raw_parts();
-        let _ = uwu(ptr, core::mem::transmute(meta));
+        let ptr = uwu(ptr, mem::transmute(meta));
+        mem::size_of_val_raw(ptr);
     }
 }
diff --git a/tests/pass/pointers.rs b/tests/pass/pointers.rs
@@ -1,4 +1,7 @@
-use std::mem::transmute;
+#![feature(ptr_metadata)]
+
+use std::mem::{self, transmute};
+use std::ptr;
 
 fn one_line_ref() -> i16 {
     *&1
@@ -71,6 +74,19 @@ fn wide_ptr_ops() {
     assert!(!(a > b));
 }
 
+fn metadata_vtable() {
+    let p = &0i32 as &dyn std::fmt::Debug;
+    let meta: ptr::DynMetadata<_> = ptr::metadata(p as *const _);
+    assert_eq!(meta.size_of(), mem::size_of::<i32>());
+    assert_eq!(meta.align_of(), mem::align_of::<i32>());
+
+    type T = [i32; 16];
+    let p = &T::default() as &dyn std::fmt::Debug;
+    let meta: ptr::DynMetadata<_> = ptr::metadata(p as *const _);
+    assert_eq!(meta.size_of(), mem::size_of::<T>());
+    assert_eq!(meta.align_of(), mem::align_of::<T>());
+}
+
 fn main() {
     assert_eq!(one_line_ref(), 1);
     assert_eq!(basic_ref(), 1);
@@ -116,4 +132,5 @@ fn main() {
     assert!(dangling >= 4);
 
     wide_ptr_ops();
+    metadata_vtable();
 }

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ impl FunnyPointer {`
`28`	`28`	`data: data as const _ as const (),`
`29`	`29`	`vtable: ptr as const _ as const (),`
`30`	`30`	`};`
`31`		`- let obj = std::mem::transmute::<FatPointer, *mut FunnyPointer>(obj); //~ ERROR: invalid drop function pointer in vtable`
	`31`	`+ let obj = std::mem::transmute::<FatPointer, *mut FunnyPointer>(obj); //~ ERROR: expected a vtable pointer`
`32`	`32`	`&*obj`
`33`	`33`	`}`
`34`	`34`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,8 +1,8 @@`
`1`		`-error: Undefined Behavior: constructing invalid value: encountered invalid drop function pointer in vtable (function has incompatible signature)`
	`1`	`+error: Undefined Behavior: constructing invalid value: encountered $HEX[ALLOC]<TAG>, but expected a vtable pointer`
`2`	`2`	`--> $DIR/issue-miri-1112.rs:LL:CC`
`3`	`3`	`\|`
`4`	`4`	`LL \| let obj = std::mem::transmute::<FatPointer, *mut FunnyPointer>(obj);`
`5`		`- \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered invalid drop function pointer in vtable (function has incompatible signature)`
	`5`	`+ \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered $HEX[ALLOC]<TAG>, but expected a vtable pointer`
`6`	`6`	`\|`
`7`	`7`	`= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior`
`8`	`8`	`= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information`
Original file line number	Diff line number	Diff line change
`@@ -7,5 +7,5 @@ fn main() {`
`7`	`7`	`#[allow(dead_code)]`
`8`	`8`	`x: *mut dyn T,`
`9`	`9`	`}`
`10`		`- dbg!(S { x: unsafe { std::mem::transmute((0usize, 0usize)) } }); //~ ERROR: encountered dangling vtable pointer in wide pointer`
	`10`	`+ dbg!(S { x: unsafe { std::mem::transmute((0usize, 0usize)) } }); //~ ERROR: encountered null pointer, but expected a vtable pointer`
`11`	`11`	`}`