Bug 1330769 - Avoid using Symbol addresses in hash codes. r=jandem, a=lizzard
authorJason Orendorff <jorendorff@mozilla.com>
Thu, 12 Jan 2017 14:29:38 -0600
changeset 353617 417ac0789e97f17f5877af0d89e6a157a379663c
parent 353616 68165dbbbd840d7459092d028c9dc65bdaeb4ef0
child 353618 595fc6dd43972bd684e648de5aed8bb798b3b569
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-esr52@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem, lizzard
bugs1330769
milestone52.0a2
Bug 1330769 - Avoid using Symbol addresses in hash codes. r=jandem, a=lizzard MozReview-Commit-ID: 9kllbUYaXLv
js/src/builtin/MapObject.cpp
js/src/jscntxt.h
js/src/jscompartment.cpp
js/src/jscompartment.h
js/src/jswatchpoint.cpp
js/src/vm/ObjectGroup.cpp
js/src/vm/Shape.h
js/src/vm/String.h
js/src/vm/Symbol.cpp
js/src/vm/Symbol.h
--- a/js/src/builtin/MapObject.cpp
+++ b/js/src/builtin/MapObject.cpp
@@ -74,24 +74,18 @@ HashValue(const Value& v, const mozilla:
     //
     // To avoid revealing GC of atoms, string-based hash codes are computed
     // from the string contents rather than any pointer; to avoid revealing
     // addresses, pointer-based hash codes are computed using the
     // HashCodeScrambler.
 
     if (v.isString())
         return v.toString()->asAtom().hash();
-    if (v.isSymbol()) {
-        Symbol* sym = v.toSymbol();
-        if (sym->isWellKnownSymbol())
-            return HashNumber(sym->code());
-        if (sym->code() == SymbolCode::InSymbolRegistry)
-            return sym->description()->hash();
-        return hcs.scramble(v.asRawBits());
-    }
+    if (v.isSymbol())
+        return v.toSymbol()->hash();
     if (v.isObject())
         return hcs.scramble(v.asRawBits());
 
     MOZ_ASSERT(v.isNull() || !v.isGCThing(), "do not reveal pointers via hash codes");
     return v.asRawBits();
 }
 
 HashNumber
--- a/js/src/jscntxt.h
+++ b/js/src/jscntxt.h
@@ -8,16 +8,17 @@
 
 #ifndef jscntxt_h
 #define jscntxt_h
 
 #include "mozilla/MemoryReporting.h"
 
 #include "js/CharacterEncoding.h"
 #include "js/GCVector.h"
+#include "js/Utility.h"
 #include "js/Vector.h"
 #include "vm/Caches.h"
 #include "vm/Runtime.h"
 
 #ifdef _MSC_VER
 #pragma warning(push)
 #pragma warning(disable:4100) /* Silence unreferenced formal parameter warnings */
 #endif
--- a/js/src/jscompartment.cpp
+++ b/js/src/jscompartment.cpp
@@ -1281,16 +1281,23 @@ JSCompartment::addTelemetry(const char* 
     if (isSystem_)
         return;
     if (!creationOptions_.addonIdOrNull() && (!filename || strncmp(filename, "http", 4) != 0))
         return;
 
     sawDeprecatedLanguageExtension[e] = true;
 }
 
