From dfeb84bfce2be11327749c152b3c8f4ea4304e12 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <vorner@vorner.cz>
Date: Thu, 10 Dec 2020 21:17:14 +0100
Subject: [PATCH] Fix soundness hole around access::Map

The assumption that the address of access's guarded reference stays the
same is not true.

Costs:
* The Map is now slower and adds an allocation.
* It can stop being Copy (but non-trivial guards weren't anyway) and
  it can stop being Sync/Send if the closure is not.
* The taken closure needs to be Clone.

Fixes #45.

Technically, it is a breaking change, but the plan is not to raise major
version, because:

* Even rust std gives exception to break compatibility for soundness
  hole fixes.
* It is not that likely people's code would break.
* Even if it breaks, they are much more likely to go to the fixed
  version then if the version got bumped and that's what they should be
  doing ASAP due to the potential UB.
---
 src/access.rs | 55 ++++++++++++++++++---------------------------------
 src/lib.rs    |  2 +-
 2 files changed, 20 insertions(+), 37 deletions(-)

diff --git a/src/access.rs b/src/access.rs
index 92778f5..81182d0 100644
--- a/src/access.rs
+++ b/src/access.rs
@@ -1,3 +1,5 @@
+#![deny(unsafe_code)]
+
 //! Abstracting over accessing parts of stored value.
 //!
 //! Sometimes, there's a big globalish data structure (like a configuration for the whole program).
@@ -205,39 +207,20 @@ where
 
 #[doc(hidden)]
 #[derive(Copy, Clone, Debug)]
-pub struct MapGuard<G, T> {
-    _guard: G,
-    value: *const T,
-}
-
-// Why these are safe:
-// * The *const T is actually used just as a &const T with 'self lifetime (which can't be done in
-//   Rust). So if the reference is Send/Sync, so is the raw pointer.
-unsafe impl<G, T> Send for MapGuard<G, T>
-where
-    G: Send,
-    for<'a> &'a T: Send,
-{
+pub struct MapGuard<G, F, T, R> {
+    guard: G,
+    projection: F,
+    _t: PhantomData<fn(&T) -> &R>,
 }
 
-unsafe impl<G, T> Sync for MapGuard<G, T>
+impl<G, F, T, R> Deref for MapGuard<G, F, T, R>
 where
-    G: Sync,
-    for<'a> &'a T: Sync,
+    G: Deref<Target = T>,
+    F: Fn(&T) -> &R,
 {
-}
-
-impl<G, T> Deref for MapGuard<G, T> {
-    type Target = T;
-    fn deref(&self) -> &T {
-        // Why this is safe:
-        // * The pointer is originally converted from a reference. It's not null, it's aligned,
-        //   it's the right type, etc.
-        // * The pointee couldn't have gone away ‒ the guard keeps the original reference alive, so
-        //   must the new one still be alive too. Moving the guard is fine, we assume the RefCnt is
-        //   Pin (because it's Arc or Rc or something like that ‒ when that one moves, the data it
-        //   points to stay at the same place).
-        unsafe { &*self.value }
+    type Target = R;
+    fn deref(&self) -> &R {
+        (self.projection)(&self.guard)
     }
 }
 
@@ -266,7 +249,7 @@ impl<A, T, F> Map<A, T, F> {
     ///   *cheap* (like only taking reference).
     pub fn new<R>(access: A, projection: F) -> Self
     where
-        F: Fn(&T) -> &R,
+        F: Fn(&T) -> &R + Clone,
     {
         Map {
             access,
@@ -276,18 +259,18 @@ impl<A, T, F> Map<A, T, F> {
     }
 }
 
-impl<A, T, F, R> Access<R> for Map<A, T, F>
+impl<A, F, T, R> Access<R> for Map<A, T, F>
 where
     A: Access<T>,
-    F: Fn(&T) -> &R,
+    F: Fn(&T) -> &R + Clone,
 {
-    type Guard = MapGuard<A::Guard, R>;
+    type Guard = MapGuard<A::Guard, F, T, R>;
     fn load(&self) -> Self::Guard {
         let guard = self.access.load();
-        let value: *const _ = (self.projection)(&guard);
         MapGuard {
-            _guard: guard,
-            value,
+            guard,
+            projection: self.projection.clone(),
+            _t: PhantomData,
         }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
index c0d326e..c70aeea 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -660,7 +660,7 @@ impl<T: RefCnt, S: Strategy<T>> ArcSwapAny<T, S> {
     /// ```
     pub fn map<I, R, F>(&self, f: F) -> Map<&Self, I, F>
     where
-        F: Fn(&I) -> &R,
+        F: Fn(&I) -> &R + Clone,
         Self: Access<I>,
     {
         Map::new(self, f)