From b78eafcfd33bf487b12b2257282d6353bc9f84ec Mon Sep 17 00:00:00 2001
From: Nick Fitzgerald <fitzgen@gmail.com>
Date: Mon, 1 Jun 2020 14:17:04 -0700
Subject: [PATCH] externref: Do not impl common traits, have free functions
 instead

If you aren't expecting `VMExternRef`'s pointer-equality semantics, then these
trait implementations can be foot guns. Instead of implementing the trait, make
free functions in the `VMExternRef` namespace. This way, callers have to be a
little more explicit.
---
 crates/runtime/src/externref.rs | 52 ++++++++++++++++++---------------
 crates/wasmtime/src/ref.rs      |  2 +-
 crates/wasmtime/src/runtime.rs  | 28 +++++++++++++++---
 3 files changed, 53 insertions(+), 29 deletions(-)

diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs
index 1a43e33932..8e8aeecfea 100644
--- a/crates/runtime/src/externref.rs
+++ b/crates/runtime/src/externref.rs
@@ -77,7 +77,7 @@ use std::alloc::Layout;
 use std::any::Any;
 use std::cell::UnsafeCell;
 use std::cmp::Ordering;
-use std::hash::{Hash, Hasher};
+use std::hash::Hasher;
 use std::mem;
 use std::ops::Deref;
 use std::ptr::{self, NonNull};
@@ -344,39 +344,43 @@ impl VMExternRef {
     }
 }
 
-impl PartialEq for VMExternRef {
+/// Methods that would normally be trait implementations, but aren't to avoid
+/// potential footguns around `VMExternRef`'s pointer-equality semantics.
+///
+/// Note that none of these methods are on `&self`, they all require a
+/// fully-qualified `VMExternRef::foo(my_ref)` invocation.
+impl VMExternRef {
+    /// Check whether two `VMExternRef`s point to the same inner allocation.
+    ///
+    /// Note that this uses pointer-equality semantics, not structural-equality
+    /// semantics, and so only pointers are compared, and doesn't use any `Eq`
+    /// or `PartialEq` implementation of the pointed-to values.
     #[inline]
-    fn eq(&self, rhs: &Self) -> bool {
-        ptr::eq(self.0.as_ptr() as *const _, rhs.0.as_ptr() as *const _)
+    pub fn eq(a: &Self, b: &Self) -> bool {
+        ptr::eq(a.0.as_ptr() as *const _, b.0.as_ptr() as *const _)
     }
-}
 
-impl Eq for VMExternRef {}
-
-impl Hash for VMExternRef {
+    /// Hash a given `VMExternRef`.
+    ///
+    /// Note that this just hashes the pointer to the inner value, it does *not*
+    /// use the inner value's `Hash` implementation (if any).
     #[inline]
-    fn hash<H>(&self, hasher: &mut H)
+    pub fn hash<H>(externref: &Self, hasher: &mut H)
     where
         H: Hasher,
     {
-        ptr::hash(self.0.as_ptr() as *const _, hasher);
+        ptr::hash(externref.0.as_ptr() as *const _, hasher);
     }
-}
 
-impl PartialOrd for VMExternRef {
+    /// Compare two `VMExternRef`s.
+    ///
+    /// Note that this uses pointer-equality semantics, not structural-equality
+    /// semantics, and so only pointers are compared, and doesn't use any `Cmp`
+    /// or `PartialCmp` implementation of the pointed-to values.
     #[inline]
-    fn partial_cmp(&self, rhs: &Self) -> Option<Ordering> {
-        let a = self.0.as_ptr() as usize;
-        let b = rhs.0.as_ptr() as usize;
-        a.partial_cmp(&b)
-    }
-}
-
-impl Ord for VMExternRef {
-    #[inline]
-    fn cmp(&self, rhs: &Self) -> Ordering {
-        let a = self.0.as_ptr() as usize;
-        let b = rhs.0.as_ptr() as usize;
+    pub fn cmp(a: &Self, b: &Self) -> Ordering {
+        let a = a.0.as_ptr() as usize;
+        let b = b.0.as_ptr() as usize;
         a.cmp(&b)
     }
 }
diff --git a/crates/wasmtime/src/ref.rs b/crates/wasmtime/src/ref.rs
index 5c6cc540cb..d779b8cc76 100755
--- a/crates/wasmtime/src/ref.rs
+++ b/crates/wasmtime/src/ref.rs
@@ -48,7 +48,7 @@ impl ExternRef {
     /// This is *only* pointer equality, and does *not* run any inner value's
     /// `Eq` implementation.
     pub fn ptr_eq(&self, other: &ExternRef) -> bool {
-        self.inner == other.inner
+        VMExternRef::eq(&self.inner, &other.inner)
     }
 
     /// Returns the host information for this `externref`, if previously created
diff --git a/crates/wasmtime/src/runtime.rs b/crates/wasmtime/src/runtime.rs
index 4c1c514bc8..c2e62dcf5c 100644
--- a/crates/wasmtime/src/runtime.rs
+++ b/crates/wasmtime/src/runtime.rs
@@ -8,6 +8,7 @@ use std::cmp;
 use std::collections::HashMap;
 use std::convert::TryFrom;
 use std::fmt;
+use std::hash::{Hash, Hasher};
 use std::path::Path;
 use std::rc::{Rc, Weak};
 use std::sync::Arc;
@@ -736,7 +737,26 @@ pub(crate) struct StoreInner {
     compiler: RefCell<Compiler>,
     instances: RefCell<Vec<InstanceHandle>>,
     signal_handler: RefCell<Option<Box<SignalHandler<'static>>>>,
-    host_info: RefCell<HashMap<VMExternRef, Rc<RefCell<dyn Any>>>>,
+    host_info: RefCell<HashMap<HostInfoKey, Rc<RefCell<dyn Any>>>>,
+}
+
+struct HostInfoKey(VMExternRef);
+
+impl PartialEq for HostInfoKey {
+    fn eq(&self, rhs: &Self) -> bool {
+        VMExternRef::eq(&self.0, &rhs.0)
+    }
+}
+
+impl Eq for HostInfoKey {}
+
+impl Hash for HostInfoKey {
+    fn hash<H>(&self, hasher: &mut H)
+    where
+        H: Hasher,
+    {
+        VMExternRef::hash(&self.0, hasher);
+    }
 }
 
 impl Store {
@@ -825,7 +845,7 @@ impl Store {
             "externref must be from this store"
         );
         let infos = self.inner.host_info.borrow();
-        infos.get(&externref.inner).cloned()
+        infos.get(&HostInfoKey(externref.inner.clone())).cloned()
     }
 
     pub(crate) fn set_host_info(
@@ -839,9 +859,9 @@ impl Store {
         );
         let mut infos = self.inner.host_info.borrow_mut();
         if let Some(info) = info {
-            infos.insert(externref.inner.clone(), info)
+            infos.insert(HostInfoKey(externref.inner.clone()), info)
         } else {
-            infos.remove(&externref.inner)
+            infos.remove(&HostInfoKey(externref.inner.clone()))
         }
     }