From 34b809fbd0db89206ddb91afc4ee8b7fc7de3b3b 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

Backport to the 0.4 version, releasing as 0.4.8.

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.
---
 CHANGELOG.md  |  5 +++++
 Cargo.toml    |  2 +-
 src/access.rs | 55 ++++++++++++++++++---------------------------------
 src/lib.rs    |  2 +-
 4 files changed, 26 insertions(+), 38 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b75e0fa..375f20f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,8 @@
+# 0.4.8
+
+* Backport of fix to soundness issue in #45 (access::Map from Constant can lead
+  to dangling references).
+
 # 0.4.7
 
 * Rename the `unstable-weak` to `weak` feature. The support is now available on
diff --git a/Cargo.toml b/Cargo.toml
index 3aaaf30..f9c01d8 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "arc-swap"
-version = "0.4.7"
+version = "0.4.8"
 authors = ["Michal 'vorner' Vaner <vorner@vorner.cz>"]
 description = "Atomically swappable Arc"
 documentation = "https://docs.rs/arc-swap"
diff --git a/src/access.rs b/src/access.rs
index 650b508..2aa1998 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).
@@ -216,39 +218,20 @@ where
 /// This is the guard type for [`Map`]. It is accessible and nameable, but is not expected to be
 /// generally used directly.
 #[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)
     }
 }
 
@@ -277,7 +260,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,
@@ -287,18 +270,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 58a196b..bc71a8b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1217,7 +1217,7 @@ impl<T: RefCnt, S: LockStorage> 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)