Skip to content

Commit

Permalink
Ensure that #[repr(packed)] cannot be used on #[pin_projectable] structs
Browse files Browse the repository at this point in the history
As described in issue taiki-e#32,
it's possible to bypass the #[repr(packed)] check via the use of an
additional procedural macro. However, we need to be able to forbid
using #[repr(packed) in order to make #[pin_projectable] sound

To enforce this, we can make use of the fact that taking references to
the fields of a #[repr(packed)] struct is unsafe. Given a #[repr(packed)]
struct Foo, we can generate code like this:

```rust
fn check_Foo(val: Foo) {
    &val.field1;
    &val.field2;
    ...
    &val.fieldn;
}
```

If `Foo` turns out to be #[repr(packed)], the compiler will generate an
error for us, since none of the field references are in an `unsafe`
block.

Unfortunately, this check requires that at least one of the struct's
fields have an alignment greater than 1. If the struct is composed
entirely of fields of alignment 1 (e.g. 'u8'), it will be safe to take a
reference to any of the fields, preventing an error from being emiited.

However, I believe that pin projecting such a struct is actually sound.
If all fields have an alignment 1, #[repr(packed)] is effectively a
no-op, as struct will already have no padding. I've added a test to
verify that the compiler does not copy the fields of such a struct when
it is dropped.
  • Loading branch information
Aaron1011 committed Aug 8, 2019
1 parent 817b7bc commit 8d7b45a
Show file tree
Hide file tree
Showing 7 changed files with 179 additions and 37 deletions.
84 changes: 76 additions & 8 deletions pin-project-internal/src/pin_projectable/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, Item, ItemEnum, ItemFn, ItemStruct, Meta,
NestedMeta, Result, ReturnType, Type, TypeTuple,
};

use crate::utils::{Nothing, VecExt};
Expand Down Expand Up @@ -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!(),
Expand Down Expand Up @@ -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() {
Expand All @@ -125,7 +129,71 @@ 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() {
field_refs.push(quote!(&val.#i;));
}
}
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.
Expand Down
50 changes: 50 additions & 0 deletions tests/repr_packed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#![warn(unsafe_code)]
#![warn(rust_2018_idioms)]
#![allow(dead_code)]

use std::cell::Cell;
use std::thread;

// 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()));
}
14 changes: 14 additions & 0 deletions tests/ui/pin_projectable/auxiliary/sneaky_macro.rs
Original file line number Diff line number Diff line change
@@ -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()
}

13 changes: 1 addition & 12 deletions tests/ui/pin_projectable/packed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#![deny(warnings)]


use pin_project::pin_projectable;

#[pin_projectable]
Expand All @@ -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() {}
22 changes: 5 additions & 17 deletions tests/ui/pin_projectable/packed.stderr
Original file line number Diff line number Diff line change
@@ -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

16 changes: 16 additions & 0 deletions tests/ui/pin_projectable/packed_sneaky.rs
Original file line number Diff line number Diff line change
@@ -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() {}
17 changes: 17 additions & 0 deletions tests/ui/pin_projectable/packed_sneaky.stderr
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 8d7b45a

Please sign in to comment.