Also lint some as_ptr methods as well

Author: Centri3

clippy_lints/src/ptr_to_temporary.rs

@@ -1,8 +1,12 @@
-use clippy_utils::{consts::is_promotable, diagnostics::span_lint_and_note, is_from_proc_macro};
+use clippy_utils::consts::is_promotable;
+use clippy_utils::diagnostics::span_lint_and_note;
+use clippy_utils::{is_from_proc_macro, is_temporary};
 use rustc_hir::{BorrowKind, Expr, ExprKind, ItemKind, OwnerNode};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -11,23 +15,27 @@ declare_clippy_lint! {
     /// [constant promotion](https://doc.rust-lang.org/stable/reference/destructors.html#constant-promotion).
     ///
     /// ### Why is this bad?
-    /// Usage of such a pointer can result in Undefined Behavior, as the pointer will stop pointing
-    /// to valid stack memory once the temporary is dropped.
+    /// Usage of such a pointer will result in Undefined Behavior, as the pointer will stop
+    /// pointing to valid stack memory once the temporary is dropped.
     ///
     /// ### Example
     /// ```rust,ignore
-    /// fn x() -> *const i32 {
+    /// fn returning_temp() -> *const i32 {
     ///     let x = 0;
     ///     &x as *const i32
     /// }
     ///
-    /// let x = x();
-    /// unsafe { *x }; // ⚠️
+    /// let px = returning_temp();
+    /// unsafe { *px }; // ⚠️
+    /// let pv = vec![].as_ptr();
+    /// unsafe { *pv }; // ⚠️
     /// ```
     #[clippy::version = "1.72.0"]
     pub PTR_TO_TEMPORARY,
-    correctness,
-    "disallows returning a raw pointer to a temporary value"
+    // TODO: Let's make it warn-by-default for now, and change this to deny-by-default once we know
+    // there are no major FPs
+    suspicious,
+    "disallows obtaining raw pointers to temporary values"
 }
 declare_lint_pass!(PtrToTemporary => [PTR_TO_TEMPORARY]);
 
@@ -37,34 +45,94 @@ impl<'tcx> LateLintPass<'tcx> for PtrToTemporary {
             return;
         }
 
-        // 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 {
-            expr
-        } else if let OwnerNode::Item(parent) = cx.tcx.hir().owner(cx.tcx.hir().get_parent_item(expr.hir_id))
-            && let ItemKind::Fn(_, _, body) = parent.kind
-            && let block = cx.tcx.hir().body(body).value
-            && let ExprKind::Block(block, _) = block.kind
-            && let Some(final_block_expr) = block.expr
-            && final_block_expr.hir_id == expr.hir_id
-        {
-            expr
-        } else {
-            return;
-        };
+        _ = check_for_returning_raw_ptr(cx, expr) || check_for_dangling_as_ptr(cx, expr);
+    }
+}
 
-        if let ExprKind::Cast(cast_expr, _) = expr.kind
-            && let ExprKind::AddrOf(BorrowKind::Ref, _, e) = cast_expr.kind
-            && !is_promotable(cx, e)
-            && !is_from_proc_macro(cx, expr)
-        {
-            span_lint_and_note(
-                cx,
-                PTR_TO_TEMPORARY,
-                expr.span,
-                "returning a raw pointer to a temporary value that cannot be promoted to a constant",
-                None,
-                "usage of this pointer by callers will cause Undefined Behavior",
-            );
-        }
+/// 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 {
+        expr
+    } else if let OwnerNode::Item(parent) = cx.tcx.hir().owner(cx.tcx.hir().get_parent_item(expr.hir_id))
+        && let ItemKind::Fn(_, _, body) = parent.kind
+        && let block = cx.tcx.hir().body(body).value
+        && let ExprKind::Block(block, _) = block.kind
+        && let Some(final_block_expr) = block.expr
+        && final_block_expr.hir_id == expr.hir_id
+    {
+        expr
+    } else {
+        return false;
+    };
+
+    if let ExprKind::Cast(cast_expr, _) = expr.kind
+        && let ExprKind::AddrOf(BorrowKind::Ref, _, e) = cast_expr.kind
+        && !is_promotable(cx, e)
+        && !is_from_proc_macro(cx, expr)
+    {
+        span_lint_and_note(
+            cx,
+            PTR_TO_TEMPORARY,
+            expr.span,
+            "returning a raw pointer to a temporary value that cannot be promoted to a constant",
+            None,
+            "usage of this pointer by callers 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",
+        );
+
+        return true;
+    }
+
+    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)
+        // These will almost always be promoted yet have the `as_ptr` method. Ideally we would
+        // check if these would be promoted but our logic considers any function call to be
+        // non-promotable, but in this case it will be as it's `'static`, soo...
+        && !matches!(
+            cx.typeck_results().expr_ty(recv).peel_refs().kind(),
+            ty::Str | ty::Array(_, _) | ty::Slice(_)
+        )
+        && is_temporary(cx, recv)
+        && !is_from_proc_macro(cx, expr)
+    {
+        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",
+        );
+
+        return true;
     }
