Remove Clone requirement for Ephemeron::value (#5107)

This PR removes the `Clone` requirement we had for fetching `Ephemeron`
values by introducing an `EphemeronValueRef` that derefs to `&V`.
Soundness is preserved by storing the key of the ephemeron in the
`EphemeronValueRef`, such that any collection won't be able to
deallocate the value. This ensures the reference to `&V` is never
invalid.

Author: jedel1043

core/engine/src/builtins/weak_map/mod.rs

@@ -157,7 +157,7 @@ impl WeakMap {
         // ii. Set p.[[Value]] to empty.
         // iii. Return true.
         // 6. Return false.
-        Ok(map.remove(key.inner()).is_some().into())
+        Ok(map.remove(key.inner()).into())
     }
 
     /// `WeakMap.prototype.get ( key )`
@@ -193,7 +193,13 @@ impl WeakMap {
         // 5. For each Record { [[Key]], [[Value]] } p of entries, do
         // a. If p.[[Key]] is not empty and SameValue(p.[[Key]], key) is true, return p.[[Value]].
         // 6. Return undefined.
-        Ok(map.get(key.inner()).unwrap_or_default())
+        if let Some(entry) = map.get(key.inner())
+            && let Some(val) = entry.value()
+        {
+            Ok(val.clone())
+        } else {
+            Ok(JsValue::undefined())
+        }
     }
 
     /// `WeakMap.prototype.has ( key )`
@@ -318,9 +324,11 @@ impl WeakMap {
         };
 
         // 4. For each Record { [[Key]], [[Value]] } p of M.[[WeakMapData]]
-        if let Some(existing) = map.borrow().data().get(key.inner()) {
+        if let Some(existing) = map.borrow().data().get(key.inner())
+            && let Some(value) = existing.value()
+        {
             // a. If p.[[Key]] is not empty and SameValue(p.[[Key]], key) is true, return p.[[Value]].
-            return Ok(existing);
+            return Ok(value.clone());
         }
 
         // 5-6. Insert the new record with provided value and return it.
@@ -378,9 +386,11 @@ impl WeakMap {
         };
 
         // 5. For each Record { [[Key]], [[Value]] } p of M.[[WeakMapData]]
-        if let Some(existing) = map.borrow().data().get(key_obj.inner()) {
+        if let Some(existing) = map.borrow().data().get(key_obj.inner())
+            && let Some(value) = existing.value()
+        {
             // a. If p.[[Key]] is not empty and SameValue(p.[[Key]], key) is true, return p.[[Value]].
-            return Ok(existing);
+            return Ok(value.clone());
         }
 
         // 6. Let value be ? Call(callback, undefined, « key »).

core/engine/src/builtins/weak_set/mod.rs

@@ -212,7 +212,7 @@ impl WeakSet {
         // i. Replace the element of entries whose value is e with an element whose value is empty.
         // ii. Return true.
         // 6. Return false.
-        Ok(set.remove(value.inner()).is_some().into())
+        Ok(set.remove(value.inner()).into())
     }
 
     /// `WeakSet.prototype.has( value )`

core/gc/src/internals/weak_map_box.rs

@@ -17,7 +17,7 @@ pub(crate) trait ErasedWeakMapBox {
     unsafe fn trace(&self, tracer: &mut Tracer);
 }
 
-impl<K: Trace + ?Sized, V: Trace + Clone> ErasedWeakMapBox for WeakMapBox<K, V> {
+impl<K: Trace + ?Sized, V: Trace> ErasedWeakMapBox for WeakMapBox<K, V> {
     fn clear_dead_entries(&self) {
         if let Some(map) = self.map.upgrade()
             && let Ok(mut map) = map.try_borrow_mut()

core/gc/src/lib.rs

@@ -164,7 +164,7 @@ impl Allocator {
         })
     }
 
-    fn alloc_weak_map<K: Trace + ?Sized, V: Trace + Clone>() -> WeakMap<K, V> {
+    fn alloc_weak_map<K: Trace + ?Sized, V: Trace>() -> WeakMap<K, V> {
         let weak_map = WeakMap {
             inner: Gc::new(GcRefCell::new(RawWeakMap::new())),
         };

core/gc/src/pointers/ephemeron.rs

@@ -5,7 +5,27 @@ use crate::{
     internals::EphemeronBox,
     trace::{Finalize, Trace},
 };
-use std::ptr::NonNull;
+use std::{ops::Deref, ptr::NonNull};
+
+/// A reference to the value held by an [`Ephemeron`].
+///
+/// This reference can be thought as a `(Gc<K>, &V)` pair, where the
+/// returned `Gc<K>` ensures that the reference `&V` is always valid until
+/// the `Gc<K>` gets dropped.
+#[derive(Debug)]
+pub struct EphemeronValueRef<'a, K: Trace + ?Sized + 'static, V> {
+    // Only required to maintain the reference `&V` alive.
+    _key: Gc<K>,
+    value: &'a V,
+}
+
+impl<K: Trace + ?Sized, V> Deref for EphemeronValueRef<'_, K, V> {
+    type Target = V;
+
+    fn deref(&self) -> &Self::Target {
+        self.value
+    }
+}
 
 /// A key-value pair where the value becomes inaccessible when the key is garbage collected.
 ///
@@ -22,16 +42,12 @@ pub struct Ephemeron<K: Trace + ?Sized + 'static, V: Trace + 'static> {
     inner_ptr: NonNull<EphemeronBox<K, V>>,
 }
 
-impl<K: Trace + ?Sized, V: Trace + Clone> Ephemeron<K, V> {
-    /// Gets the stored value of this `Ephemeron`, or `None` if the key was already garbage collected.
-    ///
-    /// This needs to return a clone of the value because holding a reference to it between
-    /// garbage collection passes could drop the underlying allocation, causing an Use After Free.
+impl<K: Trace + ?Sized, V: Trace> Ephemeron<K, V> {
+    /// Creates a new `Ephemeron`.
     #[must_use]
-    pub fn value(&self) -> Option<V> {
-        // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer
-        // `inner_ptr`.
-        unsafe { self.inner_ptr.as_ref().value().cloned() }
+    pub fn new(key: &Gc<K>, value: V) -> Self {
+        let inner_ptr = Allocator::alloc_ephemeron(EphemeronBox::new(key, value));
+        Self { inner_ptr }
     }
 
     /// Gets the stored key of this `Ephemeron`, or `None` if the key was already garbage collected.
@@ -51,21 +67,24 @@ impl<K: Trace + ?Sized, V: Trace + Clone> Ephemeron<K, V> {
         Some(unsafe { Gc::from_raw(key_ptr) })
     }
 
-    /// Checks if the [`Ephemeron`] has a value.
+    /// Gets the stored value of this `Ephemeron`, or `None` if the key was already garbage collected.
     #[must_use]
-    pub fn has_value(&self) -> bool {
+    pub fn value(&self) -> Option<EphemeronValueRef<'_, K, V>> {
+        let key = self.key()?;
+
         // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer
         // `inner_ptr`.
-        unsafe { self.inner_ptr.as_ref().value().is_some() }
+        let value = unsafe { self.inner_ptr.as_ref().value()? };
+
+        Some(EphemeronValueRef { _key: key, value })
     }
-}
 
-impl<K: Trace + ?Sized, V: Trace> Ephemeron<K, V> {
-    /// Creates a new `Ephemeron`.
+    /// Checks if the [`Ephemeron`] has a value.
     #[must_use]
-    pub fn new(key: &Gc<K>, value: V) -> Self {
-        let inner_ptr = Allocator::alloc_ephemeron(EphemeronBox::new(key, value));
-        Self { inner_ptr }
+    pub fn has_value(&self) -> bool {
+        // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer
+        // `inner_ptr`.
+        unsafe { self.inner_ptr.as_ref().value().is_some() }
     }
 
     /// Returns `true` if the two `Ephemeron`s point to the same allocation.

core/gc/src/pointers/weak_map.rs

@@ -6,7 +6,7 @@ use hashbrown::{
     hash_table::{Entry as RawEntry, Iter as RawIter},
 };
 
-use crate::{Allocator, Ephemeron, Finalize, Gc, GcRefCell, Trace, custom_trace};
+use crate::{Allocator, Ephemeron, Finalize, Gc, GcRef, GcRefCell, Trace, custom_trace};
 use std::{fmt, hash::BuildHasher, marker::PhantomData};
 
 /// A map that holds weak references to its keys and is traced by the garbage collector.
@@ -21,7 +21,7 @@ unsafe impl<K: Trace + ?Sized + 'static, V: Trace + 'static> Trace for WeakMap<K
     });
 }
 
-impl<K: Trace + ?Sized, V: Trace + Clone> WeakMap<K, V> {
+impl<K: Trace + ?Sized, V: Trace> WeakMap<K, V> {
     /// Creates a new `WeakMap`.
     #[must_use]
     #[inline]
@@ -35,9 +35,10 @@ impl<K: Trace + ?Sized, V: Trace + Clone> WeakMap<K, V> {
         self.inner.borrow_mut().insert(key, value);
     }
 
-    /// Removes a key from the map, returning the value at the key if the key was previously in the map.
+    /// Removes a key from the map, returning `true` if the key was previously in
+    /// the map.
     #[inline]
-    pub fn remove(&mut self, key: &Gc<K>) -> Option<V> {
+    pub fn remove(&mut self, key: &Gc<K>) -> bool {
         self.inner.borrow_mut().remove(key)
     }
 
@@ -48,11 +49,11 @@ impl<K: Trace + ?Sized, V: Trace + Clone> WeakMap<K, V> {
         self.inner.borrow().contains_key(key)
     }
 
-    /// Returns a reference to the value corresponding to the key.
+    /// Returns a reference to the ephemeron corresponding to the key.
     #[must_use]
     #[inline]
-    pub fn get(&self, key: &Gc<K>) -> Option<V> {
-        self.inner.borrow().get(key)
+    pub fn get<'a>(&'a self, key: &Gc<K>) -> Option<GcRef<'a, Ephemeron<K, V>>> {
+        GcRef::try_map(self.inner.borrow(), |inner| inner.get(key))
     }
 }
 
@@ -222,7 +223,7 @@ where
 impl<K, V, S> RawWeakMap<K, V, S>
 where
     K: Trace + ?Sized + 'static,
-    V: Trace + Clone + 'static,
+    V: Trace + 'static,
     S: BuildHasher,
 {
     /// Reserves capacity for at least `additional` more elements to be inserted
@@ -275,14 +276,13 @@ where
             .shrink_to(min_capacity, make_hasher::<_, V, S>(&self.hash_builder));
     }
 
-    /// Returns the value corresponding to the supplied key.
-    // TODO: make this return a reference instead of cloning.
-    pub(crate) fn get(&self, k: &Gc<K>) -> Option<V> {
+    /// Returns the ephemeron corresponding to the supplied key.
+    pub(crate) fn get(&self, k: &Gc<K>) -> Option<&Ephemeron<K, V>> {
         if self.table.is_empty() {
             None
         } else {
             let hash = make_hash_from_gc(&self.hash_builder, k);
-            self.table.find(hash, equivalent_key(k))?.value()
+            self.table.find(hash, equivalent_key(k))
         }
     }
 
@@ -313,16 +313,16 @@ where
         old
     }
 
-    /// Removes a key from the map, returning the value at the key if the key
+    /// Removes a key from the map, returning `true` if the key
     /// was previously in the map. Keeps the allocated memory for reuse.
-    pub(crate) fn remove(&mut self, k: &Gc<K>) -> Option<V> {
+    pub(crate) fn remove(&mut self, k: &Gc<K>) -> bool {
         let hash = make_hash_from_gc(&self.hash_builder, k);
-        self.table
-            .find_entry(hash, equivalent_key(k))
-            .ok()?
-            .remove()
-            .0
-            .value()
+        if let Ok(entry) = self.table.find_entry(hash, equivalent_key(k)) {
+            entry.remove();
+            true
+        } else {
+            false
+        }
     }
 
     /// Clears all the expired keys in the map.

core/gc/src/test/weak.rs

@@ -76,12 +76,12 @@ mod miri {
                 drop(cloned_gc);
                 force_collect();
                 assert_eq!(wrap.upgrade().as_deref().map(String::as_str), Some("foo"));
-                assert_eq!(eph.value(), Some(3));
+                assert_eq!(&*eph.value().unwrap(), &3);
 
                 drop(gc_value);
                 force_collect();
                 assert!(wrap.upgrade().is_none());
-                assert_eq!(eph.value(), Some(3));
+                assert_eq!(&*eph.value().unwrap(), &3);
 
                 drop(wrap);
                 force_collect();
@@ -99,7 +99,7 @@ mod miri {
             let eph = Ephemeron::new(&gc_value, 4);
             let _fourth = Gc::new("tail");
 
-            assert_eq!(eph.value(), Some(4));
+            assert_eq!(&*eph.value().unwrap(), &4);
         });
     }
 

core/gc/src/test/weak_map.rs

@@ -126,7 +126,7 @@ mod miri {
             assert!(map.contains_key(&key));
             assert!(map.contains_key(&key_copy));
 
-            assert_eq!(map.remove(&key), Some(()));
+            assert!(map.remove(&key));
 
             map.insert(&key, ());