make it operate on the MIR

Centri3 · Centri3 · commit 8c84a1222d5a · 2023-07-22T13:53:54.000-05:00
diff --git a/clippy_lints/src/ptr_to_temporary.rs b/clippy_lints/src/ptr_to_temporary.rs
@@ -1,12 +1,21 @@
 use clippy_utils::consts::is_promotable;
-use clippy_utils::diagnostics::span_lint_and_note;
-use clippy_utils::{is_from_proc_macro, is_temporary, path_res};
-use rustc_hir::{BorrowKind, Expr, ExprKind, ItemKind, OwnerNode};
+use clippy_utils::diagnostics::{span_lint_and_note, span_lint_hir_and_then};
+use clippy_utils::mir::local_assignments;
+use clippy_utils::ty::has_drop;
+use clippy_utils::{is_from_proc_macro, is_temporary, last_path_segment, path_res};
+use itertools::Itertools;
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, BorrowKind, Expr, ExprKind, FnDecl, ItemKind, OwnerNode};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::TypeVisitableExt;
+use rustc_middle::mir::visit::Visitor;
+use rustc_middle::mir::{
+    self, AggregateKind, BasicBlock, BasicBlockData, CallSource, Operand, Place, Rvalue, StatementKind, TerminatorKind,
+};
+use rustc_middle::ty::{self, TypeVisitableExt};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::sym;
+use rustc_span::{sym, Span};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -45,11 +54,48 @@ impl<'tcx> LateLintPass<'tcx> for PtrToTemporary {
             return;
         }
 
-        _ = check_for_returning_raw_ptr(cx, expr) || check_for_dangling_as_ptr(cx, expr);
+        _ = check_for_returning_raw_ptr(cx, expr)
+    }
+
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'_>,
+        decl: &FnDecl<'_>,
+        body: &Body<'_>,
+        span: Span,
+        def_id: LocalDefId,
+    ) {
+        let mir = cx.tcx.optimized_mir(def_id);
+        let mut v = DanglingPtrVisitor {
+            cx,
+            body: mir,
+            results: vec![],
+        };
+        v.visit_body(mir);
+
+        for dangling_ptr_span in v.results {
+            // TODO: We need to lint on the call in question instead, so lint attributes work fine. I'm not sure
+            // how to though
+            span_lint_hir_and_then(
+                cx,
+                PTR_TO_TEMPORARY,
+                cx.tcx.local_def_id_to_hir_id(def_id),
+                dangling_ptr_span,
+                "calling `TODO` on a temporary value",
+                |diag| {
+                    diag.note(
+                        "usage of this pointer will cause Undefined Behavior as the temporary will be deallocated at \
+                         the end of the statement, yet the pointer will continue pointing to it, resulting in a \
+                         dangling pointer",
+                    );
+                },
+            );
+        }
     }
 }
 
-/// Check for returning raw pointers to temporaries that are not promoted to a constant.
+/// Check for returning raw pointers to temporaries that are not promoted to a constant
 fn check_for_returning_raw_ptr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
     // Get the final return statement if this is a return statement, or don't lint
     let expr = if let ExprKind::Ret(Some(expr)) = expr.kind {
@@ -87,61 +133,120 @@ fn check_for_returning_raw_ptr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'t
     false
 }
 
