Skip to content

Commit

Permalink
Merge #336
Browse files Browse the repository at this point in the history 
336: Prevent abuse of private module r=taiki-e a=taiki-e

*This was originally reported by `@danielhenrymantilla` in https://discord.com/channels/273534239310479360/512792629516173323/870075511009857617*.

Currently, you can break the soundness of the pin-project by overriding the private module as follows.
Considering that it uses a hidden private API that is not guaranteed to be stable, I don't think this will actually happen except in cases where users intentionally abuse it or use a malicious crate. Also, in such a case, fixing this would not help much because the attacker can do anything.
That said, it seems relatively easy to fix this, so I'm going to fix this.

```rust
extern crate pin_project as pin_project_orig;
extern crate self as pin_project;

pub use ::pin_project_orig::*;
mod __private {
    pub use ::pin_project_orig::__private::*;
    pub trait Drop {}
}

use std::{marker::PhantomPinned, mem};

#[pin_project]
struct S {
    #[pin]
    f: (u8, PhantomPinned),
}

impl Drop for S {
    fn drop(&mut self) {
        let prev = &self.f.0 as *const _ as usize;
        let moved = mem::take(&mut self.f); // move pinned field
        let moved = &moved.0 as *const _ as usize;
        assert_eq!(prev, moved); // panic
    }
}

fn main() {
    let mut x = Box::pin(S { f: (1, PhantomPinned) });
    let _f = x.as_mut().project().f; // first mutable access
}
```

[playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=faf882d7f20845d992817a21ce444463)

---

This patch forces the macro to refer to the `pin-project` crate by referring to the `_pin_project` imported into the `const _` scope instead of `::pin_project`.

https://github.com/taiki-e/pin-project/blob/74444360566ccef04da2bde8f0273469df6ad67b/tests/expand/pub/struct.expanded.rs#L21-L23

https://github.com/taiki-e/pin-project/blob/74444360566ccef04da2bde8f0273469df6ad67b/tests/expand/pub/struct.expanded.rs#L49

In the above example, a user-implemented Drop is detected, resulting in an error.

https://github.com/taiki-e/pin-project/blob/74444360566ccef04da2bde8f0273469df6ad67b/tests/ui/pin_project/override-priv-mod.stderr#L1-L10

If other items (e.g., `PinnedDrop`) are replaced in the same way, either nothing happens (nothing is affected), or `#[pin_project]` and `#[pinned_drop]` refer to different items, resulting in an error.

Co-authored-by: Taiki Endo <te316e89@gmail.com>
  • Loading branch information
@bors @taiki-e
bors[bot] and taiki-e authored Dec 26, 2021
2 parents 86a736f + 0ea7fb4 commit 27c1aa2
Show file tree
Hide file tree
Showing 43 changed files with 780 additions and 627 deletions.
57 changes: 30 additions & 27 deletions pin-project-internal/src/pin_project/derive.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,13 @@ impl GenerateTokens {
// - https://github.com/taiki-e/pin-project/pull/53#issuecomment-525906867
// - https://github.com/taiki-e/pin-project/pull/70
#allowed_lints
#[allow(unused_qualifications)]
#[allow(clippy::semicolon_if_nothing_returned)]
#[allow(clippy::use_self)]
#[allow(clippy::used_underscore_binding)]
const _: () = {
#[allow(unused_extern_crates)]
extern crate pin_project as _pin_project;
#scoped
#unpin_impl
#drop_impl
Expand Down Expand Up @@ -581,10 +584,10 @@ fn visit_fields<'a>(
#vis #ident #colon_token ::pin_project::__private::PhantomData<#ty>,
});
proj_body.extend(quote! {
#ident #colon_token ::pin_project::__private::Pin::new_unchecked(#binding),
#ident #colon_token _pin_project::__private::Pin::new_unchecked(#binding),
});
proj_move.extend(quote! {
#ident #colon_token ::pin_project::__private::PhantomData,
#ident #colon_token _pin_project::__private::PhantomData,
});

