Pin is unsound due to the literal constructor

[hxuhack]

The safe constructor Pin::new(pointer: Ptr) restricts the pointed object with the Unpin trait. However, this restriction can be bypassed using the literal constructor of Pin or the pin! macro.

Why not prevent this by making the literal constructor private and modifying the pin! macro as follows?

pub struct Pin<Ptr> {
    __pointer: Ptr,
}

macro_rules! safe_pin {
    ($value:expr) => {
        Pin::<&mut _>::new(&mut $value)
    };
}

To the best of my knowledge, the side effect is that API users can no longer create a Pin object using pin! if the object does not implement Unpin. However, developers can still use Pin::new_unchecked(pointer: Ptr) to achieve the same result. More importantly, I believe unsoundness must not be tolerated in safe Rust.

[y21]

I don't think the __pointer field is intended to be part of the public API. It's marked #[doc(hidden)] and to actually use the literal constructor of Pin and exploit this unsoundness, you need to enable the unsafe_pin_internals feature gate, which is also marked as an internal feature. If I remember right, the use of internal features outside of the compiler/stdlib isn't really supported (related MCP).

To the best of my knowledge, the side effect is that API users can no longer create a Pin object using pin! if the object does not implement Unpin.

This sounds like a huge limitation and breaking change. Afaik the pin! macro is only useful because it allows you to safely pin any !Unpin value locally by taking ownership over the value and lifetime extending the borrow.

Maybe "unsafe fields" as a language feature would be a better way to fix this theoretical unsoundness, but again, IMHO this is hardly an issue in practice

[hxuhack]

If I remember right, the use of internal features outside of the compiler/stdlib isn't really supported (rust-lang/compiler-team#620).

The following code compiles with the nightly Rust compiler.

#![feature(unsafe_pin_internals)]
#![allow(unused)]

use std::pin::Pin;
use std::marker::PhantomPinned;

struct Data {
    _pinned: PhantomPinned,
}

impl Data {
    fn new() -> Pin<Box<Self>> {
        Box::pin(Data {
            _pinned: PhantomPinned,
        })
    }
}

fn main() {
    let mut obj = Data::new();
    //let mut p = Pin::new(obj); //This is not allowed in safe Rust because of the type of filed _pinned is PhantomPinned.
    let mut p = Pin { __pointer: &mut obj }; // But we can bypass the check through the literal constructor.
}

It's marked #[doc(hidden)]

I strongly doubt that this is a serious (proper) way to handle the issue.

this is hardly an issue in practice

Yes, I agree that there is little chance of developers using it incorrectly. But soundness is a key competitive advantage of Rust over other languages, isn't it? I believe there should be a better way to support the required features without undermining the soundness of safe Rust.

[taiki-e]

I believe unsoundness must not be tolerated in safe Rust.

I don't think your example is safe Rust because it has the “unsafe” feature unsafe_pin_internals globally enabled.

(Although I think it it better to have lints such as internal_features and unsafe_code triggered against unsafe_pin_internals feature to make that more clear (which may have already been done).)

[hxuhack]

I don't think your example is safe Rust because it has the “unsafe” feature unsafe_pin_internals globally enabled.

Oh, yes. I hadn't noticed that there is a keyword unsafe in the feature name. Should we make a formal statement that if a feature name contains unsafe, it is equivalent to employing unsafe code and may compromise Rust’s safety guarantees?Or is there already such a clarification?

Even without the unsafe feature, we can bypass the safety check through the macro pin!. See the following code.

use std::pin::Pin;
use std::pin::pin;
use std::marker::PhantomPinned;

struct Data {
    _pinned: PhantomPinned,
}

impl Data {
    fn new() -> Pin<Box<Self>> {
        Box::pin(Data {
            _pinned: PhantomPinned,
        })
    }
}

fn main() {
    let mut obj = Data::new();
    //let mut p = Pin::new(obj); //This is not allowed in safe Rust because of the type of filed _pinned is PhantomPinned.
    //let mut p = Pin { __pointer: &mut obj }; // But we can bypass the check through the literal constructor. This line of code requires enabling the feature of unsafe_pin_internals.
    let p = pin!(obj); // Now we can bypass the check without any unsafe features. 
}

[taiki-e]

Even without the unsafe feature, we can bypass the safety check through the macro pin!. See the following code.

