Skip to content

Commit

Permalink
Auto merge of #11919 - y21:manual_c_str_literals, r=xFrednet
Browse files Browse the repository at this point in the history
new lint: `manual_c_str_literals`

With rust-lang/rust#117472 merged and `c""` syntax stabilized, I think it'd be nice to have a lint for using `CStr::from_ptr` (and similar constructors) with a string literal as an argument.
We can probably also lint `"foo\0".as_ptr()` and suggest `c"foo".as_ptr()`. I might add that to this PR tomorrow if I find the time.

The byte string literal to c string literal rewriting is ugly but oh well.

changelog: new lint: `manual_c_str_literals`
[#11919](#11919)
  • Loading branch information
bors committed Feb 5, 2024
2 parents 8baeb26 + 7f80b44 commit 5bae9d4
Show file tree
Hide file tree
Showing 12 changed files with 444 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5340,6 +5340,7 @@ Released 2018-09-13
[`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
[`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits
[`manual_c_str_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_c_str_literals
[`manual_clamp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp
[`manual_filter`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter
[`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
Expand Down
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
2 changes: 1 addition & 1 deletion clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,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 = ""]
Expand Down
1 change: 1 addition & 0 deletions clippy_config/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ macro_rules! msrv_aliases {

// names may refer to stabilized feature flags or library items
msrv_aliases! {
1,77,0 { C_STR_LITERALS }
1,76,0 { PTR_FROM_REF }
1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE }
1,70,0 { OPTION_RESULT_IS_VARIANT_AND, BINARY_HEAP_RETAIN }
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::methods::ITER_SKIP_ZERO_INFO,
crate::methods::ITER_WITH_DRAIN_INFO,
crate::methods::JOIN_ABSOLUTE_PATHS_INFO,
crate::methods::MANUAL_C_STR_LITERALS_INFO,
crate::methods::MANUAL_FILTER_MAP_INFO,
crate::methods::MANUAL_FIND_MAP_INFO,
crate::methods::MANUAL_IS_VARIANT_AND_INFO,
Expand Down
197 changes: 197 additions & 0 deletions clippy_lints/src/methods/manual_c_str_literals.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::span_lint_and_sugg;
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, Node, QPath, TyKind};
use rustc_lint::LateContext;
use rustc_span::{sym, Span, Symbol};

use super::MANUAL_C_STR_LITERALS;

/// 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>,
msrv: &Msrv,
) {
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)
&& msrv.meets(msrvs::C_STR_LITERALS)
{
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::HasPlaceholders,
);
}
}

/// Checks if the callee is a "relevant" `CStr` function considered by this lint.
/// Returns the function 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 calls to the `CStr` constructor functions:
/// - `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.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) | LitKind::Str(_, StrStyle::Cooked) = lit.node =>
{
check_from_bytes(cx, expr, arg, name);
},
"from_ptr" => check_from_ptr(cx, expr, arg),
_ => {},
}
}
}

/// Checks `CStr::from_ptr(b"foo\0".as_ptr().cast())`
fn check_from_ptr(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>) {
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"#,
sugg,
Applicability::MachineApplicable,
);
}
}
/// Checks `CStr::from_bytes_with_nul(b"foo\0")`
fn check_from_bytes(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, method: &str) {
let (span, applicability) = if let Some(parent) = get_parent_expr(cx, expr)
&& let ExprKind::MethodCall(method, ..) = parent.kind
&& [sym::unwrap, sym::expect].contains(&method.ident.name)
{
(parent.span, Applicability::MachineApplicable)
} else if method == "from_bytes_with_nul_unchecked" {
// `*_unchecked` returns `&CStr` directly, nothing needs to be changed
(expr.span, Applicability::MachineApplicable)
} else {
// User needs to remove error handling, can't be machine applicable
(expr.span, Applicability::HasPlaceholders)
};

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"#,
sugg,
applicability,
);
}

