lint plain b"...".as_ptr() outside of CStr constructors

y21 · y21 · commit 6a0998d1bee6 · 2023-12-04T18:41:28.000+01:00
diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md
@@ -151,6 +151,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
 * [`manual_try_fold`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_try_fold)
 * [`manual_hash_one`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_hash_one)
 * [`iter_kv_map`](https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map)
+* [`manual_c_str_literals`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_c_str_literals)
 
 
 ## `cognitive-complexity-threshold`
diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs
@@ -249,7 +249,7 @@ define_Conf! {
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),
-    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS, MANUAL_TRY_FOLD, MANUAL_HASH_ONE, ITER_KV_MAP.
+    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS, MANUAL_TRY_FOLD, MANUAL_HASH_ONE, ITER_KV_MAP, MANUAL_C_STR_LITERALS.
     ///
     /// The minimum rust version that the project supports. Defaults to the `rust-version` field in `Cargo.toml`
     #[default_text = ""]
diff --git a/clippy_lints/src/methods/manual_c_str_literals.rs b/clippy_lints/src/methods/manual_c_str_literals.rs
@@ -4,24 +4,69 @@ use clippy_utils::get_parent_expr;
 use clippy_utils::source::snippet;
 use rustc_ast::{LitKind, StrStyle};
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, QPath, TyKind};
+use rustc_hir::{Expr, ExprKind, Node, QPath, TyKind};
 use rustc_lint::LateContext;
-use rustc_span::{sym, Span};
+use rustc_span::{sym, Span, Symbol};
 
 use super::MANUAL_C_STR_LITERALS;
 
-pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, func: &Expr<'_>, args: &[Expr<'_>], msrv: &Msrv) {
+/// Checks:
+/// - `b"...".as_ptr()`
+/// - `b"...".as_ptr().cast()`
+/// - `"...".as_ptr()`
+/// - `"...".as_ptr().cast()`
+///
+/// Iff the parent call of `.cast()` isn't `CStr::from_ptr`, to avoid linting twice.
+pub(super) fn check_as_ptr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, receiver: &'tcx Expr<'tcx>) {
+    if let ExprKind::Lit(lit) = receiver.kind
+        && let LitKind::ByteStr(_, StrStyle::Cooked) | LitKind::Str(_, StrStyle::Cooked) = lit.node
+        && let casts_removed = peel_ptr_cast_ancestors(cx, expr)
+        && !get_parent_expr(cx, casts_removed).is_some_and(
+            |parent| matches!(parent.kind, ExprKind::Call(func, _) if is_c_str_function(cx, func).is_some()),
+        )
+        && let Some(sugg) = rewrite_as_cstr(cx, lit.span)
+    {
+        span_lint_and_sugg(
+            cx,
+            MANUAL_C_STR_LITERALS,
+            receiver.span,
+            "manually constructing a nul-terminated string",
+            r#"use a `c""` literal"#,
+            sugg,
+            // an additional cast may be needed, since the type of `CStr::as_ptr` and
+            // `"".as_ptr()` can differ and is platform dependent
+            Applicability::MaybeIncorrect,
+        );
+    }
+}
+
+/// Checks if the callee is a "relevant" `CStr` function considered by this lint.
+/// Returns the method name.
+fn is_c_str_function(cx: &LateContext<'_>, func: &Expr<'_>) -> Option<Symbol> {
     if let ExprKind::Path(QPath::TypeRelative(cstr, fn_name)) = &func.kind
         && let TyKind::Path(QPath::Resolved(_, ty_path)) = &cstr.kind
         && cx.tcx.lang_items().c_str() == ty_path.res.opt_def_id()
+    {
+        Some(fn_name.ident.name)
+    } else {
+        None
+    }
+}
+
+/// Checks:
+/// - `CStr::from_bytes_with_nul(..)`
+/// - `CStr::from_bytes_with_nul_unchecked(..)`
+/// - `CStr::from_ptr(..)`
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, func: &Expr<'_>, args: &[Expr<'_>], msrv: &Msrv) {
+    if let Some(fn_name) = is_c_str_function(cx, func)
         && let [arg] = args
         && msrv.meets(msrvs::C_STR_LITERALS)
     {
-        match fn_name.ident.name.as_str() {
+        match fn_name.as_str() {
             name @ ("from_bytes_with_nul" | "from_bytes_with_nul_unchecked")
                 if !arg.span.from_expansion()
                     && let ExprKind::Lit(lit) = arg.kind
-                    && let LitKind::ByteStr(_, StrStyle::Cooked) = lit.node =>
+                    && let LitKind::ByteStr(_, StrStyle::Cooked) | LitKind::Str(_, StrStyle::Cooked) = lit.node =>
             {
                 check_from_bytes(cx, expr, arg, name);
             },
@@ -33,19 +78,20 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, func: &Expr<'_>, args
 
 /// Checks `CStr::from_bytes_with_nul(b"foo\0")`
 fn check_from_ptr(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>) {
-    if let ExprKind::MethodCall(method, lit, [], _) = peel_ptr_cast(arg).kind
+    if let ExprKind::MethodCall(method, lit, ..) = peel_ptr_cast(arg).kind
         && method.ident.name == sym::as_ptr
         && !lit.span.from_expansion()
         && let ExprKind::Lit(lit) = lit.kind
         && let LitKind::ByteStr(_, StrStyle::Cooked) = lit.node
+        && let Some(sugg) = rewrite_as_cstr(cx, lit.span)
     {
         span_lint_and_sugg(
             cx,
             MANUAL_C_STR_LITERALS,
             expr.span,
             "calling `CStr::from_ptr` with a byte string literal",
             r#"use a `c""` literal"#,
-            rewrite_as_cstr(cx, lit.span),
+            sugg,
             Applicability::MachineApplicable,
         );
     }
@@ -66,24 +112,29 @@ fn check_from_bytes(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, metho
         (expr.span, Applicability::MaybeIncorrect)
     };
 
+    let Some(sugg) = rewrite_as_cstr(cx, arg.span) else {
+        return;
+    };
+
     span_lint_and_sugg(
         cx,
         MANUAL_C_STR_LITERALS,
         span,
         "calling `CStr::new` with a byte string literal",
         r#"use a `c""` literal"#,
-        rewrite_as_cstr(cx, arg.span),
+        sugg,
         applicability,
     );
 }
 
 /// Rewrites a byte string literal to a c-str literal.
 /// `b"foo\0"` -> `c"foo"`
-pub fn rewrite_as_cstr(cx: &LateContext<'_>, span: Span) -> String {
+///
+/// Returns `None` if it doesn't end in a NUL byte.
+fn rewrite_as_cstr(cx: &LateContext<'_>, span: Span) -> Option<String> {
     let mut sugg = String::from("c") + snippet(cx, span.source_callsite(), "..").trim_start_matches('b');
 
     // NUL byte should always be right before the closing quote.
-    // (Can rfind ever return `None`?)
     if let Some(quote_pos) = sugg.rfind('"') {
         // Possible values right before the quote:
         // - literal NUL value
@@ -98,17 +149,44 @@ pub fn rewrite_as_cstr(cx: &LateContext<'_>, span: Span) -> String {
         else if sugg[..quote_pos].ends_with("\\0") {
             sugg.replace_range(quote_pos - 2..quote_pos, "");
         }
+        // No known suffix, so assume it's not a C-string.
+        else {
+            return None;
+        }
     }
 
-    sugg
+    Some(sugg)
+}
+
+fn get_cast_target<'tcx>(e: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
+    match &e.kind {
+        ExprKind::MethodCall(method, receiver, [], _) if method.ident.as_str() == "cast" => Some(receiver),
+        ExprKind::Cast(expr, _) => Some(expr),
+        _ => None,
+    }
 }
 
 /// `x.cast()` -> `x`
 /// `x as *const _` -> `x`
+/// `x` -> `x` (returns the same expression for non-cast exprs)
 fn peel_ptr_cast<'tcx>(e: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
-    match &e.kind {
-        ExprKind::MethodCall(method, receiver, [], _) if method.ident.as_str() == "cast" => peel_ptr_cast(receiver),
-        ExprKind::Cast(expr, _) => peel_ptr_cast(expr),
-        _ => e,
+    get_cast_target(e).map_or(e, peel_ptr_cast)
+}
+
+/// Same as `peel_ptr_cast`, but the other way around, by walking up the ancestor cast expressions:
+///
+/// `foo(x.cast() as *const _)`
+///      ^ given this `x` expression, returns the `foo(...)` expression
+fn peel_ptr_cast_ancestors<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
+    let mut prev = e;
+    for (_, node) in cx.tcx.hir().parent_iter(e.hir_id) {
+        if let Node::Expr(e) = node
+            && get_cast_target(e).is_some()
+        {
+            prev = e;
+        } else {
+            break;
+        }
     }
+    prev
 }
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -3755,24 +3755,32 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for calls to `CStr::from_ptr` and `CStr::from_bytes_with_nul` with byte string literals as arguments.
+    /// Checks for the manual creation of C strings (a string with a `NUL` byte at the end), either
+    /// through one of the `CStr` constructor functions, or more plainly by calling `.as_ptr()`
+    /// on a (byte) string literal.
     ///
     /// ### Why is this bad?
     /// This can be written more concisely using `c"str"` literals and is also less error-prone,
-    /// because the compiler checks for interior nul bytes.
+    /// because the compiler checks for interior `NUL` bytes and the terminating `NUL` byte is inserted automatically.
     ///
     /// ### Example
     /// ```no_run
     /// # use std::ffi::CStr;
-    /// # fn needs_cstr(_: &CStr) {}
-    /// needs_cstr(CStr::from_bytes_with_nul(b":)").unwrap());
+    /// # mod libc { pub unsafe fn puts(_: *const i8) {} }
+    /// fn needs_cstr(_: &CStr) {}
+    ///
+    /// needs_cstr(CStr::from_bytes_with_nul(b"Hello\0").unwrap());
+    /// unsafe { libc::puts("World\0".as_ptr().cast()) }
     /// ```
     /// Use instead:
     /// ```no_run
     /// # #![feature(c_str_literals)]
     /// # use std::ffi::CStr;
-    /// # fn needs_cstr(_: &CStr) {}
-    /// needs_cstr(c":)");
+    /// # mod libc { pub unsafe fn puts(_: *const i8) {} }
+    /// fn needs_cstr(_: &CStr) {}
+    ///
+    /// needs_cstr(c"Hello");
+    /// unsafe { libc::puts(c"World".as_ptr()) }
     /// ```
     #[clippy::version = "1.76.0"]
     pub MANUAL_C_STR_LITERALS,
@@ -4178,6 +4186,7 @@ impl Methods {
                     }
                 },
                 ("as_mut", []) => useless_asref::check(cx, expr, "as_mut", recv),
+                ("as_ptr", []) => manual_c_str_literals::check_as_ptr(cx, expr, recv),
                 ("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv),
                 ("assume_init", []) => uninit_assumed_init::check(cx, expr, recv),
                 ("cloned", []) => cloned_instead_of_copied::check(cx, expr, recv, span, &self.msrv),
diff --git a/tests/ui/manual_c_str_literals.fixed b/tests/ui/manual_c_str_literals.fixed
@@ -43,6 +43,11 @@ fn main() {
 
     unsafe { c"foo" };
     unsafe { c"foo" };
+    let _: *const _ = c"foo".as_ptr();
+    let _: *const _ = c"foo".as_ptr();
+    let _: *const _ = "foo".as_ptr(); // not a C-string
+    let _: *const _ = "".as_ptr();
+    let _: *const _ = c"foo".as_ptr().cast::<i8>();
 
     // Macro cases, don't lint:
     cstr!("foo");
diff --git a/tests/ui/manual_c_str_literals.rs b/tests/ui/manual_c_str_literals.rs
@@ -43,6 +43,11 @@ fn main() {
 
     unsafe { CStr::from_ptr(b"foo\0".as_ptr().cast()) };
     unsafe { CStr::from_ptr(b"foo\0".as_ptr() as *const _) };
+    let _: *const _ = b"foo\0".as_ptr();
+    let _: *const _ = "foo\0".as_ptr();
+    let _: *const _ = "foo".as_ptr(); // not a C-string
+    let _: *const _ = "".as_ptr();
+    let _: *const _ = b"foo\0".as_ptr().cast::<i8>();
 
     // Macro cases, don't lint:
     cstr!("foo");
diff --git a/tests/ui/manual_c_str_literals.stderr b/tests/ui/manual_c_str_literals.stderr
@@ -43,5 +43,23 @@ error: calling `CStr::from_ptr` with a byte string literal
 LL |     unsafe { CStr::from_ptr(b"foo\0".as_ptr() as *const _) };
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a `c""` literal: `c"foo"`
 
-error: aborting due to 7 previous errors
+error: manually constructing a nul-terminated string
+  --> $DIR/manual_c_str_literals.rs:46:23
+   |
+LL |     let _: *const _ = b"foo\0".as_ptr();
+   |                       ^^^^^^^^ help: use a `c""` literal: `c"foo"`
+
+error: manually constructing a nul-terminated string
+  --> $DIR/manual_c_str_literals.rs:47:23
+   |
+LL |     let _: *const _ = "foo\0".as_ptr();
+   |                       ^^^^^^^ help: use a `c""` literal: `c"foo"`
+
+error: manually constructing a nul-terminated string
+  --> $DIR/manual_c_str_literals.rs:50:23
+   |
+LL |     let _: *const _ = b"foo\0".as_ptr().cast::<i8>();
+   |                       ^^^^^^^^ help: use a `c""` literal: `c"foo"`
+
+error: aborting due to 10 previous errors
 
