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

'static closures with non-'static return type are unsound #84366

Open
steffahn opened this issue Apr 20, 2021 · 10 comments
Open

'static closures with non-'static return type are unsound #84366

steffahn opened this issue Apr 20, 2021 · 10 comments
Labels
A-associated-items Area: Associated items such as associated types and consts. A-async-await Area: Async & Await A-closures Area: closures (`|args| { .. }`) A-lifetimes Area: lifetime related A-traits Area: Trait system A-typesystem Area: The type system AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority S-bug-has-test Status: A `known-bug` test has been added for this bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@steffahn
Copy link
Member

steffahn commented Apr 20, 2021

Argument-free closures can return non-'static types but be 'static themselves:

fn foo<'a>() {
    let closure = || -> &'a str { "" };
    assert_static(closure);
}

fn assert_static<T: 'static>(_: T) {}

However, associated types of 'static types must be 'static, too. So the fact that the above compiles results in unsoundness, as demonstrated below:

use std::fmt;
trait Trait {
    type Associated;
}

impl<R, F: Fn() -> R> Trait for F {
    type Associated = R;
}

fn static_transfers_to_associated<T: Trait + 'static>(
    _: &T,
    x: T::Associated,
) -> Box<dyn fmt::Display /* + 'static */>
where
    T::Associated: fmt::Display,
{
    Box::new(x) // T::Associated: 'static follows from T: 'static
}

fn make_static_displayable<'a>(s: &'a str) -> Box<dyn fmt::Display> {
    let f = || -> &'a str { "" };
    // problem is: the closure type of `f` is 'static
    static_transfers_to_associated(&f, s)
}

fn main() {
    let d;
    {
        let x = "Hello World".to_string();
        d = make_static_displayable(&x);
    }
    println!("{}", d);
}

(playground)

����IV�

@rustbot modify labels: T-compiler, T-lang, A-closures, A-lifetimes, A-traits, A-typesystem, A-associated-items
and someone please add “I-unsound 💥”.

Found this bug while experimenting with variations of #84305.

Without the dyn keyword, this seems to compile “successfully” since Rust 1.4.0. Godbolt link.

@steffahn steffahn added the C-bug Category: This is a bug. label Apr 20, 2021
@rustbot rustbot added A-closures Area: closures (`|args| { .. }`) A-lifetimes Area: lifetime related A-traits Area: Trait system A-typesystem Area: The type system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. A-associated-items Area: Associated items such as associated types and consts. labels Apr 20, 2021
@wesleywiser wesleywiser added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Apr 20, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 20, 2021
@steffahn
Copy link
Member Author

steffahn commented Apr 21, 2021

This also works with functions