-/// Check for calls to `as_ptr` or `as_mut_ptr` that will always result in a dangling pointer, under
-/// the assumption of course that `as_ptr` will return a pointer to data owned by `self`, rather
-/// than returning a raw pointer to new memory.
-///
-/// This only lints `std` types as anything else could potentially be wrong if the above assumption
-/// doesn't hold (which it should for all `std` types).
-///
-/// We could perhaps extend this to some external crates as well, if we want.
-fn check_for_dangling_as_ptr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
-    if let ExprKind::MethodCall(seg, recv, [], _) = expr.kind
-        && (seg.ident.name == sym::as_ptr || seg.ident.name == sym!(as_mut_ptr))
-        && let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
-        && cx.tcx.fn_sig(def_id).skip_binder().output().skip_binder().is_unsafe_ptr()
-        && matches!(cx.tcx.crate_name(def_id.krate), sym::core | sym::alloc | sym::std)
-        && is_temporary(cx, recv)
-        && !is_from_proc_macro(cx, expr)
-        // The typeck table only contains `ReErased`.
-        && let Some(def_id) = match recv.kind {
-            ExprKind::Call(path, _) => {
-                path_res(cx, path).opt_def_id()
-            },
-            ExprKind::MethodCall(..) => {
-                cx.typeck_results().type_dependent_def_id(recv.hir_id)
-            },
-            // Anything else will either always have a `'static` lifetime or won't be dropped at
-            // the end of the statement (i.e., will have a longer lifetime), so we can skip them
-            _ => None,
+struct DanglingPtrVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    body: &'a mir::Body<'tcx>,
+    results: Vec<Span>,
+}
+
+impl<'tcx> Visitor<'tcx> for DanglingPtrVisitor<'_, 'tcx> {
+    fn visit_basic_block_data(&mut self, block: BasicBlock, data: &BasicBlockData<'tcx>) {
+        // TODO: We really need to clean this up. I hope future me has any idea what it does!
+        if let Some(term) = &data.terminator
+            && let TerminatorKind::Call {
+                // TODO: Check if this is `as_ptr`, or any of our specific functions we want to
+                // lint. Should be simple to implement.
+                func,
+                args,
+                destination,
+                target: Some(target),
+                call_source: CallSource::Normal,
+                ..
+            } = &term.kind
+            && destination.ty(&self.body.local_decls, self.cx.tcx).ty.is_unsafe_ptr()
+            && let [recv] = args.as_slice()
+            && let Some(recv) = recv.place()
+            && let [assign] = &*local_assignments(self.body, recv.local)
+            && let assign_stmt = &self.body.basic_blocks[assign.block].statements[assign.statement_index]
+            && let StatementKind::Assign(box (_, rvalue)) = &assign_stmt.kind
+            && let Rvalue::Ref(_, _, actual_recv) = rvalue
+            && let [const_assign] = &*local_assignments(self.body, actual_recv.local)
+            && if const_assign.statement_index == self.body.basic_blocks[const_assign.block].statements.len() {
+                if let TerminatorKind::Call {
+                        func,
+                        call_source: CallSource::Normal,
+                        ..
+                    } = &self.body.basic_blocks[const_assign.block].terminator().kind
+                    && let Some((def_id, _)) = func.const_fn_def()
+                    && let ty = self.cx.tcx.fn_sig(def_id).subst_identity().skip_binder().output()
+                    && (ty.has_late_bound_vars() || !ty.is_ref())
+                    && !matches!(ty.kind(), ty::Ref(region, _, _) if region.is_static() )
+                {
+                    true
+                } else {
+                    false
+                }
+            } else {
+                let StatementKind::Assign(box (_, rvalue)) =
+                    &self.body.basic_blocks[const_assign.block].statements[const_assign.statement_index].kind
+                else {
+                    return;
+                };
+
+                let ops = match rvalue {
+                    Rvalue::Use(op) => vec![op],
+                    Rvalue::Repeat(op, _) => vec![op],
+                    Rvalue::Cast(_, op, _) => vec![op],
+                    Rvalue::Aggregate(box AggregateKind::Array(_) | box AggregateKind::Tuple, ops) => {
+                        ops.into_iter().collect_vec()
+                    },
+                    _ => return,
+                };
+
+                !ops.iter().all(|op| matches!(op, Operand::Constant(_)))
+            }
+        {
+            check_for_dangling(
+                self.cx,
+                self.body,
+                *target,
+                &actual_recv,
+                destination,
+                term.source_info.span,
+                &mut self.results,
+            );
         }
-        && let ty = cx.tcx.fn_sig(def_id).subst_identity().output()
-        // afaik, any `ReLateBound` means it isn't static. Lifetimes are confusing under the
-        // hood tho so this is likely wrong, and we'll need a smarter heuristic
-        && (ty.has_late_bound_regions() || !ty.skip_binder().is_ref())
+    }
+}
+
+fn check_for_dangling<'tcx>(
+    cx: &LateContext<'tcx>,
+    body: &mir::Body<'tcx>,
+    bb: BasicBlock,
+    recv: &Place<'tcx>,
+    ptr: &Place<'_>,
+    span: Span,
+    results: &mut Vec<Span>,
+) {
+    let data = &body.basic_blocks[bb];
+
+    if let Some(term) = &data.terminator
+        && let TerminatorKind::Drop { .. } = term.kind
     {
-        span_lint_and_note(
-            cx,
-            PTR_TO_TEMPORARY,
-            expr.span,
-            "calling `as_ptr` on a temporary value",
-            None,
-            "usage of this pointer will cause Undefined Behavior as the temporary will be deallocated at the end of \
-             the statement, yet the pointer will continue pointing to it, resulting in a dangling pointer",
-        );
+        if !body.basic_blocks[bb].statements.iter().any(|stmt| {
+            matches!(stmt.kind, StatementKind::StorageDead(local) if ptr.local == local)
+        }) {
+            results.push(span);
+        }
 
-        return true;
+        return;
     }
 
-    false
-}
+    // No drop glue or it's not a temporary. Bummer. Instead let's inspect the order of dead locals
+    // to see if the pointer is dangling for even a single statement. If it's not, the reference to
+    // `self` should always be dead after the pointer.
 
