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

Coercion to Box<fn()> or Box<dyn Fn()> ignores lifetime restrictions on method of struct with lifetime #85456

Closed
elidupree opened this issue May 19, 2021 · 11 comments
Labels
C-bug Category: This is a bug.

Comments

@elidupree
Copy link

elidupree commented May 19, 2021

In this code [playground]:

struct TemporaryStruct<'a>(&'a i32);

impl<'a> TemporaryStruct<'a> {
    fn query() {
        println!("TemporaryStruct queried");
    }
}

fn evil<'a>(_: &'a Box<i32>) -> Box<dyn Fn()> {
    Box::new(TemporaryStruct::<'a>::query)
}

fn main() {
    let evil_func = {
        let temporary = Box::new(5);
        evil(&temporary)
    };
    println!("lifetime of `temporary` is over");
    (evil_func)();
}

evil stores the method TemporaryStruct::<'a>::query in a Box<dyn Fn()>, which shouldn't be possible. (The same thing also happens if the return type is Box<fn()>.) I believe this is a soundness hole in stable Rust (1.52.1).

(The rest of the code is just to demonstrate how this is unsafe; main calls evil with 'a equal to the lifetime of the local variable temporary, then uses the boxed Fn to call the query method outside the lifetime of TemporaryStruct::<'a>. I haven't found a way to produce actual memory errors in safe code using this, but I've been assured that it's a violation of Rust's safety invariants.)

@elidupree elidupree added the C-bug Category: This is a bug. label May 19, 2021
@elidupree elidupree changed the title Coercion to Box<fn()>Box<dyn Fn()>` Box<dyn Fn()>` ignores lifetime restrictions on method of struct with lifetime Coercion to Box<fn()> or Box<dyn Fn()> ignores lifetime restrictions on method of struct with lifetime May 19, 2021
@elidupree
Copy link
Author

Note that if you make query a trait method rather than an inherent method, rustc gives the correct error message:

trait Query {
    fn query();
}
impl<'a> Query for TemporaryStruct<'a> {
    fn query() {
        println!("TemporaryStruct queried");
    }
}
fn evil<'a>(_: &'a Box<i32>) -> Box<dyn Fn()> {
    Box::new(TemporaryStruct::<'a>::query)
}
error[E0759]: `fn` parameter has lifetime `'a` but it needs to satisfy a `'static` lifetime requirement

But the error vanishes again if you either make the return type Box<fn()> or put the method in a closure:

fn evil<'a>(_: &'a Box<i32>) -> Box<fn()> {
    Box::new(TemporaryStruct::<'a>::query)
}
fn evil<'a>(_: &'a Box<i32>) -> Box<dyn Fn()> {
    Box::new(|| TemporaryStruct::<'a>::query())
}

@SkiFire13
Copy link
Contributor

I'm not quite sure this is unsound, TemporaryStruct::<'a>::query has no way to use 'a, so why should it be valid for 'a only?

@elidupree
Copy link
Author

elidupree commented May 19, 2021

My understanding is that it's a safety invariant that you cannot call a method outside the lifetime of its type, and thus, unsafe code is permitted to rely on the fact that this does not happen. (So if such unsafe code caused a memory error, that would be a bug in Rust rather than a bug in that unsafe code.) It would be nice to get confirmation of this from someone who has a deeper understanding of the safety guarantees, but I'm pretty confident that it's true. And I ran into this issue while attempting to rely on this invariant to avoid needing runtime checks, so it's not a useless invariant [edit: or maybe I didn't actually need it - see below], even if it's complicated to describe how to benefit from it.

Note that you could construct a value of type Self in that method, e.g. Self(&5) - which doesn't immediately cause a memory error, but does technically produce a value of a type outside of the lifetime of that type.

@SkiFire13
Copy link
Contributor

My understanding is that it's a safety invariant that you cannot call a method outside the lifetime of its type, and thus, unsafe code is permitted to rely on the fact that this does not happen.

AFAIK it's guaranteed that function items can be coerced to function pointers with the same signature, so your statement is true for methods because they carry the type signature due to the self parameter, but false for associated functions. Is this just your understanding or is it took from reference/nomicon?

And I ran into this issue while attempting to rely on this invariant to avoid needing runtime checks, so it's not a useless invariant, even if it's complicated to describe how to benefit from it.

It would be nice to have a practical example of how this could be used. Also, why do you need it, can't you add a struct LtToken<'a>(fn(&'a ())); and take it as parameter for your query function?

Note that you could construct a value of type Self in that method, e.g. Self(&5) - which doesn't immediately cause a memory error, but does technically produce a value of a type outside of the lifetime of that type.

It's still unclear how this could escape the function. Since it has to be valid for any lifetime it has no way to know if the lifetime is 'static or an invalid lifetime, so there's no way to exploit that.

@elidupree
Copy link
Author

so, what you're saying is that because TemporaryStruct::<'a>::query() is similar to a freestanding function query<'a>(), it can always coerce to a function with a static lifetime? I guess I always assumed that TemporaryStruct::<'a> (for some specific 'a) was formally a different type than TemporaryStruct::<'static>, and lifetime erasure was an implementation detail, so in particular, you shouldn't be able to do this:

fn evil<T: Query>() -> fn() {
    T::query
}

because the lifetime of T is given by the caller, so evil shouldn't be able to infer the existence of the type TemporaryStruct::<'static> just because it's been given the type TemporaryStruct::<'a> as a parameter. But maybe my understanding is wrong?

If this is intentionally permitted, can you explain why the trait-method example gives an error?

It would be nice to have a practical example of how this could be used. Also, why do you need it, can't you add a struct LtToken<'a>(fn(&'a ())); and take it as parameter for your query function?

Oh, right - at worst, I could have a ZST guard that carries the lifetime I need. (It's just less convenient for users - which isn't as important as reliability, of course - and I ran into this issue before I thought of that approach.)

@SkiFire13
Copy link
Contributor

so, what you're saying is that because TemporaryStruct::<'a>::query() is similar to a freestanding function query<'a>(), it can always coerce to a function with a static lifetime?

It can always coerce to a function pointer (not a function) with the same signature (fn()) which contains no lifetimes in parameters or return types, so its lifetime is 'static.

I guess I always assumed that TemporaryStruct::<'a> (for some specific 'a) was formally a different type than TemporaryStruct::<'static>

They are different.

because the lifetime of T is given by the caller, so evil shouldn't be able to infer the existence of the type TemporaryStruct::<'static> just because it's been given the type TemporaryStruct::<'a> as a parameter. But maybe my understanding is wrong?

It doesn't matter what's the lifetime of T, the existance of a TemporaryStruct::<'static> is not inferred. What happens is just that T::Query has signature fn(), so it can be coerced to a function pointer of type fn() which doesn't have any parameter or return type with lifetimes, thus is 'static and can be freely returned.

If this is intentionally permitted, can you explain why the trait-method example gives an error?

IMO that's the actual bug, it shouldn't give an error there. The cast to fn() there is perfectly valid, in fact if you write it explicitly (Box::new(TemporaryStruct::<'a>::query as fn())) it works.

@elidupree
Copy link
Author

Yeah, it would make sense to me if the error message was the real bug here, although I still have some doubts about what the rules are. After you mentioned the Reference and the Nomicon, I checked them, and I was thoroughly disappointed that neither of them gives any detail about the meaning of lifetimes of types.

I still can't convince myself that the coercion to fn() is theoretically justified. In the implementation of the query method, I can easily write

struct TemporaryStruct<'a>(PhantomData<(&'a mut &'a mut i32)>);

impl<'a> Query<'a> for TemporaryStruct<'a> {
    fn query() {
        let foo: TemporaryStruct<'a> = TemporaryStruct(PhantomData);
    }
}

which produces a value of type TemporaryStruct<'a>, so if this function is parameterized with a concrete lifetime and then invoked outside that lifetime, we've now produced a value that claims to have 'a when it didn't exist for any of 'a and exists now at a time outside 'a. That seems highly suspect, even if I can't think of how to exploit it. (And I've made the struct invariant in 'a, if I remember the variance rules right.) I guess the reason it's not exploitable is that the false claim of being valid for 'a doesn't cause any problems if we can't do anything with it during the actual 'a (since that's in the past)? (And on the flip side, being valid when it doesn't claim to be valid isn't going to cause any problems.)

Either way, for context, here's what I was trying to do originally. To be clear, I have no idea whether this is (or should be) legal; it was just something I was messing around with when I ran into this issue. The idea is to have a type that represents access to a specific thread_local; the implementor of LocalKeyUser cannot know that W lives any longer than the call to LocalKeyUser::call(), so they shouldn't be able to store it. I expected the type system to prevent them from storing it, but it did not. On the other hand, this version, which uses a ZST guard, gives an appropriate error if you attempt to store it.

@SkiFire13
Copy link
Contributor

which produces a value of type TemporaryStruct<'a>, so if this function is parameterized with a concrete lifetime and then invoked outside that lifetime, we've now produced a value that claims to have 'a when it didn't exist for any of 'a and exists now at a time outside 'a.

IMO unless some other code can observe the invalid lifetime or an invalid value with that lifetime then it is sound. However:

  • The lifetime is only a compile time concept, so it would have to escape through the type system, but it isn't possible with this behaviour
  • The value valid for that lifetime only can not escape that function since the only way would be through some global state, but that requires it to have a 'static lifetime.

Another argument could be that you can already create values with invalid lifetimes, for example by leaking memory, and that's also sound.

@elidupree
Copy link
Author

Another argument could be that you can already create values with invalid lifetimes, for example by leaking memory, and that's also sound.

As a small quibble, this is different than what you do by leaking. When you leak, you fail to run destructors, but it's still impossible to refer to the value after it's leaked - so you only have meaningless data lying around somewhere in memory which code will never interact with, not a concrete variable that has an out-of-scope lifetime written in it.

Your other points seem like they could make sense though. I think what we need is someone with a deeper theoretical understanding of the lifetime system, to explain the theoretical basis behind this oddity.

In any case, it seems like there's some bug here - if the original code is sound, then the error message in the trait case is probably a bug.

@QuineDot
Copy link

I think this is the same as #89667 (or vice-versa), where

  • RFC 1214 limits the lifetime of projections into traits (but not, apparently, concrete type implementations)
  • However RFC 401 allows them to coerce to (potentially 'static) function pointers (which are lifetime-limited by their parameters, but not the implementing type)
  • Closures can call a lifetime-limited functions, but still be 'static / implement Fn() statically / coerce to dyn Fn() + 'static

@ChrisDenton ChrisDenton added the needs-triage-legacy Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged. label Jul 16, 2023
@Enselic
Copy link
Member

Enselic commented Jul 7, 2024

Triage: Let's close this as duplicate to #89667 even if it is newer, because it is more to-the-point.

@Enselic Enselic closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2024
@Enselic Enselic removed the needs-triage-legacy Old issue that were never triaged. Remove this label once the issue has been sufficiently triaged. label Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

5 participants