From 0bd7b518cf238bf044c92c55c82a0d85428a5d21 Mon Sep 17 00:00:00 2001 From: Jake Hughes Date: Mon, 2 Dec 2024 11:12:44 +0000 Subject: [PATCH] Use FSA to check for thread-locals Finalizers in Alloy are run on a separate thread, so if a user tries to access a thread-local from within a drop method used as a finalizer it is almost certainly incorrect. This commit disallows this and emit an error when it happens. --- compiler/rustc_middle/src/ty/util.rs | 7 ++++ .../src/check_finalizers.rs | 41 ++++++++++++++++++- tests/ui/static/gc/fsa/thread_locals.rs | 27 ++++++++++++ tests/ui/static/gc/fsa/thread_locals.stderr | 13 ++++++ 4 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 tests/ui/static/gc/fsa/thread_locals.rs create mode 100644 tests/ui/static/gc/fsa/thread_locals.stderr diff --git a/compiler/rustc_middle/src/ty/util.rs b/compiler/rustc_middle/src/ty/util.rs index 6d5d7ec85a74..492086ca1790 100644 --- a/compiler/rustc_middle/src/ty/util.rs +++ b/compiler/rustc_middle/src/ty/util.rs @@ -1224,6 +1224,13 @@ impl<'tcx> Ty<'tcx> { return false; } + pub fn is_thread_local(self, tcx: TyCtxt<'tcx>) -> bool { + if let ty::Adt(adt_def, ..) = self.kind() { + return tcx.get_diagnostic_item(sym::LocalKey).map_or(false, |t| adt_def.did() == t); + } + false + } + pub fn is_finalize_unchecked(self, tcx: TyCtxt<'tcx>) -> bool { if let ty::Adt(adt_def, ..) = self.kind() { return tcx diff --git a/compiler/rustc_mir_transform/src/check_finalizers.rs b/compiler/rustc_mir_transform/src/check_finalizers.rs index 3e34404e649a..25db3886bf31 100644 --- a/compiler/rustc_mir_transform/src/check_finalizers.rs +++ b/compiler/rustc_mir_transform/src/check_finalizers.rs @@ -4,6 +4,7 @@ use rustc_data_structures::fx::FxHashSet; use rustc_hir::def_id::DefId; use rustc_hir::lang_items::LangItem; use rustc_middle::mir::visit::PlaceContext; +use rustc_middle::mir::visit::TyContext; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt}; @@ -35,6 +36,7 @@ enum FinalizerErrorKind<'tcx> { InlineAsm(FnInfo<'tcx>), /// Contains a field projection where one of the projection elements is a raw pointer. RawPtr(FnInfo<'tcx>, ProjInfo<'tcx>), + ThreadLocal(FnInfo<'tcx>), } /// Information about the projection which caused the FSA error. @@ -423,6 +425,17 @@ impl<'tcx> FSAEntryPointCtxt<'tcx> { err.span_label(pi.span, "or be safe to use across threads."); err.help("`Gc` runs finalizers on a separate thread, so drop methods\ncannot safely dereference raw pointers. If you are sure that this is safe,\nconsider wrapping it in a type which implements `Send + Sync`."); } + FinalizerErrorKind::ThreadLocal(fi) => { + err = self.tcx.sess.psess.dcx.struct_span_err( + self.arg_span, + format!("The drop method for `{0}` cannot be safely finalized.", fi.drop_ty), + ); + err.span_label( + fi.span, + format!("this thread-local is not safe to run in a finalizer"), + ); + err.help("`Gc` runs finalizers on a separate thread, so thread-locals cannot be accessed."); + } } err.span_label( self.fn_span, @@ -454,6 +467,7 @@ struct DropCtxt<'ecx, 'tcx> { /// us to deal with recursive function calls. Without this, recursive calls in `drop` would /// cause FSA to loop forever. visited_fns: FxHashSet>, + return_early: bool, } impl<'ecx, 'tcx> DropCtxt<'ecx, 'tcx> { @@ -464,7 +478,7 @@ impl<'ecx, 'tcx> DropCtxt<'ecx, 'tcx> { ) -> Self { let mut callsites = VecDeque::default(); callsites.push_back(drop_instance); - Self { callsites, ecx, drop_ty, visited_fns: FxHashSet::default() } + Self { callsites, ecx, drop_ty, visited_fns: FxHashSet::default(), return_early: false } } fn check(mut self) -> Result<(), Vec>> { @@ -528,6 +542,9 @@ impl<'dcx, 'ecx, 'tcx> FuncCtxt<'dcx, 'ecx, 'tcx> { impl<'dcx, 'ecx, 'tcx> Visitor<'tcx> for FuncCtxt<'dcx, 'ecx, 'tcx> { fn visit_projection(&mut self, place_ref: PlaceRef<'tcx>, _: PlaceContext, location: Location) { + if self.dcx.return_early { + return; + } // A single projection can be comprised of other 'inner' projections (e.g. self.a.b.c), so // this loop ensures that the types of each intermediate projection is extracted and then // checked. @@ -562,6 +579,9 @@ impl<'dcx, 'ecx, 'tcx> Visitor<'tcx> for FuncCtxt<'dcx, 'ecx, 'tcx> { } fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { + if self.dcx.return_early { + return; + } let (instance, info) = match &terminator.kind { TerminatorKind::Call { func, fn_span, .. } => { match func.ty(self.body, self.tcx()).kind() { @@ -645,6 +665,25 @@ impl<'dcx, 'ecx, 'tcx> Visitor<'tcx> for FuncCtxt<'dcx, 'ecx, 'tcx> { }; self.super_terminator(terminator, location); } + + fn visit_ty(&mut self, ty: Ty<'tcx>, cx: TyContext) { + if self.dcx.return_early { + return; + } + if !ty.is_thread_local(self.tcx()) { + self.super_ty(ty); + return; + } + + let TyContext::Location(loc) = cx else { + bug!(); + }; + + // We've found a thread-local inside a finalizer, which is almost certainly incorrect. + let info = FnInfo::new(self.body.source_info(loc).span, self.dcx.drop_ty); + self.push_error(loc, FinalizerErrorKind::ThreadLocal(info)); + self.dcx.return_early = true; + } } fn in_std_lib<'tcx>(tcx: TyCtxt<'tcx>, did: DefId) -> bool { diff --git a/tests/ui/static/gc/fsa/thread_locals.rs b/tests/ui/static/gc/fsa/thread_locals.rs new file mode 100644 index 000000000000..ce5df82c23fa --- /dev/null +++ b/tests/ui/static/gc/fsa/thread_locals.rs @@ -0,0 +1,27 @@ +#![feature(gc)] +#![feature(negative_impls)] +#![allow(dead_code)] +#![allow(unused_variables)] +include!{"./auxiliary/types.rs"} + +use std::cell::Cell; + +thread_local! { + static COUNTER: Cell = Cell::new(0); +} + + +#[derive(Debug)] +struct S; + +impl Drop for S { + fn drop(&mut self) { + // Access the thread-local variable + let x = COUNTER.get(); + } +} + +fn main() { + Gc::new(FinalizerUnsafeWrapper(S)); + //~^ ERROR: The drop method for `S` cannot be safely finalized. +} diff --git a/tests/ui/static/gc/fsa/thread_locals.stderr b/tests/ui/static/gc/fsa/thread_locals.stderr new file mode 100644 index 000000000000..8c170768dcfb --- /dev/null +++ b/tests/ui/static/gc/fsa/thread_locals.stderr @@ -0,0 +1,13 @@ +error: The drop method for `S` cannot be safely finalized. + --> $DIR/thread_locals.rs:25:13 + | +LL | let x = COUNTER.get(); + | ------- this thread-local is not safe to run in a finalizer +... +LL | Gc::new(FinalizerUnsafeWrapper(S)); + | --------^^^^^^^^^^^^^^^^^^^^^^^^^- caused by trying to construct a `Gc>` here. + | + = help: `Gc` runs finalizers on a separate thread, so thread-locals cannot be accessed. + +error: aborting due to 1 previous error +