From 8cf1b0e1adda1313771dbce65e9be7b40fa17490 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 18 Aug 2020 11:25:21 -0400 Subject: [PATCH 01/13] Uplift temporary-cstring-as-ptr into rustc --- compiler/rustc_lint/src/lib.rs | 3 + compiler/rustc_lint/src/methods.rs | 102 +++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 compiler/rustc_lint/src/methods.rs diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 1db59bfc39dce..c8990842d32e6 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 0000000000000..910e07154a7ec --- /dev/null +++ b/compiler/rustc_lint/src/methods.rs @@ -0,0 +1,102 @@ +use crate::LateContext; +use crate::LateLintPass; +use crate::LintContext; +use rustc_hir::{Expr, ExprKind}; +use rustc_middle::ty; +use rustc_span::{ + symbol::{sym, Symbol, SymbolStr}, + ExpnKind, Span, +}; + +declare_lint! { + pub TEMPORARY_CSTRING_AS_PTR, + Deny, + "detects getting the inner pointer of a temporary `CString`" +} + +declare_lint_pass!(TemporaryCStringAsPtr => [TEMPORARY_CSTRING_AS_PTR]); + +/// Returns the method names and argument list of nested method call expressions that make up +/// `expr`. method/span lists are sorted with the most recent call first. +pub fn method_calls<'tcx>( + expr: &'tcx Expr<'tcx>, + max_depth: usize, +) -> (Vec, Vec<&'tcx [Expr<'tcx>]>, Vec) { + let mut method_names = Vec::with_capacity(max_depth); + let mut arg_lists = Vec::with_capacity(max_depth); + let mut spans = Vec::with_capacity(max_depth); + + let mut current = expr; + for _ in 0..max_depth { + if let ExprKind::MethodCall(path, span, args, _) = ¤t.kind { + if args.iter().any(|e| e.span.from_expansion()) { + break; + } + method_names.push(path.ident.name); + arg_lists.push(&**args); + spans.push(*span); + current = &args[0]; + } else { + break; + } + } + + (method_names, arg_lists, spans) +} + +fn in_macro(span: Span) -> bool { + if span.from_expansion() { + !matches!(span.ctxt().outer_expn_data().kind, ExpnKind::Desugaring(..)) + } else { + false + } +} + +impl<'tcx> LateLintPass<'tcx> for TemporaryCStringAsPtr { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if in_macro(expr.span) { + return; + } + + let (method_names, arg_lists, _) = method_calls(expr, 2); + let method_names: Vec = method_names.iter().map(|s| s.as_str()).collect(); + let method_names: Vec<&str> = method_names.iter().map(|s| &**s).collect(); + + if let ["as_ptr", "unwrap" | "expect"] = method_names.as_slice() { + lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]); + } + } +} + +fn lint_cstring_as_ptr( + cx: &LateContext<'_>, + expr: &rustc_hir::Expr<'_>, + 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(Symbol::intern("result_type"), def.did) { + if let ty::Adt(adt, _) = substs.type_at(0).kind { + let path = [ + sym::std, + Symbol::intern("ffi"), + Symbol::intern("c_str"), + Symbol::intern("CString"), + ]; + if cx.match_def_path(adt.did, &path) { + cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, expr.span, |diag| { + let mut diag = diag + .build("you are getting the inner pointer of a temporary `CString`"); + 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", + ); + diag.emit(); + }); + } + } + } + } +} From a2f4afe0f6e9ce451e2aca0a91100c6335be9181 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 18 Aug 2020 12:09:33 -0400 Subject: [PATCH 02/13] Add basic test --- .../ui/lint/lint-temporary-cstring-as-ptr.rs | 8 ++++++++ .../ui/lint/lint-temporary-cstring-as-ptr.stderr | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 src/test/ui/lint/lint-temporary-cstring-as-ptr.rs create mode 100644 src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr 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 0000000000000..3ca6ee5439cd7 --- /dev/null +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs @@ -0,0 +1,8 @@ +// check-fail +// ignore-tidy-linelength + +use std::ffi::CString; + +fn main() { + let s = CString::new("some text").unwrap().as_ptr(); //~ ERROR you are 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 0000000000000..7cbb14a8f4e0d --- /dev/null +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr @@ -0,0 +1,16 @@ +error: you are getting the inner pointer of a temporary `CString` + --> $DIR/lint-temporary-cstring-as-ptr.rs:7:13 + | +LL | let s = CString::new("some text").unwrap().as_ptr(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `#[deny(temporary_cstring_as_ptr)]` on by default + = note: that pointer will be invalid outside this expression +help: assign the `CString` to a variable to extend its lifetime + --> $DIR/lint-temporary-cstring-as-ptr.rs:7:13 + | +LL | let s = CString::new("some text").unwrap().as_ptr(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + From 8b65df06ce0cf78fd2298c9cd63e1f5beb40525f Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 18 Aug 2020 17:02:23 -0400 Subject: [PATCH 03/13] Address review comments --- compiler/rustc_lint/src/methods.rs | 84 ++++++++----------- compiler/rustc_span/src/symbol.rs | 6 ++ .../ui/lint/lint-temporary-cstring-as-ptr.rs | 3 +- .../lint/lint-temporary-cstring-as-ptr.stderr | 12 +-- 4 files changed, 48 insertions(+), 57 deletions(-) diff --git a/compiler/rustc_lint/src/methods.rs b/compiler/rustc_lint/src/methods.rs index 910e07154a7ec..7b595dd18ff6b 100644 --- a/compiler/rustc_lint/src/methods.rs +++ b/compiler/rustc_lint/src/methods.rs @@ -1,10 +1,10 @@ use crate::LateContext; use crate::LateLintPass; use crate::LintContext; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::{Expr, ExprKind, PathSegment}; use rustc_middle::ty; use rustc_span::{ - symbol::{sym, Symbol, SymbolStr}, + symbol::{sym, Symbol}, ExpnKind, Span, }; @@ -16,34 +16,6 @@ declare_lint! { declare_lint_pass!(TemporaryCStringAsPtr => [TEMPORARY_CSTRING_AS_PTR]); -/// Returns the method names and argument list of nested method call expressions that make up -/// `expr`. method/span lists are sorted with the most recent call first. -pub fn method_calls<'tcx>( - expr: &'tcx Expr<'tcx>, - max_depth: usize, -) -> (Vec, Vec<&'tcx [Expr<'tcx>]>, Vec) { - let mut method_names = Vec::with_capacity(max_depth); - let mut arg_lists = Vec::with_capacity(max_depth); - let mut spans = Vec::with_capacity(max_depth); - - let mut current = expr; - for _ in 0..max_depth { - if let ExprKind::MethodCall(path, span, args, _) = ¤t.kind { - if args.iter().any(|e| e.span.from_expansion()) { - break; - } - method_names.push(path.ident.name); - arg_lists.push(&**args); - spans.push(*span); - current = &args[0]; - } else { - break; - } - } - - (method_names, arg_lists, spans) -} - fn in_macro(span: Span) -> bool { if span.from_expansion() { !matches!(span.ctxt().outer_expn_data().kind, ExpnKind::Desugaring(..)) @@ -52,47 +24,61 @@ fn in_macro(span: Span) -> bool { } } +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; } - let (method_names, arg_lists, _) = method_calls(expr, 2); - let method_names: Vec = method_names.iter().map(|s| s.as_str()).collect(); - let method_names: Vec<&str> = method_names.iter().map(|s| &**s).collect(); - - if let ["as_ptr", "unwrap" | "expect"] = method_names.as_slice() { - lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]); + match first_method_call(expr) { + Some((path, args)) if path.ident.name == sym::as_ptr => { + let unwrap_arg = &args[0]; + 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, source_arg, unwrap_arg); + } + _ => return, + } + } + _ => return, } } } +const CSTRING_PATH: [Symbol; 4] = [sym::std, sym::ffi, sym::c_str, sym::CString]; + fn lint_cstring_as_ptr( cx: &LateContext<'_>, - expr: &rustc_hir::Expr<'_>, 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(Symbol::intern("result_type"), def.did) { + if cx.tcx.is_diagnostic_item(sym::result_type, def.did) { if let ty::Adt(adt, _) = substs.type_at(0).kind { - let path = [ - sym::std, - Symbol::intern("ffi"), - Symbol::intern("c_str"), - Symbol::intern("CString"), - ]; - if cx.match_def_path(adt.did, &path) { - cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, expr.span, |diag| { + if cx.match_def_path(adt.did, &CSTRING_PATH) { + cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, source.span, |diag| { let mut diag = diag - .build("you are getting the inner pointer of a temporary `CString`"); - diag.note("that pointer will be invalid outside this expression"); + .build("getting the inner pointer of a temporary `CString`"); + diag.span_label(source.span, "this pointer will be invalid"); diag.span_help( unwrap.span, - "assign the `CString` to a variable to extend its lifetime", + "this `CString` is deallocated at the end of the expression, bind it to a variable to extend its lifetime", ); + diag.note("pointers do not have a lifetime; when calling `as_ptr` the `CString` is deallocated because nothing is referencing it as far as the type system is concerned"); diag.emit(); }); } diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index bae1e4f314c01..38539643416c2 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, @@ -477,6 +480,7 @@ symbols! { existential_type, exp2f32, exp2f64, + expect, expected, expf32, expf64, @@ -500,6 +504,7 @@ symbols! { fadd_fast, fdiv_fast, feature, + ffi, ffi_const, ffi_pure, ffi_returns_twice, @@ -1167,6 +1172,7 @@ symbols! { unused_qualifications, unwind, unwind_attributes, + unwrap, unwrap_or, use_extern_macros, use_nested_groups, diff --git a/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs index 3ca6ee5439cd7..54f57e1dd7712 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs @@ -1,8 +1,7 @@ -// check-fail // ignore-tidy-linelength use std::ffi::CString; fn main() { - let s = CString::new("some text").unwrap().as_ptr(); //~ ERROR you are getting the inner pointer of a temporary `CString` + 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 index 7cbb14a8f4e0d..34e42a478051d 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr @@ -1,16 +1,16 @@ -error: you are getting the inner pointer of a temporary `CString` - --> $DIR/lint-temporary-cstring-as-ptr.rs:7:13 +error: getting the inner pointer of a temporary `CString` + --> $DIR/lint-temporary-cstring-as-ptr.rs:6:13 | LL | let s = CString::new("some text").unwrap().as_ptr(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ this pointer will be invalid | = note: `#[deny(temporary_cstring_as_ptr)]` on by default - = note: that pointer will be invalid outside this expression -help: assign the `CString` to a variable to extend its lifetime - --> $DIR/lint-temporary-cstring-as-ptr.rs:7:13 +help: this `CString` is deallocated at the end of the expression, bind it to a variable to extend its lifetime + --> $DIR/lint-temporary-cstring-as-ptr.rs:6:13 | LL | let s = CString::new("some text").unwrap().as_ptr(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: pointers do not have a lifetime; when calling `as_ptr` the `CString` is deallocated because nothing is referencing it as far as the type system is concerned error: aborting due to previous error From ce95122e95b5cb4698fa2a07e747823b64729f59 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 18 Aug 2020 18:47:52 -0400 Subject: [PATCH 04/13] Update doctest --- library/std/src/ffi/c_str.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/ffi/c_str.rs b/library/std/src/ffi/c_str.rs index 6df4eb992594f..53966396e1108 100644 --- a/library/std/src/ffi/c_str.rs +++ b/library/std/src/ffi/c_str.rs @@ -1265,7 +1265,7 @@ impl CStr { /// behavior when `ptr` is used inside the `unsafe` block: /// /// ```no_run - /// # #![allow(unused_must_use)] + /// # #![allow(unused_must_use)] #![allow(temporary_cstring_as_ptr)] /// use std::ffi::CString; /// /// let ptr = CString::new("Hello").expect("CString::new failed").as_ptr(); From 5643a0662af337c1096975400040c4442da439ca Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 18 Aug 2020 19:37:50 -0400 Subject: [PATCH 05/13] Tweak diagnostic --- compiler/rustc_lint/src/methods.rs | 10 ++++++---- src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr | 11 ++++------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_lint/src/methods.rs b/compiler/rustc_lint/src/methods.rs index 7b595dd18ff6b..1e67e9dd7deba 100644 --- a/compiler/rustc_lint/src/methods.rs +++ b/compiler/rustc_lint/src/methods.rs @@ -43,12 +43,13 @@ impl<'tcx> LateLintPass<'tcx> for TemporaryCStringAsPtr { 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, source_arg, unwrap_arg); + lint_cstring_as_ptr(cx, as_ptr_span, source_arg, unwrap_arg); } _ => return, } @@ -62,6 +63,7 @@ const CSTRING_PATH: [Symbol; 4] = [sym::std, sym::ffi, sym::c_str, sym::CString] fn lint_cstring_as_ptr( cx: &LateContext<'_>, + as_ptr_span: Span, source: &rustc_hir::Expr<'_>, unwrap: &rustc_hir::Expr<'_>, ) { @@ -70,11 +72,11 @@ fn lint_cstring_as_ptr( if cx.tcx.is_diagnostic_item(sym::result_type, def.did) { if let ty::Adt(adt, _) = substs.type_at(0).kind { if cx.match_def_path(adt.did, &CSTRING_PATH) { - cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, source.span, |diag| { + 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(source.span, "this pointer will be invalid"); - diag.span_help( + 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 expression, bind it to a variable to extend its lifetime", ); diff --git a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr index 34e42a478051d..9ceb71ba8d9eb 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr @@ -1,15 +1,12 @@ error: getting the inner pointer of a temporary `CString` - --> $DIR/lint-temporary-cstring-as-ptr.rs:6:13 + --> $DIR/lint-temporary-cstring-as-ptr.rs:6:48 | LL | let s = CString::new("some text").unwrap().as_ptr(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ this pointer will be invalid + | ---------------------------------- ^^^^^^ this pointer will be invalid + | | + | this `CString` is deallocated at the end of the expression, bind it to a variable to extend its lifetime | = note: `#[deny(temporary_cstring_as_ptr)]` on by default -help: this `CString` is deallocated at the end of the expression, bind it to a variable to extend its lifetime - --> $DIR/lint-temporary-cstring-as-ptr.rs:6:13 - | -LL | let s = CString::new("some text").unwrap().as_ptr(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: pointers do not have a lifetime; when calling `as_ptr` the `CString` is deallocated because nothing is referencing it as far as the type system is concerned error: aborting due to previous error From 737bfeffd2805f2372c934999afd8ea87921d835 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Sun, 23 Aug 2020 14:21:58 -0400 Subject: [PATCH 06/13] Change to warn by default / fix typo --- compiler/rustc_lint/src/methods.rs | 2 +- library/std/src/ffi/c_str.rs | 2 +- src/test/ui/lint/lint-temporary-cstring-as-ptr.rs | 1 + src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr | 8 ++++++-- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_lint/src/methods.rs b/compiler/rustc_lint/src/methods.rs index 1e67e9dd7deba..f1e44fbe2a250 100644 --- a/compiler/rustc_lint/src/methods.rs +++ b/compiler/rustc_lint/src/methods.rs @@ -10,7 +10,7 @@ use rustc_span::{ declare_lint! { pub TEMPORARY_CSTRING_AS_PTR, - Deny, + Warn, "detects getting the inner pointer of a temporary `CString`" } diff --git a/library/std/src/ffi/c_str.rs b/library/std/src/ffi/c_str.rs index 53966396e1108..a9ab0d1d83bc4 100644 --- a/library/std/src/ffi/c_str.rs +++ b/library/std/src/ffi/c_str.rs @@ -1265,7 +1265,7 @@ impl CStr { /// behavior when `ptr` is used inside the `unsafe` block: /// /// ```no_run - /// # #![allow(unused_must_use)] #![allow(temporary_cstring_as_ptr)] + /// # #![allow(unused_must_use, 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-ptr.rs b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs index 54f57e1dd7712..73a9d6677c757 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs @@ -1,4 +1,5 @@ // ignore-tidy-linelength +#![deny(temporary_cstring_as_ptr)] use std::ffi::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 index 9ceb71ba8d9eb..ee06bfc2fadb3 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr @@ -1,12 +1,16 @@ error: getting the inner pointer of a temporary `CString` - --> $DIR/lint-temporary-cstring-as-ptr.rs:6:48 + --> $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 expression, bind it to a variable to extend its lifetime | - = note: `#[deny(temporary_cstring_as_ptr)]` on by default +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` is deallocated because nothing is referencing it as far as the type system is concerned error: aborting due to previous error From 1bcd2452fe0abd1510b21cfa9aef19898a5c14fe Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 21 Sep 2020 16:32:28 -0400 Subject: [PATCH 07/13] Address review comments --- compiler/rustc_lint/src/methods.rs | 19 ++++++++----------- compiler/rustc_span/src/symbol.rs | 1 + library/std/src/ffi/c_str.rs | 2 ++ .../lint/lint-temporary-cstring-as-param.rs | 10 ++++++++++ .../lint-temporary-cstring-as-param.stderr | 19 +++++++++++++++++++ .../lint/lint-temporary-cstring-as-ptr.stderr | 6 ++++-- 6 files changed, 44 insertions(+), 13 deletions(-) create mode 100644 src/test/ui/lint/lint-temporary-cstring-as-param.rs create mode 100644 src/test/ui/lint/lint-temporary-cstring-as-param.stderr diff --git a/compiler/rustc_lint/src/methods.rs b/compiler/rustc_lint/src/methods.rs index f1e44fbe2a250..3852cc4f2986a 100644 --- a/compiler/rustc_lint/src/methods.rs +++ b/compiler/rustc_lint/src/methods.rs @@ -3,10 +3,7 @@ use crate::LateLintPass; use crate::LintContext; use rustc_hir::{Expr, ExprKind, PathSegment}; use rustc_middle::ty; -use rustc_span::{ - symbol::{sym, Symbol}, - ExpnKind, Span, -}; +use rustc_span::{symbol::sym, ExpnKind, Span}; declare_lint! { pub TEMPORARY_CSTRING_AS_PTR, @@ -59,8 +56,6 @@ impl<'tcx> LateLintPass<'tcx> for TemporaryCStringAsPtr { } } -const CSTRING_PATH: [Symbol; 4] = [sym::std, sym::ffi, sym::c_str, sym::CString]; - fn lint_cstring_as_ptr( cx: &LateContext<'_>, as_ptr_span: Span, @@ -68,19 +63,21 @@ fn lint_cstring_as_ptr( unwrap: &rustc_hir::Expr<'_>, ) { let source_type = cx.typeck_results().expr_ty(source); - if let ty::Adt(def, substs) = source_type.kind { + 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.match_def_path(adt.did, &CSTRING_PATH) { + 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 expression, bind it to a variable to extend its lifetime", + "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` is deallocated because nothing is referencing it as far as the type system is concerned"); + diag.note("pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement..."); + diag.note("...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 38539643416c2..10d47b824bcf1 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -400,6 +400,7 @@ symbols! { crate_type, crate_visibility_modifier, crt_dash_static: "crt-static", + cstring_type, ctlz, ctlz_nonzero, ctpop, diff --git a/library/std/src/ffi/c_str.rs b/library/std/src/ffi/c_str.rs index a9ab0d1d83bc4..1cfd73863a617 100644 --- a/library/std/src/ffi/c_str.rs +++ b/library/std/src/ffi/c_str.rs @@ -109,7 +109,9 @@ use crate::sys; /// documentation of `CString` before use, as improper ownership management /// 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. 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 0000000000000..197b7fd704f3c --- /dev/null +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.rs @@ -0,0 +1,10 @@ +// ignore-tidy-linelength +#![deny(temporary_cstring_as_ptr)] + +use std::ffi::CString; + +fn some_function(data: *const i8) {} + +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 0000000000000..1e0d0d954dc87 --- /dev/null +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.stderr @@ -0,0 +1,19 @@ +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: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... + = note: ...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.stderr b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr index ee06bfc2fadb3..8fca03b49df2c 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr @@ -4,14 +4,16 @@ error: getting the inner pointer of a temporary `CString` LL | let s = CString::new("some text").unwrap().as_ptr(); | ---------------------------------- ^^^^^^ this pointer will be invalid | | - | this `CString` is deallocated at the end of the expression, bind it to a variable to extend its lifetime + | 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` is deallocated because nothing is referencing it as far as the type system is concerned + = note: pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement... + = note: ...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 From 576eb2a30cab62992e36f3acaf8aedbadf58ba75 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 22 Sep 2020 11:20:06 -0400 Subject: [PATCH 08/13] Write docs for lint / fix review nit --- compiler/rustc_lint/src/methods.rs | 22 +++++++++++++++++-- .../lint-temporary-cstring-as-param.stderr | 3 +-- .../ui/lint/lint-temporary-cstring-as-ptr.rs | 1 + .../lint/lint-temporary-cstring-as-ptr.stderr | 7 +++--- 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_lint/src/methods.rs b/compiler/rustc_lint/src/methods.rs index 3852cc4f2986a..2fad02bf883aa 100644 --- a/compiler/rustc_lint/src/methods.rs +++ b/compiler/rustc_lint/src/methods.rs @@ -6,6 +6,25 @@ 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)] + /// 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`" @@ -75,8 +94,7 @@ fn lint_cstring_as_ptr( 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..."); - diag.note("...because nothing is referencing it as far as the type system is concerned"); + 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/src/test/ui/lint/lint-temporary-cstring-as-param.stderr b/src/test/ui/lint/lint-temporary-cstring-as-param.stderr index 1e0d0d954dc87..b30ebd5e409e8 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-param.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.stderr @@ -11,8 +11,7 @@ note: the lint level is defined here | 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... - = note: ...because nothing is referencing it as far as the type system is concerned + = 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 index 73a9d6677c757..e463f84c48d66 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs @@ -1,4 +1,5 @@ // ignore-tidy-linelength +// 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; diff --git a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr index 8fca03b49df2c..009cfe343a633 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr @@ -1,5 +1,5 @@ error: getting the inner pointer of a temporary `CString` - --> $DIR/lint-temporary-cstring-as-ptr.rs:7:48 + --> $DIR/lint-temporary-cstring-as-ptr.rs:8:48 | LL | let s = CString::new("some text").unwrap().as_ptr(); | ---------------------------------- ^^^^^^ this pointer will be invalid @@ -7,12 +7,11 @@ LL | let s = CString::new("some text").unwrap().as_ptr(); | 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 + --> $DIR/lint-temporary-cstring-as-ptr.rs:3: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... - = note: ...because nothing is referencing it as far as the type system is concerned + = 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 From 5ac1688f4bd87ac6c1a416841e5a53fa1517cfab Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 22 Sep 2020 12:23:22 -0400 Subject: [PATCH 09/13] Remove lint from clippy --- src/tools/clippy/.github/driver.sh | 6 +- src/tools/clippy/clippy_lints/src/lib.rs | 3 - .../clippy/clippy_lints/src/methods/mod.rs | 56 ------------------- .../clippy/clippy_lints/src/utils/paths.rs | 1 - src/tools/clippy/src/lintlist/mod.rs | 7 --- src/tools/clippy/tests/ui/cstring.rs | 24 -------- src/tools/clippy/tests/ui/cstring.stderr | 46 --------------- 7 files changed, 3 insertions(+), 140 deletions(-) delete mode 100644 src/tools/clippy/tests/ui/cstring.rs delete mode 100644 src/tools/clippy/tests/ui/cstring.stderr diff --git a/src/tools/clippy/.github/driver.sh b/src/tools/clippy/.github/driver.sh index 2c17c4203ae5c..fc4dca5042b2a 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 d4d2f92a6a695..b5ca63cefec4f 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 c0824bacbc735..d250bfd71e936 100644 --- a/src/tools/clippy/clippy_lints/src/methods/mod.rs +++ b/src/tools/clippy/clippy_lints/src/methods/mod.rs @@ -798,40 +798,6 @@ declare_clippy_lint! { "using a single-character str where a char could be used, e.g., `_.split(\"x\")`" } -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. /// @@ -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 5e769c690a690..7566da80982a0 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 6301d623a2b12..dcbb8a6a31da9 100644 --- a/src/tools/clippy/src/lintlist/mod.rs +++ b/src/tools/clippy/src/lintlist/mod.rs @@ -2258,13 +2258,6 @@ vec![ deprecation: None, 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", diff --git a/src/tools/clippy/tests/ui/cstring.rs b/src/tools/clippy/tests/ui/cstring.rs deleted file mode 100644 index 6cdd6b4ff6e77..0000000000000 --- 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 87cb29be57758..0000000000000 --- 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 - From 6ba127d3f06aa785eaf3df96f0adb505de67e74c Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 22 Sep 2020 12:38:50 -0400 Subject: [PATCH 10/13] Fix doctest --- compiler/rustc_lint/src/methods.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/rustc_lint/src/methods.rs b/compiler/rustc_lint/src/methods.rs index 2fad02bf883aa..8732845af0cec 100644 --- a/compiler/rustc_lint/src/methods.rs +++ b/compiler/rustc_lint/src/methods.rs @@ -13,6 +13,7 @@ declare_lint! { /// /// ```rust /// # #![allow(unused)] + /// # use std::ffi::CString; /// let c_str = CString::new("foo").unwrap().as_ptr(); /// ``` /// From cb8b9012dba9e467c052b3cc047e0c7babfcf724 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 26 Oct 2020 19:19:06 -0400 Subject: [PATCH 11/13] Address review comments --- library/std/src/ffi/c_str.rs | 1 - src/test/ui/lint/lint-temporary-cstring-as-param.rs | 4 ++-- src/test/ui/lint/lint-temporary-cstring-as-param.stderr | 4 ++-- src/test/ui/lint/lint-temporary-cstring-as-ptr.rs | 4 ++-- src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr | 4 ++-- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/library/std/src/ffi/c_str.rs b/library/std/src/ffi/c_str.rs index 1cfd73863a617..4a249d72bbccf 100644 --- a/library/std/src/ffi/c_str.rs +++ b/library/std/src/ffi/c_str.rs @@ -109,7 +109,6 @@ use crate::sys; /// documentation of `CString` before use, as improper ownership management /// 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")] diff --git a/src/test/ui/lint/lint-temporary-cstring-as-param.rs b/src/test/ui/lint/lint-temporary-cstring-as-param.rs index 197b7fd704f3c..2d40f6e871d9f 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-param.rs +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.rs @@ -1,4 +1,3 @@ -// ignore-tidy-linelength #![deny(temporary_cstring_as_ptr)] use std::ffi::CString; @@ -6,5 +5,6 @@ use std::ffi::CString; fn some_function(data: *const i8) {} fn main() { - some_function(CString::new("").unwrap().as_ptr()); //~ ERROR getting the inner pointer of a temporary `CString` + 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 index b30ebd5e409e8..c72d56f2a5fca 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-param.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.stderr @@ -1,5 +1,5 @@ error: getting the inner pointer of a temporary `CString` - --> $DIR/lint-temporary-cstring-as-param.rs:9:45 + --> $DIR/lint-temporary-cstring-as-param.rs:8:45 | LL | some_function(CString::new("").unwrap().as_ptr()); | ------------------------- ^^^^^^ this pointer will be invalid @@ -7,7 +7,7 @@ LL | some_function(CString::new("").unwrap().as_ptr()); | 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:2:9 + --> $DIR/lint-temporary-cstring-as-param.rs:1:9 | LL | #![deny(temporary_cstring_as_ptr)] | ^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs index e463f84c48d66..7aa4f2e1e005c 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.rs @@ -1,9 +1,9 @@ -// ignore-tidy-linelength // 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` + 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 index 009cfe343a633..e69d2dd533a40 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr @@ -1,5 +1,5 @@ error: getting the inner pointer of a temporary `CString` - --> $DIR/lint-temporary-cstring-as-ptr.rs:8:48 + --> $DIR/lint-temporary-cstring-as-ptr.rs:7:48 | LL | let s = CString::new("some text").unwrap().as_ptr(); | ---------------------------------- ^^^^^^ this pointer will be invalid @@ -7,7 +7,7 @@ LL | let s = CString::new("some text").unwrap().as_ptr(); | 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:3:9 + --> $DIR/lint-temporary-cstring-as-ptr.rs:2:9 | LL | #![deny(temporary_cstring_as_ptr)] | ^^^^^^^^^^^^^^^^^^^^^^^^ From 39941e6281e8da26ab6dfa776ff9583ec94cba71 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 26 Oct 2020 22:09:47 -0400 Subject: [PATCH 12/13] Fix bootstrap doctest failure --- library/std/src/ffi/c_str.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/ffi/c_str.rs b/library/std/src/ffi/c_str.rs index 4a249d72bbccf..8c6d6c80402fa 100644 --- a/library/std/src/ffi/c_str.rs +++ b/library/std/src/ffi/c_str.rs @@ -1266,7 +1266,7 @@ impl CStr { /// behavior when `ptr` is used inside the `unsafe` block: /// /// ```no_run - /// # #![allow(unused_must_use, temporary_cstring_as_ptr)] + /// # #![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(); From 572cd358d379e4e641ad44bc693cdf48ceaf3581 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Tue, 27 Oct 2020 10:10:30 -0400 Subject: [PATCH 13/13] Fix test --- src/test/ui/lint/lint-temporary-cstring-as-param.rs | 3 ++- src/test/ui/lint/lint-temporary-cstring-as-param.stderr | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/ui/lint/lint-temporary-cstring-as-param.rs b/src/test/ui/lint/lint-temporary-cstring-as-param.rs index 2d40f6e871d9f..9f5805367e43d 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-param.rs +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.rs @@ -1,8 +1,9 @@ #![deny(temporary_cstring_as_ptr)] use std::ffi::CString; +use std::os::raw::c_char; -fn some_function(data: *const i8) {} +fn some_function(data: *const c_char) {} fn main() { some_function(CString::new("").unwrap().as_ptr()); diff --git a/src/test/ui/lint/lint-temporary-cstring-as-param.stderr b/src/test/ui/lint/lint-temporary-cstring-as-param.stderr index c72d56f2a5fca..0a9e5a4bf4aa5 100644 --- a/src/test/ui/lint/lint-temporary-cstring-as-param.stderr +++ b/src/test/ui/lint/lint-temporary-cstring-as-param.stderr @@ -1,5 +1,5 @@ error: getting the inner pointer of a temporary `CString` - --> $DIR/lint-temporary-cstring-as-param.rs:8:45 + --> $DIR/lint-temporary-cstring-as-param.rs:9:45 | LL | some_function(CString::new("").unwrap().as_ptr()); | ------------------------- ^^^^^^ this pointer will be invalid