Auto merge of #136776 - BoxyUwU:forbid_object_lifetime_casts, r=lcnr

Forbid freely casting lifetime bounds of dyn-types

Fixes #136702

Reference PR:

- rust-lang/reference#1951

Background reading about VTable calls/dyn compatibility: https://hackmd.io/zUp-sgZ0RFuFgsNfD4JqYw

This PR causes us to start enforcing that lifetimes of dyn types are constrained through pointer casts. Currently on stable casting `*mut dyn Trait + 'a` to `*mut dyn Trait + 'b` passes with no requirements on `'a` or `'b`. Under this PR we now require `'a` to outlive `'b`.

Even though the pointee of `*mut` pointers is considered to be invariant, we still use subtyping rather than equality. This mirrors how we support coercing `&mut dyn Trait + 'a` to `&mut dyn Trait + 'b` while requiring only `'a: 'b`. I believe this coercion is sound as there is no way for safe code to `mem::swap` two `dyn Trait`'s, and the same is definitely true of raw pointers.

See the changes to this test: https://github.com/rust-lang/rust/pull/136776/files#diff-5523f20a800287a89c9f3e92646c887f3f7599be006b29dd9315f734a2137764

We also do not enforce any constraints on the lifetime of the dyn types if there are multiple pointer indirections. For example `*mut *mut dyn Trait + 'a` is allowed to be casted to `*mut *mut dyn Trait + 'b` with no requirements on `'a` or 'b`. This case is just a normal thin pointer cast where we do not care about the pointee type as there is no VTable in play.

Test: https://github.com/rust-lang/rust/pull/136776/files#diff-3b6c8da342bb6530524158d686455a545bb8fd6f59cf5ff50d1d991ce74c9649

Finally, this is about *any* cast where the pointee is *unsized* with dyn-type metadata, not just *literally* the pointee type being a dyn-type. E.g. casting `*mut Wrapper<dyn Trait + 'a>` to `*mut Wrapper<dyn Trait + 'b>` requires `'a: 'b` under this PR.

Test: https://github.com/rust-lang/rust/pull/136776/files#diff-ca0c44df62ae1ad1be70f892f01a59714336c7baf78602a5887ac1cf81145c96

### Breakage

This is a breaking change.
Crater Report Comment: #136776 (comment)
Generated Report: https://crater-reports.s3.amazonaws.com/pr-136776-2/index.html

The majority of the breakage is caused by the `metrics` crate with 142 of the regressions, and the `may` crate with 14 of the regressions. The `metrics` crate has been fixed and has backported the fix to previous versions of the crate that were also affected. The`may` crate has also been fixed.

PRs against affected crates have been opened and can be seen here:
- belalang-project/belalang#6
- tyilo/multi-vec#1
- luksan/lox#1
- pfzetto/bring-your-own-memory-demo#1
- vitorhnn/bfr#1
- gipsyh/PPSMC#1
- orengine/orengine#33
- maroider/async_scoped_task#1
- WorldSEnder/scoped_worker_thread#1
- Wind-Corporation/trapiron#5
- Thombrom/snek#1
- Xudong-Huang/may#113
- metrics-rs/metrics#564
- DouglasDwyer/micropool#1
- Magicolo/phylactery#8
- HellButcher/pulz#29
- UxuginPython/rrtk#1
- wvwwvwwv/scalable-delayed-dealloc#4
- ultimaweapon/tsuki#32

There were six regressions I've not filed PRs against:
- https://github.com/weiznich/diesel_benches depends on a ~6year old version of diesel (where the regression is)
- https://crates.io/crates/cogo/0.1.36 is an old version of cogo, since that release cogo has already been updated to not depend on pattern this PR breaks
- https://github.com/cruise-automation/webviz-rust-framework is an archived read only repo so 🤷‍♀️
- makepad_render, doesn't seem to have source available and is 6 years old 🤷‍♀️
- outsource-heap - not on github
- zaplib - I couldn't get it to compile locally as it failed to compile a dependency

r? `@ghost`

Author: bors

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

