From 1cf0db19d45ca9fb4a3a10999dc116c37e06adf8 Mon Sep 17 00:00:00 2001 From: Konrad Borowski Date: Thu, 5 Dec 2019 00:01:03 +0100 Subject: [PATCH 1/4] Use deref target in Pin trait implementations Using deref target instead of pointer itself avoids providing access to `&Rc` for malicious implementations, which would allow calling `Rc::get_mut`. This is a breaking change necessary due to unsoundness, however the impact of it should be minimal. This only fixes the issue with malicious `PartialEq` implementations, other `Pin` soundness issues are still here. See for more details. --- src/libcore/pin.rs | 52 +++++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/src/libcore/pin.rs b/src/libcore/pin.rs index 88fa718ae9e..f4e0e647434 100644 --- a/src/libcore/pin.rs +++ b/src/libcore/pin.rs @@ -376,6 +376,7 @@ use crate::cmp::{self, PartialEq, PartialOrd}; use crate::fmt; +use crate::hash::{Hash, Hasher}; use crate::marker::{Sized, Unpin}; use crate::ops::{CoerceUnsized, Deref, DerefMut, DispatchFromDyn, Receiver}; @@ -390,55 +391,72 @@ use crate::ops::{CoerceUnsized, Deref, DerefMut, DispatchFromDyn, Receiver}; /// [`Unpin`]: ../../std/marker/trait.Unpin.html /// [`pin` module]: ../../std/pin/index.html // -// Note: the derives below, and the explicit `PartialEq` and `PartialOrd` -// implementations, are allowed because they all only use `&P`, so they cannot move -// the value behind `pointer`. +// Note: the `Clone` derive below causes unsoundness as it's possible to implement +// `Clone` for mutable references. +// See for more details. #[stable(feature = "pin", since = "1.33.0")] #[lang = "pin"] #[fundamental] #[repr(transparent)] -#[derive(Copy, Clone, Hash, Eq, Ord)] +#[derive(Copy, Clone)] pub struct Pin

{ pointer: P, } -#[stable(feature = "pin_partialeq_partialord_impl_applicability", since = "1.34.0")] -impl PartialEq> for Pin

+#[stable(feature = "pin_trait_impls", since = "1.41.0")] +impl PartialEq> for Pin

where - P: PartialEq, + P::Target: PartialEq, { fn eq(&self, other: &Pin) -> bool { - self.pointer == other.pointer + **self == **other } fn ne(&self, other: &Pin) -> bool { - self.pointer != other.pointer + **self != **other } } -#[stable(feature = "pin_partialeq_partialord_impl_applicability", since = "1.34.0")] -impl PartialOrd> for Pin

+#[stable(feature = "pin_trait_impls", since = "1.41.0")] +impl> Eq for Pin

{} + +#[stable(feature = "pin_trait_impls", since = "1.41.0")] +impl PartialOrd> for Pin

where - P: PartialOrd, + P::Target: PartialOrd, { fn partial_cmp(&self, other: &Pin) -> Option { - self.pointer.partial_cmp(&other.pointer) + (**self).partial_cmp(other) } fn lt(&self, other: &Pin) -> bool { - self.pointer < other.pointer + **self < **other } fn le(&self, other: &Pin) -> bool { - self.pointer <= other.pointer + **self <= **other } fn gt(&self, other: &Pin) -> bool { - self.pointer > other.pointer + **self > **other } fn ge(&self, other: &Pin) -> bool { - self.pointer >= other.pointer + **self >= **other + } +} + +#[stable(feature = "pin_trait_impls", since = "1.41.0")] +impl> Ord for Pin

{ + fn cmp(&self, other: &Self) -> cmp::Ordering { + (**self).cmp(other) + } +} + +#[stable(feature = "pin_trait_impls", since = "1.41.0")] +impl> Hash for Pin

{ + fn hash(&self, state: &mut H) { + (**self).hash(state); } } From dfcf764d093445d1c5eedbc7ef1f72c7fa5dcac8 Mon Sep 17 00:00:00 2001 From: Konrad Borowski Date: Thu, 5 Dec 2019 10:28:11 +0100 Subject: [PATCH 2/4] Document why Pin implementations aren't derived --- src/libcore/pin.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libcore/pin.rs b/src/libcore/pin.rs index f4e0e647434..015a75ba3dc 100644 --- a/src/libcore/pin.rs +++ b/src/libcore/pin.rs @@ -403,6 +403,12 @@ pub struct Pin

{ pointer: P, } +// The following implementations aren't derived in order to avoid soundness +// issues. `&self.pointer` should not be accessible to untrusted trait +// implementations. +// +// See for more details. + #[stable(feature = "pin_trait_impls", since = "1.41.0")] impl PartialEq> for Pin

where From 6996ae1e79cf67a53115862c1273bfbec5790496 Mon Sep 17 00:00:00 2001 From: Konrad Borowski Date: Thu, 5 Dec 2019 13:21:51 +0100 Subject: [PATCH 3/4] Add UI test for Pin PartialEq unsoundness --- .../issue-67039-unsound-pin-partialeq.rs | 27 +++++++++++++++++++ .../issue-67039-unsound-pin-partialeq.stderr | 13 +++++++++ 2 files changed, 40 insertions(+) create mode 100644 src/test/ui/issues/issue-67039-unsound-pin-partialeq.rs create mode 100644 src/test/ui/issues/issue-67039-unsound-pin-partialeq.stderr diff --git a/src/test/ui/issues/issue-67039-unsound-pin-partialeq.rs b/src/test/ui/issues/issue-67039-unsound-pin-partialeq.rs new file mode 100644 index 00000000000..a496e58a79b --- /dev/null +++ b/src/test/ui/issues/issue-67039-unsound-pin-partialeq.rs @@ -0,0 +1,27 @@ +// Pin's PartialEq implementation allowed to access the pointer allowing for +// unsoundness by using Rc::get_mut to move value within Rc. +// See https://internals.rust-lang.org/t/unsoundness-in-pin/11311/73 for more details. + +use std::ops::Deref; +use std::pin::Pin; +use std::rc::Rc; + +struct Apple; + +impl Deref for Apple { + type Target = Apple; + fn deref(&self) -> &Apple { + &Apple + } +} + +impl PartialEq> for Apple { + fn eq(&self, _rc: &Rc) -> bool { + unreachable!() + } +} + +fn main() { + let _ = Pin::new(Apple) == Rc::pin(Apple); + //~^ ERROR type mismatch resolving +} diff --git a/src/test/ui/issues/issue-67039-unsound-pin-partialeq.stderr b/src/test/ui/issues/issue-67039-unsound-pin-partialeq.stderr new file mode 100644 index 00000000000..3330d60242f --- /dev/null +++ b/src/test/ui/issues/issue-67039-unsound-pin-partialeq.stderr @@ -0,0 +1,13 @@ +error[E0271]: type mismatch resolving ` as std::ops::Deref>::Target == std::rc::Rc` + --> $DIR/issue-67039-unsound-pin-partialeq.rs:25:29 + | +LL | let _ = Pin::new(Apple) == Rc::pin(Apple); + | ^^ expected struct `Apple`, found struct `std::rc::Rc` + | + = note: expected type `Apple` + found struct `std::rc::Rc` + = note: required because of the requirements on the impl of `std::cmp::PartialEq>>` for `std::pin::Pin` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0271`. From 61d9c001465f14be10b519fbb3030f5cebe22199 Mon Sep 17 00:00:00 2001 From: Konrad Borowski Date: Sat, 7 Dec 2019 16:23:43 +0100 Subject: [PATCH 4/4] Explicitly refer to operator methods in Pin impls --- src/libcore/pin.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libcore/pin.rs b/src/libcore/pin.rs index 015a75ba3dc..6a0c5bbebc1 100644 --- a/src/libcore/pin.rs +++ b/src/libcore/pin.rs @@ -415,11 +415,11 @@ where P::Target: PartialEq, { fn eq(&self, other: &Pin) -> bool { - **self == **other + P::Target::eq(self, other) } fn ne(&self, other: &Pin) -> bool { - **self != **other + P::Target::ne(self, other) } } @@ -432,37 +432,37 @@ where P::Target: PartialOrd, { fn partial_cmp(&self, other: &Pin) -> Option { - (**self).partial_cmp(other) + P::Target::partial_cmp(self, other) } fn lt(&self, other: &Pin) -> bool { - **self < **other + P::Target::lt(self, other) } fn le(&self, other: &Pin) -> bool { - **self <= **other + P::Target::le(self, other) } fn gt(&self, other: &Pin) -> bool { - **self > **other + P::Target::gt(self, other) } fn ge(&self, other: &Pin) -> bool { - **self >= **other + P::Target::ge(self, other) } } #[stable(feature = "pin_trait_impls", since = "1.41.0")] impl> Ord for Pin

{ fn cmp(&self, other: &Self) -> cmp::Ordering { - (**self).cmp(other) + P::Target::cmp(self, other) } } #[stable(feature = "pin_trait_impls", since = "1.41.0")] impl> Hash for Pin

{ fn hash(&self, state: &mut H) { - (**self).hash(state); + P::Target::hash(self, state); } }