pin macro in your example is fine. That macro is implemented in a sound way called stack pinning. See the documentation: https://doc.rust-lang.org/nightly/core/pin/macro.pin.html

[hxuhack]

That macro is implemented in a sound way called stack pinning.

Can you explain in more detail why it is sound? Thanks!

[taiki-e]

That macro is implemented in a sound way called stack pinning.

Can you explain in more detail why it is sound? Thanks!

It takes ownership of the value, creates a temporary which inaccessible from the user, and creates a pinned reference from a reference to it. Unless the value implements Unpin, the user cannot obtain the normal mutable reference or ownership to the value without the unsafe code.

See the comment in pin macro for the complete explanation of what pin macro does:

rust/library/core/src/pin.rs

Lines 1948 to 2017 in ecb170a

	// This is `Pin::new_unchecked(&mut { $value })`, so, for starters, let's
	// review such a hypothetical macro (that any user-code could define):
	//
	// ```rust
	// macro_rules! pin {( $value:expr ) => (
	// match &mut { $value } { at_value => unsafe { // Do not wrap `$value` in an `unsafe` block.
	// $crate::pin::Pin::<&mut _>::new_unchecked(at_value)
	// }}
	// )}
	// ```
	//
	// Safety:
	// - `type P = &mut _`. There are thus no pathological `Deref{,Mut}` impls
	// that would break `Pin`'s invariants.
	// - `{ $value }` is braced, making it a _block expression_, thus **moving**
	// the given `$value`, and making it _become an **anonymous** temporary_.
	// By virtue of being anonymous, it can no longer be accessed, thus
	// preventing any attempts to `mem::replace` it or `mem::forget` it, _etc._
	//
	// This gives us a `pin!` definition that is sound, and which works, but only
	// in certain scenarios:
	// - If the `pin!(value)` expression is _directly_ fed to a function call:
	// `let poll = pin!(fut).poll(cx);`
	// - If the `pin!(value)` expression is part of a scrutinee:
	// ```rust
	// match pin!(fut) { pinned_fut => {
	// pinned_fut.as_mut().poll(...);
	// pinned_fut.as_mut().poll(...);
	// }} // <- `fut` is dropped here.
	// ```
	// Alas, it doesn't work for the more straight-forward use-case: `let` bindings.
	// ```rust
	// let pinned_fut = pin!(fut); // <- temporary value is freed at the end of this statement
	// pinned_fut.poll(...) // error[E0716]: temporary value dropped while borrowed
	// // note: consider using a `let` binding to create a longer lived value
	// ```
	// - Issues such as this one are the ones motivating https://github.com/rust-lang/rfcs/pull/66
	//
	// This makes such a macro incredibly unergonomic in practice, and the reason most macros
	// out there had to take the path of being a statement/binding macro (_e.g._, `pin!(future);`)
	// instead of featuring the more intuitive ergonomics of an expression macro.
	//
	// Luckily, there is a way to avoid the problem. Indeed, the problem stems from the fact that a
	// temporary is dropped at the end of its enclosing statement when it is part of the parameters
	// given to function call, which has precisely been the case with our `Pin::new_unchecked()`!
	// For instance,
	// ```rust
	// let p = Pin::new_unchecked(&mut <temporary>);
	// ```
	// becomes:
	// ```rust
	// let p = { let mut anon = <temporary>; &mut anon };
	// ```
	//
	// However, when using a literal braced struct to construct the value, references to temporaries
	// can then be taken. This makes Rust change the lifespan of such temporaries so that they are,
	// instead, dropped _at the end of the enscoping block_.
	// For instance,
	// ```rust
	// let p = Pin { __pointer: &mut <temporary> };
	// ```
	// becomes:
	// ```rust
	// let mut anon = <temporary>;
	// let p = Pin { __pointer: &mut anon };
	// ```
	// which is *exactly* what we want.
	//
	// See https://doc.rust-lang.org/1.58.1/reference/destructors.html#temporary-lifetime-extension
	// for more info.

[Noratrieb]

Should we make a formal statement that if a feature name contains unsafe, it is equivalent to employing unsafe code and may compromise Rust’s safety guarantees? Or is there already such a clarification?

Is it not obvious that a feature named "unsafe pin internals" is not intended for consumers that aren't, well, pin itself?

[Noratrieb]

That said the macro implementation will likely change the future anyways to make this no longer necessary. Until then there will be this public field hack, but that doesn't make it any less sound in practice.

[hxuhack]