@@ -410,6 +410,7 @@ impl<'tcx> BorrowExplanation<'tcx> {
                 cx.add_sized_or_copy_bound_info(err, category, &path);
 
                 if let ConstraintCategory::Cast {
+                    is_raw_ptr_dyn_type_cast: _,
                     is_implicit_coercion: true,
                     unsize_to: Some(unsize_ty),
                 } = category

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

@@ -541,6 +541,23 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         self.add_placeholder_from_predicate_note(&mut diag, &path);
         self.add_sized_or_copy_bound_info(&mut diag, category, &path);
 
+        for constraint in &path {
+            if let ConstraintCategory::Cast { is_raw_ptr_dyn_type_cast: true, .. } =
+                constraint.category
+            {
+                diag.span_note(
+                    constraint.span,
+                    format!("raw pointer casts of trait objects cannot extend lifetimes"),
+                );
+                diag.note(format!(
+                    "this was previously accepted by the compiler but was changed recently"
+                ));
+                diag.help(format!(
+                    "see <https://github.com/rust-lang/rust/issues/141402> for more information"
+                ));
+            }
+        }
+
         self.buffer_error(diag);
     }
 

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -1697,6 +1697,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 // should be as limited as possible; the note is prone to false positives and this
                 // constraint usually isn't best to blame.
                 ConstraintCategory::Cast {
+                    is_raw_ptr_dyn_type_cast: _,
                     unsize_to: Some(unsize_ty),
                     is_implicit_coercion: true,
                 } if to_region == self.universal_regions().fr_static

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -1113,15 +1113,23 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                             self.prove_predicate(
                                 ty::ClauseKind::WellFormed(src_ty.into()),
                                 location.to_locations(),
-                                ConstraintCategory::Cast { is_implicit_coercion, unsize_to: None },
+                                ConstraintCategory::Cast {
+                                    is_raw_ptr_dyn_type_cast: false,
+                                    is_implicit_coercion,
+                                    unsize_to: None,
+                                },
                             );
 
                             let src_ty = self.normalize(src_ty, location);
                             if let Err(terr) = self.sub_types(
                                 src_ty,
                                 *ty,
                                 location.to_locations(),
-                                ConstraintCategory::Cast { is_implicit_coercion, unsize_to: None },
+                                ConstraintCategory::Cast {
+                                    is_raw_ptr_dyn_type_cast: false,
+                                    is_implicit_coercion,
+                                    unsize_to: None,
+                                },
                             ) {
                                 span_mirbug!(
                                     self,
@@ -1142,7 +1150,11 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                         self.prove_predicate(
                             ty::ClauseKind::WellFormed(src_ty.into()),
                             location.to_locations(),
-                            ConstraintCategory::Cast { is_implicit_coercion, unsize_to: None },
+                            ConstraintCategory::Cast {
+                                is_raw_ptr_dyn_type_cast: false,
+                                is_implicit_coercion,
+                                unsize_to: None,
+                            },
                         );
 
                         // The type that we see in the fcx is like
@@ -1155,7 +1167,11 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                             src_ty,
                             *ty,
                             location.to_locations(),
-                            ConstraintCategory::Cast { is_implicit_coercion, unsize_to: None },
+                            ConstraintCategory::Cast {
+                                is_raw_ptr_dyn_type_cast: false,
+                                is_implicit_coercion,
+                                unsize_to: None,
+                            },
                         ) {
                             span_mirbug!(
                                 self,
@@ -1184,7 +1200,11 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                             ty_fn_ptr_from,
                             *ty,
                             location.to_locations(),
-                            ConstraintCategory::Cast { is_implicit_coercion, unsize_to: None },
+                            ConstraintCategory::Cast {
+                                is_raw_ptr_dyn_type_cast: false,
+                                is_implicit_coercion,
+                                unsize_to: None,
+                            },
                         ) {
                             span_mirbug!(
                                 self,
@@ -1217,7 +1237,11 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                             ty_fn_ptr_from,
                             *ty,
                             location.to_locations(),
-                            ConstraintCategory::Cast { is_implicit_coercion, unsize_to: None },
+                            ConstraintCategory::Cast {
+                                is_raw_ptr_dyn_type_cast: false,
+                                is_implicit_coercion,
+                                unsize_to: None,
+                            },
                         ) {
                             span_mirbug!(
                                 self,
@@ -1246,6 +1270,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                             trait_ref,
                             location.to_locations(),
                             ConstraintCategory::Cast {
+                                is_raw_ptr_dyn_type_cast: false,
                                 is_implicit_coercion,
                                 unsize_to: Some(unsize_to),
                             },
@@ -1271,7 +1296,11 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                             *ty_from,
                             *ty_to,
                             location.to_locations(),
-                            ConstraintCategory::Cast { is_implicit_coercion, unsize_to: None },
+                            ConstraintCategory::Cast {
+                                is_raw_ptr_dyn_type_cast: false,
+                                is_implicit_coercion,
+                                unsize_to: None,
+                            },
                         ) {
                             span_mirbug!(
                                 self,
@@ -1334,7 +1363,11 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                             *ty_elem,
                             *ty_to,
                             location.to_locations(),
-                            ConstraintCategory::Cast { is_implicit_coercion, unsize_to: None },
+                            ConstraintCategory::Cast {
+                                is_raw_ptr_dyn_type_cast: false,
+                                is_implicit_coercion,
+                                unsize_to: None,
+                            },
                         ) {
                             span_mirbug!(
                                 self,
@@ -1491,55 +1524,90 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                                 trait_ref,
                                 location.to_locations(),
                                 ConstraintCategory::Cast {
+                                    is_raw_ptr_dyn_type_cast: false,
                                     is_implicit_coercion: true,
                                     unsize_to: None,
                                 },
                             );
-                        } else if let ty::Dynamic(src_tty, _src_lt) =
+                        } else if let ty::Dynamic(src_tty, src_lt) =
                             *self.struct_tail(src.ty, location).kind()
                             && let ty::Dynamic(dst_tty, dst_lt) =
                                 *self.struct_tail(dst.ty, location).kind()
-                            && src_tty.principal().is_some()
-                            && dst_tty.principal().is_some()
                         {
-                            // This checks (lifetime part of) vtable validity for pointer casts,
-                            // which is irrelevant when there are aren't principal traits on
-                            // both sides (aka only auto traits).
-                            //
-                            // Note that other checks (such as denying `dyn Send` -> `dyn
-                            // Debug`) are in `rustc_hir_typeck`.
-
-                            // Remove auto traits.
-                            // Auto trait checks are handled in `rustc_hir_typeck` as FCW.
-                            let src_obj = Ty::new_dynamic(
-                                tcx,
-                                tcx.mk_poly_existential_predicates(
-                                    &src_tty.without_auto_traits().collect::<Vec<_>>(),
-                                ),
-                                // FIXME: Once we disallow casting `*const dyn Trait + 'short`
-                                // to `*const dyn Trait + 'long`, then this can just be `src_lt`.
-                                dst_lt,
-                            );
-                            let dst_obj = Ty::new_dynamic(
-                                tcx,
-                                tcx.mk_poly_existential_predicates(
-                                    &dst_tty.without_auto_traits().collect::<Vec<_>>(),
+                            match (src_tty.principal(), dst_tty.principal()) {
+                                (Some(_), Some(_)) => {
+                                    // This checks (lifetime part of) vtable validity for pointer casts,
+                                    // which is irrelevant when there are aren't principal traits on
+                                    // both sides (aka only auto traits).
+                                    //
+                                    // Note that other checks (such as denying `dyn Send` -> `dyn
+                                    // Debug`) are in `rustc_hir_typeck`.
+
+                                    // Remove auto traits.
+                                    // Auto trait checks are handled in `rustc_hir_typeck`.
+                                    let src_obj = Ty::new_dynamic(
+                                        tcx,
+                                        tcx.mk_poly_existential_predicates(
+                                            &src_tty.without_auto_traits().collect::<Vec<_>>(),
+                                        ),
+                                        src_lt,
+                                    );
+                                    let dst_obj = Ty::new_dynamic(
+                                        tcx,
+                                        tcx.mk_poly_existential_predicates(
+                                            &dst_tty.without_auto_traits().collect::<Vec<_>>(),
+                                        ),
+                                        dst_lt,
+                                    );
+
+                                    debug!(?src_tty, ?dst_tty, ?src_obj, ?dst_obj);
+
+                                    // Trait parameters are invariant, the only part that actually has
+                                    // subtyping here is the lifetime bound of the dyn-type.
+                                    //
+                                    // For example in `dyn Trait<'a> + 'b <: dyn Trait<'c> + 'd`  we would
+                                    // require that `'a == 'c` but only that `'b: 'd`.
+                                    //
+                                    // We must not allow freely casting lifetime bounds of dyn-types as it
+                                    // may allow for inaccessible VTable methods being callable: #136702
+                                    self.sub_types(
+                                        src_obj,
+                                        dst_obj,
+                                        location.to_locations(),
+                                        ConstraintCategory::Cast {
+                                            is_raw_ptr_dyn_type_cast: true,
+                                            is_implicit_coercion: false,
+                                            unsize_to: None,
+                                        },
+                                    )
+                                    .unwrap();
+                                }
+                                (None, None) => {
+                                    // The principalless (no non-auto traits) case:
+                                    // You can only cast `dyn Send + 'long` to `dyn Send + 'short`.
+                                    self.constraints.outlives_constraints.push(
+                                        OutlivesConstraint {
+                                            sup: src_lt.as_var(),
+                                            sub: dst_lt.as_var(),
+                                            locations: location.to_locations(),
+                                            span: location.to_locations().span(self.body),
+                                            category: ConstraintCategory::Cast {
+                                                is_raw_ptr_dyn_type_cast: true,
+                                                is_implicit_coercion: false,
+                                                unsize_to: None,
+                                            },
+                                            variance_info: ty::VarianceDiagInfo::default(),
+                                            from_closure: false,
+                                        },
+                                    );
+                                }
+                                (None, Some(_)) => bug!(
+                                    "introducing a principal should have errored in HIR typeck"
                                 ),
-                                dst_lt,
-                            );
-
-                            debug!(?src_tty, ?dst_tty, ?src_obj, ?dst_obj);
-
-                            self.sub_types(
-                                src_obj,
-                                dst_obj,
-                                location.to_locations(),
-                                ConstraintCategory::Cast {
-                                    is_implicit_coercion: false,
-                                    unsize_to: None,
-                                },
-                            )
-                            .unwrap();
+                                (Some(_), None) => {
+                                    bug!("dropping the principal should have been an unsizing cast")
+                                }
+                            }
                         }
                     }
                     CastKind::Transmute => {

compiler/rustc_middle/src/mir/query.rs

@@ -108,6 +108,7 @@ pub enum ConstraintCategory<'tcx> {
     UseAsStatic,
     TypeAnnotation(AnnotationSource),
     Cast {
+        is_raw_ptr_dyn_type_cast: bool,
         /// Whether this cast is a coercion that was automatically inserted by the compiler.
         is_implicit_coercion: bool,
         /// Whether this is an unsizing coercion and if yes, this contains the target type.

library/std/src/thread/lifecycle.rs

@@ -111,7 +111,12 @@ where
     // SAFETY: dynamic size and alignment of the Box remain the same. See below for why the
     // lifetime change is justified.
     let rust_start = unsafe {
-        Box::from_raw(Box::into_raw(Box::new(rust_start)) as *mut (dyn FnOnce() + Send + 'static))
+        let ptr = Box::into_raw(Box::new(rust_start));
+        let ptr = crate::mem::transmute::<
+            *mut (dyn FnOnce() + Send + '_),
+            *mut (dyn FnOnce() + Send + 'static),
+        >(ptr);
+        Box::from_raw(ptr)
     };
 
     let init = Box::new(ThreadInit { handle: thread.clone(), rust_start });

tests/ui/cast/ptr-ptr-to-ptr-ptr-different-regions.rs

@@ -0,0 +1,15 @@
+//@ check-pass
+
+// We allow extending lifetimes of object types if they are behind two layers
+// of pointer indirection (as opposed to one). This is because this is the more
+// general case of casting between two sized types (`*mut T as *mut U`).
+
+trait Trait {
+    fn foo(&self) {}
+}
+
+fn bar<'a>(a: *mut *mut (dyn Trait + 'a)) -> *mut *mut (dyn Trait + 'static) {
+    a as _
+}
+
+fn main() {}

tests/ui/cast/ptr-to-ptr-different-regions.rs

@@ -1,20 +1,24 @@
-//@ check-pass
-
 // https://github.com/rust-lang/rust/issues/113257
 
 #![deny(trivial_casts)] // The casts here are not trivial.
 
-struct Foo<'a> { a: &'a () }
+struct Foo<'a> {
+    a: &'a (),
+}
 
 fn extend_lifetime_very_very_safely<'a>(v: *const Foo<'a>) -> *const Foo<'static> {
-    // This should pass because raw pointer casts can do anything they want.
+    // This should pass because raw pointer casts can do anything they want when
+    // VTables are not involved
     v as *const Foo<'static>
 }
 
 trait Trait {}
 
+// We want to forbid this as extending lifetimes on object types may allow for
+// uncallable VTable methods to become accessible.
 fn assert_static<'a>(ptr: *mut (dyn Trait + 'a)) -> *mut (dyn Trait + 'static) {
     ptr as _
+    //~^ ERROR: lifetime may not live long enough
 }
 
 fn main() {

tests/ui/cast/ptr-to-ptr-different-regions.stderr

@@ -0,0 +1,31 @@
+error: lifetime may not live long enough
+  --> $DIR/ptr-to-ptr-different-regions.rs:20:5
+   |
+LL | fn assert_static<'a>(ptr: *mut (dyn Trait + 'a)) -> *mut (dyn Trait + 'static) {
+   |                  -- lifetime `'a` defined here
+LL |     ptr as _
+   |     ^^^^^^^^ returning this value requires that `'a` must outlive `'static`
+   |
+   = note: requirement occurs because of a mutable pointer to `dyn Trait`
+   = note: mutable pointers are invariant over their type parameter
+   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
+note: raw pointer casts of trait objects cannot extend lifetimes
+  --> $DIR/ptr-to-ptr-different-regions.rs:20:5
+   |
+LL |     ptr as _
+   |     ^^^^^^^^
+   = note: this was previously accepted by the compiler but was changed recently
+   = help: see <https://github.com/rust-lang/rust/issues/141402> for more information
+help: consider changing the trait object's explicit `'static` bound to the lifetime of argument `ptr`
+   |
+LL - fn assert_static<'a>(ptr: *mut (dyn Trait + 'a)) -> *mut (dyn Trait + 'static) {
+LL + fn assert_static<'a>(ptr: *mut (dyn Trait + 'a)) -> *mut (dyn Trait + 'a) {
+   |
+help: alternatively, add an explicit `'static` bound to this reference
+   |
+LL - fn assert_static<'a>(ptr: *mut (dyn Trait + 'a)) -> *mut (dyn Trait + 'static) {
+LL + fn assert_static<'a>(ptr: *mut (dyn Trait + 'static)) -> *mut (dyn Trait + 'static) {
+   |
+
+error: aborting due to 1 previous error
+

tests/ui/cast/ptr-to-ptr-indirect-different-regions.rs

@@ -0,0 +1,15 @@
+// We want to forbid extending lifetimes on object types behind ptrs
+// as it may allow for uncallable VTable methods to become accessible.
+
+trait Trait {
+    fn foo(&self) {}
+}
+
+struct MyWrap<T: ?Sized>(T);
+
+fn bar<'a>(a: *mut MyWrap<(dyn Trait + 'a)>) -> *mut MyWrap<(dyn Trait + 'static)> {
+    a as _
+    //~^ ERROR: lifetime may not live long enough
+}
+
+fn main() {}