squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Auto merge of #75671 - nathanwhit:cstring-temp-lint, r=oli-obk

Browse source 
Uplift `temporary-cstring-as-ptr` lint from `clippy` into rustc

The general consensus seems to be that this lint covers a common enough mistake to warrant inclusion in rustc.
The diagnostic message might need some tweaking, as I'm not sure the use of second-person perspective matches the rest of rustc, but I'd like to hear others' thoughts on that.

(cc #53224).

r? `@oli-obk`
This commit is contained in:
bors 2020-10-27 22:59:13 +00:00
parent 07e968b640 572cd358d3
commit 90e6d0d46b
15 changed files with 177 additions and 141 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
compiler/rustc_lint/src/lib.rs
View file

@ -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,
]
);
};

106
compiler/rustc_lint/src/methods.rs Normal file
View file

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

7
compiler/rustc_span/src/symbol.rs
View file

@ -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,

3
library/std/src/ffi/c_str.rs
View file

@ -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();

11
src/test/ui/lint/lint-temporary-cstring-as-param.rs Normal file
View file

@ -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`
}

18
src/test/ui/lint/lint-temporary-cstring-as-param.stderr Normal file
View file

@ -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

9
src/test/ui/lint/lint-temporary-cstring-as-ptr.rs Normal file
View file

@ -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`
}

18
src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr Normal file
View file

@ -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

6
src/tools/clippy/.github/driver.sh vendored
View file

@ -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

3
src/tools/clippy/clippy_lints/src/lib.rs
View file

@ -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),

56
src/tools/clippy/clippy_lints/src/methods/mod.rs
View file

@ -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));

1
src/tools/clippy/clippy_lints/src/utils/paths.rs
View file

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

7
src/tools/clippy/src/lintlist/mod.rs
View file

@ -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",

24
src/tools/clippy/tests/ui/cstring.rs
View file

@ -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()) }
}
}

46
src/tools/clippy/tests/ui/cstring.stderr
View file

@ -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