/// Rewrites a byte string literal to a c-str literal.
/// `b"foo\0"` -> `c"foo"`
///
/// 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.
if let Some(quote_pos) = sugg.rfind('"') {
// Possible values right before the quote:
// - literal NUL value
if sugg.as_bytes()[quote_pos - 1] == b'\0' {
sugg.remove(quote_pos - 1);
}
// - \x00
else if sugg[..quote_pos].ends_with("\\x00") {
sugg.replace_range(quote_pos - 4..quote_pos, "");
}
// - \0
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;
}
}

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> {
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
}
37 changes: 37 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ mod iter_skip_zero;
mod iter_with_drain;
mod iterator_step_by_zero;
mod join_absolute_paths;
mod manual_c_str_literals;
mod manual_is_variant_and;
mod manual_next_back;
mod manual_ok_or;
Expand Down Expand Up @@ -3977,6 +3978,39 @@ declare_clippy_lint! {
"making no use of the \"map closure\" when calling `.map_or_else(|err| handle_error(err), |n| n)`"
}

declare_clippy_lint! {
/// 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 with a hardcoded `\0` byte at the end.
///
/// ### 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 and the terminating `NUL` byte is inserted automatically.
///
/// ### Example
/// ```no_run
/// # use std::ffi::CStr;
/// # 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
/// # use std::ffi::CStr;
/// # 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,
pedantic,
r#"creating a `CStr` through functions when `c""` literals can be used"#
}

pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
Expand Down Expand Up @@ -4136,6 +4170,7 @@ impl_lint_pass!(Methods => [
STR_SPLIT_AT_NEWLINE,
OPTION_AS_REF_CLONED,
UNNECESSARY_RESULT_MAP_OR_ELSE,
MANUAL_C_STR_LITERALS,
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -4163,6 +4198,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
hir::ExprKind::Call(func, args) => {
from_iter_instead_of_collect::check(cx, expr, args, func);
unnecessary_fallible_conversions::check_function(cx, expr, func);
manual_c_str_literals::check(cx, expr, func, args, &self.msrv);
},
hir::ExprKind::MethodCall(method_call, receiver, args, _) => {
let method_span = method_call.ident.span;
Expand Down Expand Up @@ -4381,6 +4417,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, &self.msrv),
("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv),
("assume_init", []) => uninit_assumed_init::check(cx, expr, recv),
("cloned", []) => {
Expand Down
60 changes: 60 additions & 0 deletions tests/ui/manual_c_str_literals.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#![warn(clippy::manual_c_str_literals)]
#![allow(clippy::no_effect)]

use std::ffi::CStr;

macro_rules! cstr {
($s:literal) => {
CStr::from_bytes_with_nul(concat!($s, "\0").as_bytes()).unwrap()
};
}

macro_rules! macro_returns_c_str {
() => {
CStr::from_bytes_with_nul(b"foo\0").unwrap();
};
}

macro_rules! macro_returns_byte_string {
() => {
b"foo\0"
};
}

#[clippy::msrv = "1.76.0"]
fn pre_stabilization() {
CStr::from_bytes_with_nul(b"foo\0");
}

#[clippy::msrv = "1.77.0"]
fn post_stabilization() {
c"foo";
}

fn main() {
c"foo";
c"foo";
c"foo";
c"foo\\0sdsd";
CStr::from_bytes_with_nul(br"foo\\0sdsd\0").unwrap();
CStr::from_bytes_with_nul(br"foo\x00").unwrap();
CStr::from_bytes_with_nul(br##"foo#a\0"##).unwrap();

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>();
let _ = "电脑".as_ptr();
let _ = "电脑\\".as_ptr();
let _ = c"电脑\\".as_ptr();
let _ = c"电脑".as_ptr();
let _ = c"电脑".as_ptr();

// Macro cases, don't lint:
cstr!("foo");
macro_returns_c_str!();
CStr::from_bytes_with_nul(macro_returns_byte_string!()).unwrap();
}
Loading

0 comments on commit 5bae9d4

Please sign in to comment.