Add a new lint for catching unbound lifetimes in return values #4908

metajack · 2019-12-17T15:49:44Z

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.

changelog: add lint: unbound_return_lifetimes

bjorn3 · 2019-12-17T15:53:59Z

For future reference: diem/diem#1949 is the PR which fixed the mentioned bug.

bjorn3 · 2019-12-17T16:01:16Z

clippy_lints/src/unbound_return_lifetimes.rs

+    /// struct WrappedStr(str);
+    /// fn foo<'a>(x: &'a str) -> &'a WrappedStr {
+    ///   unsafe { &*(x as *const str as *const WrappedStr) }
+    /// }


Could you move WrappedStr above the Bad comment and use it in bad function as return type for more symmetry between the two examples? Also could you please separate the } and // with a newline?

Maybe also say that when the bad foo takes a String, 'a can be equal to 'static?

Added an example note.

tesuji · 2019-12-17T19:30:25Z

clippy_lints/src/unbound_return_lifetimes.rs

+                        if param
+                            .bounds
+                            .iter()
+                            .any(|b| if let GenericBound::Outlives(_) = b { true } else { false })


Suggested change

.any(|b| if let GenericBound::Outlives(_) = b { true } else { false })

.any(|b| matches!(b, GenericBound::Outlives(_)))

Don't forge to append #![feature(matches_macro)] in crate root.

tesuji · 2019-12-17T19:31:58Z

clippy_lints/src/unbound_return_lifetimes.rs

+                if let Some(param) = generics.get_named(target_lifetime.name.ident().name) {
+                    if param
+                        .bounds
+                        .iter()
+                        .any(|b| if let GenericBound::Outlives(_) = b { true } else { false })
+                    {
+                        return;
+                    }
+                }


This block and below one is similar. Maybe split it to a closure or function.

bjorn3 · 2019-12-17T19:37:40Z

clippy_lints/src/unbound_return_lifetimes.rs