+
+    false
+}
+
+// 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
 }
+*/

clippy_utils/src/lib.rs

@@ -2458,6 +2458,16 @@ pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool {
             && item.ident.name.as_str().split('_').any(|a| a == "test" || a == "tests")
 }
 
+/// Returns whether `expr` is a temporary value that will be freed at the end of the statement.
+pub fn is_temporary(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    !expr.is_place_expr(|base| {
+        cx.typeck_results()
+            .adjustments()
+            .get(base.hir_id)
+            .is_some_and(|x| x.iter().any(|adj| matches!(adj.kind, Adjust::Deref(_))))
+    })
+}
+
 /// Walks the HIR tree from the given expression, up to the node where the value produced by the
 /// expression is consumed. Calls the function for every node encountered this way until it returns
 /// `Some`.

tests/ui/ptr_to_temporary.rs

@@ -1,12 +1,24 @@
-//@aux-build:proc_macros.rs
-#![allow(clippy::borrow_deref_ref, clippy::deref_addrof, clippy::identity_op, unused)]
+//@aux-build:proc_macros.rs:proc-macro
+#![allow(
+    clippy::borrow_deref_ref,
+    clippy::deref_addrof,
+    clippy::identity_op,
+    temporary_cstring_as_ptr,
+    unused
+)]
 #![warn(clippy::ptr_to_temporary)]
 #![no_main]
 
 #[macro_use]
 extern crate proc_macros;
 
+use std::cell::{Cell, RefCell};
+use std::ffi::CString;
+use std::sync::atomic::AtomicBool;
+
 fn bad_1() -> *const i32 {
+    // NOTE: `is_const_evaluatable` misses this. This may be a bug in that utils function, and if
+    // possible we should switch to it.
     &(100 + *&0) as *const i32
 }
 
@@ -35,6 +47,37 @@ fn bad_5() -> *const i32 {
     &a as *const i32
 }
 
+fn bad_6() {
+    let pv = vec![1].as_ptr();
+}
+
+fn bad_7() {
+    fn helper() -> [i32; 1] {
+        [1]
+    }
+
+    let pa = helper().as_ptr();
+}
+
+fn bad_8() {
+    let pc = Cell::new("oops ub").as_ptr();
+}
+
+fn bad_9() {
+    let prc = RefCell::new("oops more ub").as_ptr();
+}
+
+fn bad_10() {
+    // Our lint and `temporary_cstring_as_ptr` both catch this. Maybe we can deprecate that one?
+    let pcstr = unsafe { CString::new(vec![]).unwrap().as_ptr() };
+}
+
+fn bad_11() {
+    let pab = unsafe { AtomicBool::new(true).as_ptr() };
+}
+
+// TODO: We need more tests here...
+
 fn fine_1() -> *const i32 {
     &100 as *const i32
 }
@@ -49,6 +92,27 @@ fn fine_3() -> *const i32 {
     &(*A) as *const i32
 }
 
