Array and Vec's Clone specialization is maybe unsound with conditionally Copy types. #132442

theemathas · 2024-11-01T03:30:18Z

Currently, the Clone impl for [T; N] uses specialization. If T implements Copy, then cloning a [T; N] will do a memcpy (ignoring T's Clone impl). If T doesn't implement Copy, then it will iterate and call T's Clone impl.

However, specialization doesn't look at lifetimes. Therefore, even if T implements Copy for only some lifetimes, cloning a [T; N] will do a memcopy. This is incorrect in the case where T actually doesn't implement Copy. For example:

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

impl Clone for Weird<'_> {
    fn clone(&self) -> Self {
        println!("clone() called");
        Weird(self.0)
    }
}

impl Copy for Weird<'static> {}

fn main() {
    let local = 1;
    let _ = [Weird(&local)].clone();
}

In the above code, Weird only implements Copy when its lifetime is 'static. Therefore, I think that cloning a [Weird<'a>; 1] should call the clone() method. However, running the above code in either stable (1.82.0) or nightly (1.84.0-nightly (2024-10-30 759e07f063fb8e6306ff)) rust doesn't print anything. I believe that this is incorrect.

I am unsure whether or not this can cause UB in purely safe code, (hence the "maybe" in the issue title). But maybe someone else could figure out how to do that?

However, I have written this code, which uses unsafe code to implement a WeirdCow type whose public API I believe is sound on its own, but can be used in combination with array's Clone specialization to cause use-after-free. Either this WeirdCow type is unsound, or the array's Clone implementation is unsound, and I can't figure out which.

@rustbot label +I-unsound

The text was updated successfully, but these errors were encountered:

ChayimFriedman2 · 2024-11-01T03:48:47Z

I believe it was discussed before, and the conclusion was that the specialization is okay and it's invalid to implement Copy depending on lifetimes, and the compiler should probably lint/err at it.

scottmcm · 2024-11-01T05:25:53Z

@theemathas Is this at all specific to the array impl? Can you do the same thing with Vec, which has the same specialization?

rust/library/alloc/src/slice.rs

Line 156 in a8e1186

impl<T: Copy> ConvertVec for T {

theemathas · 2024-11-01T06:10:26Z

Can you do the same thing with Vec

Yes. This code also doesn't print anything.

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

impl Clone for Weird<'_> {
    fn clone(&self) -> Self {
        println!("clone() called");
        Weird(self.0)
    }
}

impl Copy for Weird<'static> {}

fn main() {
    let local = 1;
    let _ = vec![Weird(&local)].clone();
}

Noratrieb · 2024-11-01T06:54:44Z

Implementing copy conditionally on lifetimes is nonsensical and unsupported. MIR building will lower all moves of such a value to copy's, which borrowck will then error about when it's not static.
This impl should be an error (which is probably super hard, the same way sound specialization is).

hanna-kruppe · 2024-11-01T09:27:06Z

More discussion of impl Copy for Foo<'static>: #126179

lcnr · 2024-11-01T09:30:36Z

Related to #89948. I think that until we actually forbid such Copy impls, I strongly feel like this is a std bug and your WeirdCow should be a sound abstraction. We should not specialize on Copy and doing so is unsound as it ignores lifetime constraints.

Whether Copy with lifetime constraints makes your type pretty much unusable (it prevents ever moving it as all moves get upgraded to use Copy) does not feel relevant to whether std should be allowed to do this. It does provide a good argument for linting/forbidding the Copy impls, at which point std can specialization on Copy without using the unsound (from a type system POV) #[rustc_unsafe_specialization_marker] trait:

rust/compiler/rustc_typeck/src/impl_wf_check/min_specialization.rs

Lines 58 to 66 in 7fbd4ce

    
           //! ### rustc_unsafe_specialization_marker 
        
           //! 
        
           //! There are also some specialization on traits with no methods, including the 
        
           //! stable `FusedIterator` trait. We allow marking marker traits with an 
        
           //! unstable attribute that means we ignore them in point 3 of the checks 
        
           //! above. This is unsound, in the sense that the specialized impl may be used 
        
           //! when it doesn't apply, but we allow it in the short term since it can't 
        
           //! cause use after frees with purely safe code in the same way as specializing 
        
           //! on traits with methods can.

However, given that std does specialize on Copy and I am quite confident this that won't change in the medium-term, anything relying on lifetime constraints of Copy for soundness is not a sound abstraction until then. I do think we should keep this open as a bug until either specialization is sound wrt to lifetimes or we've changed these Copy impls to be an error.

theemathas · 2024-11-01T09:41:06Z

