Address review comments

This commit is contained in:
Nathan Whitaker 2020-09-21 16:32:28 -04:00
parent 737bfeffd2
commit 1bcd2452fe
6 changed files with 44 additions and 13 deletions

View file

@ -3,10 +3,7 @@ use crate::LateLintPass;
use crate::LintContext; use crate::LintContext;
use rustc_hir::{Expr, ExprKind, PathSegment}; use rustc_hir::{Expr, ExprKind, PathSegment};
use rustc_middle::ty; use rustc_middle::ty;
use rustc_span::{ use rustc_span::{symbol::sym, ExpnKind, Span};
symbol::{sym, Symbol},
ExpnKind, Span,
};
declare_lint! { declare_lint! {
pub TEMPORARY_CSTRING_AS_PTR, 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( fn lint_cstring_as_ptr(
cx: &LateContext<'_>, cx: &LateContext<'_>,
as_ptr_span: Span, as_ptr_span: Span,
@ -68,19 +63,21 @@ fn lint_cstring_as_ptr(
unwrap: &rustc_hir::Expr<'_>, unwrap: &rustc_hir::Expr<'_>,
) { ) {
let source_type = cx.typeck_results().expr_ty(source); 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 cx.tcx.is_diagnostic_item(sym::result_type, def.did) {
if let ty::Adt(adt, _) = substs.type_at(0).kind { if let ty::Adt(adt, _) = substs.type_at(0).kind() {
if cx.match_def_path(adt.did, &CSTRING_PATH) { if cx.tcx.is_diagnostic_item(sym::cstring_type, adt.did) {
cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, as_ptr_span, |diag| { cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, as_ptr_span, |diag| {
let mut diag = diag let mut diag = diag
.build("getting the inner pointer of a temporary `CString`"); .build("getting the inner pointer of a temporary `CString`");
diag.span_label(as_ptr_span, "this pointer will be invalid"); diag.span_label(as_ptr_span, "this pointer will be invalid");
diag.span_label( diag.span_label(
unwrap.span, 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(); diag.emit();
}); });
} }

View file

@ -400,6 +400,7 @@ symbols! {
crate_type, crate_type,
crate_visibility_modifier, crate_visibility_modifier,
crt_dash_static: "crt-static", crt_dash_static: "crt-static",
cstring_type,
ctlz, ctlz,
ctlz_nonzero, ctlz_nonzero,
ctpop, ctpop,

View file

@ -109,7 +109,9 @@ use crate::sys;
/// documentation of `CString` before use, as improper ownership management /// documentation of `CString` before use, as improper ownership management
/// of `CString` instances can lead to invalid memory accesses, memory leaks, /// of `CString` instances can lead to invalid memory accesses, memory leaks,
/// and other memory errors. /// and other memory errors.
#[derive(PartialEq, PartialOrd, Eq, Ord, Hash, Clone)] #[derive(PartialEq, PartialOrd, Eq, Ord, Hash, Clone)]
#[cfg_attr(not(test), rustc_diagnostic_item = "cstring_type")]
#[stable(feature = "rust1", since = "1.0.0")] #[stable(feature = "rust1", since = "1.0.0")]
pub struct CString { pub struct CString {
// Invariant 1: the slice ends with a zero byte and has a length of at least one. // Invariant 1: the slice ends with a zero byte and has a length of at least one.

View file

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

View file

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

View file

@ -4,14 +4,16 @@ error: getting the inner pointer of a temporary `CString`
LL | let s = CString::new("some text").unwrap().as_ptr(); 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 | 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 note: the lint level is defined here
--> $DIR/lint-temporary-cstring-as-ptr.rs:2:9 --> $DIR/lint-temporary-cstring-as-ptr.rs:2:9
| |
LL | #![deny(temporary_cstring_as_ptr)] 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 error: aborting due to previous error