diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dc43f77f83..ad3722d4d0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2798,6 +2798,7 @@ Released 2018-09-13 [`string_from_utf8_as_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_from_utf8_as_bytes [`string_lit_as_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_lit_as_bytes [`string_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_to_string +[`strlen_on_c_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#strlen_on_c_strings [`struct_excessive_bools`]: https://rust-lang.github.io/rust-clippy/master/index.html#struct_excessive_bools [`suboptimal_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#suboptimal_flops [`suspicious_arithmetic_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_arithmetic_impl diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0a468ccf25a..82519a9b5b7 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -338,6 +338,7 @@ mod size_of_in_element_count; mod slow_vector_initialization; mod stable_sort_primitive; mod strings; +mod strlen_on_c_strings; mod suspicious_operation_groupings; mod suspicious_trait_impl; mod swap; @@ -914,6 +915,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: strings::STRING_LIT_AS_BYTES, strings::STRING_TO_STRING, strings::STR_TO_STRING, + strlen_on_c_strings::STRLEN_ON_C_STRINGS, suspicious_operation_groupings::SUSPICIOUS_OPERATION_GROUPINGS, suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL, suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL, @@ -1410,6 +1412,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), LintId::of(stable_sort_primitive::STABLE_SORT_PRIMITIVE), LintId::of(strings::STRING_FROM_UTF8_AS_BYTES), + LintId::of(strlen_on_c_strings::STRLEN_ON_C_STRINGS), LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), LintId::of(swap::ALMOST_SWAPPED), @@ -1639,6 +1642,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(reference::REF_IN_DEREF), LintId::of(repeat_once::REPEAT_ONCE), LintId::of(strings::STRING_FROM_UTF8_AS_BYTES), + LintId::of(strlen_on_c_strings::STRLEN_ON_C_STRINGS), LintId::of(swap::MANUAL_SWAP), LintId::of(temporary_assignment::TEMPORARY_ASSIGNMENT), LintId::of(transmute::CROSSPOINTER_TRANSMUTE), @@ -2096,6 +2100,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || box missing_enforced_import_rename::ImportRename::new(import_renames.clone())); let scripts = conf.allowed_scripts.clone(); store.register_early_pass(move || box disallowed_script_idents::DisallowedScriptIdents::new(&scripts)); + store.register_late_pass(|| box strlen_on_c_strings::StrlenOnCStrings); } #[rustfmt::skip] diff --git a/clippy_lints/src/strlen_on_c_strings.rs b/clippy_lints/src/strlen_on_c_strings.rs new file mode 100644 index 00000000000..2ccf3a3796d --- /dev/null +++ b/clippy_lints/src/strlen_on_c_strings.rs @@ -0,0 +1,82 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::in_macro; +use clippy_utils::paths; +use clippy_utils::source::snippet_with_macro_callsite; +use clippy_utils::ty::{is_type_diagnostic_item, is_type_ref_to_diagnostic_item}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::{sym, Symbol}; + +declare_clippy_lint! { + /// **What it does:** Checks for usage of `libc::strlen` on a `CString` or `CStr` value, + /// and suggest calling `as_bytes().len()` or `to_bytes().len()` respectively instead. + /// + /// **Why is this bad?** This avoids calling an unsafe `libc` function. + /// Currently, it also avoids calculating the length. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust, ignore + /// use std::ffi::CString; + /// let cstring = CString::new("foo").expect("CString::new failed"); + /// let len = unsafe { libc::strlen(cstring.as_ptr()) }; + /// ``` + /// Use instead: + /// ```rust, no_run + /// use std::ffi::CString; + /// let cstring = CString::new("foo").expect("CString::new failed"); + /// let len = cstring.as_bytes().len(); + /// ``` + pub STRLEN_ON_C_STRINGS, + complexity, + "using `libc::strlen` on a `CString` or `CStr` value, while `as_bytes().len()` or `to_bytes().len()` respectively can be used instead" +} + +declare_lint_pass!(StrlenOnCStrings => [STRLEN_ON_C_STRINGS]); + +impl LateLintPass<'tcx> for StrlenOnCStrings { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { + if in_macro(expr.span) { + return; + } + + if_chain! { + if let hir::ExprKind::Call(func, [recv]) = expr.kind; + if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = func.kind; + + if (&paths::LIBC_STRLEN).iter().map(|x| Symbol::intern(x)).eq( + path.segments.iter().map(|seg| seg.ident.name)); + if let hir::ExprKind::MethodCall(path, _, args, _) = recv.kind; + if args.len() == 1; + if !args.iter().any(|e| e.span.from_expansion()); + if path.ident.name == sym::as_ptr; + then { + let cstring = &args[0]; + let ty = cx.typeck_results().expr_ty(cstring); + let val_name = snippet_with_macro_callsite(cx, cstring.span, ".."); + let sugg = if is_type_diagnostic_item(cx, ty, sym::cstring_type){ + format!("{}.as_bytes().len()", val_name) + } else if is_type_ref_to_diagnostic_item(cx, ty, sym::CStr){ + format!("{}.to_bytes().len()", val_name) + } else { + return; + }; + + span_lint_and_sugg( + cx, + STRLEN_ON_C_STRINGS, + expr.span, + "using `libc::strlen` on a `CString` or `CStr` value", + "try this (you might also need to get rid of `unsafe` block in some cases):", + sugg, + Applicability::Unspecified // Sometimes unnecessary `unsafe` block + ); + } + } + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index b913d1ce8b1..9ebfbd6b423 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -82,6 +82,7 @@ pub const ITER_REPEAT: [&str; 5] = ["core", "iter", "sources", "repeat", "repeat pub const KW_MODULE: [&str; 3] = ["rustc_span", "symbol", "kw"]; #[cfg(feature = "internal-lints")] pub const LATE_CONTEXT: [&str; 2] = ["rustc_lint", "LateContext"]; +pub const LIBC_STRLEN: [&str; 2] = ["libc", "strlen"]; pub const LINKED_LIST: [&str; 4] = ["alloc", "collections", "linked_list", "LinkedList"]; #[cfg(any(feature = "internal-lints", feature = "metadata-collector-lint"))] pub const LINT: [&str; 2] = ["rustc_lint_defs", "Lint"]; diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index a92d3be5d3c..9e271b71204 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -231,6 +231,17 @@ pub fn is_recursively_primitive_type(ty: Ty<'_>) -> bool { } } +/// Checks if the type is a reference equals to a diagnostic item +pub fn is_type_ref_to_diagnostic_item(cx: &LateContext<'_>, ty: Ty<'_>, diag_item: Symbol) -> bool { + match ty.kind() { + ty::Ref(_, ref_ty, _) => match ref_ty.kind() { + ty::Adt(adt, _) => cx.tcx.is_diagnostic_item(diag_item, adt.did), + _ => false, + }, + _ => false, + } +} + /// Checks if the type is equal to a diagnostic item /// /// If you change the signature, remember to update the internal lint `MatchTypeOnDiagItem` diff --git a/tests/ui/strlen_on_c_strings.rs b/tests/ui/strlen_on_c_strings.rs new file mode 100644 index 00000000000..21902fa8483 --- /dev/null +++ b/tests/ui/strlen_on_c_strings.rs @@ -0,0 +1,16 @@ +#![warn(clippy::strlen_on_c_strings)] +#![allow(dead_code)] +#![feature(rustc_private)] +extern crate libc; + +use std::ffi::{CStr, CString}; + +fn main() { + // CString + let cstring = CString::new("foo").expect("CString::new failed"); + let len = unsafe { libc::strlen(cstring.as_ptr()) }; + + // CStr + let cstr = CStr::from_bytes_with_nul(b"foo\0").expect("CStr::from_bytes_with_nul failed"); + let len = unsafe { libc::strlen(cstr.as_ptr()) }; +} diff --git a/tests/ui/strlen_on_c_strings.stderr b/tests/ui/strlen_on_c_strings.stderr new file mode 100644 index 00000000000..a212bd327c3 --- /dev/null +++ b/tests/ui/strlen_on_c_strings.stderr @@ -0,0 +1,25 @@ +error: using `libc::strlen` on a `CString` or `CStr` value + --> $DIR/strlen_on_c_strings.rs:11:24 + | +LL | let len = unsafe { libc::strlen(cstring.as_ptr()) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::strlen-on-c-strings` implied by `-D warnings` +help: try this (you might also need to get rid of `unsafe` block in some cases): + | +LL | let len = unsafe { cstring.as_bytes().len() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: using `libc::strlen` on a `CString` or `CStr` value + --> $DIR/strlen_on_c_strings.rs:15:24 + | +LL | let len = unsafe { libc::strlen(cstr.as_ptr()) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try this (you might also need to get rid of `unsafe` block in some cases): + | +LL | let len = unsafe { cstr.to_bytes().len() }; + | ^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors +