From e2a44d0c96a6aa7ccb291f75c3aadb05f8c105e6 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Thu, 17 Oct 2019 17:20:38 +0900 Subject: [PATCH] Fix an issue where PinnedDrop implementations can call unsafe code without an unsafe block --- examples/pinned_drop-expanded.rs | 5 +- pin-project-internal/src/pinned_drop.rs | 180 +++++++++++++++++- tests/pinned_drop.rs | 104 ++++++++++ tests/ui/pin_project/self-in-where-clause.rs | 14 ++ .../pin_project/self-in-where-clause.stderr | 32 ++++ tests/ui/pinned_drop/ref-self.rs | 12 ++ tests/ui/pinned_drop/ref-self.stderr | 11 ++ tests/ui/pinned_drop/self-match.rs | 54 ++++++ tests/ui/pinned_drop/self-match.stderr | 95 +++++++++ tests/ui/pinned_drop/unsafe-code.rs | 17 ++ tests/ui/pinned_drop/unsafe-code.stderr | 7 + 11 files changed, 525 insertions(+), 6 deletions(-) create mode 100644 tests/ui/pin_project/self-in-where-clause.rs create mode 100644 tests/ui/pin_project/self-in-where-clause.stderr create mode 100644 tests/ui/pinned_drop/ref-self.rs create mode 100644 tests/ui/pinned_drop/ref-self.stderr create mode 100644 tests/ui/pinned_drop/self-match.rs create mode 100644 tests/ui/pinned_drop/self-match.stderr create mode 100644 tests/ui/pinned_drop/unsafe-code.rs create mode 100644 tests/ui/pinned_drop/unsafe-code.stderr diff --git a/examples/pinned_drop-expanded.rs b/examples/pinned_drop-expanded.rs index f04c0624..6187a987 100644 --- a/examples/pinned_drop-expanded.rs +++ b/examples/pinned_drop-expanded.rs @@ -94,7 +94,10 @@ impl ::pin_project::__private::PinnedDrop for Foo<'_, T> { // Since calling it twice on the same object would be UB, // this method is unsafe. unsafe fn drop(self: Pin<&mut Self>) { - **self.project().was_dropped = true; + fn __drop_inner(__self: Pin<&mut Foo<'_, T>>) { + **__self.project().was_dropped = true; + } + __drop_inner(self); } } diff --git a/pin-project-internal/src/pinned_drop.rs b/pin-project-internal/src/pinned_drop.rs index 9e80b02c..30329615 100644 --- a/pin-project-internal/src/pinned_drop.rs +++ b/pin-project-internal/src/pinned_drop.rs @@ -1,6 +1,13 @@ -use proc_macro2::{Span, TokenStream}; +use std::mem; + +use proc_macro2::{Group, Span, TokenStream, TokenTree}; use quote::{quote, quote_spanned, ToTokens}; -use syn::{spanned::Spanned, *}; +use syn::{ + punctuated::Punctuated, + spanned::Spanned, + visit_mut::{self, VisitMut}, + *, +}; use crate::utils::{parse_as_empty, CURRENT_PRIVATE_MODULE}; @@ -145,9 +152,172 @@ fn parse(item: &mut ItemImpl) -> Result<()> { } } - if let ImplItem::Method(method) = &mut item.items[0] { - method.sig.unsafety = Some(token::Unsafe::default()); - } + expand_item(item); Ok(()) } + +// from: +// +// fn drop(self: Pin<&mut Self>) { +// // something +// } +// +// into: +// +// unsafe fn drop(self: Pin<&mut Self>) { +// fn __drop_inner(__self: Pin<&mut Foo<'_, T>>) { +// // something +// } +// __drop_inner(self); +// } +// +fn expand_item(item: &mut ItemImpl) { + let method = + if let ImplItem::Method(method) = &mut item.items[0] { method } else { unreachable!() }; + let mut drop_inner = method.clone(); + + // `fn drop(mut self: Pin<&mut Self>)` -> `fn __drop_inner(mut __self: Pin<&mut Receiver>)` + drop_inner.sig.ident = Ident::new("__drop_inner", drop_inner.sig.ident.span()); + drop_inner.sig.generics = item.generics.clone(); + if let FnArg::Typed(arg) = &mut drop_inner.sig.inputs[0] { + if let Pat::Ident(ident) = &mut *arg.pat { + prepend_underscore_to_self(&mut ident.ident); + } + } + let mut visitor = ReplaceReceiver::new(&item.self_ty); + visitor.visit_signature_mut(&mut drop_inner.sig); + visitor.visit_block_mut(&mut drop_inner.block); + + // `fn drop(mut self: Pin<&mut Self>)` -> `unsafe fn drop(self: Pin<&mut Self>)` + method.sig.unsafety = Some(token::Unsafe::default()); + if let FnArg::Typed(arg) = &mut method.sig.inputs[0] { + if let Pat::Ident(ident) = &mut *arg.pat { + ident.mutability = None; + } + } + + method.block = syn::parse_quote! {{ + #drop_inner + __drop_inner(self); + }}; +} + +// Replace `self` and `Self` with `__self` and `Receiver`. +// Based on https://github.com/dtolnay/async-trait/blob/1.0.15/src/receiver.rs + +struct ReplaceReceiver<'a> { + self_ty: &'a Type, +} + +#[allow(clippy::similar_names)] // allow `qself` +impl<'a> ReplaceReceiver<'a> { + fn new(self_ty: &'a Type) -> Self { + Self { self_ty } + } + + fn self_to_qself(&mut self, qself: &mut Option, path: &mut Path) { + if path.leading_colon.is_some() { + return; + } + + let first = &path.segments[0]; + if first.ident != "Self" || !first.arguments.is_empty() { + return; + } + + match path.segments.pairs().next().unwrap().punct() { + Some(colon) => path.leading_colon = Some(**colon), + None => return, + } + + *qself = Some(QSelf { + lt_token: token::Lt::default(), + ty: Box::new(self.self_ty.clone()), + position: 0, + as_token: None, + gt_token: token::Gt::default(), + }); + + let segments = mem::replace(&mut path.segments, Punctuated::new()); + path.segments = segments.into_pairs().skip(1).collect(); + } +} + +impl VisitMut for ReplaceReceiver<'_> { + // `Self` -> `Receiver` + fn visit_type_mut(&mut self, ty: &mut Type) { + if let Type::Path(node) = ty { + if node.qself.is_none() && node.path.is_ident("Self") { + *ty = self.self_ty.clone(); + } else { + self.visit_type_path_mut(node); + } + } else { + visit_mut::visit_type_mut(self, ty); + } + } + + // `Self::Assoc` -> `::Assoc` + fn visit_type_path_mut(&mut self, ty: &mut TypePath) { + if ty.qself.is_none() { + self.self_to_qself(&mut ty.qself, &mut ty.path); + } + visit_mut::visit_type_path_mut(self, ty); + } + + // `Self::method` -> `::method` + fn visit_expr_path_mut(&mut self, expr: &mut ExprPath) { + if expr.qself.is_none() { + prepend_underscore_to_self(&mut expr.path.segments[0].ident); + self.self_to_qself(&mut expr.qself, &mut expr.path); + } + visit_mut::visit_expr_path_mut(self, expr); + } + + fn visit_macro_mut(&mut self, node: &mut Macro) { + // We can't tell in general whether `self` inside a macro invocation + // refers to the self in the argument list or a different self + // introduced within the macro. Heuristic: if the macro input contains + // `fn`, then `self` is more likely to refer to something other than the + // outer function's self argument. + if !contains_fn(node.tokens.clone()) { + node.tokens = fold_token_stream(node.tokens.clone()); + } + } + + fn visit_item_mut(&mut self, _: &mut Item) { + // Do not recurse into nested items. + } +} + +fn contains_fn(tokens: TokenStream) -> bool { + tokens.into_iter().any(|tt| match tt { + TokenTree::Ident(ident) => ident == "fn", + TokenTree::Group(group) => contains_fn(group.stream()), + _ => false, + }) +} + +fn fold_token_stream(tokens: TokenStream) -> TokenStream { + tokens + .into_iter() + .map(|tt| match tt { + TokenTree::Ident(mut ident) => { + prepend_underscore_to_self(&mut ident); + TokenTree::Ident(ident) + } + TokenTree::Group(group) => { + let content = fold_token_stream(group.stream()); + TokenTree::Group(Group::new(group.delimiter(), content)) + } + other => other, + }) + .collect() +} + +fn prepend_underscore_to_self(ident: &mut Ident) { + if ident == "self" { + *ident = Ident::new("__self", ident.span()); + } +} diff --git a/tests/pinned_drop.rs b/tests/pinned_drop.rs index 2247b76d..fbafb2dc 100644 --- a/tests/pinned_drop.rs +++ b/tests/pinned_drop.rs @@ -25,3 +25,107 @@ fn safe_project() { drop(Foo { was_dropped: &mut was_dropped, field: 42 }); assert!(was_dropped); } + +#[test] +fn test_mut_argument() { + #[pin_project(PinnedDrop)] + struct Struct { + data: usize, + } + + #[pinned_drop] + impl PinnedDrop for Struct { + fn drop(mut self: Pin<&mut Self>) { + let _: &mut _ = &mut self.data; + } + } +} + +#[test] +fn test_self_in_vec() { + #[pin_project(PinnedDrop)] + struct Struct { + data: usize, + } + + #[pinned_drop] + impl PinnedDrop for Struct { + fn drop(self: Pin<&mut Self>) { + let _: Vec<_> = vec![self.data]; + } + } +} + +#[test] +fn test_self_in_macro_containing_fn() { + #[pin_project(PinnedDrop)] + pub struct Struct { + data: usize, + } + + macro_rules! emit { + ($($tt:tt)*) => { + $($tt)* + }; + } + + #[pinned_drop] + impl PinnedDrop for Struct { + fn drop(self: Pin<&mut Self>) { + let _ = emit!({ + impl Struct { + pub fn f(self) {} + } + }); + self.data; + } + } +} + +#[test] +fn test_call_self() { + #[pin_project(PinnedDrop)] + pub struct Struct { + data: usize, + } + + trait Trait { + fn self_ref(&self) {} + fn self_pin_ref(self: Pin<&Self>) {} + fn self_mut(&mut self) {} + fn self_pin_mut(self: Pin<&mut Self>) {} + fn assoc_fn(_this: Pin<&mut Self>) {} + } + + impl Trait for Struct {} + + #[pinned_drop] + impl PinnedDrop for Struct { + fn drop(mut self: Pin<&mut Self>) { + self.self_ref(); + self.as_ref().self_pin_ref(); + self.self_mut(); + self.as_mut().self_pin_mut(); + Self::assoc_fn(self.as_mut()); + ::assoc_fn(self.as_mut()); + } + } +} + +#[test] +fn test_self_match() { + #[pin_project(PinnedDrop)] + pub struct TupleStruct(usize); + + #[pinned_drop] + impl PinnedDrop for TupleStruct { + #[allow(irrefutable_let_patterns)] + fn drop(mut self: Pin<&mut Self>) { + match *self { + Self(_) => {} + } + if let Self(_) = *self {} + let _: Self = Self(0); + } + } +} diff --git a/tests/ui/pin_project/self-in-where-clause.rs b/tests/ui/pin_project/self-in-where-clause.rs new file mode 100644 index 00000000..e9b2d434 --- /dev/null +++ b/tests/ui/pin_project/self-in-where-clause.rs @@ -0,0 +1,14 @@ +use pin_project::pin_project; + +trait Trait {} + +#[pin_project] +pub struct Struct +where + Self: Trait, //~ ERROR cannot find type `Self` in this scope [E0411] +{ + x: usize, + y: T, +} + +fn main() {} diff --git a/tests/ui/pin_project/self-in-where-clause.stderr b/tests/ui/pin_project/self-in-where-clause.stderr new file mode 100644 index 00000000..c1db2eca --- /dev/null +++ b/tests/ui/pin_project/self-in-where-clause.stderr @@ -0,0 +1,32 @@ +error[E0411]: cannot find type `Self` in this scope + --> $DIR/self-in-where-clause.rs:8:5 + | +8 | Self: Trait, //~ ERROR cannot find type `Self` in this scope [E0411] + | ^^^^ `Self` is only available in impls, traits, and type definitions + +error[E0277]: the trait bound `__Struct<'pin, T>: Trait` is not satisfied + --> $DIR/self-in-where-clause.rs:5:1 + | +5 | #[pin_project] + | ^^^^^^^^^^^^^- + | | | + | | required by `__Struct` + | the trait `Trait` is not implemented for `__Struct<'pin, T>` + +error[E0277]: the trait bound `__StructProjection<'pin, T>: Trait` is not satisfied + --> $DIR/self-in-where-clause.rs:5:14 + | +5 | #[pin_project] + | ^ + | | + | the trait `Trait` is not implemented for `__StructProjection<'pin, T>` + | required by `__StructProjection` + +error[E0277]: the trait bound `__StructProjectionRef<'pin, T>: Trait` is not satisfied + --> $DIR/self-in-where-clause.rs:5:14 + | +5 | #[pin_project] + | ^ + | | + | the trait `Trait` is not implemented for `__StructProjectionRef<'pin, T>` + | required by `__StructProjectionRef` diff --git a/tests/ui/pinned_drop/ref-self.rs b/tests/ui/pinned_drop/ref-self.rs new file mode 100644 index 00000000..06970cfd --- /dev/null +++ b/tests/ui/pinned_drop/ref-self.rs @@ -0,0 +1,12 @@ +use std::pin::Pin; + +pub struct Foo { + field: u8, +} + +impl Foo { + fn method_ref(ref self: Pin<&mut Self>) {} //~ ERROR expected identifier, found keyword `self` + fn method_ref_mut(ref mut self: Pin<&mut Self>) {} //~ ERROR expected identifier, found keyword `self` +} + +fn main() {} diff --git a/tests/ui/pinned_drop/ref-self.stderr b/tests/ui/pinned_drop/ref-self.stderr new file mode 100644 index 00000000..b832bdd3 --- /dev/null +++ b/tests/ui/pinned_drop/ref-self.stderr @@ -0,0 +1,11 @@ +error: expected identifier, found keyword `self` + --> $DIR/ref-self.rs:8:23 + | +8 | fn method_ref(ref self: Pin<&mut Self>) {} //~ ERROR expected identifier, found keyword `self` + | ^^^^ expected identifier, found keyword + +error: expected identifier, found keyword `self` + --> $DIR/ref-self.rs:9:31 + | +9 | fn method_ref_mut(ref mut self: Pin<&mut Self>) {} //~ ERROR expected identifier, found keyword `self` + | ^^^^ expected identifier, found keyword diff --git a/tests/ui/pinned_drop/self-match.rs b/tests/ui/pinned_drop/self-match.rs new file mode 100644 index 00000000..6b1e769a --- /dev/null +++ b/tests/ui/pinned_drop/self-match.rs @@ -0,0 +1,54 @@ +use pin_project::{pin_project, pinned_drop}; +use std::pin::Pin; + +#[pin_project(PinnedDrop)] +pub struct Struct { + x: usize, +} + +#[pinned_drop] +impl PinnedDrop for Struct { + fn drop(mut self: Pin<&mut Self>) { + match *self { + Self { x: _ } => {} //~ ERROR can't use generic parameters from outer function [E0401] + } + if let Self { x: _ } = *self {} //~ ERROR can't use generic parameters from outer function [E0401] + let _: Self = Self { x: 0 }; //~ ERROR can't use generic parameters from outer function [E0401] + } +} + +#[pin_project(PinnedDrop)] +pub struct TupleStruct(usize); + +#[pinned_drop] +impl PinnedDrop for TupleStruct { + fn drop(mut self: Pin<&mut Self>) { + match *self { + Self(_) => {} + } + if let Self(_) = *self {} + let _: Self = Self(0); + } +} + +#[pin_project(PinnedDrop)] +pub enum Enum { + StructVariant { x: usize }, + TupleVariant(usize), +} + +#[pinned_drop] +impl PinnedDrop for Enum { + fn drop(mut self: Pin<&mut Self>) { + match *self { + Self::StructVariant { x: _ } => {} //~ ERROR can't use generic parameters from outer function [E0401] + Self::TupleVariant(_) => {} //~ ERROR can't use generic parameters from outer function [E0401] + } + if let Self::StructVariant { x: _ } = *self {} //~ ERROR can't use generic parameters from outer function [E0401] + if let Self::TupleVariant(_) = *self {} //~ ERROR can't use generic parameters from outer function [E0401] + let _: Self = Self::StructVariant { x: 0 }; //~ ERROR can't use generic parameters from outer function [E0401] + let _: Self = Self::TupleVariant(0); + } +} + +fn main() {} \ No newline at end of file diff --git a/tests/ui/pinned_drop/self-match.stderr b/tests/ui/pinned_drop/self-match.stderr new file mode 100644 index 00000000..9a98ff3c --- /dev/null +++ b/tests/ui/pinned_drop/self-match.stderr @@ -0,0 +1,95 @@ +error[E0401]: can't use generic parameters from outer function + --> $DIR/self-match.rs:13:13 + | +10 | impl PinnedDrop for Struct { + | ---- `Self` type implicitly declared here, by this `impl` +... +13 | Self { x: _ } => {} //~ ERROR can't use generic parameters from outer function [E0401] + | ^^^^ + | | + | use of generic parameter from outer function + | use a type here instead + +error[E0401]: can't use generic parameters from outer function + --> $DIR/self-match.rs:15:16 + | +10 | impl PinnedDrop for Struct { + | ---- `Self` type implicitly declared here, by this `impl` +... +15 | if let Self { x: _ } = *self {} //~ ERROR can't use generic parameters from outer function [E0401] + | ^^^^ + | | + | use of generic parameter from outer function + | use a type here instead + +error[E0401]: can't use generic parameters from outer function + --> $DIR/self-match.rs:16:23 + | +10 | impl PinnedDrop for Struct { + | ---- `Self` type implicitly declared here, by this `impl` +... +16 | let _: Self = Self { x: 0 }; //~ ERROR can't use generic parameters from outer function [E0401] + | ^^^^ + | | + | use of generic parameter from outer function + | use a type here instead + +error[E0401]: can't use generic parameters from outer function + --> $DIR/self-match.rs:44:13 + | +41 | impl PinnedDrop for Enum { + | ---- `Self` type implicitly declared here, by this `impl` +... +44 | Self::StructVariant { x: _ } => {} //~ ERROR can't use generic parameters from outer function [E0401] + | ^^^^^^^^^^^^^^^^^^^ + | | + | use of generic parameter from outer function + | use a type here instead + +error[E0401]: can't use generic parameters from outer function + --> $DIR/self-match.rs:45:13 + | +41 | impl PinnedDrop for Enum { + | ---- `Self` type implicitly declared here, by this `impl` +... +45 | Self::TupleVariant(_) => {} //~ ERROR can't use generic parameters from outer function [E0401] + | ^^^^^^^^^^^^^^^^^^ + | | + | use of generic parameter from outer function + | use a type here instead + +error[E0401]: can't use generic parameters from outer function + --> $DIR/self-match.rs:47:16 + | +41 | impl PinnedDrop for Enum { + | ---- `Self` type implicitly declared here, by this `impl` +... +47 | if let Self::StructVariant { x: _ } = *self {} //~ ERROR can't use generic parameters from outer function [E0401] + | ^^^^^^^^^^^^^^^^^^^ + | | + | use of generic parameter from outer function + | use a type here instead + +error[E0401]: can't use generic parameters from outer function + --> $DIR/self-match.rs:48:16 + | +41 | impl PinnedDrop for Enum { + | ---- `Self` type implicitly declared here, by this `impl` +... +48 | if let Self::TupleVariant(_) = *self {} //~ ERROR can't use generic parameters from outer function [E0401] + | ^^^^^^^^^^^^^^^^^^ + | | + | use of generic parameter from outer function + | use a type here instead + +error[E0401]: can't use generic parameters from outer function + --> $DIR/self-match.rs:49:23 + | +41 | impl PinnedDrop for Enum { + | ---- `Self` type implicitly declared here, by this `impl` +... +49 | let _: Self = Self::StructVariant { x: 0 }; //~ ERROR can't use generic parameters from outer function [E0401] + | ^^^^^^^^^^^^^^^^^^^ + | | + | use of generic parameter from outer function + | use a type here instead diff --git a/tests/ui/pinned_drop/unsafe-code.rs b/tests/ui/pinned_drop/unsafe-code.rs new file mode 100644 index 00000000..51f313be --- /dev/null +++ b/tests/ui/pinned_drop/unsafe-code.rs @@ -0,0 +1,17 @@ +use pin_project::{pin_project, pinned_drop}; +use std::pin::Pin; + +#[pin_project(PinnedDrop)] +pub struct Foo { + #[pin] + field: u8, +} + +#[pinned_drop] +impl PinnedDrop for Foo { + fn drop(self: Pin<&mut Self>) { + self.project().field.get_unchecked_mut(); //~ ERROR call to unsafe function is unsafe and requires unsafe function or block [E0133] + } +} + +fn main() {} diff --git a/tests/ui/pinned_drop/unsafe-code.stderr b/tests/ui/pinned_drop/unsafe-code.stderr new file mode 100644 index 00000000..98a945f6 --- /dev/null +++ b/tests/ui/pinned_drop/unsafe-code.stderr @@ -0,0 +1,7 @@ +error[E0133]: call to unsafe function is unsafe and requires unsafe function or block + --> $DIR/unsafe-code.rs:13:9 + | +13 | self.project().field.get_unchecked_mut(); //~ ERROR call to unsafe function is unsafe and requires unsafe function or block [E0133] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function + | + = note: consult the function's documentation for information on how to avoid undefined behavior