Skip to content

Commit

Permalink
Add a new lint for catching unbound lifetimes in return values
Browse files Browse the repository at this point in the history
This lint is based on some unsoundness found in Libra during security
audits. Some function signatures can look sound, but based on the concrete
types that end up being used, lifetimes may be unbound instead of anchored to
the references of the arguments or the fields of a struct. When combined with
unsafe code, this can cause transmuted lifetimes to be not what was intended.
  • Loading branch information
metajack committed Dec 18, 2019
1 parent c62396d commit 5a06127
Show file tree
Hide file tree
Showing 12 changed files with 255 additions and 25 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,7 @@ Released 2018-09-13
[`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err
[`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
[`type_repetition_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds
[`unbound_return_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#unbound_return_lifetimes
[`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
[`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
[`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are 340 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are 341 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:

Expand Down
6 changes: 6 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#![cfg_attr(feature = "deny-warnings", deny(warnings))]
#![feature(crate_visibility_modifier)]
#![feature(concat_idents)]
#![feature(matches_macro)]

// FIXME: switch to something more ergonomic here, once available.
// (Currently there is no way to opt into sysroot crates without `extern crate`.)
Expand Down Expand Up @@ -289,6 +290,7 @@ pub mod transmuting_null;
pub mod trivially_copy_pass_by_ref;
pub mod try_err;
pub mod types;
pub mod unbound_return_lifetimes;
pub mod unicode;
pub mod unsafe_removed_from_name;
pub mod unused_io_amount;
Expand Down Expand Up @@ -769,6 +771,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
&types::UNIT_CMP,
&types::UNNECESSARY_CAST,
&types::VEC_BOX,
&unbound_return_lifetimes::UNBOUND_RETURN_LIFETIMES,
&unicode::NON_ASCII_LITERAL,
&unicode::UNICODE_NOT_NFC,
&unicode::ZERO_WIDTH_SPACE,
Expand Down Expand Up @@ -936,6 +939,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
store.register_late_pass(|| box trait_bounds::TraitBounds);
store.register_late_pass(|| box comparison_chain::ComparisonChain);
store.register_late_pass(|| box mul_add::MulAddCheck);
store.register_late_pass(|| box unbound_return_lifetimes::UnboundReturnLifetimes);
store.register_early_pass(|| box reference::DerefAddrOf);
store.register_early_pass(|| box reference::RefInDeref);
store.register_early_pass(|| box double_parens::DoubleParens);
Expand Down Expand Up @@ -1299,6 +1303,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
LintId::of(&types::UNIT_CMP),
LintId::of(&types::UNNECESSARY_CAST),
LintId::of(&types::VEC_BOX),
LintId::of(&unbound_return_lifetimes::UNBOUND_RETURN_LIFETIMES),
LintId::of(&unicode::ZERO_WIDTH_SPACE),
LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
Expand Down Expand Up @@ -1546,6 +1551,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
LintId::of(&types::CAST_PTR_ALIGNMENT),
LintId::of(&types::CAST_REF_TO_MUT),
LintId::of(&types::UNIT_CMP),
LintId::of(&unbound_return_lifetimes::UNBOUND_RETURN_LIFETIMES),
LintId::of(&unicode::ZERO_WIDTH_SPACE),
LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
LintId::of(&unwrap::PANICKING_UNWRAP),
Expand Down
150 changes: 150 additions & 0 deletions clippy_lints/src/unbound_return_lifetimes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
use if_chain::if_chain;
use rustc::declare_lint_pass;
use rustc::hir::*;
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use rustc_session::declare_tool_lint;

use crate::utils::span_lint;

declare_clippy_lint! {
/// **What it does:** Checks for lifetime annotations on return values
/// which might not always get bound.
///
/// **Why is this bad?** If the function contains unsafe code which
/// transmutes to the lifetime, the resulting lifetime may live longer
/// than was intended.
///
/// **Known problems*: None.
///
/// **Example:**
/// ```rust
/// struct WrappedStr(str);
///
/// // Bad: unbound return lifetime causing unsoundness (e.g. when x is String)
/// fn unbound<'a>(x: impl AsRef<str> + 'a) -> &'a WrappedStr {
/// let s = x.as_ref();
/// unsafe { &*(s as *const str as *const WrappedStr) }
/// }
///
/// // Good: bound return lifetime is sound
/// fn bound<'a>(x: &'a str) -> &'a WrappedStr {
/// unsafe { &*(x as *const str as *const WrappedStr) }
/// }
/// ```
pub UNBOUND_RETURN_LIFETIMES,
correctness,
"unbound lifetimes in function return values"
}

declare_lint_pass!(UnboundReturnLifetimes => [UNBOUND_RETURN_LIFETIMES]);

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnboundReturnLifetimes {
fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) {
if let ItemKind::Fn(ref sig, ref generics, _id) = item.kind {
check_fn_inner(cx, &sig.decl, generics, None);
}
}

fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx ImplItem) {
if let ImplItemKind::Method(ref sig, _) = item.kind {
let parent_generics = impl_generics(cx, item.hir_id);
check_fn_inner(cx, &sig.decl, &item.generics, parent_generics);
}
}
}

fn check_fn_inner<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
decl: &'tcx FnDecl,
generics: &'tcx Generics,
parent_generics: Option<(&'tcx Generics, Option<&'tcx Generics>, Option<&'tcx Generics>)>,
) {
let output_type = if let FunctionRetTy::Return(ref output_type) = decl.output {
output_type
} else {
return;
};

let lifetime = if let TyKind::Rptr(ref lifetime, _) = output_type.kind {
lifetime
} else {
return;
};

if matches!(lifetime.name, LifetimeName::Param(_)) {
let target_lifetime = lifetime;

// check function generics
// the target lifetime parameter must appear on the left of some outlives relation
if lifetime_outlives_something(target_lifetime, generics) {
return;
}

// check parent generics
// the target lifetime parameter must appear on the left of some outlives relation
if let Some((ref parent_generics, _, _)) = parent_generics {
if lifetime_outlives_something(target_lifetime, parent_generics) {
return;
}
}

// check type generics
// the target lifetime parameter must be included in the struct
if let Some((_, _, Some(ref typ_generics))) = parent_generics {
if typ_generics.get_named(target_lifetime.name.ident().name).is_some() {
return;
}
}

// check arguments
// the target lifetime parameter must be included as a lifetime of a reference
for input in decl.inputs.iter() {
if let TyKind::Rptr(ref lifetime, _) = input.kind {
if lifetime.name == target_lifetime.name {
return;
}
}
}

span_lint(
cx,
UNBOUND_RETURN_LIFETIMES,
target_lifetime.span,
"lifetime is unconstrained",
);
}
}

fn lifetime_outlives_something<'tcx>(target_lifetime: &'tcx Lifetime, generics: &'tcx Generics) -> bool {
if let Some(param) = generics.get_named(target_lifetime.name.ident().name) {
if param
.bounds
.iter()
.any(|b| matches!(b, GenericBound::Outlives(_)))
{
return true;
}
}
false
}