+HashNumber
+JSCompartment::randomHashCode()
+{
+    ensureRandomNumberGenerator();
+    return HashNumber(randomNumberGenerator.ref().next());
+}
+
 mozilla::HashCodeScrambler
 JSCompartment::randomHashCodeScrambler()
 {
     return mozilla::HashCodeScrambler(randomKeyGenerator_.next(),
                                       randomKeyGenerator_.next());
 }
 
 AutoSetNewObjectMetadata::AutoSetNewObjectMetadata(ExclusiveContext* ecx
--- a/js/src/jscompartment.h
+++ b/js/src/jscompartment.h
@@ -700,16 +700,18 @@ struct JSCompartment
 
     // Initialize randomNumberGenerator if needed.
     void ensureRandomNumberGenerator();
 
   private:
     mozilla::non_crypto::XorShift128PlusRNG randomKeyGenerator_;
 
   public:
+    js::HashNumber randomHashCode();
+
     mozilla::HashCodeScrambler randomHashCodeScrambler();
 
     static size_t offsetOfRegExps() {
         return offsetof(JSCompartment, regExps);
     }
 
   private:
     JSCompartment* thisForCtor() { return this; }
--- a/js/src/jswatchpoint.cpp
+++ b/js/src/jswatchpoint.cpp
@@ -6,16 +6,17 @@
 
 #include "jswatchpoint.h"
 
 #include "jsatom.h"
 #include "jscompartment.h"
 #include "jsfriendapi.h"
 
 #include "gc/Marking.h"
+#include "vm/Shape.h"
 
 #include "jsgcinlines.h"
 
 using namespace js;
 using namespace js::gc;
 
 inline HashNumber
 WatchKeyHasher::hash(const Lookup& key)
--- a/js/src/vm/ObjectGroup.cpp
+++ b/js/src/vm/ObjectGroup.cpp
@@ -10,16 +10,17 @@
 #include "jsobj.h"
 
 #include "gc/Marking.h"
 #include "gc/Policy.h"
 #include "gc/StoreBuffer.h"
 #include "gc/Zone.h"
 #include "js/CharacterEncoding.h"
 #include "vm/ArrayObject.h"
+#include "vm/Shape.h"
 #include "vm/TaggedProto.h"
 #include "vm/UnboxedObject.h"
 
 #include "jsobjinlines.h"
 
 #include "vm/UnboxedObject-inl.h"
 
 using namespace js;
--- a/js/src/vm/Shape.h
+++ b/js/src/vm/Shape.h
@@ -24,16 +24,18 @@
 #include "gc/Heap.h"
 #include "gc/Marking.h"
 #include "gc/Rooting.h"
 #include "js/HashTable.h"
 #include "js/MemoryMetrics.h"
 #include "js/RootingAPI.h"
 #include "js/UbiNode.h"
 #include "vm/ObjectGroup.h"
+#include "vm/String.h"
+#include "vm/Symbol.h"
 
 #ifdef _MSC_VER
 #pragma warning(push)
 #pragma warning(disable:4800)
 #pragma warning(push)
 #pragma warning(disable:4100) /* Silence unreferenced formal parameter warnings */
 #endif
 
@@ -558,16 +560,40 @@ struct StackBaseShape : public DefaultHa
             MOZ_ASSERT(!base->isOwned());
         }
     };
 
     static inline HashNumber hash(const Lookup& lookup);
     static inline bool match(ReadBarriered<UnownedBaseShape*> key, const Lookup& lookup);
 };
 
