prevent deref coercions in pin!

[dianne]

View all comments

Mitigates #153438 using a (hopefully temporary!) typed macro idiom to ensure that when pin! produces a Pin<&mut T>, its argument is of type T. See #153438 (comment) for my ideas on how this could be changed in the future.

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 15 candidates

[dianne]

I expect this needs crater, so: @bors try

[dianne]

Just realized I could format things more cleanly (diff). This shouldn't affect the correctness of the try build.

[eggyal]

A more explicit approach could be to do something like:

super let mut pinned = $value;

#[allow(unreachable_code)]
'p: {
    fn unreachable_type_constraint<'a, T>(_: T) -> Pin<&'a mut T> {
        unreachable!()
    }

    break 'p unsafe { Pin::new_unchecked(&mut pinned) };
    unreachable_type_constraint(pinned)
}

[dianne]

So that's how to add types to macros! Funky idiom, but I do prefer the explicitness of actually having types, yeah. I think that'd be less likely to break inference too if anything's started relying on it; iirc type expectations are propagated from the block to the break expression (via the block's coercion target type). But it's fine because we'll encounter an error instead of silently adding a coercion if things don't match up.. I think.

Would you prefer to make your own PR @eggyal, or should I just work that into this one? I'm not a library reviewer so I can't say which would be preferable from that perspective, but from the perspective I do have, I think adding a type constraint is a better fix.

[eggyal]

By all means work it into this one. This idiom was originally suggested to me by lcnr some years ago, so I claim no credit for it!

[Kivooeo]

this change requires someone from libs team to be approved, so i'm reassigning

r? libs

[rust-bors]

☀️ Try build successful (CI)
Build commit: 048f484 (048f484e31141edc1fa71e20406300e92a9c16ba, parent: 64b72a1fa5449d928d5f553b01a596b78ee255d2)

[dianne]

Went ahead with the unreachable type constraint version of this (diff). There's unfortunately a bit of a diagnostics regression due to the extra type checking. I still think it's worth the lower likelihood of breakage, but it's a tradeoff. I'll be firing off a fresh try build, but if the diagnostics are a sticking point, maybe it'd be worth comparing crater results for both approaches?

[dianne]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5d96fa0 (5d96fa0e954d77528204a1ba3b8847ec083c779b, parent: 64b72a1fa5449d928d5f553b01a596b78ee255d2)

[dianne]

I'll crater the type constraint approach first since that's my preferred quick fix at the moment.
@craterbot check

Also going to try re-running pr-check-2. It never restarted after being canceled it looks like?

[craterbot]

👌 Experiment pr-153457 created and queued.
🤖 Automatically detected try build 5d96fa0
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[theemathas]

I'm not yet convinced that this approach is 100% sound. Although I think it would be a good idea to do this fix even if it's not 100% sound, since any soundness exploit of this would be significantly more convoluted than the current exploit.

Given a value x of type Src, it might be possible with this PR for the expression pin!(x) to create a value of type Pin<&mut Dst>, where Src and Dst are different types. This can happen and cause unsoundness if:

• Src can be coerced to Dst (this coercion would be done in the parameter of unreachable_type_constraint); and
• &mut Src can be coerced to &mut Dst.(this coercion would be done in the parameter of Pin::new_unchecked); and
• Src and Dst are different types and have no subtyping relationship with each other; and
• Dst does not implement Unpin

I am not sure whether there exist types Src and Dst such that these two coercions can happen.

Edit: Added two more bullet points

Edit 2: Changed a requirement into the Unpin requirement.

[dianne]

@rustbot review

[jackh726]

Okay, so reading over the issue and this thread, I think the current approach is okay.

I'm not sure @theemathas's comment is applicable. I think there is an additional requisite that the pin!(...) call must actually constrain Dst (i.e. let x: Pin<&mut NotUnpin> = pin!(...);.

I was attempting to actually create a reproducer. It is definitely possible to have Src -> Dst and &mut Src -> &mut Dst both be possible - unsizing "just works" for example. Whether it's possible and exploitable I was trying to work on - and ran into a wall, and now I need to pause.

Some info on my experimentation

I have the start of a reproducer at https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=a3589e147b0b337b259a8f01da53fd79

I'm not entirely sure why we're getting a mismatched types error. I expected that the output would coerce first and then the arguments, so not sure what's going on.

---

All in all though, I think what we can do is fcp merge this for types. I don't know that I'd quite be happy to close the issue (because it still may be possible to exploit this), but this gets us much closer to soundness at the very least.

[theemathas]

It is definitely possible to have Src -> Dst and &mut Src -> &mut Dst both be possible - unsizing "just works" for example.

@jackh726 This is not true. If &mut Src could be coerced into a &mut Dst via an unsize coercion, then this implies that Dst does not implement Sized. Therefore, it is impossible for Src to be coerced into Dst, since coercion can't produce a non-Sized value.

[jackh726]

If &mut Src could be coerced into a &mut Dst via an unsize coercion, then this implies that Dst does not implement Sized.

This is true.

Therefore, it is impossible for Src to be coerced into Dst, since coercion can't produce a non-Sized value.

This is not.

This playground gets similarly close, just differently. In this, both &mut Src -> &mut Dst and Src -> Dst happen through unsizing: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=66147a3f63c3f584f5b02cf6fb2f4818

---

Actually, I think the most constrained rule to satisfy is the Src -> Dst coercion:

• As said, this can be done through unsizing trivially, but I do not think that there is any way that this could result in a Dst that is !Unpin but Src is, because both Unpin impls and unsizing must be fully generic and there exists no Unsize step that changes Unpin-ness
• That means that Src -> Dst must coerce through deref":

For a deref coercion to happen, Src and Dst must effectively be Src = &T and Dst = &U where T: Deref<Target = U>. Then, we now have an "updated" constraint that &mut &T coerces to &mut &U. We know that this coercion can't happen through deref (because there exists the &T: Deref<Target = T> impl. And, the unsize impl on &mut X requires X: Unsize, and &T: Unsize does not hold.

---

I'm not sure if I've missed something, but I am feeling fairly confident that the change in this PR is always sound w.r.t. "possible" coercions. I think this analyses does lead to constraints, which is that:

• we can never let users write an non-fully-generic Unpin impl - otherwise they could write impl !Unpin for Wrapper<[u8]> {} (with Wrapper<[u8; N]> holding), for example
• *we can never let users write "user-defined" unsizing - CoerceUnsized could never to be "non-generic" over the unsized data

[jackh726]

Alright, to move this forward:

I think we should just merge this. I'm a little on the fence of whether or not this needs an FCP. My gut feeling is "typically, seems like something we'd FCP" - that's probably on libs? I think types is probably a better reviewer - which I have (and I think @theemathas has thought through this a bunch too), but ultimately this is lib's territory.

However, that being said, this feels more like a "bug fix" for unintended behavior, than anything. So I'm more inclined to just merge without FCP (especially given that crater is clean).

As far as #153438, technically that could be closed once this is merged, since the reported unsoundness is gone. However, given the theoretical concern raised by @theemathas, my instinct is to leave it open even after merging this PR. Then, to ensure that issue doesn't stall open, I think a useful process here might be to fcp close types on that - with a checkbox essentially saying "I've thought about this enough that I don't think there's a theoretical issue".

This lets us merge this and fix at least the known unsoundness - and puts us on a concrete path to close the issue itself.

@dianne does this seem reasonable to you? If so, r=me in a day or so (to give people time to respond).

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[dianne]

I'm not familiar with T-libs or T-libs-api procedure, so I couldn't say there. The process I'm used to for language bug fixes is to get team approval via fcp merge, sometimes waiving the 10-day wait (though waiving the comment period might just be for beta regressions?).

Leaving #153438 open here and closing it via types FCP sounds reasonable to me! I've changed the description so it shouldn't automatically be closed.

Looking over the diagnostics again, I'm still not happy with how adding the extra function call affected those, but I think I've convinced myself that those are issues with the diagnostics, rather than the macro idiom. I can try and open a separate PR to make note_wrong_return_ty_due_to_generic_arg not leak implementation details of external macros. It already doesn't fire on calls from desugarings.

One last change, also: I've finally moved the function with the type signature of pin! into a separate helper so that it's not recreated for each pin! invocation. I'll once again hand this over to libs review to make sure I did that right. r? @Mark-Simulacrum since you've already reviewed this

[Mark-Simulacrum]

@bors r=Mark-Simulacrum,jackh726 rollup=iffy

View changes since this review

[rust-bors]

📌 Commit 5276fcd has been approved by Mark-Simulacrum,jackh726

It is now in the queue for this repository.