+    generics: &'tcx Generics,
+    parent_generics: Option<(&'tcx Generics, Option<&'tcx Generics>, Option<&'tcx Generics>)>,
+) {
+    if let FunctionRetTy::Return(ref typ) = decl.output {


Suggested change

if let FunctionRetTy::Return(ref typ) = decl.output {

let output_type = if let FunctionRetTy::Return(ref output_type) = decl.output {

output_type

} else {

return;

};

This reduces indentation of the code below.

bjorn3 · 2019-12-17T19:39:41Z

clippy_lints/src/unbound_return_lifetimes.rs

+    parent_generics: Option<(&'tcx Generics, Option<&'tcx Generics>, Option<&'tcx Generics>)>,
+) {
+    if let FunctionRetTy::Return(ref typ) = decl.output {
+        if let TyKind::Rptr(ref lifetime, _) = typ.kind {


metajack · 2019-12-18T16:12:28Z

Updated based on review comments and rebased onto master.

ghost · 2019-12-21T03:48:05Z

This is a great lint but is it actually detecting unbound lifetimes? According to the Rustonomicon, "Given a function, any output lifetimes that don't derive from inputs are unbounded." In the example fn unbound<'a>(x: impl AsRef<str> + 'a) -> &'a WrappedStr, isn't 'a actually an derived from the inputs. Also, in the original PR, I didn't see anyone actually use the term "unbounded lifetime". 🤔

--

I tested this lint on the top 50 most downloaded crates on crates.io. I got two warnings which seem to be false positives. Neither of them use unsafe code.

regex

warning: lifetime is unconstrained
  --> src/re_unicode.rs:62:31
   |
62 |     fn from(m: Match<'t>) -> &'t str {
   |                               ^^
   |

unicase

warning: lifetime is unconstrained
   --> src/lib.rs:298:13
    |
298 | into_impl!(&'a str);
    |             ^^
    |

dtolnay · 2019-12-21T22:32:55Z

In the example fn unbound<'a>(x: impl AsRef<str> + 'a) -> &'a WrappedStr, isn't 'a actually an derived from the inputs?

No; the bound in x: impl AsRef<str> + 'a means the owner of x is allowed to keep x alive at most as long as 'a if they want. It does not mean x outlives 'a or lives at least as long as 'a. It means actually pretty close to the opposite of that. Deriving an output reference, which is required to live at least as long as 'a, from a value which lives at most as long as 'a, is unsound.

dtolnay · 2019-12-21T22:42:43Z

I can confirm that both https://github.com/rust-lang/regex/blob/e36670a90585b940b0d4f780a053c6db1be7b863/src/re_unicode.rs#L62 and https://github.com/seanmonstar/unicase/blob/7b116bc70c16b972a06c2e084ad1e9f3edfbaa2f/src/lib.rs#L298 are false positives. There should be enough information available to Clippy to make the lint not trigger in these cases.

Separately, it may be worth considering only triggering this lint if the function body contains an unsafe block.

bors · 2019-12-22T09:56:01Z

☔ The latest upstream changes (presumably #4930) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2019-12-24T13:52:06Z

☔ The latest upstream changes (presumably #4885) made this pull request unmergeable. Please resolve the merge conflicts.

llogiq · 2019-12-25T19:48:30Z

@dtolnay I'd lint all cases, even those without unsafe code, as you cannot be sure if there is unsafe code calling into the function elsewhere.

dtolnay · 2019-12-25T21:22:51Z

@llogiq it's your call, but I feel quite strongly that this lint would be better to trigger only on functions containing unsafe code. Clippy's most severe flaw in my experience has been noisy low-signal lints that are enabled by default (and I have deleted or downgraded several). This lint's point is to catch memory unsafety from incorrectly constrained output lifetimes. A function containing only safe code isn't going to have that memory unsafety, regardless of whether it is called from a different function that is unsafe. I think this lint is too important to make it low-signal.

Regarding the false positives:

In regex the 't lifetime is bound in the function argument. I have not looked at the lint implementation but I don't know why it would be triggering on that case.
In unicase the 'a lifetime is bound in the function argument, whose type is Self i.e. Unicase<&'a str>.

llogiq · 2019-12-26T00:49:58Z

@dtolnay fair enough.

I believe the false positives are due to the lint checking HIR types, which don't carry typeck information.

Getting the ty::Ty's instead will likely solve the problem.

metajack · 2020-01-08T20:09:53Z

I'll look into updating the PR to fix these false positives.

@mikerite Did you run clippy on those 50 crates manually, or is there some way to do that automatically? I was looking around for a good way to do that and hadn't come across anything yet.

flip1995 · 2020-01-09T13:48:19Z

You can use our integration testing framework for this:

INTEGRATION=serde-rs/serde cargo test --test integration --features integration

runs clippy on serde for example. Note, that this suppresses the output of Clippy. You can get the output, by just adding a println!("{}", stderr) here:

rust-clippy/tests/integration.rs

Line 45 in 50403de

let stderr = String::from_utf8_lossy(&output.stderr);

To test your use case, I also recommend to only enable the lint, you want to test here:

rust-clippy/tests/integration.rs

Lines 39 to 40 in 50403de

    
           "-Wclippy::pedantic", 
        
           "-Wclippy::nursery",

(-Aclippy::all and -Wclippy::unbound_return_lifetimes; Maybe this doesn't work though... #4778)

metajack · 2020-01-09T17:01:43Z

I found a couple of cases where I wasn't properly looking at all the parent impl type's lifetimes, so I rewrote the code to use visitors which fixed the problem and simplified the code quite a bit. I've also rebased this on master.

With these changes rust-lang/regex and seanmonstar/unicase are both clean.

metajack · 2020-01-09T17:03:36Z

@flip1995 I cannot seem to get the integration test to ever fail. I can check out a crate myself, see that my lint fails on it, and then run the integration test on the same crate and watch it succeed. The println you suggested does not seem cause any output to show up running the test.

flip1995 · 2020-01-09T17:49:22Z

The integration test is supposed not to fail on lint failures, since it only checks for ICEs. I just suggested this as a hack, I don't know how @mikerite did this. Do you get any output when adding the println!? You could try adding a panic!("{}", stderr). That should get printed.

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.

metajack · 2020-01-10T02:12:11Z

@lzutao @bjorn3 I think this is ready to go now.

ghost · 2020-01-10T03:55:40Z

I found an old internals thread where this example of a safe unbound lifetime was given:

fn foo<'a>() -> &'a str {
    "string"
}

Does this get linted? I can't check right now because by master toolchain installation is broken.

@mikerite Did you run clippy on those 50 crates manually, or is there some way to do that automatically? I was looking around for a good way to do that and hadn't come across anything yet.

I have a really ugly app that scrapes crates.io for the top crates, clones the source with git and runs clippy on it. I'd have to seriously clean it up before I'd feel comfortable sharing it.

metajack · 2020-01-13T21:45:51Z

@mikerite For that I think I agree with @llogiq that linting that case seems fine. Thanks for the pointer to this thread.

flip1995

I didn't follow this PR very closely. @llogiq can you review it, please?

Only thing, I noticed is the tests/ui/unbound_return_lifetimes.stdout file, which should be deleted, before merging.

llogiq · 2020-01-18T20:05:31Z

I'll see if I find the time for a review tomorrow.

llogiq · 2020-01-19T22:01:01Z

clippy_lints/src/unbound_return_lifetimes.rs

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


What about lifetimes in the output type generics, e.g. Cow<'x, str>? Shouldn't those be hamdled, too?

Also what about arrays or tuples containing refs?

llogiq · 2020-01-19T22:02:59Z

tests/ui/unbound_return_lifetimes.rs

+
+struct FooStr(str);
+
+fn unbound_fun<'a>(x: impl AsRef<str> + 'a) -> &'a FooStr {


I'd like to see a test with a &'static _ return type, to rule out false positives.

llogiq · 2020-01-19T22:04:14Z

clippy_lints/src/unbound_return_lifetimes.rs

+    cx: &LateContext<'a, 'tcx>,
+    decl: &'tcx FnDecl<'tcx>,
+    generics: &'tcx Generics<'tcx>,
+    parent_data: Option<(&'tcx Generics<'tcx>, &'tcx Ty<'tcx>)>,


Can inner functions refer to the lifetimes of outer functions? If yes, going one level up the stack might not be enough.

llogiq · 2020-01-19T22:06:50Z

tests/ui/unbound_return_lifetimes.rs

+fn bound_fun<'a>(x: &'a str) -> &'a FooStr {
+    unsafe { &*(x as *const str as *const FooStr) }
+}
+


Also perhaps some methods which return tuples or opaque types (impl trait) would be nice.

llogiq · 2020-01-19T22:08:46Z

tests/ui/unbound_return_lifetimes.rs

+    K: Hash + PartialEq,
+{
+    pub fn or_insert(&self, key: K, value: V) -> &'a V {
+        unreachable!()


Also how about a function where the output lifetime is wrong (say it should be 'a, but is 'b, which is unbound)?

bors · 2020-01-20T18:01:48Z

☔ The latest upstream changes (presumably #5067) made this pull request unmergeable. Please resolve the merge conflicts.

phansch · 2020-04-15T19:15:30Z

Hi @metajack, are you still up to finish this PR? Let us know if you have any further questions =)

metajack · 2020-04-15T19:55:22Z

Yes, I've just been busy with other stuff. I'll put some time aside soon to get this over the finish line.

flip1995 · 2020-05-25T15:35:54Z

Thanks for contributing to Clippy! Sadly this PR was not updated in quite some time. If you waited on input from a reviewer, we're sorry that this fell under the radar. If you want to continue to work on this, just reopen the PR and/or ping a reviewer.

bjorn3 reviewed Dec 17, 2019

View reviewed changes

metajack force-pushed the return-unbound-lifetime branch from 4e321b6 to 6e5fdee Compare December 17, 2019 19:26

tesuji reviewed Dec 17, 2019

View reviewed changes

bjorn3 suggested changes Dec 17, 2019

View reviewed changes

metajack force-pushed the return-unbound-lifetime branch 2 times, most recently from c3c1f58 to 4744356 Compare December 18, 2019 16:12

metajack force-pushed the return-unbound-lifetime branch 2 times, most recently from 5a06127 to df9e86c Compare December 18, 2019 16:55

phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 18, 2019

flip1995 added the A-lint Area: New lints label Jan 8, 2020

metajack force-pushed the return-unbound-lifetime branch from df9e86c to d64a77b Compare January 9, 2020 16:59

metajack force-pushed the return-unbound-lifetime branch from d64a77b to 80d2f78 Compare January 10, 2020 01:54

flip1995 reviewed Jan 18, 2020

View reviewed changes

llogiq reviewed Jan 19, 2020

View reviewed changes

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 15, 2020

flip1995 closed this May 25, 2020

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 25, 2020

	.any(\|b\| if let GenericBound::Outlives(_) = b { true } else { false })
	.any(\|b\| matches!(b, GenericBound::Outlives(_)))


		struct FooStr(str);

		fn unbound_fun<'a>(x: impl AsRef<str> + 'a) -> &'a FooStr {

Add a new lint for catching unbound lifetimes in return values #4908

Add a new lint for catching unbound lifetimes in return values #4908

Conversation

metajack commented Dec 17, 2019

bjorn3 commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metajack commented Dec 18, 2019

ghost commented Dec 21, 2019 • edited by ghost Loading

dtolnay commented Dec 21, 2019

dtolnay commented Dec 21, 2019

bors commented Dec 22, 2019

bors commented Dec 24, 2019

llogiq commented Dec 25, 2019

dtolnay commented Dec 25, 2019 • edited Loading

llogiq commented Dec 26, 2019

metajack commented Jan 8, 2020

flip1995 commented Jan 9, 2020

metajack commented Jan 9, 2020

metajack commented Jan 9, 2020

flip1995 commented Jan 9, 2020

metajack commented Jan 10, 2020

ghost commented Jan 10, 2020

metajack commented Jan 13, 2020

flip1995 left a comment

Choose a reason for hiding this comment

llogiq commented Jan 18, 2020

llogiq Jan 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 20, 2020

phansch commented Apr 15, 2020

metajack commented Apr 15, 2020

flip1995 commented May 25, 2020

ghost commented Dec 21, 2019 •

edited by ghost

Loading

dtolnay commented Dec 25, 2019 •

edited

Loading

llogiq Jan 19, 2020 •

edited

Loading