Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid reported unsoundness for implied lifetime bounds #12471

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

YpeKingma
Copy link

changelog: [lifetimes_bound_nested_ref]: Avoid some unsoundness for implied lifetime bounds

The lints here are unusual in that they are related to a compiler bug.
This zulip discussion was my starting point:
https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/cve-rs

In the discussion, this overview on implied bounds was mentioned:
https://github.com/rust-lang/rustc-dev-guide/blob/478a77a902f64e5128e7164e4e8a3980cfe4b133/src/traits/implied-bounds.md

and these are reported compiler bugs:

rust-lang/rust#25860
Implied bounds on nested references + variance = soundness hole
rust-lang/rust#25860 (comment)

rust-lang/rust#84591
HRTB on subtrait unsoundly provides HTRB on supertrait with weaker implied bounds

rust-lang/rust#100051
Implied bounds from projections in impl header can be unsound

One lint here suggests to add a generic lifetime bound for each function argument and return value
that is a nested reference with lifetimes, rust-lang/rust#25860 .

This generic lifetime bound is implied by this nested reference,
the nested reference lifetime should outlive the initial reference lifetime.

Without the added lifetime bound the current compiler (1.76.0) can generate unsound code,
as reported above.
However when the generic lifetime bound implied by the nested reference is added,
the compiler produces an error message when the function is used.
Therefore this lint can help to avoid the unsoundness in this case.

The other lint here is the reverse of the first one.
It suggests to delete this generic lifetime bound because it is implied.
This lint can help to clean up code, but only after the corresponding unsoundness is fixed
in the compiler.
I have added the reverse lint because of the unusual situation here.

For the other two cases
rust-lang/rust#84591 and
rust-lang/rust#100051 ,
the situation is similar: when manually adding a generic lifetime bound that is implied
by a nested reference, the compiler gives an error instead of producing unsound code.

Pending the review here, I want to continue with similar pairs of lints for these other two cases.

Is it necessary to document the error messages that the compiler gives when
the generic lifetime bounds are added in the reported examples?

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 12, 2024
@Manishearth
Copy link
Member

r? @oli-obk would you be willing to review this?

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2024

Failed to set assignee to oli-obk: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be too noisy to use, because it's only a problem when the lifetimes are used again in other arguments and return types, and I don't actually know the details of where it will be problematic. Maybe at minimum check that the lifetimes are used again at all