I found a somewhat popular crate (2K stars) that specifically depends on this Clone specialization existing, and depends on a Copy impl that is conditional on type equality.

https://github.com/AFLplusplus/LibAFL/blob/89cff637025c1652c24e8d97a30a2e3d01f187a4/libafl_bolts/src/tuples.rs#L27-L49

/// Returns if the type `T` is equal to `U`, ignoring lifetimes.

purplesyringa · 2024-11-01T09:42:52Z

Note that it's possible to avoid this specialization by using the typeid crate in that case (without losing optimizations). But code like this might easily be duplicated all over the ecosystem.

theemathas · 2024-11-01T09:54:49Z

lol, this issue has been known since 2020 in #71321

rust/library/core/src/marker.rs

Lines 408 to 413 in 9fa9ef3

    
           // 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.

purplesyringa · 2024-11-01T09:58:14Z

Note that there's plenty other uses of #[rustc_unsafe_specialization_marker]. None of the specializations call user-supplied code, but neither does the Copy spec and this issue still exists, so it might be time to consider the bigger picture. Removing one spec isn't going to fix all other misspecs.

lolbinarycat · 2024-11-02T01:43:59Z

It's also worth noting that there's nothing in the docs that says Copy and Clone shouldn't diverge. there probably should be a warning that this will cause weird problems.

hanna-kruppe · 2024-11-02T09:32:08Z

https://doc.rust-lang.org/std/clone/trait.Clone.html#how-can-i-implement-clone

Types that are Copy should have a trivial implementation of Clone. More formally: if T: Copy, x: T, and y: &T, then let x = y.clone(); is equivalent to let x = *y;. Manual implementations should be careful to uphold this invariant; however, unsafe code must not rely on it to ensure memory safety.

the8472 · 2024-11-02T11:04:44Z

I found a somewhat popular crate (2K stars) that specifically depends on this Clone specialization existing, and depends on a Copy impl that is conditional on type equality.

https://github.com/AFLplusplus/LibAFL/blob/89cff637025c1652c24e8d97a30a2e3d01f187a4/libafl_bolts/src/tuples.rs#L27-L49

/// Returns if the type T is equal to U, ignoring lifetimes.

We don't need to accommodate that though, since it's not part of our stable API. I have asked people to not propagate this hack without attaching the appropriate warnings, but it appears that I'm shouting into the void.

purplesyringa · 2024-11-02T13:13:27Z

People do that because it solves a problem other methods can't solve. Adding an intended solution to core would let people switch away from this workaround. Now, I'm not saying that's easy, and it'd require a better design and an RFC, but personally, I'd be fine with

macro_rules! implements_for_static {
    ($type:ty: $($trait:tt)*) => { /* compiler intrinsic */ };
};

such that implements_for_static!(T: Trait) if substituting all lifetimes in T for 'static results in a type that implements Trait. (I think this exactly matches the present specialization behavior, but I'm not 100% sure.)

RalfJung · 2024-11-03T16:20:04Z

it's invalid to implement Copy depending on lifetimes, and the compiler should probably lint/err at it.

I have not heard this as an official t-types opinion. If it is, we should start linting against this.

apiraino · 2024-11-04T10:28:16Z

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

jieyouxu · 2024-11-04T10:35:57Z

E-needs-investigation: should figure out if a decision was made previously and why, and if this should be linted against going forward.

jasondyoungberg · 2025-02-05T08:06:25Z

I have another example, where the Copy specialization is used to check at runtime if a lifetime is static. It's fine if this is non functional, but it shouldn't be unsound.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=bff81b6ec9c6899d864dabc850adea18

RalfJung · 2025-02-05T08:33:23Z

That's definitely unsound I am afraid -- any kind of dispatch-of-lifetime is. Just because things are useful doesn't mean they are magically sound.

addisoncrump · 2025-02-06T15:57:52Z

it appears that I'm shouting into the void.

Hi, I'm the person who implemented the type_eq in LibAFL. We have previously been told that we need to stop doing this, but without an alternative.

We have only depended on this behaviour out of necessity, and without the typeid crate being available when we implemented this originally, it was simply the only option we could come up with (that wasn't some horrible casting or unnecessary 'static bound). Now that the typeid crate is available, it is clearly superior given the const capability here and we are transitioning accordingly. We also had CI to make sure this method continued working (for our use case at least) so we weren't in any danger of having sudden incorrectness (we would've just yanked earlier versions).

As a reminder, we have historically used this because we absolutely need selective access to specialization. From our perspective, if core can do it, then we need to be able to as well. It would be even better if we could implement this properly with proper specialization, but as far as I know, specialization stops us with impl repeats parameter T and there isn't a way around this right now.