+static MOZ_ALWAYS_INLINE js::HashNumber
+HashId(jsid id)
+{
+    // HashGeneric alone would work, but bits of atom and symbol addresses
+    // could then be recovered from the hash code. See bug 1330769.
+    if (MOZ_LIKELY(JSID_IS_ATOM(id)))
+        return JSID_TO_ATOM(id)->hash();
+    if (JSID_IS_SYMBOL(id))
+        return JSID_TO_SYMBOL(id)->hash();
+    return mozilla::HashGeneric(JSID_BITS(id));
+}
+
+template <>
+struct DefaultHasher<jsid>
+{
+    typedef jsid Lookup;
+    static HashNumber hash(jsid id) {
+        return HashId(id);
+    }
+    static bool match(jsid id1, jsid id2) {
+        return id1 == id2;
+    }
+};
+
 using BaseShapeSet = JS::GCHashSet<ReadBarriered<UnownedBaseShape*>,
                                    StackBaseShape,
                                    SystemAllocPolicy>;
 
 class Shape : public gc::TenuredCell
 {
     friend class ::JSObject;
     friend class ::JSFunction;
--- a/js/src/vm/String.h
+++ b/js/src/vm/String.h
@@ -1295,36 +1295,16 @@ template <js::AllowGC allowGC>
 inline JSFlatString*
 NewStringCopyUTF8Z(JSContext* cx, const JS::ConstUTF8CharsZ utf8)
 {
     return NewStringCopyUTF8N<allowGC>(cx, JS::UTF8Chars(utf8.c_str(), strlen(utf8.c_str())));
 }
 
 JS_STATIC_ASSERT(sizeof(HashNumber) == 4);
 
-static MOZ_ALWAYS_INLINE js::HashNumber
-HashId(jsid id)
-{
-    if (MOZ_LIKELY(JSID_IS_ATOM(id)))
-        return JSID_TO_ATOM(id)->hash();
-    return mozilla::HashGeneric(JSID_BITS(id));
-}
-
-template <>
-struct DefaultHasher<jsid>
-{
-    typedef jsid Lookup;
-    static HashNumber hash(jsid id) {
-        return HashId(id);
-    }
-    static bool match(jsid id1, jsid id2) {
-        return id1 == id2;
-    }
-};
-
 } /* namespace js */
 
 // Addon IDs are interned atoms which are never destroyed. This detail is
 // not exposed outside the API.
 class JSAddonId : public JSAtom
 {};
 
 MOZ_ALWAYS_INLINE bool
--- a/js/src/vm/Symbol.cpp
+++ b/js/src/vm/Symbol.cpp
@@ -15,45 +15,45 @@
 #include "vm/StringBuffer.h"
 
 #include "jscompartmentinlines.h"
 
 using JS::Symbol;
 using namespace js;
 
 Symbol*
