Auto merge of #75671 - nathanwhit:cstring-temp-lint, r=oli-obk

Uplift `temporary-cstring-as-ptr` lint from `clippy` into rustc

The general consensus seems to be that this lint covers a common enough mistake to warrant inclusion in rustc.
The diagnostic message might need some tweaking, as I'm not sure the use of second-person perspective matches the rest of rustc, but I'd like to hear others' thoughts on that.

(cc #53224).

r? `@oli-obk`

Author: bors

compiler/rustc_lint/src/lib.rs

@@ -49,6 +49,7 @@ mod early;
 mod internal;
 mod late;
 mod levels;
+mod methods;
 mod non_ascii_idents;
 mod nonstandard_style;
 mod passes;
@@ -73,6 +74,7 @@ use rustc_span::Span;
 use array_into_iter::ArrayIntoIter;
 use builtin::*;
 use internal::*;
+use methods::*;
 use non_ascii_idents::*;
 use nonstandard_style::*;
 use redundant_semicolon::*;
@@ -160,6 +162,7 @@ macro_rules! late_lint_passes {
                 ArrayIntoIter: ArrayIntoIter,
                 ClashingExternDeclarations: ClashingExternDeclarations::new(),
                 DropTraitConstraints: DropTraitConstraints,
+                TemporaryCStringAsPtr: TemporaryCStringAsPtr,
             ]
         );
     };

compiler/rustc_lint/src/methods.rs

@@ -0,0 +1,106 @@
+use crate::LateContext;
+use crate::LateLintPass;
+use crate::LintContext;
+use rustc_hir::{Expr, ExprKind, PathSegment};
+use rustc_middle::ty;
+use rustc_span::{symbol::sym, ExpnKind, Span};
+
+declare_lint! {
+    /// The `temporary_cstring_as_ptr` lint detects getting the inner pointer of
+    /// a temporary `CString`.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// # #![allow(unused)]
+    /// # use std::ffi::CString;
+    /// let c_str = CString::new("foo").unwrap().as_ptr();
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// The inner pointer of a `CString` lives only as long as the `CString` it
+    /// points to. Getting the inner pointer of a *temporary* `CString` allows the `CString`
+    /// to be dropped at the end of the statement, as it is not being referenced as far as the typesystem
+    /// is concerned. This means outside of the statement the pointer will point to freed memory, which
+    /// causes undefined behavior if the pointer is later dereferenced.
+    pub TEMPORARY_CSTRING_AS_PTR,
+    Warn,
+    "detects getting the inner pointer of a temporary `CString`"
+}
+
+declare_lint_pass!(TemporaryCStringAsPtr => [TEMPORARY_CSTRING_AS_PTR]);
+
+fn in_macro(span: Span) -> bool {
+    if span.from_expansion() {
+        !matches!(span.ctxt().outer_expn_data().kind, ExpnKind::Desugaring(..))
+    } else {
+        false
+    }
+}
+
+fn first_method_call<'tcx>(
+    expr: &'tcx Expr<'tcx>,
+) -> Option<(&'tcx PathSegment<'tcx>, &'tcx [Expr<'tcx>])> {
+    if let ExprKind::MethodCall(path, _, args, _) = &expr.kind {
+        if args.iter().any(|e| e.span.from_expansion()) { None } else { Some((path, *args)) }
+    } else {
+        None
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for TemporaryCStringAsPtr {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if in_macro(expr.span) {
+            return;
+        }
+
+        match first_method_call(expr) {
+            Some((path, args)) if path.ident.name == sym::as_ptr => {
+                let unwrap_arg = &args[0];
+                let as_ptr_span = path.ident.span;
+                match first_method_call(unwrap_arg) {
+                    Some((path, args))
+                        if path.ident.name == sym::unwrap || path.ident.name == sym::expect =>
+                    {
+                        let source_arg = &args[0];
+                        lint_cstring_as_ptr(cx, as_ptr_span, source_arg, unwrap_arg);
+                    }
+                    _ => return,
+                }
+            }
+            _ => return,
+        }
+    }
+}
+
+fn lint_cstring_as_ptr(
+    cx: &LateContext<'_>,
+    as_ptr_span: Span,
+    source: &rustc_hir::Expr<'_>,
+    unwrap: &rustc_hir::Expr<'_>,
+) {
+    let source_type = cx.typeck_results().expr_ty(source);
+    if let ty::Adt(def, substs) = source_type.kind() {
+        if cx.tcx.is_diagnostic_item(sym::result_type, def.did) {
+            if let ty::Adt(adt, _) = substs.type_at(0).kind() {
+                if cx.tcx.is_diagnostic_item(sym::cstring_type, adt.did) {
+                    cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, as_ptr_span, |diag| {
+                        let mut diag = diag
+                            .build("getting the inner pointer of a temporary `CString`");
+                        diag.span_label(as_ptr_span, "this pointer will be invalid");
+                        diag.span_label(
+                            unwrap.span,
+                            "this `CString` is deallocated at the end of the statement, bind it to a variable to extend its lifetime",
+                        );
+                        diag.note("pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned");
+                        diag.help("for more information, see https://doc.rust-lang.org/reference/destructors.html");
+                        diag.emit();
+                    });
+                }
+            }
+        }
+    }
+}