RalfJung · 2025-02-06T16:16:09Z

From our perspective, if core can do it, then we need to be able to as well.

libcore (and the sysroot as a whole) gets special privileges no other crate gets -- so this is absolutely not an acceptable argument. We try to keep this at a minimum, but it cannot always be fully avoided. If this social contract starts eroding, that does not bode well for our ability to keep evolving the language.

addisoncrump · 2025-02-06T16:22:01Z

I understand that, at this time, there are certain features which cannot be guaranteed to be safe and are thus restricted. It is very frustrating, however, to have certain features be limited when they apply a non-negotiable constraint into what we can implement (or, at least, the level of optimisation with which we can do so) and this is really unfortunate. I understand from a design perspective, but from a user perspective, this can be very frustrating.

I think this is mostly off-topic for this thread, so I'll move away here, but if there's more to be said I'm happy to follow up in the Zulip.

RalfJung · 2025-02-06T16:27:10Z

Believe me, it is equally frustrating for me that we don't have specialization. :) However, it is also frustrating to see people abuse the compiler with ~~obtuse~~ fragile hacks that hamper the further development of the language.

(EDIT: I looked up that word and it doesn't mean what I thought it means, sorry for that.)

You can't make a feature "non-negotiable" that just doesn't exist in Rust. There are many things I'd like to make non-negotiable about all sorts of things in life, but that doesn't mean I get to have them. ;)

workingjubilee · 2025-02-06T18:04:30Z

Code that has deliberately bypassed stability guarantees, knowing that it depends on unstable details, is not subject to stability guarantees.

We are permitted to break your code tomorrow.

workingjubilee · 2025-02-06T18:11:54Z

As a reminder, we have historically used this because we absolutely need selective access to specialization. From our perspective, if core can do it, then we need to be able to as well.

If you want the powers of core's internals, then you get the stability promises of core's internals:

None.

purplesyringa · 2025-02-06T18:25:04Z

I understand the emotions here, but can we please get a bit less angry at people looking for alternatives and a bit more solution-oriented, especially on what has apparently become a tracking issue of sorts?

