diff --git a/README.md b/README.md index fee6d066e7c2..c7cee794db45 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ Table of contents: * [License](#license) ##Lints -There are 140 lints included in this crate: +There are 141 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -135,6 +135,7 @@ name [suspicious_assignment_formatting](https://github.com/Manishearth/rust-clippy/wiki#suspicious_assignment_formatting) | warn | suspicious formatting of `*=`, `-=` or `!=` [suspicious_else_formatting](https://github.com/Manishearth/rust-clippy/wiki#suspicious_else_formatting) | warn | suspicious formatting of `else if` [temporary_assignment](https://github.com/Manishearth/rust-clippy/wiki#temporary_assignment) | warn | assignments to temporaries +[temporary_cstring_as_ptr](https://github.com/Manishearth/rust-clippy/wiki#temporary_cstring_as_ptr) | warn | getting the inner pointer of a temporary `CString` [too_many_arguments](https://github.com/Manishearth/rust-clippy/wiki#too_many_arguments) | warn | functions with too many arguments [toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg) | warn | An entire binding was declared as `ref`, in a function argument (`fn foo(ref x: Bar)`), or a `let` statement (`let ref x = foo()`). In such cases, it is preferred to take references with `&`. [transmute_ptr_to_ref](https://github.com/Manishearth/rust-clippy/wiki#transmute_ptr_to_ref) | warn | transmutes from a pointer to a reference type diff --git a/src/lib.rs b/src/lib.rs index 1b48c7b3dbbc..c1a7732e3f5b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -325,6 +325,7 @@ pub fn plugin_registrar(reg: &mut Registry) { methods::SEARCH_IS_SOME, methods::SHOULD_IMPLEMENT_TRAIT, methods::SINGLE_CHAR_PATTERN, + methods::TEMPORARY_CSTRING_AS_PTR, methods::WRONG_SELF_CONVENTION, minmax::MIN_MAX, misc::CMP_NAN, diff --git a/src/methods.rs b/src/methods.rs index 45f87fba9dda..8a51b576b3df 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -13,8 +13,8 @@ use syntax::ptr::P; use utils::{get_trait_def_id, implements_trait, in_external_macro, in_macro, match_path, match_trait_method, match_type, method_chain_args, return_ty, same_tys, snippet, snippet_opt, span_lint, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth}; -use utils::{BTREEMAP_ENTRY_PATH, DEFAULT_TRAIT_PATH, HASHMAP_ENTRY_PATH, OPTION_PATH, RESULT_PATH, - VEC_PATH}; +use utils::{CSTRING_NEW_PATH, BTREEMAP_ENTRY_PATH, DEFAULT_TRAIT_PATH, HASHMAP_ENTRY_PATH, + OPTION_PATH, RESULT_PATH, VEC_PATH}; use utils::MethodArgs; #[derive(Clone)] @@ -286,6 +286,33 @@ declare_lint! { `_.split(\"x\")`" } +/// **What it does:** This lint 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,ignore +/// let c_str = CString::new("foo").unwrap().as_ptr(); +/// unsafe { +/// call_some_ffi_func(c_str); +/// } +/// ``` +/// Here `c_str` point to a freed address. The correct use would be: +/// ```rust,ignore +/// let c_str = CString::new("foo").unwrap(); +/// unsafe { +/// call_some_ffi_func(c_str.as_ptr()); +/// } +/// ``` +declare_lint! { + pub TEMPORARY_CSTRING_AS_PTR, + Warn, + "getting the inner pointer of a temporary `CString`" +} + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(EXTEND_FROM_SLICE, @@ -303,7 +330,8 @@ impl LintPass for MethodsPass { CLONE_DOUBLE_REF, NEW_RET_NO_SELF, SINGLE_CHAR_PATTERN, - SEARCH_IS_SOME) + SEARCH_IS_SOME, + TEMPORARY_CSTRING_AS_PTR) } } @@ -334,7 +362,11 @@ impl LateLintPass for MethodsPass { lint_search_is_some(cx, expr, "rposition", arglists[0], arglists[1]); } else if let Some(arglists) = method_chain_args(expr, &["extend"]) { lint_extend(cx, expr, arglists[0]); + } else if let Some(arglists) = method_chain_args(expr, &["unwrap", "as_ptr"]) { + lint_cstring_as_ptr(cx, expr, &arglists[0][0], &arglists[1][0]); } + + lint_or_fun_call(cx, expr, &name.node.as_str(), &args); if args.len() == 1 && name.node.as_str() == "clone" { lint_clone_on_copy(cx, expr); @@ -554,6 +586,22 @@ fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) { } } +fn lint_cstring_as_ptr(cx: &LateContext, expr: &Expr, new: &Expr, unwrap: &Expr) { + if_let_chain!{[ + let ExprCall(ref fun, ref args) = new.node, + args.len() == 1, + let ExprPath(None, ref path) = fun.node, + match_path(path, &CSTRING_NEW_PATH), + ], { + span_lint_and_then(cx, TEMPORARY_CSTRING_AS_PTR, expr.span, + "you are getting the inner pointer of a temporary `CString`", + |db| { + db.fileline_note(expr.span, "that pointer will be invalid outside this expression"); + db.span_help(unwrap.span, "assign the `CString` to a variable to extend its lifetime"); + }); + }} +} + fn derefs_to_slice(cx: &LateContext, expr: &Expr, ty: &ty::Ty) -> Option<(Span, &'static str)> { fn may_slice(cx: &LateContext, ty: &ty::Ty) -> bool { match ty.sty { diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 761d5d3df9dd..75637b67d9dc 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -28,11 +28,13 @@ pub type MethodArgs = HirVec>; // module DefPaths for certain structs/enums we check for pub const BEGIN_UNWIND: [&'static str; 3] = ["std", "rt", "begin_unwind"]; pub const BOX_NEW_PATH: [&'static str; 4] = ["std", "boxed", "Box", "new"]; +pub const BOX_PATH: [&'static str; 3] = ["std", "boxed", "Box"]; pub const BTREEMAP_ENTRY_PATH: [&'static str; 4] = ["collections", "btree", "map", "Entry"]; pub const BTREEMAP_PATH: [&'static str; 4] = ["collections", "btree", "map", "BTreeMap"]; pub const CLONE_PATH: [&'static str; 3] = ["clone", "Clone", "clone"]; pub const CLONE_TRAIT_PATH: [&'static str; 2] = ["clone", "Clone"]; pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"]; +pub const CSTRING_NEW_PATH: [&'static str; 4] = ["std", "ffi", "CString", "new"]; pub const DEBUG_FMT_METHOD_PATH: [&'static str; 4] = ["std", "fmt", "Debug", "fmt"]; pub const DEFAULT_TRAIT_PATH: [&'static str; 3] = ["core", "default", "Default"]; pub const DISPLAY_FMT_METHOD_PATH: [&'static str; 4] = ["std", "fmt", "Display", "fmt"]; @@ -59,7 +61,6 @@ pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"]; pub const TRANSMUTE_PATH: [&'static str; 4] = ["core", "intrinsics", "", "transmute"]; pub const VEC_FROM_ELEM_PATH: [&'static str; 3] = ["std", "vec", "from_elem"]; pub const VEC_PATH: [&'static str; 3] = ["collections", "vec", "Vec"]; -pub const BOX_PATH: [&'static str; 3] = ["std", "boxed", "Box"]; /// Produce a nested chain of if-lets and ifs from the patterns: /// diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 9d938ebb19e6..1869fd12a69b 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -470,3 +470,15 @@ fn single_char_pattern() { //~| HELP try using a char instead: //~| SUGGESTION x.trim_right_matches('x'); } + +#[allow(result_unwrap_used)] +fn temporary_cstring() { + use std::ffi::CString; + + ( // extra parenthesis to better test spans + //~^ ERROR you are getting the inner pointer of a temporary `CString` + //~| NOTE that pointer will be invalid outside this expression + CString::new("foo").unwrap() + //~^ HELP assign the `CString` to a variable to extend its lifetime + ).as_ptr(); +}