prevent deref coercions in pin!

[dianne]

View all comments

Fixes #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]

Ooh, I hadn't considered that. Even if this is 100% sound, that means the argument for it is subtle enough that I'm not sure I'd want to rely on it over something simpler. Maybe going back to an inference-limiting approach (or combining approaches) would be best? That way we can be sure there's no coercions in the first place. For ease of comparison, the trick in my original version of this PR was

let pinned_ref = unsafe { $crate::pin::Pin::new_unchecked(&mut pinned) };
pinned_ref

Since the type of pinned_ref isn't known when checking the let statement, we won't get coercions in the argument of Pin::new_unchecked.

If that's too subtle, I think there are other ways to limit inference/coercions too. This is probably even hackier, but to have an example, a trick I've used for preventing coercions in tests is defining trait Is<T> {} with impl<T> Is<T> for T {}. Making a function take impl Is<T> instead of T makes calls of it unable to coerce that argument even if T is known, since impl Is<T> is a different parameter and only constrained later. To combine this with the type constraint approach, unreachable_type_constraint could take an Is<T> to prevent coercions there. A #[diagnostic::on_unimplemented] attribute could maybe prevent the diagnostics from getting too much worse if a trait-based approach is desired? I've never used the attribute myself though.

[danielhenrymantilla]

Nice, I like the helper function over merely relying on an extra let binding to prevent the deref coërcion

View changes since this review

[danielhenrymantilla]

This ought to be factored out in some macro-internals module of ::core, to reduce compiler overhead (we shouldn't be defining an ad-hoc fn for every pin! invocation).

• Tangentially, we may be able to avoid the unreachable_code warning and the 'p-labelled block with an if false { unreachable_type_constraint(pinned) } else { Pin::new_unchecked(&mut pinner) }, although this touches on a matter of taste and preference (there is some appeal in that expect, after all)

[dianne]

This ought to be factored out in some macro-internals module of ::core, to reduce compiler overhead (we shouldn't be defining an ad-hoc fn for every pin! invocation).

Good point! I've been holding off on addressing that to allow more time to discuss which approach would be best, but I'll make sure to do that if this ends up involving a helper function.

• Tangentially, we may be able to avoid the unreachable_code warning and the 'p-labelled block with an if false { unreachable_type_constraint(pinned) } else { Pin::new_unchecked(&mut pinner) }, although this touches on a matter of taste and preference (there is some appeal in that expect, after all)

As ugly as the labeled block and break are, I'd like for static analyses to see that the only non-panicking control-flow path is the one that goes through Pin::new_unchecked(&mut pinned), and unreachable_type_constraint is never called, nor is its argument moved. Not having a branch at all is conceptually the most straightforward way to do that, but an unreachable!() inside the if's unreachable branch may do it too; I think its control flow graph representation's only outgoing edge is for panicking, so the unreachable_type_constraint call would properly be unreachable.

[craterbot]

🚧 Experiment pr-153457 is now running

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

[craterbot]

🎉 Experiment pr-153457 is completed!
📊 1 regressed and 5 fixed (839795 total)
📊 2279 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-153457/retry-regressed-list.txt ↩

[theemathas]

Wait what? Why did crater not catch https://github.com/restatedev/restate/blob/a472bad4e12427012f6215947edd35561d62d0b3/crates/vqueues/src/scheduler/drr.rs#L563 ?

[Skgland]

Wait what? Why did crater not catch https://github.com/restatedev/restate/blob/a472bad4e12427012f6215947edd35561d62d0b3/crates/vqueues/src/scheduler/drr.rs#L563 ?

The create does not appear to be on crates.io (it has package.publish = "false") nor does the repo appear to be in https://github.com/rust-lang/rust-repos/blob/master/data/github.csv.

[camsteffen]

the trick in my original version of this PR was

let pinned_ref = unsafe { $crate::pin::Pin::new_unchecked(&mut pinned) };
pinned_ref

seems good enough to me

[robofinch]

• Src can be coerced to Dst (this coercion would be done in the parameter of unreachable_type_constraint); and

This might already have been adequately addressed, but just to note - the operand of &raw mut _ does not have any form of coercion applied to it, and a coercion from *mut *mut T to *mut *mut U is only possible if T is the same type as U (not merely in the “mutual subtypes” definition, but the “same TypeId”¹ definition). I believe the following would suffice:

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

    break 'p unsafe { Pin::new_unchecked(&mut pinned) };
    // The RHS of `let _ = _` is only coerced if the LHS is given a `: _` type ascription.
    // The operand of `&raw mut _` is not coerced.
    let mut raw_mut = &raw mut pinned;
    // Function arguments are coerced, but we make any non-identity coercions impossible.
    unreachable_type_constraint(&raw mut raw_mut)
}

Footnotes

1. Well, “same TypeId” only works for 'static types. TypeId exposes equality-by-subtyping vs normal-form-syntactic-equality unsoundness. #97156 uses the term “syntactic equality”, which seems good. ↩

[Mark-Simulacrum]

In general this feels like something ~T-types should perhaps review. I don't feel like I have the knowledge to say whether this aligns with our inference logic sufficiently well to guarantee the behavior we want is respected. From a libs perspective this seems fine, I'd be happy to r+, but I can't really say whether it accomplishes the goal or not.

(And I guess there's potentially Crater / FCP for the breaking change; it seems likely we want to accept it since it's a soundness fix, but knowing how much would be useful).

View changes since this review

[dianne]

In general this feels like something ~T-types should perhaps review. I don't feel like I have the knowledge to say whether this aligns with our inference logic sufficiently well to guarantee the behavior we want is respected.

In that case, r? types

(And I guess there's potentially Crater / FCP for the breaking change; it seems likely we want to accept it since it's a soundness fix, but knowing how much would be useful).

Crater looked clean last week for the unreachable type constraint idiom currently in the PR; results are at #153457 (comment). If one of the inference-limiting fixes I proposed in #153457 (comment) would be preferable, that'd need a fresh crater run; I imagine they have higher potential for breakage (though it'd be returning to previous behavior, so hopefully not too many people started relying on inference there!). @robofinch's version of the unreachable type constraint in #153457 (comment) probably doesn't break anything more than the current version, but it might warrant a fresh crater run anyway.

[dianne]

@rustbot review