-// TODO: Add a check here for some blocklist for methods that return a raw pointer that we should
-// lint. We can't bulk-deny these because we don't know whether it's returning something owned by
-// `self` (and will thus be dropped at the end of the statement) or is returning a pointer to newly
-// allocated memory, like what allocators do.
-/*
-fn check_for_denied_ptr_method<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
-    true
+    let mut recv_dead = false;
+
+    for stmt in &data.statements {
+        if let StatementKind::StorageDead(local) = stmt.kind {
+            match (local == recv.local, local == ptr.local) {
+                (true, false) => recv_dead = true,
+                (false, true) if recv_dead => {
+                    results.push(span);
+                },
+                _ => continue,
+            }
+        }
+    }
 }
-*/
diff --git a/tests/ui/ptr_to_temporary.rs b/tests/ui/ptr_to_temporary.rs
@@ -1,4 +1,4 @@
-//@aux-build:proc_macros.rs:proc-macro
+//#aux-build:proc_macros.rs:proc-macro
 #![allow(
     clippy::borrow_deref_ref,
     clippy::deref_addrof,
@@ -9,8 +9,8 @@
 #![warn(clippy::ptr_to_temporary)]
 #![no_main]
 
-#[macro_use]
-extern crate proc_macros;
+// #[macro_use]
+// extern crate proc_macros;
 
 use std::cell::{Cell, RefCell};
 use std::ffi::CString;
@@ -51,6 +51,12 @@ fn bad_6() {
     let pv = vec![1].as_ptr();
 }
 
+// TODO: Not linted yet. I believe this is because there's a cast in the MIR between the call to
+// `helper` and `as_ptr`. We likely need to be smarter with traversing assignments backwards.
+//
+// We can also probably optimize it by doing one pass over the body to get local assignments, that
+// way we don't need to call `local_assignments` a ton of times (and thus do a ton of completely
+// avoidable passes as well).
 fn bad_7() {
     fn helper() -> [i32; 1] {
         [1]
@@ -81,10 +87,13 @@ fn bad_12() {
 }
 
 fn bad_13() {
-    // TODO: Not linted. We need to normalize these
     let ps = <[i32]>::as_ptr(Vec::as_slice(&vec![1]));
 }
 
+fn bad_14() {
+    CString::new("asd".to_owned()).unwrap().as_c_str().as_ptr();
+}
+
 // TODO: We need more tests here...
 
 fn fine_1() -> *const i32 {
@@ -137,17 +146,16 @@ fn fine_8() {
 fn fine_9() {
     let pcstr = CString::new("asd".to_owned()).unwrap();
     // Not UB, as `pcstr` is not a temporary
-    // TODO: Don't lint this! We need a check for whether a chain begins with a local...
     pcstr.as_c_str().as_ptr();
 }
 
 fn fine_10() {
     let pcstr = CString::new("asd".to_owned()).unwrap();
     // Not UB, as `pcstr` is not a temporary
-    // TODO: Same here, but this time it needs to be normalized
     CString::as_c_str(&pcstr).as_ptr();
 }
 
+/*
 external! {
     fn fine_external() -> *const i32 {
         let a = 0i32;
@@ -162,3 +170,4 @@ with_span! {
         &a as *const i32
     }
 }
+*/
diff --git a/tests/ui/ptr_to_temporary.stderr b/tests/ui/ptr_to_temporary.stderr