let TyKind::Ref(mut lifetime, mut mut_ty) = ty.kind else {
return implied_bounds;
};
while let TyKind::Ref(nested_lifetime, nested_mut_ty) = mut_ty.ty.kind {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these references may be nested behind other types, too (e.g. &'a (&'b i32, u32)). I'm also not sure whether references are the only things that exhibit this behaviour. There can be struct Foo<'a>(&'a ()) that then get used as &'a Foo<'b>.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also this doesn't handle type aliases. It may be easier to do this analysis on the ty datastructures instead of on the hir datastructures. You can get the ty::FnSig via cx.tcx.fn_sig(local_def_id).

Copy link
Author

@YpeKingma YpeKingma Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also this doesn't handle type aliases. It may be easier to do this analysis on the ty datastructures instead of on the hir datastructures. You can get the ty::FnSig via cx.tcx.fn_sig(local_def_id).

I'll try and use the ty::FnSig instead of the things from hir to collect the implied bounds. In the example &'a (&'b i32, u32) I see that this will probably need recursion, and implementing that might take a while.

Having more implied bounds could add to the noise...

@YpeKingma
Copy link
Author

I think this will be too noisy to use, because it's only a problem when the lifetimes are used again in other arguments and return types, and I don't actually know the details of where it will be problematic.

Adding any number of implicit lifetime bounds is correct (but should not be necessary, see the reverse lint.)
When this happens too often it could become noisy, and then this lint might be extended to automatically apply the suggested addition.

I could try check how often this occurs by using cargo dogfood. I suppose that includes the source code of clippy,
and I see plenty of lifetimes in there.

The unknown problem in this case is the possible unsoundness when an implicit lifetimes bound is added and the compiler does not complain about the new code.

Maybe at minimum check that the lifetimes are used again at all

Checking for lifetimes that are not used again at all sounds like another lint, but I may misunderstand this.
I suppose this can occur for pub functions/... that are not used elsewhere.

In the reported pieces of code, after adding an implicit lifetime the compiler gives an error at later use.
A check for the possibility of a later compiler error could be added in the clippy run, but that might lead to false negatives when this check is not implemented correctly.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 12, 2024

Checking for lifetimes that are not used again at all sounds like another lint, but I may misunderstand this.

I mean to not emit the lint unless the lifetime appears twice in the signature.

As far as I understood it, that is how you can get the unsoundness. Not with a function that's just missing an implied bound on two lifetimes, but additionally has one (or both?) Lifetimes again elsewhere in the same signature.

I don't have a strong opinion on whether to land the lint as is or improve it. I think you'd want to first run it on a few crates to see if it is useful in practice, or if it mainly asks to annotate items that don't have any soundness concerns.

It's fine to land with lots of missed situations. I just worry about lots of false alarms.

ctx,
ADD_REDUNDANT_LIFETIMES_BOUND_NESTED_REF_FN,
generics.span,
&format!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one thing I think should happen before landing this is to make it a structured suggestion, so people can just rustfix their crates instead of having to manually adjust possibly hundreds of functions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I can get this to work as a structured suggestion.

@@ -0,0 +1,222 @@
/// Lints to help dealing with unsoundness due to a compiler bug described here:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting a thread to localize this discussion:

My question is about the motivation of including this into Clippy: If Clippy is able to detect those unsoundness issues, shouldn't this be implemented in the compiler and fix the unsoundness bugs?

One explanation for this approach that I see is, that this lint will have false positives (FPs), so implementing it in the compiler would restrict the language too much. Am I right in that assumption?

If this should be the case, putting the lint into a warn-by-default group might be hard. We definitely should run our lintcheck tool to look for FPs in popular crates.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting a thread to localize this discussion:

My question is about the motivation of including this into Clippy: If Clippy is able to detect those unsoundness issues, shouldn't this be implemented in the compiler and fix the unsoundness bugs?

Ideally yes, and I hope the compiler gets fixed before this is ready.
But the 25860 issue was reported in 2015...

One explanation for this approach that I see is, that this lint will have false positives (FPs), so implementing it in the compiler would restrict the language too much. Am I right in that assumption?

An implied bound is correct because it is implied by the compiler input code. Adding an implied bound is also correct, but it adds redundancy in the input code. I think there is no language restriction here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this isn't implemented in rustc, because it would require bounds in a lot of places, while the new solver that can fix this (citation needed) is still about a year out.

I just wonder how many ppl will use this lint. I don't want you to waste time writing a complex lint that then becomes mostly unused

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually adding the implied lifetimes bound fixes the three reported cases: the compiler then gives an error message for them. The compiler could also do this addition internally.

clippy_lints/src/lifetimes_bound_nested_ref.rs Outdated Show resolved Hide resolved
clippy_lints/src/lifetimes_bound_nested_ref.rs Outdated Show resolved Hide resolved
/// pub const fn lifetime_translator<'a, 'b: 'a, T>(val_a: &'a &'b (), val_b: &'b T) -> &'a T {...}
/// ```
#[clippy::version = "1.78.0"]
pub ADD_REDUNDANT_LIFETIMES_BOUND_NESTED_REF_FN,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those lint names are a mouthful.

Maybe: IMPLICIT_LIFETIME_BOUNDS here and EXPLICIT_* for the lint that removes those again?

Copy link
Author

@YpeKingma YpeKingma Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those lint names are a mouthful.

Maybe: IMPLICIT_LIFETIME_BOUNDS here and EXPLICIT_* for the lint that removes those again?

From back to front:

The _FN suffix is there to indicate to the 25860 issue. The idea was to start off with different suffixes for the three issues but merging the lints into the same name makes them easier to use, so I'll drop the suffix.

LIFETIME_BOUNDS is easier to read by not accurate: the lint gives an error message for a single bound,
and each bound has two lifetimes.

I'll change the lint names to use IMPLICIT instead of ADD_REDUNDANT, and similarly for the explicit and remove redundant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LIFETIME_BOUNDS is easier to read by not accurate

Please refer to the lint naming conventions, 2. paragraph:

The basic rule is: the lint name should make sense when read as "allow lint-name" or "allow lint-name items". For example, "allow deprecated items" and "allow dead_code" makes sense, while "allow unsafe_block" is ungrammatical (should be plural).


I think you can keep the _FN suffix to separate those lints, but I'm not convinced that the NESTED_REF really has to be part of the lint name.

@YpeKingma
Copy link
Author

Checking for lifetimes that are not used again at all sounds like another lint, but I may misunderstand this.

I mean to not emit the lint unless the lifetime appears twice in the signature.

As far as I understood it, that is how you can get the unsoundness. Not with a function that's just missing an implied bound on two lifetimes, but additionally has one (or both?) Lifetimes again elsewhere in the same signature.

I'll try and do that for the 25860 issue.
For the other two issues a function signature may not be sufficient to detect this.

I don't have a strong opinion on whether to land the lint as is or improve it. I think you'd want to first run it on a few crates to see if it is useful in practice, or if it mainly asks to annotate items that don't have any soundness concerns.

It's fine to land with lots of missed situations. I just worry about lots of false alarms.

I agree this requires careful treading.

@bors
Copy link
Contributor

bors commented Mar 17, 2024

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

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

@YpeKingma
Copy link
Author

YpeKingma commented Mar 17, 2024

Well, after some back and forth with CI/CD I don´t know the root cause of the last error in the Clippy Test / base (pull_request) above. It's in the code docs, so I'll leave it at that for now.

The actual code is for issue 25860:

I renamed the lints to IMPLICIT_LIFETIMES_BOUND_NESTED_REF and EXPLICIT_LIFETIMES_BOUND_NESTED_REF.
Still a mouth full, but a bit shorter than earlier with ADD_IMPLIED.... and REMOVE_EXPLICIT... .

The implicit lifetime bounds are now derived from the fn_sig() as suggested.
The recursion on Ty for this worked out well, but perhaps it can be done by a visitor instead,
see collect_nested_ref_implied_bounds().

For the rest, I changed the lifetime bound pair struct to use String for simplicity.
This assumes that the lifetime names are unique, i.e. they should be taken from a single scope.

Not yet done:

Check that a lifetime bound has a lifetime that occurs at least twice in the function signature,
before reporting the bound.
Should that check depend on which lifetime (long/outlived) is involved?

Make the lints a structured suggestion to allow automatic fixing, both ways.
This will need better dealing with the source spans, currently on the generics span is used.
The source span for an implied nested reference could also be useful in the error message.

Add test cases to have better coverage for the Ty recursion: tuple, array and slice types.

Remaining cases for lints:

// from https://github.com/rust-lang/rust/issues/100051
impl<'a: 'b, 'b> Extend<'a, 'b> for <&'b &'a () as Trait>::Type {...}
// impl<'a, 'b> Extend<'a, 'b> for <&'b &'a () as Trait>::Type {...}


// from https://github.com/rust-lang/rust/issues/84591
impl<'a: 'b, 'b> Subtrait<'a, 'b, &'b &'a ()> for MyStruct {}
// impl<'a, 'b> Subtrait<'a, 'b, &'b &'a ()> for MyStruct {}

How much code documentation is needed?

@YpeKingma YpeKingma requested a review from oli-obk March 17, 2024 22:14
@YpeKingma
Copy link
Author

The commits today for the other two went faster than I expected. Please include them for review.

@YpeKingma
Copy link
Author

cargo dev dogfood does not pass here, fixing...

@YpeKingma
Copy link
Author

YpeKingma commented Mar 20, 2024

cargo uitest fails nicely on useless_asref.rs line 23:

impl<'a, 'b, 'c> AsRef<&'a &'b &'c MoreRef> for MoreRef

with two error messages for missing implied bounds 'b: 'a and 'c: 'b.
I don't think that needs to be corrected.

I also made a local version with the IMPLICIT* lint in the category suspicious instead of nursery.
cargo lintcheck with local version did not make the clippy::implicit-lifetimes-bound-nested-ref lint show up in the stats,
so it could be that there is no noise from there.

@YpeKingma
Copy link
Author

YpeKingma commented Mar 21, 2024

A possible way to assign these lints to categories over longer periods of time, several releases around the compiler fix for the related unsoundness:

  • implicit-lifetimes-bound-nested-ref : first nursery to try, then pedantic, then (may be) suspicious, then compiler fix, then deprecated.
  • explicit-lifetimes-bound-nested-ref : first nursery to wait, then compiler fix, then complexity.

The pedantic category fits nicely with the implication here.

@YpeKingma
Copy link
Author

The Clippy Test / base is failing because I missed a cargo uibless. Fixing...

@YpeKingma
Copy link
Author

Rustc was updated to 1.77.0 today, and I'm using it now.
How do I deal with this update here?
Can I merge master into the lifetimes_bound_nested_ref branch, or should I squash the update here?

@flip1995
Copy link
Member

flip1995 commented Mar 22, 2024

Running this on the top 500 crates by downloads, triggered the lint 4 times:

clippy 0.1.78 (8732557 2024-03-20)

Reports

target/lintcheck/sources/clap_builder-4.5.2/src/parser/features/suggestions.rs:38:32 clippy::implicit_lifetimes_bound_nested_ref "missing lifetime bound declation: '_: '_"
target/lintcheck/sources/httparse-1.8.0/src/lib.rs:903:22 clippy::implicit_lifetimes_bound_nested_ref "missing lifetime bound declation: '_: '_"
target/lintcheck/sources/httparse-1.8.0/src/lib.rs:916:27 clippy::implicit_lifetimes_bound_nested_ref "missing lifetime bound declation: 'b: 'a"
target/lintcheck/sources/httparse-1.8.0/src/lib.rs:936:29 clippy::implicit_lifetimes_bound_nested_ref "missing lifetime bound declation: '_: '_"

Stats:

lint count
clippy::implicit_lifetimes_bound_nested_ref 4

I don't have the time to look into those rn.

@YpeKingma YpeKingma changed the title Avoid some unsoundness for implied lifetime bounds Avoid reported unsoundness for implied lifetime bounds Mar 22, 2024
@YpeKingma
Copy link
Author

Running this on the top 500 crates by downloads, triggered the lint 4 times:

...

Thank you.

The reported bounds with empty lifetime names indicate a bug here.
I'll get these two crates to use them as test cases, and then:

  • try and recompile after manually adding this reported bound: httparse-1.8.0/src/lib.rs:916:27
  • investigate the reporting of empty lifetime names.

@YpeKingma
Copy link
Author

This is most likely the source code for the reported missing bounds (currently at line 1027 in src/lib.rs of the httparse crate):

unsafe fn deinit_slice_mut<'a, 'b, T>(s: &'a mut &'b mut [T]) -> &'a mut &'b mut [MaybeUninit<T>] {
    let s: *mut &mut [T] = s;
    let s = s as *mut &mut [MaybeUninit<T>];
    &mut *s
}

Since the argument and return type have the same reference structure, I would not expect this to cause unsoundness.

Shall I add a condition for the lint here that the implied bounds from input arguments and from the return type are not the same?

@flip1995
Copy link
Member

Shall I add a condition for the lint here that the implied bounds from input arguments and from the return type are not the same?

If it can't cause unsoundness issues and your motivated to add a check for it, I'd say sure, go for it.


Sidenote: there's a typo in the error message: declation

@YpeKingma
Copy link
Author

YpeKingma commented Mar 22, 2024

This is most likely the source code for the reported missing bounds (currently at line 1027 in src/lib.rs of the httparse crate):

unsafe fn deinit_slice_mut<'a, 'b, T>(s: &'a mut &'b mut [T]) -> &'a mut &'b mut [MaybeUninit<T>] {
    let s: *mut &mut [T] = s;
    let s = s as *mut &mut [MaybeUninit<T>];

I added the implied bound manually on its current master, and it still compiles and tests cleanly:

ype@v110:~/gitrepos/httparse$ git diff -U0
diff --git a/src/lib.rs b/src/lib.rs
index 3232852..457f225 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1027 +1027 @@ fn parse_headers_iter<'a>(
-unsafe fn deinit_slice_mut<'a, 'b, T>(s: &'a mut &'b mut [T]) -> &'a mut &'b mut [MaybeUninit<T>] {
+unsafe fn deinit_slice_mut<'a, 'b: 'a, T>(s: &'a mut &'b mut [T]) -> &'a mut &'b mut [MaybeUninit<T>] {
&mut *s
> }
> ```

@xFrednet
Copy link
Member

Ping @flip1995, do you still want to give this PR a review or reroll?

@flip1995
Copy link
Member

Let's reroll. I enjoyed the nice weather over the weekend instead and won't have time this week either.

r? clippy

I really want to give this another look though. But merging shouldn't be blocked by me.

@rustbot rustbot assigned Manishearth and unassigned flip1995 Jun 26, 2024
@Manishearth
Copy link
Member

r? clippy

also rerolling, tad busy today

@rustbot rustbot assigned Jarcho and unassigned Manishearth Jun 26, 2024
@YpeKingma YpeKingma force-pushed the lifetimes_bound_nested_ref branch from 3f0f0dc to bb6d11d Compare July 10, 2024 20:11
@YpeKingma
Copy link
Author

YpeKingma commented Jul 10, 2024

On today's push: see the 3 commit messages.

Only one CI failure is left, and it indicates an unclosed delimiter for the docs of the main function. However I still cannot find the root cause for this failure.

@YpeKingma YpeKingma force-pushed the lifetimes_bound_nested_ref branch from 865401c to 979051b Compare October 16, 2024 12:05
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 16, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

@YpeKingma YpeKingma force-pushed the lifetimes_bound_nested_ref branch from 979051b to ddf9aa9 Compare October 16, 2024 12:09
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 16, 2024
@YpeKingma
Copy link
Author

YpeKingma commented Oct 16, 2024

Today's push: no functional changes, only updates to follow the master branch.

Meanwhile there were some comments on the unsoundness bugs mentioned above.

I think the best next step would be to build this into the compiler to fix these bugs.
But I have no idea how to start with that, and I don't know how these recent comments relate to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants