Fix soundness hole around access::Map

vorner · vorner · commit 34b809fbd0db · 2020-12-11T18:56:24.000+01:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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)

Original file line number	Diff line number	Diff line change
`@@ -1217,7 +1217,7 @@ impl<T: RefCnt, S: LockStorage> ArcSwapAny<T, S> {`
`1217`	`1217`	/// ```
`1218`	`1218`	`pub fn map<I, R, F>(&self, f: F) -> Map<&Self, I, F>`
`1219`	`1219`	`where`
`1220`		`- F: Fn(&I) -> &R,`
	`1220`	`+ F: Fn(&I) -> &R + Clone,`
`1221`	`1221`	`Self: Access<I>,`
`1222`	`1222`	`{`
`1223`	`1223`	`Map::new(self, f)`