compiler/rustc_span/src/symbol.rs

@@ -127,6 +127,7 @@ symbols! {
         ArgumentV1,
         Arguments,
         C,
+        CString,
         Center,
         Clone,
         Copy,
@@ -261,6 +262,7 @@ symbols! {
         arm_target_feature,
         array,
         arrays,
+        as_ptr,
         as_str,
         asm,
         assert,
@@ -310,6 +312,7 @@ symbols! {
         breakpoint,
         bridge,
         bswap,
+        c_str,
         c_variadic,
         call,
         call_mut,
@@ -397,6 +400,7 @@ symbols! {
         crate_type,
         crate_visibility_modifier,
         crt_dash_static: "crt-static",
+        cstring_type,
         ctlz,
         ctlz_nonzero,
         ctpop,
@@ -478,6 +482,7 @@ symbols! {
         existential_type,
         exp2f32,
         exp2f64,
+        expect,
         expected,
         expf32,
         expf64,
@@ -501,6 +506,7 @@ symbols! {
         fadd_fast,
         fdiv_fast,
         feature,
+        ffi,
         ffi_const,
         ffi_pure,
         ffi_returns_twice,
@@ -1170,6 +1176,7 @@ symbols! {
         unused_qualifications,
         unwind,
         unwind_attributes,
+        unwrap,
         unwrap_or,
         use_extern_macros,
         use_nested_groups,

library/std/src/ffi/c_str.rs

@@ -110,6 +110,7 @@ use crate::sys;
 /// of `CString` instances can lead to invalid memory accesses, memory leaks,
 /// and other memory errors.
 #[derive(PartialEq, PartialOrd, Eq, Ord, Hash, Clone)]
+#[cfg_attr(not(test), rustc_diagnostic_item = "cstring_type")]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub struct CString {
     // Invariant 1: the slice ends with a zero byte and has a length of at least one.
@@ -1265,7 +1266,7 @@ impl CStr {
     /// behavior when `ptr` is used inside the `unsafe` block:
     ///
     /// ```no_run
-    /// # #![allow(unused_must_use)]
+    /// # #![allow(unused_must_use)] #![cfg_attr(not(bootstrap), allow(temporary_cstring_as_ptr))]
     /// use std::ffi::CString;
     ///
     /// let ptr = CString::new("Hello").expect("CString::new failed").as_ptr();

src/test/ui/lint/lint-temporary-cstring-as-param.rs

@@ -0,0 +1,11 @@
+#![deny(temporary_cstring_as_ptr)]
+
+use std::ffi::CString;
+use std::os::raw::c_char;
+
+fn some_function(data: *const c_char) {}
+
+fn main() {
+    some_function(CString::new("").unwrap().as_ptr());
+    //~^ ERROR getting the inner pointer of a temporary `CString`
+}

src/test/ui/lint/lint-temporary-cstring-as-param.stderr

@@ -0,0 +1,18 @@
+error: getting the inner pointer of a temporary `CString`
+  --> $DIR/lint-temporary-cstring-as-param.rs:9:45
+   |
+LL |     some_function(CString::new("").unwrap().as_ptr());
+   |                   ------------------------- ^^^^^^ this pointer will be invalid
+   |                   |
+   |                   this `CString` is deallocated at the end of the statement, bind it to a variable to extend its lifetime
+   |
+note: the lint level is defined here
+  --> $DIR/lint-temporary-cstring-as-param.rs:1:9
+   |
+LL | #![deny(temporary_cstring_as_ptr)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned
+   = help: for more information, see https://doc.rust-lang.org/reference/destructors.html
+
+error: aborting due to previous error
+

src/test/ui/lint/lint-temporary-cstring-as-ptr.rs

@@ -0,0 +1,9 @@
+// this program is not technically incorrect, but is an obscure enough style to be worth linting
+#![deny(temporary_cstring_as_ptr)]
+
+use std::ffi::CString;
+
+fn main() {
+    let s = CString::new("some text").unwrap().as_ptr();
+    //~^ ERROR getting the inner pointer of a temporary `CString`
+}

src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr

@@ -0,0 +1,18 @@
+error: getting the inner pointer of a temporary `CString`
+  --> $DIR/lint-temporary-cstring-as-ptr.rs:7:48
+   |
+LL |     let s = CString::new("some text").unwrap().as_ptr();
+   |             ---------------------------------- ^^^^^^ this pointer will be invalid
+   |             |
+   |             this `CString` is deallocated at the end of the statement, bind it to a variable to extend its lifetime
+   |
+note: the lint level is defined here
+  --> $DIR/lint-temporary-cstring-as-ptr.rs:2:9
+   |
+LL | #![deny(temporary_cstring_as_ptr)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned
+   = help: for more information, see https://doc.rust-lang.org/reference/destructors.html
+
+error: aborting due to previous error
+

src/tools/clippy/.github/driver.sh

@@ -22,9 +22,9 @@ unset CARGO_MANIFEST_DIR
 
 # Run a lint and make sure it produces the expected output. It's also expected to exit with code 1
 # FIXME: How to match the clippy invocation in compile-test.rs?
-./target/debug/clippy-driver -Dwarnings -Aunused -Zui-testing --emit metadata --crate-type bin tests/ui/cstring.rs 2> cstring.stderr && exit 1
-sed -e "s,tests/ui,\$DIR," -e "/= help/d" cstring.stderr > normalized.stderr
-diff normalized.stderr tests/ui/cstring.stderr
+./target/debug/clippy-driver -Dwarnings -Aunused -Zui-testing --emit metadata --crate-type bin tests/ui/cast.rs 2> cast.stderr && exit 1
+sed -e "s,tests/ui,\$DIR," -e "/= help/d" cast.stderr > normalized.stderr
+diff normalized.stderr tests/ui/cast.stderr
 
 
 # make sure "clippy-driver --rustc --arg" and "rustc --arg" behave the same

src/tools/clippy/clippy_lints/src/lib.rs

@@ -707,7 +707,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::SKIP_WHILE_NEXT,
         &methods::STRING_EXTEND_CHARS,
         &methods::SUSPICIOUS_MAP,
-        &methods::TEMPORARY_CSTRING_AS_PTR,
         &methods::UNINIT_ASSUMED_INIT,
         &methods::UNNECESSARY_FILTER_MAP,
         &methods::UNNECESSARY_FOLD,
@@ -1417,7 +1416,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::SKIP_WHILE_NEXT),
         LintId::of(&methods::STRING_EXTEND_CHARS),
         LintId::of(&methods::SUSPICIOUS_MAP),
-        LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR),
         LintId::of(&methods::UNINIT_ASSUMED_INIT),
         LintId::of(&methods::UNNECESSARY_FILTER_MAP),
         LintId::of(&methods::UNNECESSARY_FOLD),
@@ -1765,7 +1763,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT),
         LintId::of(&methods::CLONE_DOUBLE_REF),
         LintId::of(&methods::ITERATOR_STEP_BY_ZERO),
-        LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR),
         LintId::of(&methods::UNINIT_ASSUMED_INIT),
         LintId::of(&methods::ZST_OFFSET),
         LintId::of(&minmax::MIN_MAX),

src/tools/clippy/clippy_lints/src/methods/mod.rs

@@ -798,40 +798,6 @@ declare_clippy_lint! {
     "using a single-character str where a char could be used, e.g., `_.split(\"x\")`"
 }
 
-declare_clippy_lint! {
-    /// **What it does:** Checks for getting the inner pointer of a temporary
-    /// `CString`.
-    ///
-    /// **Why is this bad?** The inner pointer of a `CString` is only valid as long
-    /// as the `CString` is alive.
-    ///
-    /// **Known problems:** None.
-    ///
-    /// **Example:**
-    /// ```rust
-    /// # use std::ffi::CString;
-    /// # fn call_some_ffi_func(_: *const i8) {}
-    /// #
-    /// let c_str = CString::new("foo").unwrap().as_ptr();
-    /// unsafe {
-    ///     call_some_ffi_func(c_str);
-    /// }
-    /// ```
-    /// Here `c_str` points to a freed address. The correct use would be:
-    /// ```rust
-    /// # use std::ffi::CString;
-    /// # fn call_some_ffi_func(_: *const i8) {}
-    /// #
-    /// let c_str = CString::new("foo").unwrap();
-    /// unsafe {
-    ///     call_some_ffi_func(c_str.as_ptr());
-    /// }
-    /// ```
-    pub TEMPORARY_CSTRING_AS_PTR,
-    correctness,
-    "getting the inner pointer of a temporary `CString`"
-}
-
 declare_clippy_lint! {
     /// **What it does:** Checks for calling `.step_by(0)` on iterators which panics.
     ///
@@ -1406,7 +1372,6 @@ declare_lint_pass!(Methods => [
     SINGLE_CHAR_PATTERN,
     SINGLE_CHAR_PUSH_STR,
     SEARCH_IS_SOME,
-    TEMPORARY_CSTRING_AS_PTR,
     FILTER_NEXT,
     SKIP_WHILE_NEXT,
     FILTER_MAP,
@@ -1490,7 +1455,6 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                 lint_search_is_some(cx, expr, "rposition", arg_lists[1], arg_lists[0], method_spans[1])
             },
             ["extend", ..] => lint_extend(cx, expr, arg_lists[0]),
-            ["as_ptr", "unwrap" | "expect"] => lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]),
             ["nth", "iter"] => lint_iter_nth(cx, expr, &arg_lists, false),
             ["nth", "iter_mut"] => lint_iter_nth(cx, expr, &arg_lists, true),
             ["nth", ..] => lint_iter_nth_zero(cx, expr, arg_lists[0]),
@@ -2207,26 +2171,6 @@ fn lint_extend(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>
     }
 }
 
-fn lint_cstring_as_ptr(cx: &LateContext<'_>, expr: &hir::Expr<'_>, source: &hir::Expr<'_>, unwrap: &hir::Expr<'_>) {
-    if_chain! {
-        let source_type = cx.typeck_results().expr_ty(source);
-        if let ty::Adt(def, substs) = source_type.kind();
-        if cx.tcx.is_diagnostic_item(sym!(result_type), def.did);
-        if match_type(cx, substs.type_at(0), &paths::CSTRING);
-        then {
-            span_lint_and_then(
-                cx,
-                TEMPORARY_CSTRING_AS_PTR,
-                expr.span,
-                "you are getting the inner pointer of a temporary `CString`",
-                |diag| {
-                    diag.note("that pointer will be invalid outside this expression");
-                    diag.span_help(unwrap.span, "assign the `CString` to a variable to extend its lifetime");
-                });
-        }
-    }
-}
-
 fn lint_iter_cloned_collect<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, iter_args: &'tcx [hir::Expr<'_>]) {
     if_chain! {
         if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym!(vec_type));

src/tools/clippy/clippy_lints/src/utils/paths.rs

@@ -21,7 +21,6 @@ pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
 pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"];
 pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"];
 pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"];
-pub const CSTRING: [&str; 4] = ["std", "ffi", "c_str", "CString"];
 pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"];
 pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"];
 pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];