Ensure that #[repr(packed)] cannot be used on #[pin_projectable] structs

pin-project-internal/src/lib.rs

@@ -5,6 +5,7 @@
 #![warn(rust_2018_idioms, unreachable_pub)]
 #![warn(single_use_lifetimes)]
 #![warn(clippy::all, clippy::pedantic)]
+#![allow(clippy::use_self)]
 
 extern crate proc_macro;
 

pin-project-internal/src/pin_projectable/mod.rs

@@ -2,8 +2,8 @@ use proc_macro2::{Ident, Span, TokenStream};
 use quote::{quote, ToTokens};
 use syn::parse::{Parse, ParseStream};
 use syn::{
-    Attribute, Generics, Item, ItemEnum, ItemFn, ItemStruct, Meta, NestedMeta, Result, ReturnType,
-    Type, TypeTuple,
+    Fields, FieldsNamed, FieldsUnnamed, Generics, Index, Item, ItemEnum, ItemFn, ItemStruct, Meta,
+    NestedMeta, Result, ReturnType, Type, TypeTuple,
 };
 
 use crate::utils::{Nothing, VecExt};
@@ -33,11 +33,15 @@ impl Parse for PinProject {
 fn handle_type(args: TokenStream, item: Item, pinned_drop: Option<ItemFn>) -> Result<TokenStream> {
     match item {
         Item::Struct(item) => {
-            ensure_not_packed(&item.attrs)?;
-            structs::parse(args, item, pinned_drop)
+            let packed_check = ensure_not_packed(&item)?;
+            let mut res = structs::parse(args, item, pinned_drop)?;
+            //println!("Packed check: {}", packed_check);
+            res.extend(packed_check);
+            Ok(res)
         }
         Item::Enum(item) => {
-            ensure_not_packed(&item.attrs)?;
+            // We don't need to check for '#[repr(packed)]',
+            // since it does not apply to enums
             enums::parse(args, item, pinned_drop)
         }
         _ => unreachable!(),
@@ -108,8 +112,8 @@ pub(super) fn attribute(args: TokenStream, input: TokenStream) -> Result<TokenSt
     }
 }
 
-fn ensure_not_packed(attrs: &[Attribute]) -> Result<()> {
-    for meta in attrs.iter().filter_map(|attr| attr.parse_meta().ok()) {
+fn ensure_not_packed(item: &ItemStruct) -> Result<TokenStream> {
+    for meta in item.attrs.iter().filter_map(|attr| attr.parse_meta().ok()) {
         if let Meta::List(l) = meta {
             if l.ident == "repr" {
                 for repr in l.nested.iter() {
@@ -125,7 +129,72 @@ fn ensure_not_packed(attrs: &[Attribute]) -> Result<()> {
             }
         }
     }
-    Ok(())
+
+    // Workaround for https://github.com/taiki-e/pin-project/issues/32
+    // Through the tricky use of proc macros, it's possible to bypass
+    // the above check for the 'repr' attribute.
+    // To ensure that it's impossible to use pin projections on a #[repr(packed)][
+    // struct, we generate code like this:
+    //
+    // #[deny(safe_packed_borrows)]
+    // fn enforce_not_packed_for_MyStruct(val: MyStruct) {
+    //  let _field1 = &val.field1;
+    //  let _field2 = &val.field2;
+    //  ...
+    //  let _fieldn = &val.fieldn;
+    // }
+    //
+    // Taking a reference to a packed field is unsafe, amd appplying
+    // #[deny(safe_packed_borrows)] makes sure that doing this without
+    // an 'unsafe' block (which we deliberately do not generate)
+    // is a hard error.
+    //
+    // If the struct ends up having #[repr(packed)] applied somehow,
+    // this will generate an (unfriendly) error message. Under all reasonable
+    // circumstances, we'll detect the #[repr(packed)] attribute, and generate
+    // a much nicer error above.
+    //
+    // There is one exception: If the type of a struct field has a alignemtn of 1
+    // (e.g. u8), it is always safe to take a reference to it, even if the struct
+    // is #[repr(packed)]. If the struct is composed entirely of types of alignent 1,
+    // our generated method will not trigger an error if the struct is #[repr(packed)]
+    //
+    // Fortunately, this should have no observable consequence - #[repr(packed)]
+    // is essentially a no-op on such a type. Nevertheless, we include a test
+    // to ensure that the compiler doesn't ever try to copy the fields on
+    // such a struct when trying to drop it - which is reason we prevent
+    // #[repr(packed)] in the first place
+    let mut field_refs = vec![];
+    match &item.fields {
+        Fields::Named(FieldsNamed { named, .. }) => {
+            for field in named.iter() {
+                let ident = field.ident.as_ref().unwrap();
+                field_refs.push(quote!(&val.#ident;));
+            }
+        }
+        Fields::Unnamed(FieldsUnnamed { unnamed, .. }) => {
+            for (i, _) in unnamed.iter().enumerate() {
+                let index = Index::from(i);
+                field_refs.push(quote!(&val.#index;));
+            }
+        }
+        Fields::Unit => {}
+    }
+
+    let (impl_generics, ty_generics, where_clause) = item.generics.split_for_impl();
+
+    let struct_name = &item.ident;
+    let method_name = Ident::new(
+        &("__pin_project_assert_not_repr_packed_".to_string() + &item.ident.to_string()),
+        Span::call_site(),
+    );
+    let test_fn = quote! {
+        #[deny(safe_packed_borrows)]
+        fn #method_name #impl_generics (val: #struct_name #ty_generics) #where_clause {
+            #(#field_refs)*
+        }
+    };
+    Ok(test_fn)
 }
 
 /// Makes the generics of projected type from the reference of the original generics.

src/lib.rs

@@ -148,6 +148,7 @@
 #![warn(rust_2018_idioms, unreachable_pub)]
 #![warn(single_use_lifetimes)]
 #![warn(clippy::all, clippy::pedantic)]
+#![allow(clippy::use_self)]
 
 /// An attribute to support pattern matching.
 ///

tests/repr_packed.rs

@@ -0,0 +1,49 @@
+#![warn(unsafe_code)]
+#![warn(rust_2018_idioms)]
+#![allow(dead_code)]
+
+use std::cell::Cell;
+
+// Ensure that the compiler doesn't copy the fields
+// of #[repr(packed)] types during drop, if the field has alignment 1
+// (that is, any reference to the field is guaranteed to have proper alignment)
+// We are currently unable to statically prevent the usage of #[pin_projectable]
+// on #[repr(packed)] types composed entirely of fields of alignment 1.
+// This shouldn't lead to undefined behavior, as long as the compiler doesn't
+// try to move the field anyway during drop.
+//
+// This tests validates that the compiler is doing what we expect.
+#[test]
+fn weird_repr_packed() {
+    // We keep track of the field address during
+    // drop using a thread local, to avoid changing
+    // the layout of our #[repr(packed)] type
+    thread_local! {
+        static FIELD_ADDR: Cell<usize> = Cell::new(0);
+    }
+
+    #[repr(packed)]
+    struct Foo {
+        field: u8,
+    }
+
+    impl Drop for Foo {
+        fn drop(&mut self) {
+            FIELD_ADDR.with(|f| {
+                f.set(&self.field as *const u8 as usize);
+            })
+        }
+    }
+
+    let field_addr = {
+        // We let this field drop by going out of scope,
+        // rather than explicitly calling drop(foo).
+        // Calling drop(foo) causes 'foo' to be moved
+        // into the 'drop' function, resulinting a different
+        // address
+        let foo = Foo { field: 27 };
+        let field_addr = &foo.field as *const u8 as usize;
+        field_addr
+    };
+    assert_eq!(field_addr, FIELD_ADDR.with(|f| f.get()));
+}

tests/ui/pin_projectable/auxiliary/sneaky_macro.rs

@@ -0,0 +1,14 @@
+// force-host 
+// no-prefer-dynamic
+
+#![crate_type = "proc-macro"]
+
+extern crate proc_macro;
+
+use proc_macro::TokenStream;
+
+#[proc_macro_attribute]
+pub fn hidden_repr(attr: TokenStream, item: TokenStream) -> TokenStream {
+    format!("#[repr({})] {}", attr, item).parse().unwrap()
+}
+

tests/ui/pin_projectable/packed.rs

@@ -2,6 +2,7 @@
 
 #![deny(warnings)]
 
+
 use pin_project::pin_projectable;
 
 #[pin_projectable]
@@ -19,16 +20,4 @@ struct Foo2 {
     field: u8,
 }
 
-#[pin_projectable]
-#[repr(packed, C)] //~ ERROR may not be used on #[repr(packed)] type
-enum Blah {
-    Tuple(#[pin] u8),
-}
-
-#[repr(packed, C)] //~ ERROR may not be used on #[repr(packed)] type
-#[pin_projectable]
-enum Blah2 {
-    Tuple(#[pin] u8),
-}
-
 fn main() {}

tests/ui/pin_projectable/packed.stderr

@@ -1,26 +1,14 @@
 error: pin_projectable may not be used on #[repr(packed)] types
- --> $DIR/packed.rs:8:8
+ --> $DIR/packed.rs:9:8
   |
-8 | #[repr(packed, C)] //~ ERROR may not be used on #[repr(packed)] type
+9 | #[repr(packed, C)] //~ ERROR may not be used on #[repr(packed)] type
   |        ^^^^^^
 
 error: pin_projectable may not be used on #[repr(packed)] types
-  --> $DIR/packed.rs:15:8
+  --> $DIR/packed.rs:16:8
    |
-15 | #[repr(packed, C)] //~ ERROR may not be used on #[repr(packed)] type
+16 | #[repr(packed, C)] //~ ERROR may not be used on #[repr(packed)] type
    |        ^^^^^^
 
-error: pin_projectable may not be used on #[repr(packed)] types
-  --> $DIR/packed.rs:23:8
-   |
-23 | #[repr(packed, C)] //~ ERROR may not be used on #[repr(packed)] type
-   |        ^^^^^^
-
-error: pin_projectable may not be used on #[repr(packed)] types
-  --> $DIR/packed.rs:28:8
-   |
-28 | #[repr(packed, C)] //~ ERROR may not be used on #[repr(packed)] type
-   |        ^^^^^^
-
-error: aborting due to 4 previous errors
+error: aborting due to 2 previous errors
 

tests/ui/pin_projectable/packed_sneaky.rs

@@ -0,0 +1,16 @@
+// compile-fail
+// aux-build:sneaky_macro.rs
+
+#[macro_use]
+extern crate sneaky_macro;
+
+use pin_project::pin_projectable;
+
+#[pin_projectable] //~ ERROR borrow of packed field is unsafe and requires unsafe function or block
+#[hidden_repr(packed)]
+struct Bar {
+    #[pin]
+    field: u32
+}
+
+fn main() {}

tests/ui/pin_projectable/packed_sneaky.stderr

@@ -0,0 +1,17 @@
+error: borrow of packed field is unsafe and requires unsafe function or block (error E0133)
+ --> $DIR/packed_sneaky.rs:9:1
+  |
+9 | #[pin_projectable] //~ ERROR borrow of packed field is unsafe and requires unsafe function or block
+  | ^^^^^^^^^^^^^^^^^^
+  |
+note: lint level defined here
+ --> $DIR/packed_sneaky.rs:9:1
+  |
+9 | #[pin_projectable] //~ ERROR borrow of packed field is unsafe and requires unsafe function or block
+  | ^^^^^^^^^^^^^^^^^^
+  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+  = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>
+  = note: fields of packed structs might be misaligned: dereferencing a misaligned pointer or even just creating a misaligned reference is undefined behavior
+
+error: aborting due to previous error
+