diff options
| -rw-r--r-- | compiler/rustc_lint/src/lib.rs | 3 | ||||
| -rw-r--r-- | compiler/rustc_lint/src/methods.rs | 106 | ||||
| -rw-r--r-- | compiler/rustc_span/src/symbol.rs | 7 | ||||
| -rw-r--r-- | library/std/src/ffi/c_str.rs | 3 | ||||
| -rw-r--r-- | src/test/ui/lint/lint-temporary-cstring-as-param.rs | 11 | ||||
| -rw-r--r-- | src/test/ui/lint/lint-temporary-cstring-as-param.stderr | 18 | ||||
| -rw-r--r-- | src/test/ui/lint/lint-temporary-cstring-as-ptr.rs | 9 | ||||
| -rw-r--r-- | src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr | 18 | ||||
| -rw-r--r-- | src/tools/clippy/.github/driver.sh | 6 | ||||
| -rw-r--r-- | src/tools/clippy/clippy_lints/src/lib.rs | 3 | ||||
| -rw-r--r-- | src/tools/clippy/clippy_lints/src/methods/mod.rs | 56 | ||||
| -rw-r--r-- | src/tools/clippy/clippy_lints/src/utils/paths.rs | 1 | ||||
| -rw-r--r-- | src/tools/clippy/src/lintlist/mod.rs | 7 | ||||
| -rw-r--r-- | src/tools/clippy/tests/ui/cstring.rs | 24 | ||||
| -rw-r--r-- | src/tools/clippy/tests/ui/cstring.stderr | 46 |
15 files changed, 177 insertions, 141 deletions
diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 1db59bfc39d..c8990842d32 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -49,6 +49,7 @@ mod early; mod internal; mod late; mod levels; +mod methods; mod non_ascii_idents; mod nonstandard_style; mod passes; @@ -73,6 +74,7 @@ use rustc_span::Span; use array_into_iter::ArrayIntoIter; use builtin::*; use internal::*; +use methods::*; use non_ascii_idents::*; use nonstandard_style::*; use redundant_semicolon::*; @@ -160,6 +162,7 @@ macro_rules! late_lint_passes { ArrayIntoIter: ArrayIntoIter, ClashingExternDeclarations: ClashingExternDeclarations::new(), DropTraitConstraints: DropTraitConstraints, + TemporaryCStringAsPtr: TemporaryCStringAsPtr, ] ); }; diff --git a/compiler/rustc_lint/src/methods.rs b/compiler/rustc_lint/src/methods.rs new file mode 100644 index 00000000000..8732845af0c --- /dev/null +++ b/compiler/rustc_lint/src/methods.rs @@ -0,0 +1,106 @@ +use crate::LateContext; +use crate::LateLintPass; +use crate::LintContext; +use rustc_hir::{Expr, ExprKind, PathSegment}; +use rustc_middle::ty; +use rustc_span::{symbol::sym, ExpnKind, Span}; + +declare_lint! { + /// The `temporary_cstring_as_ptr` lint detects getting the inner pointer of + /// a temporary `CString`. + /// + /// ### Example + /// + /// ```rust + /// # #![allow(unused)] + /// # use std::ffi::CString; + /// let c_str = CString::new("foo").unwrap().as_ptr(); + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// The inner pointer of a `CString` lives only as long as the `CString` it + /// points to. Getting the inner pointer of a *temporary* `CString` allows the `CString` + /// to be dropped at the end of the statement, as it is not being referenced as far as the typesystem + /// is concerned. This means outside of the statement the pointer will point to freed memory, which + /// causes undefined behavior if the pointer is later dereferenced. + pub TEMPORARY_CSTRING_AS_PTR, + Warn, + "detects getting the inner pointer of a temporary `CString`" +} + +declare_lint_pass!(TemporaryCStringAsPtr => [TEMPORARY_CSTRING_AS_PTR]); + +fn in_macro(span: Span) -> bool { + if span.from_expansion() { + !matches!(span.ctxt().outer_expn_data().kind, ExpnKind::Desugaring(..)) + } else { + false + } +} + +fn first_method_call<'tcx>( + expr: &'tcx Expr<'tcx>, +) -> Option<(&'tcx PathSegment<'tcx>, &'tcx [Expr<'tcx>])> { + if let ExprKind::MethodCall(path, _, args, _) = &expr.kind { + if args.iter().any(|e| e.span.from_expansion()) { None } else { Some((path, *args)) } + } else { + None + } +} + +impl<'tcx> LateLintPass<'tcx> for TemporaryCStringAsPtr { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if in_macro(expr.span) { + return; + } + + match first_method_call(expr) { + Some((path, args)) if path.ident.name == sym::as_ptr => { + let unwrap_arg = &args[0]; + let as_ptr_span = path.ident.span; + match first_method_call(unwrap_arg) { + Some((path, args)) + if path.ident.name == sym::unwrap || path.ident.name == sym::expect => + { + let source_arg = &args[0]; + lint_cstring_as_ptr(cx, as_ptr_span, source_arg, unwrap_arg); + } + _ => return, + } + } + _ => return, + } + } +} + +fn lint_cstring_as_ptr( + cx: &LateContext<'_>, + as_ptr_span: Span, + source: &rustc_hir::Expr<'_>, + unwrap: &rustc_hir::Expr<'_>, +) { + let source_type = cx.typeck_results().expr_ty(source); + if let ty::Adt(def, substs) = source_type.kind() { + if cx.tcx.is_diagnostic_item(sym::result_type, def.did) { + if let ty::Adt(adt, _) = substs.type_at(0).kind() { + if cx.tcx.is_diagnostic_item(sym::cstring_type, adt.did) { + cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, as_ptr_span, |diag| { + let mut diag = diag + .build("getting the inner pointer of a temporary `CString`"); + diag.span_label(as_ptr_span, "this pointer will be invalid"); + diag.span_label( + unwrap.span, + "this `CString` is deallocated at the end of the statement, bind it to a variable to extend its lifetime", + ); + diag.note("pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned"); + diag.help("for more information, see https://doc.rust-lang.org/reference/destructors.html"); + diag.emit(); + }); + } + } + } + } +} diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index ef0f09ae818..34c00429ccc 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -127,6 +127,7 @@ symbols! { ArgumentV1, Arguments, C, + CString, Center, Clone, Copy, @@ -261,6 +262,7 @@ symbols! { arm_target_feature, array, arrays, + as_ptr, as_str, asm, assert, @@ -310,6 +312,7 @@ symbols! { breakpoint, bridge, bswap, + c_str, c_variadic, call, call_mut, @@ -397,6 +400,7 @@ symbols! { crate_type, crate_visibility_modifier, crt_dash_static: "crt-static", + cstring_type, ctlz, ctlz_nonzero, ctpop, @@ -478,6 +482,7 @@ symbols! { existential_type, exp2f32, exp2f64, + expect, expected, expf32, expf64, @@ -501,6 +506,7 @@ symbols! { fadd_fast, fdiv_fast, feature, + ffi, ffi_const, ffi_pure, ffi_returns_twice, @@ -1170,6 +1176,7 @@ symbols! { unused_qualifications, unwind, unwind_attributes, + unwrap, unwrap_or, use_extern_macros, use_nested_groups, diff --git a/library/std/src/ffi/c_str.rs b/library/std/src/ffi/c_str.rs index 6df4eb99259..8c6d6c80402 100644 --- a/library/std/src/ffi/c_str.rs +++ b/library/std/src/ffi/c_str.rs @@ -110,6 +110,7 @@ use crate::sys; /// of `CString` instances can lead to invalid memory accesses, memory leaks, /// and other memory errors. #[derive(PartialEq, PartialOrd, Eq, Ord, Hash, Clone)] +#[cfg_attr(not(test), rustc_diagnostic_item = "cstring_type")] #[stable(feature = "rust1", since = "1.0.0")] pub struct CString { // Invariant 1: the slice ends with a zero byte and has a length of at least one. @@ -1265,7 +1266,7 @@ impl CStr { /// behavior when `ptr` is used inside the `unsafe` block: /// /// ```no_run - /// # #![allow(unused_must_use)] + /// # #![allow(unused_must_use)] #![cfg_attr(not(bootstrap), allow(temporary_cstring_as_ptr))] /// use std::ffi::CString; /// /// let ptr = CString::new("Hello").expect("CString::new failed").as_ptr(); diff --git a/src/test/ui/lint/lint-temporary-cstring-as-param.rs b/src/test/ui/lint/lint-temporary-cstring-as-param.rs new file mode 100644 index 00000000000..9f5805367e4 --- /dev/null +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.rs @@ -0,0 +1,11 @@ +#![deny(temporary_cstring_as_ptr)] + +use std::ffi::CString; +use std::os::raw::c_char; + +fn some_function(data: *const c_char) {} + +fn main() { + some_function(CString::new("").unwrap().as_ptr()); + //~^ ERROR getting the inner pointer of a temporary `CString` +} diff --git a/src/test/ui/lint/lint-temporary-cstring-as-param.stderr b/src/test/ui/lint/lint-temporary-cstring-as-param.stderr new file mode 100644 index 00000000000..0a9e5a4bf4a --- /dev/null +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.stderr @@ -0,0 +1,18 @@ +error: getting the inner pointer of a temporary `CString` + --> $DIR/lint-temporary-cstring-as-param.rs:9:45 + | +LL | some_function(CString::new("").unwrap().as_ptr()); + | ------------------------- ^^^^^^ this pointer will be invalid + | | + | this `CString` is deallocated at the end of the statement, bind it to a variable to extend its lifetime + | +note: the lint level is defined here + --> $DIR/lint-temporary-cstring-as-param.rs:1:9 + | +LL | #![deny(temporary_cstring_as_ptr)] + | ^^^^^^^^^^^^^^^^^^^^^^^^ + = note: pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned + = help: for more information, see https://doc.rust-lang.org/reference/destructors.html + +error: aborting due to previous error + diff --git a/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs new file mode 100644 index 00000000000..7aa4f2e1e00 --- /dev/null +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs @@ -0,0 +1,9 @@ +// this program is not technically incorrect, but is an obscure enough style to be worth linting +#![deny(temporary_cstring_as_ptr)] + +use std::ffi::CString; + +fn main() { + let s = CString::new("some text").unwrap().as_ptr(); + //~^ ERROR getting the inner pointer of a temporary `CString` +} diff --git a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr new file mode 100644 index 00000000000..e69d2dd533a --- /dev/null +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr @@ -0,0 +1,18 @@ +error: getting the inner pointer of a temporary `CString` + --> $DIR/lint-temporary-cstring-as-ptr.rs:7:48 + | +LL | let s = CString::new("some text").unwrap().as_ptr(); + | ---------------------------------- ^^^^^^ this pointer will be invalid + | | + | this `CString` is deallocated at the end of the statement, bind it to a variable to extend its lifetime + | +note: the lint level is defined here + --> $DIR/lint-temporary-cstring-as-ptr.rs:2:9 + | +LL | #![deny(temporary_cstring_as_ptr)] + | ^^^^^^^^^^^^^^^^^^^^^^^^ + = note: pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned + = help: for more information, see https://doc.rust-lang.org/reference/destructors.html + +error: aborting due to previous error + diff --git a/src/tools/clippy/.github/driver.sh b/src/tools/clippy/.github/driver.sh index 2c17c4203ae..fc4dca5042b 100644 --- a/src/tools/clippy/.github/driver.sh +++ b/src/tools/clippy/.github/driver.sh @@ -22,9 +22,9 @@ unset CARGO_MANIFEST_DIR # Run a lint and make sure it produces the expected output. It's also expected to exit with code 1 # FIXME: How to match the clippy invocation in compile-test.rs? -./target/debug/clippy-driver -Dwarnings -Aunused -Zui-testing --emit metadata --crate-type bin tests/ui/cstring.rs 2> cstring.stderr && exit 1 -sed -e "s,tests/ui,\$DIR," -e "/= help/d" cstring.stderr > normalized.stderr -diff normalized.stderr tests/ui/cstring.stderr +./target/debug/clippy-driver -Dwarnings -Aunused -Zui-testing --emit metadata --crate-type bin tests/ui/cast.rs 2> cast.stderr && exit 1 +sed -e "s,tests/ui,\$DIR," -e "/= help/d" cast.stderr > normalized.stderr +diff normalized.stderr tests/ui/cast.stderr # make sure "clippy-driver --rustc --arg" and "rustc --arg" behave the same diff --git a/src/tools/clippy/clippy_lints/src/lib.rs b/src/tools/clippy/clippy_lints/src/lib.rs index d4d2f92a6a6..b5ca63cefec 100644 --- a/src/tools/clippy/clippy_lints/src/lib.rs +++ b/src/tools/clippy/clippy_lints/src/lib.rs @@ -707,7 +707,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &methods::SKIP_WHILE_NEXT, &methods::STRING_EXTEND_CHARS, &methods::SUSPICIOUS_MAP, - &methods::TEMPORARY_CSTRING_AS_PTR, &methods::UNINIT_ASSUMED_INIT, &methods::UNNECESSARY_FILTER_MAP, &methods::UNNECESSARY_FOLD, @@ -1417,7 +1416,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&methods::SKIP_WHILE_NEXT), LintId::of(&methods::STRING_EXTEND_CHARS), LintId::of(&methods::SUSPICIOUS_MAP), - LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR), LintId::of(&methods::UNINIT_ASSUMED_INIT), LintId::of(&methods::UNNECESSARY_FILTER_MAP), LintId::of(&methods::UNNECESSARY_FOLD), @@ -1765,7 +1763,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT), LintId::of(&methods::CLONE_DOUBLE_REF), LintId::of(&methods::ITERATOR_STEP_BY_ZERO), - LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR), LintId::of(&methods::UNINIT_ASSUMED_INIT), LintId::of(&methods::ZST_OFFSET), LintId::of(&minmax::MIN_MAX), diff --git a/src/tools/clippy/clippy_lints/src/methods/mod.rs b/src/tools/clippy/clippy_lints/src/methods/mod.rs index c0824bacbc7..d250bfd71e9 100644 --- a/src/tools/clippy/clippy_lints/src/methods/mod.rs +++ b/src/tools/clippy/clippy_lints/src/methods/mod.rs @@ -799,40 +799,6 @@ declare_clippy_lint! { } declare_clippy_lint! { - /// **What it does:** 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 - /// # use std::ffi::CString; - /// # fn call_some_ffi_func(_: *const i8) {} - /// # - /// let c_str = CString::new("foo").unwrap().as_ptr(); - /// unsafe { - /// call_some_ffi_func(c_str); - /// } - /// ``` - /// Here `c_str` points to a freed address. The correct use would be: - /// ```rust - /// # use std::ffi::CString; - /// # fn call_some_ffi_func(_: *const i8) {} - /// # - /// let c_str = CString::new("foo").unwrap(); - /// unsafe { - /// call_some_ffi_func(c_str.as_ptr()); - /// } - /// ``` - pub TEMPORARY_CSTRING_AS_PTR, - correctness, - "getting the inner pointer of a temporary `CString`" -} - -declare_clippy_lint! { /// **What it does:** Checks for calling `.step_by(0)` on iterators which panics. /// /// **Why is this bad?** This very much looks like an oversight. Use `panic!()` instead if you @@ -1406,7 +1372,6 @@ declare_lint_pass!(Methods => [ SINGLE_CHAR_PATTERN, SINGLE_CHAR_PUSH_STR, SEARCH_IS_SOME, - TEMPORARY_CSTRING_AS_PTR, FILTER_NEXT, SKIP_WHILE_NEXT, FILTER_MAP, @@ -1490,7 +1455,6 @@ impl<'tcx> LateLintPass<'tcx> for Methods { lint_search_is_some(cx, expr, "rposition", arg_lists[1], arg_lists[0], method_spans[1]) }, ["extend", ..] => lint_extend(cx, expr, arg_lists[0]), - ["as_ptr", "unwrap" | "expect"] => lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]), ["nth", "iter"] => lint_iter_nth(cx, expr, &arg_lists, false), ["nth", "iter_mut"] => lint_iter_nth(cx, expr, &arg_lists, true), ["nth", ..] => lint_iter_nth_zero(cx, expr, arg_lists[0]), @@ -2207,26 +2171,6 @@ fn lint_extend(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_> } } -fn lint_cstring_as_ptr(cx: &LateContext<'_>, expr: &hir::Expr<'_>, source: &hir::Expr<'_>, unwrap: &hir::Expr<'_>) { - if_chain! { - let source_type = cx.typeck_results().expr_ty(source); - if let ty::Adt(def, substs) = source_type.kind(); - if cx.tcx.is_diagnostic_item(sym!(result_type), def.did); - if match_type(cx, substs.type_at(0), &paths::CSTRING); - then { - span_lint_and_then( - cx, - TEMPORARY_CSTRING_AS_PTR, - expr.span, - "you are getting the inner pointer of a temporary `CString`", - |diag| { - diag.note("that pointer will be invalid outside this expression"); - diag.span_help(unwrap.span, "assign the `CString` to a variable to extend its lifetime"); - }); - } - } -} - fn lint_iter_cloned_collect<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, iter_args: &'tcx [hir::Expr<'_>]) { if_chain! { if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym!(vec_type)); diff --git a/src/tools/clippy/clippy_lints/src/utils/paths.rs b/src/tools/clippy/clippy_lints/src/utils/paths.rs index 5e769c690a6..7566da80982 100644 --- a/src/tools/clippy/clippy_lints/src/utils/paths.rs +++ b/src/tools/clippy/clippy_lints/src/utils/paths.rs @@ -21,7 +21,6 @@ pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"]; pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"]; pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"]; pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"]; -pub const CSTRING: [&str; 4] = ["std", "ffi", "c_str", "CString"]; pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"]; pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"]; pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"]; diff --git a/src/tools/clippy/src/lintlist/mod.rs b/src/tools/clippy/src/lintlist/mod.rs index 6301d623a2b..dcbb8a6a31d 100644 --- a/src/tools/clippy/src/lintlist/mod.rs +++ b/src/tools/clippy/src/lintlist/mod.rs @@ -2259,13 +2259,6 @@ vec![ module: "temporary_assignment", }, Lint { - name: "temporary_cstring_as_ptr", - group: "correctness", - desc: "getting the inner pointer of a temporary `CString`", - deprecation: None, - module: "methods", - }, - Lint { name: "to_digit_is_some", group: "style", desc: "`char.is_digit()` is clearer", diff --git a/src/tools/clippy/tests/ui/cstring.rs b/src/tools/clippy/tests/ui/cstring.rs deleted file mode 100644 index 6cdd6b4ff6e..00000000000 --- a/src/tools/clippy/tests/ui/cstring.rs +++ /dev/null @@ -1,24 +0,0 @@ -#![deny(clippy::temporary_cstring_as_ptr)] - -fn main() {} - -fn temporary_cstring() { - use std::ffi::CString; - - CString::new("foo").unwrap().as_ptr(); - CString::new("foo").expect("dummy").as_ptr(); -} - -mod issue4375 { - use std::ffi::CString; - use std::os::raw::c_char; - - extern "C" { - fn foo(data: *const c_char); - } - - pub fn bar(v: &[u8]) { - let cstr = CString::new(v); - unsafe { foo(cstr.unwrap().as_ptr()) } - } -} diff --git a/src/tools/clippy/tests/ui/cstring.stderr b/src/tools/clippy/tests/ui/cstring.stderr deleted file mode 100644 index 87cb29be577..00000000000 --- a/src/tools/clippy/tests/ui/cstring.stderr +++ /dev/null @@ -1,46 +0,0 @@ -error: you are getting the inner pointer of a temporary `CString` - --> $DIR/cstring.rs:8:5 - | -LL | CString::new("foo").unwrap().as_ptr(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -note: the lint level is defined here - --> $DIR/cstring.rs:1:9 - | -LL | #![deny(clippy::temporary_cstring_as_ptr)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: that pointer will be invalid outside this expression -help: assign the `CString` to a variable to extend its lifetime - --> $DIR/cstring.rs:8:5 - | -LL | CString::new("foo").unwrap().as_ptr(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: you are getting the inner pointer of a temporary `CString` - --> $DIR/cstring.rs:9:5 - | -LL | CString::new("foo").expect("dummy").as_ptr(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = note: that pointer will be invalid outside this expression -help: assign the `CString` to a variable to extend its lifetime - --> $DIR/cstring.rs:9:5 - | -LL | CString::new("foo").expect("dummy").as_ptr(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: you are getting the inner pointer of a temporary `CString` - --> $DIR/cstring.rs:22:22 - | -LL | unsafe { foo(cstr.unwrap().as_ptr()) } - | ^^^^^^^^^^^^^^^^^^^^^^ - | - = note: that pointer will be invalid outside this expression -help: assign the `CString` to a variable to extend its lifetime - --> $DIR/cstring.rs:22:22 - | -LL | unsafe { foo(cstr.unwrap().as_ptr()) } - | ^^^^^^^^^^^^^ - -error: aborting due to 3 previous errors - |