As mentioned above, typeid should work fine for AFL's usecases. I'm still looking for a generic trait resolution mechanism (see above: bikeshed_macro!(T: Trait) returning a boolean indicating if replacing all lifetimes in T with 'static produces a type that implements Trait), but I understand if that's not planned at the moment. If I remember correctly, the buggy implementations have been found by hand using GitHub regex search. Can anything be done on the rustc side to get a better estimate of the effect removing this will have with a crater run?

workingjubilee · 2025-02-06T18:44:47Z

This issue will probably be closed without everyone finding a satisfactory alternative for whatever hack they implemented this way. That seems like it will be true whether it gets closed because we made these implementations illegal or removed the Copy specialization inside libcore, or whether it gets closed because we decide to do nothing.

That suggests those matters would be new issues.

If any of the dependent concerns are distinct, anyways, it would be easier to keep track of them that way, I think.

stop specializing on `Copy` fixes rust-lang#132442 `std` specializes on `Copy` to optimize certain library functions such as `clone_from_slice`. This is unsound, however, as the `Copy` implementation may not be always applicable because of lifetime bounds, which specialization does not take into account; the result being that values are copied even though they are not `Copy`. For instance, this code: ```rust struct SometimesCopy<'a>(&'a Cell<bool>); impl<'a> Clone for SometimesCopy<'a> { fn clone(&self) -> Self { self.0.set(true); Self(self.0) } } impl Copy for SometimesCopy<'static> {} let clone_called = Cell::new(false); // As SometimesCopy<'clone_called> is not 'static, this must run `clone`, // setting the value to `true`. let _ = [SometimesCopy(&clone_called)].clone(); assert!(clone_called.get()); ``` should not panic, but does ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6be7a48cad849d8bd064491616fdb43c)). To solve this, this PR introduces a new `unsafe` trait: `TrivialClone`. This trait may be implemented whenever the `Clone` implementation is equivalent to copying the value (so e.g. `fn clone(&self) -> Self { *self }`). Because of lifetime erasure, there is no way for the `Clone` implementation to observe lifetime bounds, meaning that even if the `TrivialClone` has stricter bounds than the `Clone` implementation, its invariant still holds. Therefore, it is sound to specialize on `TrivialClone`. I've changed all `Copy` specializations in the standard library to specialize on `TrivialClone` instead. Unfortunately, the unsound `#[rustc_unsafe_specialization_marker]` attribute on `Copy` cannot be removed in this PR as `hashbrown` still depends on it. I'll make a PR updating `hashbrown` once this lands. With `Copy` no longer being considered for specialization, this change alone would result in the standard library optimizations not being applied for user types unaware of `TrivialClone`. To avoid this and restore the optimizations in most cases, I have changed the expansion of `#[derive(Clone)]`: Currently, whenever both `Clone` and `Copy` are derived, the `clone` method performs a copy of the value. With this PR, the derive macro also adds a `TrivialClone` implementation to make this case observable using specialization. I anticipate that most users will use `#[derive(Clone, Copy)]` whenever both are applicable, so most users will still profit from the library optimizations. Unfortunately, Hyrum's law applies to this PR: there are some popular crates which rely on the precise specialization behaviour of `core` to implement "specialization at home", e.g. [`libAFL`](https://github.com/AFLplusplus/LibAFL/blob/89cff637025c1652c24e8d97a30a2e3d01f187a4/libafl_bolts/src/tuples.rs#L27-L49). I have no remorse for breaking such horrible code, but perhaps we should open other, better ways to satisfy their needs – for example by dropping the `'static` bound on `TypeId::of`...

theemathas · 2025-03-01T12:57:37Z

I present to you, a type which unconditionally doesn't implement Copy (for some definition of "unconditional"), and yet specialization thinks that it does implement Copy.

trait IsStatic {}
impl<'a: 'static> IsStatic for &'a () {}

struct NotCopy(i32);

impl Copy for NotCopy where for<'a> &'a (): IsStatic {}

impl Clone for NotCopy {
    fn clone(&self) -> Self {
        println!("cloned");
        NotCopy(self.0)
    }
}

fn main() {
    // does not print anything
    let _ = [NotCopy(1)].clone();
}

stop specializing on `Copy` fixes rust-lang#132442 `std` specializes on `Copy` to optimize certain library functions such as `clone_from_slice`. This is unsound, however, as the `Copy` implementation may not be always applicable because of lifetime bounds, which specialization does not take into account; the result being that values are copied even though they are not `Copy`. For instance, this code: ```rust struct SometimesCopy<'a>(&'a Cell<bool>); impl<'a> Clone for SometimesCopy<'a> { fn clone(&self) -> Self { self.0.set(true); Self(self.0) } } impl Copy for SometimesCopy<'static> {} let clone_called = Cell::new(false); // As SometimesCopy<'clone_called> is not 'static, this must run `clone`, // setting the value to `true`. let _ = [SometimesCopy(&clone_called)].clone(); assert!(clone_called.get()); ``` should not panic, but does ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6be7a48cad849d8bd064491616fdb43c)). To solve this, this PR introduces a new `unsafe` trait: `TrivialClone`. This trait may be implemented whenever the `Clone` implementation is equivalent to copying the value (so e.g. `fn clone(&self) -> Self { *self }`). Because of lifetime erasure, there is no way for the `Clone` implementation to observe lifetime bounds, meaning that even if the `TrivialClone` has stricter bounds than the `Clone` implementation, its invariant still holds. Therefore, it is sound to specialize on `TrivialClone`. I've changed all `Copy` specializations in the standard library to specialize on `TrivialClone` instead. Unfortunately, the unsound `#[rustc_unsafe_specialization_marker]` attribute on `Copy` cannot be removed in this PR as `hashbrown` still depends on it. I'll make a PR updating `hashbrown` once this lands. With `Copy` no longer being considered for specialization, this change alone would result in the standard library optimizations not being applied for user types unaware of `TrivialClone`. To avoid this and restore the optimizations in most cases, I have changed the expansion of `#[derive(Clone)]`: Currently, whenever both `Clone` and `Copy` are derived, the `clone` method performs a copy of the value. With this PR, the derive macro also adds a `TrivialClone` implementation to make this case observable using specialization. I anticipate that most users will use `#[derive(Clone, Copy)]` whenever both are applicable, so most users will still profit from the library optimizations. Unfortunately, Hyrum's law applies to this PR: there are some popular crates which rely on the precise specialization behaviour of `core` to implement "specialization at home", e.g. [`libAFL`](https://github.com/AFLplusplus/LibAFL/blob/89cff637025c1652c24e8d97a30a2e3d01f187a4/libafl_bolts/src/tuples.rs#L27-L49). I have no remorse for breaking such horrible code, but perhaps we should open other, better ways to satisfy their needs – for example by dropping the `'static` bound on `TypeId::of`...

theemathas added the C-bug label Nov 1, 2024

rustbot added needs-triage I-unsound I-prioritize labels Nov 1, 2024

jieyouxu added T-compiler T-libs labels Nov 1, 2024

theemathas changed the title ~~Array's Clone specialization is maybe unsound with conditionally Copy types.~~ Array and Vec's Clone specialization is maybe unsound with conditionally Copy types. Nov 1, 2024

jieyouxu added A-specialization and removed needs-triage labels Nov 1, 2024

rustbot added P-medium and removed I-prioritize labels Nov 4, 2024

jieyouxu added the E-needs-investigation label Nov 4, 2024

joboet linked a pull request Jan 17, 2025 that will close this issue

stop specializing on Copy #135634

Open

theemathas mentioned this issue Feb 6, 2025

Specialization hack might stop working in future rust versions. AFLplusplus/LibAFL#2945

Closed

theemathas mentioned this issue Mar 1, 2025

Unions require Copy fields, but ignore regions in the check #98102

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Array and Vec's Clone specialization is maybe unsound with conditionally Copy types. #132442

Array and Vec's Clone specialization is maybe unsound with conditionally Copy types. #132442

theemathas commented Nov 1, 2024

ChayimFriedman2 commented Nov 1, 2024

scottmcm commented Nov 1, 2024

theemathas commented Nov 1, 2024

Noratrieb commented Nov 1, 2024 •

edited

Loading

hanna-kruppe commented Nov 1, 2024

lcnr commented Nov 1, 2024 •

edited

Loading

theemathas commented Nov 1, 2024 •

edited

Loading

purplesyringa commented Nov 1, 2024 •

edited

Loading

theemathas commented Nov 1, 2024

purplesyringa commented Nov 1, 2024 •

edited

Loading

lolbinarycat commented Nov 2, 2024

hanna-kruppe commented Nov 2, 2024

the8472 commented Nov 2, 2024

purplesyringa commented Nov 2, 2024

RalfJung commented Nov 3, 2024

apiraino commented Nov 4, 2024

jieyouxu commented Nov 4, 2024

jasondyoungberg commented Feb 5, 2025

RalfJung commented Feb 5, 2025

addisoncrump commented Feb 6, 2025

RalfJung commented Feb 6, 2025

addisoncrump commented Feb 6, 2025

RalfJung commented Feb 6, 2025 •

edited

Loading

workingjubilee commented Feb 6, 2025

workingjubilee commented Feb 6, 2025

purplesyringa commented Feb 6, 2025 •

edited

Loading

workingjubilee commented Feb 6, 2025

theemathas commented Mar 1, 2025 •

edited

Loading

Array and Vec's Clone specialization is maybe unsound with conditionally Copy types. #132442

Array and Vec's Clone specialization is maybe unsound with conditionally Copy types. #132442

Comments

theemathas commented Nov 1, 2024

ChayimFriedman2 commented Nov 1, 2024

scottmcm commented Nov 1, 2024

theemathas commented Nov 1, 2024

Noratrieb commented Nov 1, 2024 • edited Loading

hanna-kruppe commented Nov 1, 2024

lcnr commented Nov 1, 2024 • edited Loading

theemathas commented Nov 1, 2024 • edited Loading

purplesyringa commented Nov 1, 2024 • edited Loading

theemathas commented Nov 1, 2024

purplesyringa commented Nov 1, 2024 • edited Loading

lolbinarycat commented Nov 2, 2024

hanna-kruppe commented Nov 2, 2024

the8472 commented Nov 2, 2024

purplesyringa commented Nov 2, 2024

RalfJung commented Nov 3, 2024

apiraino commented Nov 4, 2024

jieyouxu commented Nov 4, 2024

jasondyoungberg commented Feb 5, 2025

RalfJung commented Feb 5, 2025

addisoncrump commented Feb 6, 2025

RalfJung commented Feb 6, 2025

addisoncrump commented Feb 6, 2025

RalfJung commented Feb 6, 2025 • edited Loading

workingjubilee commented Feb 6, 2025

workingjubilee commented Feb 6, 2025

purplesyringa commented Feb 6, 2025 • edited Loading

workingjubilee commented Feb 6, 2025

theemathas commented Mar 1, 2025 • edited Loading

Noratrieb commented Nov 1, 2024 •

edited

Loading

lcnr commented Nov 1, 2024 •

edited

Loading

theemathas commented Nov 1, 2024 •

edited

Loading

purplesyringa commented Nov 1, 2024 •

edited

Loading

purplesyringa commented Nov 1, 2024 •

edited

Loading

RalfJung commented Feb 6, 2025 •

edited

Loading

purplesyringa commented Feb 6, 2025 •

edited

Loading

theemathas commented Mar 1, 2025 •

edited

Loading