From 6db8e8bc4a03a8854ed5dce2ddca9d4cbcf0eb24 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 7 Aug 2019 20:49:57 -0400 Subject: [PATCH 1/6] Ensure that #[repr(packed)] cannot be used on #[pin_projectable] structs As described in issue https://github.com/taiki-e/pin-project/issues/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. --- .../src/pin_projectable/mod.rs | 84 +++++++++++++++++-- tests/repr_packed.rs | 49 +++++++++++ .../pin_projectable/auxiliary/sneaky_macro.rs | 14 ++++ tests/ui/pin_projectable/packed.rs | 13 +-- tests/ui/pin_projectable/packed.stderr | 22 ++--- tests/ui/pin_projectable/packed_sneaky.rs | 16 ++++ tests/ui/pin_projectable/packed_sneaky.stderr | 17 ++++ 7 files changed, 178 insertions(+), 37 deletions(-) create mode 100644 tests/repr_packed.rs create mode 100644 tests/ui/pin_projectable/auxiliary/sneaky_macro.rs create mode 100644 tests/ui/pin_projectable/packed_sneaky.rs create mode 100644 tests/ui/pin_projectable/packed_sneaky.stderr diff --git a/pin-project-internal/src/pin_projectable/mod.rs b/pin-project-internal/src/pin_projectable/mod.rs index e2954f47..aed2e788 100644 --- a/pin-project-internal/src/pin_projectable/mod.rs +++ b/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, 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) -> Result { 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 Result<()> { - for meta in attrs.iter().filter_map(|attr| attr.parse_meta().ok()) { +fn ensure_not_packed(item: &ItemStruct) -> Result { + 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,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. diff --git a/tests/repr_packed.rs b/tests/repr_packed.rs new file mode 100644 index 00000000..2596360d --- /dev/null +++ b/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 = 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())); +} diff --git a/tests/ui/pin_projectable/auxiliary/sneaky_macro.rs b/tests/ui/pin_projectable/auxiliary/sneaky_macro.rs new file mode 100644 index 00000000..0d4f49a4 --- /dev/null +++ b/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() +} + diff --git a/tests/ui/pin_projectable/packed.rs b/tests/ui/pin_projectable/packed.rs index 13c98458..d78bfb1e 100644 --- a/tests/ui/pin_projectable/packed.rs +++ b/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() {} diff --git a/tests/ui/pin_projectable/packed.stderr b/tests/ui/pin_projectable/packed.stderr index d41dba8b..e627e2a6 100644 --- a/tests/ui/pin_projectable/packed.stderr +++ b/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 diff --git a/tests/ui/pin_projectable/packed_sneaky.rs b/tests/ui/pin_projectable/packed_sneaky.rs new file mode 100644 index 00000000..c2212170 --- /dev/null +++ b/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() {} diff --git a/tests/ui/pin_projectable/packed_sneaky.stderr b/tests/ui/pin_projectable/packed_sneaky.stderr new file mode 100644 index 00000000..0c02158c --- /dev/null +++ b/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 + = 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 + From 484e554294bb6d221d8a316d0bce4c8bdc931d6a Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 7 Aug 2019 21:30:01 -0400 Subject: [PATCH 2/6] Allow clippy::use_self This lint is triggering on existing code, completely unrelated to this PR --- src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib.rs b/src/lib.rs index 55f02880..29f14905 100644 --- a/src/lib.rs +++ b/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. /// From 799aeb0fc4a6189ec99917afb85142d55481045a Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 7 Aug 2019 21:30:23 -0400 Subject: [PATCH 3/6] Properly generate tuple index See https://github.com/rust-lang/rust/issues/60210 --- pin-project-internal/src/pin_projectable/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pin-project-internal/src/pin_projectable/mod.rs b/pin-project-internal/src/pin_projectable/mod.rs index aed2e788..3136e4ae 100644 --- a/pin-project-internal/src/pin_projectable/mod.rs +++ b/pin-project-internal/src/pin_projectable/mod.rs @@ -3,7 +3,7 @@ use quote::{quote, ToTokens}; use syn::parse::{Parse, ParseStream}; use syn::{ Fields, FieldsNamed, FieldsUnnamed, Generics, Item, ItemEnum, ItemFn, ItemStruct, Meta, - NestedMeta, Result, ReturnType, Type, TypeTuple, + NestedMeta, Result, ReturnType, Type, TypeTuple, Index }; use crate::utils::{Nothing, VecExt}; @@ -174,7 +174,8 @@ fn ensure_not_packed(item: &ItemStruct) -> Result { } Fields::Unnamed(FieldsUnnamed { unnamed, .. }) => { for (i, _) in unnamed.iter().enumerate() { - field_refs.push(quote!(&val.#i;)); + let index = Index::from(i); + field_refs.push(quote!(&val.#index;)); } } Fields::Unit => {} From bbbda4c0f8cd6d658abc70e942172e5bc9a9c28c Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 7 Aug 2019 21:34:00 -0400 Subject: [PATCH 4/6] Run 'cargo fmt' --- pin-project-internal/src/pin_projectable/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pin-project-internal/src/pin_projectable/mod.rs b/pin-project-internal/src/pin_projectable/mod.rs index 3136e4ae..74e4f0de 100644 --- a/pin-project-internal/src/pin_projectable/mod.rs +++ b/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::{ - Fields, FieldsNamed, FieldsUnnamed, Generics, Item, ItemEnum, ItemFn, ItemStruct, Meta, - NestedMeta, Result, ReturnType, Type, TypeTuple, Index + Fields, FieldsNamed, FieldsUnnamed, Generics, Index, Item, ItemEnum, ItemFn, ItemStruct, Meta, + NestedMeta, Result, ReturnType, Type, TypeTuple, }; use crate::utils::{Nothing, VecExt}; From db3031cf70189c6a24f33e00e6816ef633e9b8fd Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 7 Aug 2019 21:39:58 -0400 Subject: [PATCH 5/6] Add another clippy allow --- pin-project-internal/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/pin-project-internal/src/lib.rs b/pin-project-internal/src/lib.rs index dc405081..ffbfcc33 100644 --- a/pin-project-internal/src/lib.rs +++ b/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; From 8f6d7fdf0a9bf22727616fbf60f81345229fd300 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sun, 11 Aug 2019 01:19:07 +0900 Subject: [PATCH 6/6] Apply suggestions from code review --- pin-project-internal/src/pin_projectable/mod.rs | 1 - tests/repr_packed.rs | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pin-project-internal/src/pin_projectable/mod.rs b/pin-project-internal/src/pin_projectable/mod.rs index 74e4f0de..e70cb287 100644 --- a/pin-project-internal/src/pin_projectable/mod.rs +++ b/pin-project-internal/src/pin_projectable/mod.rs @@ -35,7 +35,6 @@ fn handle_type(args: TokenStream, item: Item, pinned_drop: Option) -> Re Item::Struct(item) => { 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) } diff --git a/tests/repr_packed.rs b/tests/repr_packed.rs index 2596360d..0d9d64bc 100644 --- a/tests/repr_packed.rs +++ b/tests/repr_packed.rs @@ -17,7 +17,7 @@ use std::cell::Cell; 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 + // the layout of our #[repr(packed)] type. thread_local! { static FIELD_ADDR: Cell = Cell::new(0); } @@ -39,8 +39,8 @@ fn weird_repr_packed() { // 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 + // into the 'drop' function, resulting in a different + // address. let foo = Foo { field: 27 }; let field_addr = &foo.field as *const u8 as usize; field_addr