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

A Lifetime-generic Copy impl can allow fields which are only Copy when 'static #88901

Closed
danielhenrymantilla opened this issue Sep 12, 2021 · 5 comments · Fixed by #105102
Closed
Assignees
Labels
A-lifetimes Area: Lifetimes / regions A-traits Area: Trait system 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-medium Medium priority 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

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Sep 12, 2021

I tried this code:

#[derive(Clone)]
struct Foo<'lt>(&'lt ());

impl Copy for Foo<'static> {}

#[derive(Clone)]
struct Bar<'lt>(Foo<'lt>);

impl<'any> Copy for Bar<'any> {}

fn check(b: Bar<'_>) -> (Bar<'_>, Bar<'_>) {
    (b, b)
}
  • (Note about #[derive(Clone)]: while this simple snippet features a Clone impl for all lifetimes, it is possible to restrict the impl of Clone for Foo to 'static as well, and yet offer a Bar<'any> which is Cloneable (thanks to it being Copyable). See below for a more detailed example about that).

I expected to see this happen:

  • Either the impl Copy for Bar<'_> {} should have failed, or, at the very least, the borrow checker ought to have complained about the actual copy of b (from looking at the old related issue, a "late" / on-use borrow-checker issue was deemed good enough since such a check does prevent unsoundness).

    Alas, we can see here that that check can be dodged by using a wrapper type.

Instead, this happened:

  • the code compiled fine.

Meta

stable, beta and nightly are affected by this, and I suspect all versions of Rust do.

Impact

While contrived, this ought to be a I-unsound issue, since a library could feature a lifetime-generic type Foo<'lt> with an exploited safety invariant of Foo<'not_static> not being cloneable. Demo

  • That being said, I'd qualify it as low priority, since the example is indeed very contrived.

Bonus / tangential issue 🎁

A type which is only Copy when 'static becomes, in practice / in non-generic context, unmoveable when non-'static:

#[derive(Clone)]
struct Foo<'lt>(&'lt ());

impl Copy for Foo<'static> {}

fn check(f: Foo<'_>) {
    assert!(Some(f).is_some()); // Error, lifetime `'static` required
}
  • Playground

  • Indeed, the move_or_copy heuristic decides to go for copy since the type is indeed copy_modulo_regions, but then the borrow checker (correctly) forbids the Copy operation since the lifetime isn't (can't be proven to be) 'static

@danielhenrymantilla danielhenrymantilla added the C-bug Category: This is a bug. label Sep 12, 2021
@rustbot rustbot added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-traits Area: Trait system I-prioritize Issue: Indicates that prioritization has been requested for this issue. and removed I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-traits Area: Trait system labels Sep 12, 2021
@camelid camelid added the A-lifetimes Area: Lifetimes / regions label Sep 12, 2021
@steffahn
Copy link
Member

steffahn commented Sep 13, 2021

Specialization on Copy in the standard library has the same kind of issue. Your contrived weird_lib example can be exploited there as-well:

use weird_lib::*;

struct Wrapped<'a>(Foo<'a>);
impl Clone for Wrapped<'_> {
    fn clone(&self) -> Self {
        panic!("please don't reach this code!")
    }
}
// properly only on `'static`
impl Copy for Wrapped<'static> {}

fn main() {
    let s = Foo::with_new(|ref mut foo| {
        let mut x = vec![Wrapped(foo.take().unwrap())];
        let mut y = x.clone(); // <- doesn't actually call `clone` on `Wrapped` due to specialization
        Foo::proved_static(&mut x[0].0, &mut y[0].0)
    });
    println!("{}", s.into_inner());
}

(playground)

@apiraino
Copy link
Contributor

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

Also tagging T-lang as this issue could benefit from a opinion from the team (@danielhenrymantilla left some thoughts in the Zulip thread)

@rustbot label -I-prioritize +P-medium +T-lang

@rustbot rustbot added P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 14, 2021
@QuineDot
Copy link

Specialization on Copy in the standard library has the same kind of issue.

From PR 71321 which contains this comment block:

// FIXME(matthewjasper) This allows copying a type that doesn't implement
// `Copy` because of unsatisfied lifetime bounds (copying `A<'_>` when only
// `A<'static>: Copy` and `A<'_>: Clone`).
// We have this attribute here for now only because there are quite a few
// existing specializations on `Copy` that already exist in the standard
// library, and there's no way to safely have this behavior right now.
#[rustc_unsafe_specialization_marker]

@steffahn
Copy link
Member

steffahn commented Sep 19, 2021

@QuineDot my code example above goes slightly further and shows that this can (as a consequence) also allow copying a type that doesn't even implement Clone.

@compiler-errors
Copy link
Member

We talked about this in the T-types meetup today -- I'm gonna take a stab at changing the Copy impl check considering regions, and see what falls out of crater.

@bors bors closed this as completed in 94a300b Jan 21, 2023
flip1995 pushed a commit to flip1995/rust that referenced this issue Jan 27, 2023
…g-regions, r=lcnr

Check ADT fields for copy implementations considering regions

Fixes rust-lang#88901
r? `@ghost`
@compiler-errors compiler-errors added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions A-traits Area: Trait system 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-medium Medium priority 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
None yet
Development

Successfully merging a pull request may close this issue.

7 participants