cx.pinned_fields.push(ty);
Expand All @@ -603,7 +606,7 @@ fn visit_fields<'a>(
#binding,
});
proj_move.extend(quote! {
#ident #colon_token ::pin_project::__private::ptr::read(#binding),
#ident #colon_token _pin_project::__private::ptr::read(#binding),
});
}
}
Expand Down Expand Up @@ -658,7 +661,7 @@ fn proj_own_body(
// if any of the destructors panic.
{
#(
let __guard = ::pin_project::__private::UnsafeDropInPlaceGuard(#pinned_fields);
let __guard = _pin_project::__private::UnsafeDropInPlaceGuard(#pinned_fields);
)*
}

Expand All @@ -682,14 +685,14 @@ fn make_unpin_impl(cx: &Context<'_>) -> TokenStream {

// Make the error message highlight `UnsafeUnpin` argument.
proj_generics.make_where_clause().predicates.push(parse_quote_spanned! { span =>
::pin_project::__private::Wrapper<#lifetime, Self>: ::pin_project::UnsafeUnpin
_pin_project::__private::Wrapper<#lifetime, Self>: _pin_project::UnsafeUnpin
});

let (impl_generics, _, where_clause) = proj_generics.split_for_impl();
let ty_generics = cx.orig.generics.split_for_impl().1;

quote_spanned! { span =>
impl #impl_generics ::pin_project::__private::Unpin for #orig_ident #ty_generics
impl #impl_generics _pin_project::__private::Unpin for #orig_ident #ty_generics
#where_clause
{
}
Expand All @@ -701,9 +704,9 @@ fn make_unpin_impl(cx: &Context<'_>) -> TokenStream {
let lifetime = &cx.proj.lifetime;

proj_generics.make_where_clause().predicates.push(parse_quote! {
::pin_project::__private::Wrapper<
#lifetime, ::pin_project::__private::PhantomPinned
>: ::pin_project::__private::Unpin
_pin_project::__private::Wrapper<
#lifetime, _pin_project::__private::PhantomPinned
>: _pin_project::__private::Unpin
});

let (proj_impl_generics, _, proj_where_clause) = proj_generics.split_for_impl();
Expand All @@ -713,7 +716,7 @@ fn make_unpin_impl(cx: &Context<'_>) -> TokenStream {
// call-site span.
let unsafety = <Token![unsafe]>::default();
quote_spanned! { span =>
impl #proj_impl_generics ::pin_project::__private::Unpin
impl #proj_impl_generics _pin_project::__private::Unpin
for #orig_ident #ty_generics
#proj_where_clause
{
Expand All @@ -726,7 +729,7 @@ fn make_unpin_impl(cx: &Context<'_>) -> TokenStream {
// impl, they'll get a "conflicting implementations of trait" error when
// coherence checks are run.
#[doc(hidden)]
#unsafety impl #proj_impl_generics ::pin_project::UnsafeUnpin
#unsafety impl #proj_impl_generics _pin_project::UnsafeUnpin
for #orig_ident #ty_generics
#proj_where_clause
{
Expand Down Expand Up @@ -781,7 +784,7 @@ fn make_unpin_impl(cx: &Context<'_>) -> TokenStream {
let (_, ty_generics, where_clause) = cx.orig.generics.split_for_impl();

full_where_clause.predicates.push(parse_quote! {
#struct_ident #proj_ty_generics: ::pin_project::__private::Unpin
#struct_ident #proj_ty_generics: _pin_project::__private::Unpin
});

quote! {
Expand All @@ -797,15 +800,15 @@ fn make_unpin_impl(cx: &Context<'_>) -> TokenStream {
// this 'public' type by creating this type in the inside of `const`.
#[allow(missing_debug_implementations)]
#vis struct #struct_ident #proj_generics #where_clause {
__pin_project_use_generics: ::pin_project::__private::AlwaysUnpin<
#lifetime, (#(::pin_project::__private::PhantomData<#type_params>),*)
__pin_project_use_generics: _pin_project::__private::AlwaysUnpin<
#lifetime, (#(_pin_project::__private::PhantomData<#type_params>),*)
>,

#(#fields,)*
#(#lifetime_fields,)*
}

impl #proj_impl_generics ::pin_project::__private::Unpin
impl #proj_impl_generics _pin_project::__private::Unpin
for #orig_ident #ty_generics
#full_where_clause
{
Expand All @@ -818,7 +821,7 @@ fn make_unpin_impl(cx: &Context<'_>) -> TokenStream {
// impl, they'll get a "conflicting implementations of trait" error when
// coherence checks are run.
#[doc(hidden)]
unsafe impl #proj_impl_generics ::pin_project::UnsafeUnpin
unsafe impl #proj_impl_generics _pin_project::UnsafeUnpin
for #orig_ident #ty_generics
#full_where_clause
{
Expand All @@ -843,18 +846,18 @@ fn make_drop_impl(cx: &Context<'_>) -> TokenStream {
// call-site span.
let unsafety = <Token![unsafe]>::default();
quote_spanned! { span =>
impl #impl_generics ::pin_project::__private::Drop for #ident #ty_generics
impl #impl_generics _pin_project::__private::Drop for #ident #ty_generics
#where_clause
{
fn drop(&mut self) {
#unsafety {
// Safety - we're in 'drop', so we know that 'self' will
// never move again.
let __pinned_self = ::pin_project::__private::Pin::new_unchecked(self);
let __pinned_self = _pin_project::__private::Pin::new_unchecked(self);
// We call `pinned_drop` only once. Since `PinnedDrop::drop`
// is an unsafe method and a private API, it is never called again in safe
// code *unless the user uses a maliciously crafted macro*.
::pin_project::__private::PinnedDrop::drop(__pinned_self);
_pin_project::__private::PinnedDrop::drop(__pinned_self);
}
}
}
Expand Down Expand Up @@ -886,7 +889,7 @@ fn make_drop_impl(cx: &Context<'_>) -> TokenStream {
// This will result in a compilation error, which is exactly what we want.
trait #trait_ident {}
#[allow(clippy::drop_bounds, drop_bounds)]
impl<T: ::pin_project::__private::Drop> #trait_ident for T {}
impl<T: _pin_project::__private::Drop> #trait_ident for T {}
impl #impl_generics #trait_ident for #ident #ty_generics #where_clause {}

// Generate a dummy impl of `PinnedDrop`, to ensure that the user cannot implement it.
Expand All @@ -900,10 +903,10 @@ fn make_drop_impl(cx: &Context<'_>) -> TokenStream {
// they'll get a "conflicting implementations of trait" error when coherence
// checks are run.
#[doc(hidden)]
impl #impl_generics ::pin_project::__private::PinnedDrop for #ident #ty_generics
impl #impl_generics _pin_project::__private::PinnedDrop for #ident #ty_generics
#where_clause
{
unsafe fn drop(self: ::pin_project::__private::Pin<&mut Self>) {}
unsafe fn drop(self: _pin_project::__private::Pin<&mut Self>) {}
}
}
}
Expand Down Expand Up @@ -934,7 +937,7 @@ fn make_proj_impl(

let mut project = Some(quote! {
#vis fn project<#lifetime>(
self: ::pin_project::__private::Pin<&#lifetime mut Self>,
self: _pin_project::__private::Pin<&#lifetime mut Self>,
) -> #proj_ident #proj_ty_generics {
unsafe {
#proj_body
Expand All @@ -944,7 +947,7 @@ fn make_proj_impl(
let mut project_ref = Some(quote! {
#[allow(clippy::missing_const_for_fn)]
#vis fn project_ref<#lifetime>(
self: ::pin_project::__private::Pin<&#lifetime Self>,
self: _pin_project::__private::Pin<&#lifetime Self>,
) -> #proj_ref_ident #proj_ty_generics {
unsafe {
#proj_ref_body
Expand All @@ -955,7 +958,7 @@ fn make_proj_impl(
// It is enough to only set the span of the signature.
let sig = quote_spanned! { span =>
#vis fn project_replace(
self: ::pin_project::__private::Pin<&mut Self>,
self: _pin_project::__private::Pin<&mut Self>,
__replacement: Self,
) -> #proj_own_ident #orig_ty_generics
};
Expand All @@ -966,9 +969,9 @@ fn make_proj_impl(

// Destructors will run in reverse order, so next create a guard to overwrite
// `self` with the replacement value without calling destructors.
let __guard = ::pin_project::__private::UnsafeOverwriteGuard {
let __guard = _pin_project::__private::UnsafeOverwriteGuard {
target: __self_ptr,
value: ::pin_project::__private::ManuallyDrop::new(__replacement),
value: _pin_project::__private::ManuallyDrop::new(__replacement),
};

#proj_own_body
Expand Down
35 changes: 19 additions & 16 deletions tests/expand/default/enum.expanded.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,39 +62,42 @@ where
#[allow(clippy::pattern_type_mismatch)]
#[allow(clippy::redundant_pub_crate)]
#[allow(clippy::type_repetition_in_bounds)]
#[allow(unused_qualifications)]
#[allow(clippy::semicolon_if_nothing_returned)]
#[allow(clippy::use_self)]
#[allow(clippy::used_underscore_binding)]
const _: () = {
#[allow(unused_extern_crates)]
extern crate pin_project as _pin_project;
impl<T, U> Enum<T, U> {
fn project<'pin>(
self: ::pin_project::__private::Pin<&'pin mut Self>,
self: _pin_project::__private::Pin<&'pin mut Self>,
) -> EnumProj<'pin, T, U> {
unsafe {
match self.get_unchecked_mut() {
Self::Struct { pinned, unpinned } => EnumProj::Struct {
pinned: ::pin_project::__private::Pin::new_unchecked(pinned),
pinned: _pin_project::__private::Pin::new_unchecked(pinned),
unpinned,
},
Self::Tuple(_0, _1) => {
EnumProj::Tuple(::pin_project::__private::Pin::new_unchecked(_0), _1)
EnumProj::Tuple(_pin_project::__private::Pin::new_unchecked(_0), _1)
}
Self::Unit => EnumProj::Unit,
}
}
}
#[allow(clippy::missing_const_for_fn)]
fn project_ref<'pin>(
self: ::pin_project::__private::Pin<&'pin Self>,
self: _pin_project::__private::Pin<&'pin Self>,
) -> EnumProjRef<'pin, T, U> {
unsafe {
match self.get_ref() {
Self::Struct { pinned, unpinned } => EnumProjRef::Struct {
pinned: ::pin_project::__private::Pin::new_unchecked(pinned),
pinned: _pin_project::__private::Pin::new_unchecked(pinned),
unpinned,
},
Self::Tuple(_0, _1) => {
EnumProjRef::Tuple(::pin_project::__private::Pin::new_unchecked(_0), _1)
EnumProjRef::Tuple(_pin_project::__private::Pin::new_unchecked(_0), _1)
}
Self::Unit => EnumProjRef::Unit,
}
Expand All @@ -103,32 +106,32 @@ const _: () = {
}
#[allow(missing_debug_implementations)]
struct __Enum<'pin, T, U> {
__pin_project_use_generics: ::pin_project::__private::AlwaysUnpin<
__pin_project_use_generics: _pin_project::__private::AlwaysUnpin<
'pin,
(
::pin_project::__private::PhantomData<T>,
::pin_project::__private::PhantomData<U>,
_pin_project::__private::PhantomData<T>,
_pin_project::__private::PhantomData<U>,
),
>,
__field0: T,
__field1: T,
}
impl<'pin, T, U> ::pin_project::__private::Unpin for Enum<T, U> where
__Enum<'pin, T, U>: ::pin_project::__private::Unpin
impl<'pin, T, U> _pin_project::__private::Unpin for Enum<T, U> where
__Enum<'pin, T, U>: _pin_project::__private::Unpin
{
}
#[doc(hidden)]
unsafe impl<'pin, T, U> ::pin_project::UnsafeUnpin for Enum<T, U> where
__Enum<'pin, T, U>: ::pin_project::__private::Unpin
unsafe impl<'pin, T, U> _pin_project::UnsafeUnpin for Enum<T, U> where
__Enum<'pin, T, U>: _pin_project::__private::Unpin
{
}
trait EnumMustNotImplDrop {}
#[allow(clippy::drop_bounds, drop_bounds)]
impl<T: ::pin_project::__private::Drop> EnumMustNotImplDrop for T {}
impl<T: _pin_project::__private::Drop> EnumMustNotImplDrop for T {}
impl<T, U> EnumMustNotImplDrop for Enum<T, U> {}
#[doc(hidden)]
impl<T, U> ::pin_project::__private::PinnedDrop for Enum<T, U> {
unsafe fn drop(self: ::pin_project::__private::Pin<&mut Self>) {}
impl<T, U> _pin_project::__private::PinnedDrop for Enum<T, U> {
unsafe fn drop(self: _pin_project::__private::Pin<&mut Self>) {}
}
};
fn main() {}
31 changes: 17 additions & 14 deletions tests/expand/default/struct.expanded.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,13 @@ struct Struct<T, U> {
#[allow(clippy::pattern_type_mismatch)]
#[allow(clippy::redundant_pub_crate)]
#[allow(clippy::type_repetition_in_bounds)]
#[allow(unused_qualifications)]
#[allow(clippy::semicolon_if_nothing_returned)]
#[allow(clippy::use_self)]
#[allow(clippy::used_underscore_binding)]
const _: () = {
#[allow(unused_extern_crates)]
extern crate pin_project as _pin_project;
#[allow(dead_code)]
#[allow(clippy::mut_mut)]
struct __StructProjection<'pin, T, U>
Expand All @@ -38,24 +41,24 @@ const _: () = {
}
impl<T, U> Struct<T, U> {
fn project<'pin>(
self: ::pin_project::__private::Pin<&'pin mut Self>,
self: _pin_project::__private::Pin<&'pin mut Self>,
) -> __StructProjection<'pin, T, U> {
unsafe {
let Self { pinned, unpinned } = self.get_unchecked_mut();
__StructProjection {
pinned: ::pin_project::__private::Pin::new_unchecked(pinned),
pinned: _pin_project::__private::Pin::new_unchecked(pinned),
unpinned,
}
}
}
#[allow(clippy::missing_const_for_fn)]
fn project_ref<'pin>(
self: ::pin_project::__private::Pin<&'pin Self>,
self: _pin_project::__private::Pin<&'pin Self>,
) -> __StructProjectionRef<'pin, T, U> {
unsafe {
let Self { pinned, unpinned } = self.get_ref();
__StructProjectionRef {
pinned: ::pin_project::__private::Pin::new_unchecked(pinned),
pinned: _pin_project::__private::Pin::new_unchecked(pinned),
unpinned,
}
}
Expand All @@ -68,31 +71,31 @@ const _: () = {
}
#[allow(missing_debug_implementations)]
struct __Struct<'pin, T, U> {
__pin_project_use_generics: ::pin_project::__private::AlwaysUnpin<
__pin_project_use_generics: _pin_project::__private::AlwaysUnpin<
'pin,
(
::pin_project::__private::PhantomData<T>,
::pin_project::__private::PhantomData<U>,
_pin_project::__private::PhantomData<T>,
_pin_project::__private::PhantomData<U>,
),
>,
__field0: T,
}
impl<'pin, T, U> ::pin_project::__private::Unpin for Struct<T, U> where
__Struct<'pin, T, U>: ::pin_project::__private::Unpin
impl<'pin, T, U> _pin_project::__private::Unpin for Struct<T, U> where
__Struct<'pin, T, U>: _pin_project::__private::Unpin
{
}
#[doc(hidden)]
unsafe impl<'pin, T, U> ::pin_project::UnsafeUnpin for Struct<T, U> where
__Struct<'pin, T, U>: ::pin_project::__private::Unpin
unsafe impl<'pin, T, U> _pin_project::UnsafeUnpin for Struct<T, U> where
__Struct<'pin, T, U>: _pin_project::__private::Unpin
{
}
trait StructMustNotImplDrop {}
#[allow(clippy::drop_bounds, drop_bounds)]
impl<T: ::pin_project::__private::Drop> StructMustNotImplDrop for T {}
impl<T: _pin_project::__private::Drop> StructMustNotImplDrop for T {}
impl<T, U> StructMustNotImplDrop for Struct<T, U> {}
#[doc(hidden)]
impl<T, U> ::pin_project::__private::PinnedDrop for Struct<T, U> {
unsafe fn drop(self: ::pin_project::__private::Pin<&mut Self>) {}
impl<T, U> _pin_project::__private::PinnedDrop for Struct<T, U> {
unsafe fn drop(self: _pin_project::__private::Pin<&mut Self>) {}
}
};
fn main() {}
Loading

0 comments on commit 27c1aa2

Please sign in to comment.