Auto merge of rust-lang#7243 - mgacek8:issue7145_strlen_on_c_strings, r=giraffate

Add new lint: `strlen_on_c_strings`

~~This is WIP, linting in case of `CString` has been added, but for `CStr`, its diagnostic item needs to be available for clippy.
[PR that adds diagnostic item for CStr on rust repo](rust-lang#85439
Ready for the review. Please take a look.
fixes rust-lang#7145
changelog: Add new lint: `strlen_on_c_strings`, that lints on `libc::strlen(some_cstring.as_ptr())`

Author: bors

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

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]

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
+                );
+            }
+        }
+    }
+}

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"];

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`

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()) };
+}

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
+