Is it not obvious that a feature named "unsafe pin internals" is not intended for consumers that aren't, well, pin itself?

Yes, it is obvious, but it derives from Rust's initial design, specifically the five types of unsafe code described in the book. If some of these features are unsafe, I think a better approach might be something like #![feature(unsafe(pin_internals))].

@taiki-e Thanks for you explanation. My concern here is not the possibility to move the object, but the consistency of design with respect to Unpin and PhantomPinned.

Besides, I have a question about the Unpin trait. As I understand it, the design philosophy of Unpin is similar to that of Send and Sync, which prevent data structures containing raw pointers from automatically deriving Send or Sync. For Unpin, PhantomPinned serves a similar role to raw pointers, ensuring that certain types cannot be moved. However, unlike Send and Sync, we can implement Unpin without using unsafe. This means we can impl a !Unpin type with Unpin without unsafe code, effectively bypassing the Unpin bound of Pin::new. See the following code. Should implementing Unpin be marked as unsafe?

use std::pin::Pin;
use std::marker::PhantomPinned;

struct Data {
    _pinned: PhantomPinned,
}

//impl Unpin for Data {}

impl Data {
    fn new() -> Pin<Box<Self>> {
        Box::pin(Data {
            _pinned: PhantomPinned,
        })
    }
}

fn main() {
    let mut obj = Data::new();
    //let mut p = Pin::new(obj); //This is not allowed without impl Unpin for Data {}
    let mut p = unsafe { Pin::new_unchecked(obj) } ; 
}

[taiki-e]

My concern here is not the possibility to move the object, but the consistency of design with respect to Unpin and PhantomPinned.

Besides, I have a question about the Unpin trait. As I understand it, the design philosophy of Unpin is similar to that of Send and Sync, which prevent data structures containing raw pointers from automatically deriving Send or Sync. For Unpin, PhantomPinned serves a similar role to raw pointers, ensuring that certain types cannot be moved. However, unlike Send and Sync, we can implement Unpin without using unsafe. This means we can impl a !Unpin type with Unpin without unsafe code, effectively bypassing the Unpin bound of Pin::new. See the following code. Should implementing Unpin be marked as unsafe?

You are free to mark any structure with a !Unpin field (e.g., PhantomPinned) as Unpin. However, it only allows you to pin that structure, and it requires unsafe code to actually pin the !Unpin structure that is its field. Therefore, it is fine that Unpin is safe, unlike Send/Sync, which can cause undefined behavior without any other unsafe code.

Where the fact that Unpin is safe becomes problematic is when one wants to provide a safe abstraction for a process that projects pinning to a field of structure (called pin projection or structural pinning). In fact, pin-project/pin-project-lite, which are used in most places in the ecosystem for safe pin projection, has a mechanism to forbid users to implement Unpin manually without unsafe code.

[scottmcm]

There's no tracking issue for the field, which means it's not stabilization-path, so you shouldn't be using it.

Not to mention that there's already a comment about this in the code:

rust/library/core/src/pin.rs

Lines 1090 to 1103 in 2a06022

	// FIXME(#93176): this field is made `#[unstable] #[doc(hidden)] pub` to:
	// - deter downstream users from accessing it (which would be unsound!),
	// - let the `pin!` macro access it (such a macro requires using struct
	// literal syntax in order to benefit from lifetime extension).
	//
	// However, if the `Deref` impl exposes a field with the same name as this
	// field, then the two will collide, resulting in a confusing error when the
	// user attempts to access the field through a `Pin<Ptr>`. Therefore, the
	// name `__pointer` is designed to be unlikely to collide with any other
	// field. Long-term, macro hygiene is expected to offer a more robust
	// alternative, alongside `unsafe` fields.
	#[unstable(feature = "unsafe_pin_internals", issue = "none")]
	#[doc(hidden)]
	pub __pointer: Ptr,

There's lots of nightly features that are unsound -- most famously #31844 -- so I think this is just not anything worth doing about right now. It's not on a stabilization track to expose this, so personally I'd just close the issue.

At most, we could "fix" this by adding a stronger warning if you enable a feature without a tracking issue.

[hxuhack]

@scottmcm Thanks for the feedback. We are reviewing the safety of the entire std-lib, which is why we raised this concern. I believe the community needs a place to track all temporary solutions that could compromise the soundness of safe Rust before they escalate to an uncontrollable scale. I’m okay with closing the issue.