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

[theemathas]

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

[ChayimFriedman2]

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]

@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]

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]

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]

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

[lcnr]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

[jieyouxu]

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