+fn fine_4() {
+    let pa = ([1],).0.as_ptr();
+}
+
+fn fine_5() {
+    fn helper() -> &'static str {
+        "i'm not ub"
+    }
+
+    let ps = helper().as_ptr();
+}
+
+fn fine_6() {
+    fn helper() -> &'static [i32; 1] {
+        &[1]
+    }
+
+    let pa = helper().as_ptr();
+    unsafe { *pa };
+}
+
 external! {
     fn fine_external() -> *const i32 {
         let a = 0i32;

tests/ui/ptr_to_temporary.stderr

@@ -1,43 +1,91 @@
+error: lint `clippy::temporary_cstring_as_ptr` has been renamed to `temporary_cstring_as_ptr`
+  --> $DIR/ptr_to_temporary.rs:6:5
+   |
+LL |     clippy::temporary_cstring_as_ptr,
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `temporary_cstring_as_ptr`
+   |
+   = note: `-D renamed-and-removed-lints` implied by `-D warnings`
+
 error: returning a raw pointer to a temporary value that cannot be promoted to a constant
-  --> $DIR/ptr_to_temporary.rs:10:5
+  --> $DIR/ptr_to_temporary.rs:22:5
    |
 LL |     &(100 + *&0) as *const i32
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = note: usage of this pointer by callers will cause Undefined Behavior
+   = note: usage of this pointer by callers 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
    = note: `-D clippy::ptr-to-temporary` implied by `-D warnings`
 
 error: returning a raw pointer to a temporary value that cannot be promoted to a constant
-  --> $DIR/ptr_to_temporary.rs:15:5
+  --> $DIR/ptr_to_temporary.rs:27:5
    |
 LL |     &(*&a) as *const i32
    |     ^^^^^^^^^^^^^^^^^^^^
    |
-   = note: usage of this pointer by callers will cause Undefined Behavior
+   = note: usage of this pointer by callers 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
 
 error: returning a raw pointer to a temporary value that cannot be promoted to a constant
-  --> $DIR/ptr_to_temporary.rs:20:5
+  --> $DIR/ptr_to_temporary.rs:32:5
    |
 LL |     &a as *const i32
    |     ^^^^^^^^^^^^^^^^
    |
-   = note: usage of this pointer by callers will cause Undefined Behavior
+   = note: usage of this pointer by callers 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
 
 error: returning a raw pointer to a temporary value that cannot be promoted to a constant
-  --> $DIR/ptr_to_temporary.rs:25:5
+  --> $DIR/ptr_to_temporary.rs:37:5
    |
 LL |     &a as *const i32
    |     ^^^^^^^^^^^^^^^^
    |
-   = note: usage of this pointer by callers will cause Undefined Behavior
+   = note: usage of this pointer by callers 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
 
 error: returning a raw pointer to a temporary value that cannot be promoted to a constant
-  --> $DIR/ptr_to_temporary.rs:35:5
+  --> $DIR/ptr_to_temporary.rs:47:5
    |
 LL |     &a as *const i32
    |     ^^^^^^^^^^^^^^^^
    |
-   = note: usage of this pointer by callers will cause Undefined Behavior
+   = note: usage of this pointer by callers 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
+
+error: calling `as_ptr` on a temporary value
+  --> $DIR/ptr_to_temporary.rs:51:14
+   |
+LL |     let pv = vec![1].as_ptr();
+   |              ^^^^^^^^^^^^^^^^
+   |
+   = 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
+
+error: calling `as_ptr` on a temporary value
+  --> $DIR/ptr_to_temporary.rs:63:14
+   |
+LL |     let pc = Cell::new("oops ub").as_ptr();
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = 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
+
+error: calling `as_ptr` on a temporary value
+  --> $DIR/ptr_to_temporary.rs:67:15
+   |
+LL |     let prc = RefCell::new("oops more ub").as_ptr();
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = 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
+
+error: calling `as_ptr` on a temporary value
+  --> $DIR/ptr_to_temporary.rs:71:26
+   |
+LL |     let pcstr = unsafe { CString::new(vec![]).unwrap().as_ptr() };
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = 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
+
+error: calling `as_ptr` on a temporary value
+  --> $DIR/ptr_to_temporary.rs:75:24
+   |
+LL |     let pab = unsafe { AtomicBool::new(true).as_ptr() };
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = 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
 
-error: aborting due to 5 previous errors
+error: aborting due to 11 previous errors