fn make_static_displayable<'a>(s: &'a str) -> Box<dyn fmt::Display> {
    //let f = || -> &'a str { "" };
    fn f<'a>() -> &'a str { "" }
    // problem is: the type of `f::<'a>` is 'static
    static_transfers_to_associated(&f::<'a>, s)
}

but only since Rust 1.20. I don’t know if this would be better put as a separate issue since that’s probably a regression then.

1.19 gives

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> <source>:24:36
   |
24 |     static_transfers_to_associated(&f::<'a>, s)
   |                                    ^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime 'a as defined on the function body at 20:1...
  --> <source>:20:1
   |
20 | / fn make_static_displayable<'a>(s: &'a str) -> Box<fmt::Display> {
21 | |     //let f = || -> &'a str { "" };
22 | |     fn f<'a>() -> &'a str { "" }
23 | |     // problem is: the type of `f::<'a>` is 'static
24 | |     static_transfers_to_associated(&f::<'a>, s)
25 | | }
   | |_^
note: ...so that reference does not outlive borrowed content
  --> <source>:24:46
   |
24 |     static_transfers_to_associated(&f::<'a>, s)
   |                                              ^
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the type `fn() -> &str {make_static_displayable::f::<'a>}` will meet its required lifetime bounds
  --> <source>:24:5
   |
24 |     static_transfers_to_associated(&f::<'a>, s)
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error(s)

Compiler returned: 101

(compiler explorer)

@steffahn
Copy link
Member Author

The version with functions regressed (started compiling without error) at nightly-2017-06-29.

> rustc +nightly-2017-06-28 --version
rustc 1.20.0-nightly (f590a44ce 2017-06-27)
> rustc +nightly-2017-06-29 --version
rustc 1.20.0-nightly (69c65d296 2017-06-28)

This includes

4079e6128f6cff3270ed2b0ebdc62fb6b1f4b5d7 - Auto merge of #42417 - eddyb:separate-fn-sig, r=nikomatsakis
88c3242ef26956266970d62571a1d7d079b5799a - Auto merge of #42431 - nagisa:core-float-2, r=alexcrichton
5bc89416388f0a852f6e48542bf2fc021682fcb5 - Auto merge of #42709 - stepancheg:discriminant-hash, r=jseyfried
6b52a1162eff2469c0da88300577863e5cc0c78f - Auto merge of #42931 - arielb1:statement-visitor, r=eddyb
c16930762a4a7762b26579f7d44273d3bab4c11e - Auto merge of #42745 - sfackler:1.19-stabilization, r=alexcrichton
47faf1d51952ecd9d4c8a7325332fba34fbe00bd - Auto merge of #42819 - scottmcm:swap-nonoverlapping, r=sfackler
69c65d29615c391c958ebf75dd65258ec23e175c - Auto merge of #42850 - estebank:unwanted-return-rotj, r=nikomatsakis

Looking at those, almost certainly #42417 is the relevant PR.

@apiraino
Copy link
Contributor

Assigning priority as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 21, 2021
@steffahn
Copy link
Member Author

steffahn commented Apr 21, 2021

Finally figured out how to make a generic lifetime extension function out of this. Also not requiring dyn … anymore. Still works since Rust 1.4.0:

use std::marker::PhantomData;

trait FancyTrait<'b, T: ?Sized> {
    type Associated;
    fn convert(self, x: Self::Associated, proof: PhantomData<&'b Self::Associated>) -> &'b T;
}

impl<'a, 'b, T: ?Sized + 'a, F> FancyTrait<'b, T> for F
where
    F: Fn() -> &'a str,
{
    type Associated = &'a T;
    fn convert(self, x: &'a T, _: PhantomData<&'b &'a T>) -> &'b T {
        x
    }
}

fn extend_lifetime<'a, 'b, T: ?Sized>(x: &'a T) -> &'b T {
    // closure has inferred return type `&'a str`
    static_transfers_to_associated(|| "", x)
}

fn static_transfers_to_associated<'a, 'b, F, T: ?Sized>(f: F, x: F::Associated) -> &'b T
where
    F: FancyTrait<'b, T> + 'b,
{
    // `F: 'b` implies `F::Assocaited: 'b`
    f.convert(x, PhantomData) // callable since `&'b F::Associated` is a valid type
}

fn main() {
    let r;
    {
        let x = String::from("Hello World?");
        r = extend_lifetime(&x);
    }
    println!("{}", r);
}

@steffahn
Copy link
Member Author

steffahn commented Apr 29, 2021

For the record, async blocks do, unsurprisingly, come with the same problem:

fn foo<'a, F: Future<Output = &'a str> + 'static>(f: F) {}

fn bar<'a>() {
    let fut = async { "" };
    foo::<'a>(fut);
}

@rustbot label A-async-await

@rustbot rustbot added the A-async-await Area: Async & Await label Apr 29, 2021
@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Apr 30, 2021
@nikomatsakis
Copy link
Contributor

OK, I'm thinking about this issue. So, in a top-level function, it is considered "ok" for a lifetime to appear in the return type that does not also appear in the arguments, but that is only because it is forced to be early bound and hence it is part of the generics of the FnDef type:

fn foo<'a>() -> &'a str { "" }
// 'desugars' to the rough equivalent of `struct foo<'a>; impl<'a> Fn<()> for foo<'a> { .. }`

For closures, the return type (as #84385 points out) has not been considered part of the "outlives" relation, which means that it is effectively not part of the "self type" of the closure. However, that means that you wind up with an impl like

struct ClosureType;
impl<'a> Fn<()> for ClosureType {
    type Output = &'a str;
}

This impl would not normally be permitted and it violates RFC 1214, leading to this unsoundness.

The fix in #84385 is effectively to make the return type part of the self type:

struct ClosureType<R>;
impl<'a> Fn<()> for ClosureType<&'a str> {
    type Output = &'a str;
}

At first I was unsure if this was the fix I wanted, but now I think it's exactly right, as it preserves a fairly analogous relationship between the fn type and closure types.

@nikomatsakis
Copy link
Contributor

I believe the problem with async generators is the same, but for yield types. That fix should be included in #84385 as well.

@steffahn
Copy link
Member Author

steffahn commented May 4, 2021

fn foo<'a>() -> &'a str { "" }
// 'desugars' to the rough equivalent of `struct foo<'a>; impl<'a> Fn<()> for foo<'a> { .. }`

that “desugaring” however breaks down with the fact that type_of(foo::<'a>): 'static seems to be true, doesn’t it?

That fix should be included in #84385 as well.

Hmm, if I recall correctly, when I compiled and tested the PR branch of #84385 it happened to already fix the async example I gave. Edit: Seems like I might’ve been misunderstood what you were saying and this isn’t any news to you :'D. The PR didn’t fix the version with functions (instead of closures) though.

@nikomatsakis
Copy link
Contributor

@steffahn to be honest, I had forgotten about that, but I do remember now =) similar flaw. I want to think over the best fix a bit more in any case. Another option would be to impose smarter limits at the point where we are inferring the return types for closures.

@dtolnay dtolnay changed the title Closures are unsound: 'static closures with non-'static return types. 'static closures with non-'static return type are unsound Jan 17, 2022
@oli-obk oli-obk added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Oct 21, 2022
@pnkfelix
Copy link
Member

pnkfelix commented Dec 2, 2022

Visiting for P-high review. This is very much a T-types matter.

Officially assigning to @Aaron1011 based on their ongoing efforts in PR #84385.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 23, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? `@jackh726`

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? ``@jackh726``

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? `@jackh726`

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? ``@jackh726``

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? `@jackh726`

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? ``@jackh726``

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
@jackh726 jackh726 added the S-bug-has-test Status: A `known-bug` test has been added for this bug. label Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items such as associated types and consts. A-async-await Area: Async & Await A-closures Area: closures (`|args| { .. }`) A-lifetimes Area: lifetime related A-traits Area: Trait system A-typesystem Area: The type system AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority S-bug-has-test Status: A `known-bug` test has been added for this bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
Status: unknown
Development

Successfully merging a pull request may close this issue.

10 participants