fn impl_generics<'tcx>(
cx: &LateContext<'_, 'tcx>,
hir_id: HirId,
) -> Option<(&'tcx Generics, Option<&'tcx Generics>, Option<&'tcx Generics>)> {
let parent_impl = cx.tcx.hir().get_parent_item(hir_id);
if_chain! {
if parent_impl != CRATE_HIR_ID;
if let Node::Item(item) = cx.tcx.hir().get(parent_impl);
if let ItemKind::Impl(_, _, _, ref parent_generics, ref _trait_ref, ref ty, _) = item.kind;
then {
if let TyKind::Path(ref qpath) = ty.kind {
if let Some(typ_def_id) = cx.tables.qpath_res(qpath, ty.hir_id).opt_def_id() {
let typ_generics = cx.tcx.hir().get_generics(typ_def_id);
return Some((parent_generics, None, typ_generics))
}
}
}
}
None
}
9 changes: 8 additions & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub use lint::Lint;
pub use lint::LINT_LEVELS;

// begin lint list, do not remove this comment, it’s used in `update_lints`
pub const ALL_LINTS: [Lint; 340] = [
pub const ALL_LINTS: [Lint; 341] = [
Lint {
name: "absurd_extreme_comparisons",
group: "correctness",
Expand Down Expand Up @@ -2037,6 +2037,13 @@ pub const ALL_LINTS: [Lint; 340] = [
deprecation: None,
module: "trait_bounds",
},
Lint {
name: "unbound_return_lifetimes",
group: "correctness",
desc: "unbound lifetimes in function return values",
deprecation: None,
module: "unbound_return_lifetimes",
},
Lint {
name: "unicode_not_nfc",
group: "pedantic",
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/extra_unused_lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
dead_code,
clippy::needless_lifetimes,
clippy::needless_pass_by_value,
clippy::trivially_copy_pass_by_ref
clippy::trivially_copy_pass_by_ref,
clippy::unbound_return_lifetimes
)]
#![warn(clippy::extra_unused_lifetimes)]

Expand Down
8 changes: 4 additions & 4 deletions tests/ui/extra_unused_lifetimes.stderr
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
error: this lifetime isn't used in the function definition
--> $DIR/extra_unused_lifetimes.rs:14:14
--> $DIR/extra_unused_lifetimes.rs:15:14
|
LL | fn unused_lt<'a>(x: u8) {}
| ^^
|
= note: `-D clippy::extra-unused-lifetimes` implied by `-D warnings`

error: this lifetime isn't used in the function definition
--> $DIR/extra_unused_lifetimes.rs:16:25
--> $DIR/extra_unused_lifetimes.rs:17:25
|
LL | fn unused_lt_transitive<'a, 'b: 'a>(x: &'b u8) {
| ^^

error: this lifetime isn't used in the function definition
--> $DIR/extra_unused_lifetimes.rs:41:10
--> $DIR/extra_unused_lifetimes.rs:42:10
|
LL | fn x<'a>(&self) {}
| ^^

error: this lifetime isn't used in the function definition
--> $DIR/extra_unused_lifetimes.rs:67:22
--> $DIR/extra_unused_lifetimes.rs:68:22
|
LL | fn unused_lt<'a>(x: u8) {}
| ^^
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/needless_lifetimes.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
#![warn(clippy::needless_lifetimes)]
#![allow(dead_code, clippy::needless_pass_by_value, clippy::trivially_copy_pass_by_ref)]
#![allow(
dead_code,
clippy::needless_pass_by_value,
clippy::trivially_copy_pass_by_ref,
clippy::unbound_return_lifetimes
)]

fn distinct_lifetimes<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: u8) {}

Expand Down
Loading

0 comments on commit 5a06127

Please sign in to comment.