-Symbol::newInternal(ExclusiveContext* cx, JS::SymbolCode code, JSAtom* description,
+Symbol::newInternal(ExclusiveContext* cx, JS::SymbolCode code, uint32_t hash, JSAtom* description,
                     AutoLockForExclusiveAccess& lock)
 {
     MOZ_ASSERT(cx->compartment() == cx->atomsCompartment(lock));
 
     // Following js::AtomizeString, we grudgingly forgo last-ditch GC here.
     Symbol* p = Allocate<JS::Symbol, NoGC>(cx);
     if (!p) {
         ReportOutOfMemory(cx);
         return nullptr;
     }
-    return new (p) Symbol(code, description);
+    return new (p) Symbol(code, hash, description);
 }
 
 Symbol*
 Symbol::new_(ExclusiveContext* cx, JS::SymbolCode code, JSString* description)
 {
     JSAtom* atom = nullptr;
     if (description) {
         atom = AtomizeString(cx, description);
         if (!atom)
             return nullptr;
     }
 
     // Lock to allocate. If symbol allocation becomes a bottleneck, this can
     // probably be replaced with an assertion that we're on the main thread.
     AutoLockForExclusiveAccess lock(cx);
     AutoCompartment ac(cx, cx->atomsCompartment(lock), &lock);
-    return newInternal(cx, code, atom, lock);
+    return newInternal(cx, code, cx->compartment()->randomHashCode(), atom, lock);
 }
 
 Symbol*
 Symbol::for_(js::ExclusiveContext* cx, HandleString description)
 {
     JSAtom* atom = AtomizeString(cx, description);
     if (!atom)
         return nullptr;
@@ -61,17 +61,17 @@ Symbol::for_(js::ExclusiveContext* cx, H
     AutoLockForExclusiveAccess lock(cx);
 
     SymbolRegistry& registry = cx->symbolRegistry(lock);
     SymbolRegistry::AddPtr p = registry.lookupForAdd(atom);
     if (p)
         return *p;
 
     AutoCompartment ac(cx, cx->atomsCompartment(lock), &lock);
-    Symbol* sym = newInternal(cx, SymbolCode::InSymbolRegistry, atom, lock);
+    Symbol* sym = newInternal(cx, SymbolCode::InSymbolRegistry, atom->hash(), atom, lock);
     if (!sym)
         return nullptr;
 
     // p is still valid here because we have held the lock since the
     // lookupForAdd call, and newInternal can't GC.
     if (!registry.add(p, sym)) {
         // SystemAllocPolicy does not report OOM.
         ReportOutOfMemory(cx);
--- a/js/src/vm/Symbol.h
+++ b/js/src/vm/Symbol.h
@@ -11,58 +11,65 @@
 
 #include <stdio.h>
 
 #include "jsalloc.h"
 #include "jsapi.h"
 
 #include "gc/Barrier.h"
 #include "gc/Marking.h"
-
 #include "js/GCHashTable.h"
 #include "js/RootingAPI.h"
 #include "js/TypeDecls.h"
+#include "js/Utility.h"
+#include "vm/String.h"
 
 namespace js {
 class AutoLockForExclusiveAccess;
 } // namespace js
 
 namespace JS {
 
 class Symbol : public js::gc::TenuredCell
 {
   private:
     SymbolCode code_;
+
+    // Each Symbol gets its own hash code so that we don't have to use
+    // addresses as hash codes (a security hazard).
+    js::HashNumber hash_;
+
     JSAtom* description_;
 
     // The minimum allocation size is sizeof(JSString): 16 bytes on 32-bit
-    // architectures and 24 bytes on 64-bit.  8 bytes of padding makes Symbol
+    // architectures and 24 bytes on 64-bit.  A size_t of padding makes Symbol
     // the minimum size on both.
-    uint64_t unused2_;
+    size_t unused_;
 
-    Symbol(SymbolCode code, JSAtom* desc)
-        : code_(code), description_(desc)
+    Symbol(SymbolCode code, js::HashNumber hash, JSAtom* desc)
+        : code_(code), hash_(hash), description_(desc)
     {
-        // Silence warnings about unused2 being... unused.
-        (void)unused2_;
+        // Silence warnings about unused_ being... unused.
+        (void)unused_;
     }
 
     Symbol(const Symbol&) = delete;
     void operator=(const Symbol&) = delete;
 
     static Symbol*
-    newInternal(js::ExclusiveContext* cx, SymbolCode code, JSAtom* description,
-                js::AutoLockForExclusiveAccess& lock);
+    newInternal(js::ExclusiveContext* cx, SymbolCode code, js::HashNumber hash,
+                JSAtom* description, js::AutoLockForExclusiveAccess& lock);
 
   public:
     static Symbol* new_(js::ExclusiveContext* cx, SymbolCode code, JSString* description);
     static Symbol* for_(js::ExclusiveContext* cx, js::HandleString description);
 
     JSAtom* description() const { return description_; }
     SymbolCode code() const { return code_; }
+    js::HashNumber hash() const { return hash_; }
 
     bool isWellKnownSymbol() const { return uint32_t(code_) < WellKnownSymbolLimit; }
 
     static const JS::TraceKind TraceKind = JS::TraceKind::Symbol;
     inline void traceChildren(JSTracer* trc) {
         if (description_)
             js::TraceManuallyBarrieredEdge(trc, &description_, "description");
     }
@@ -88,17 +95,17 @@ namespace js {
 
 /* Hash policy used by the SymbolRegistry. */
 struct HashSymbolsByDescription
 {
     typedef JS::Symbol* Key;
     typedef JSAtom* Lookup;
 
     static HashNumber hash(Lookup l) {
-        return HashNumber(reinterpret_cast<uintptr_t>(l));
+        return HashNumber(l->hash());
     }
     static bool match(Key sym, Lookup l) {
         return sym->description() == l;
     }
 };
 
 /*
  * The runtime-wide symbol registry, used to implement Symbol.for().