Bug 1330769 - Avoid using Symbol addresses in hash codes. r=jandem.
authorJason Orendorff <jorendorff@mozilla.com>
Thu, 12 Jan 2017 14:29:38 -0600
changeset 329718 26ed78caca3d
parent 329717 b353d014c22b
child 329719 79d6e9229fda
push id31222
push userryanvm@gmail.com
push dateWed, 18 Jan 2017 14:24:23 +0000
treeherdermozilla-central@5b544b8ab06b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1330769
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1330769 - Avoid using Symbol addresses in hash codes. r=jandem. 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.isGCThing(), "do not reveal pointers via hash codes");
     return v.asRawBits();
 }
 
 HashNumber
--- a/js/src/jscntxt.h
+++ b/js/src/jscntxt.h
@@ -9,16 +9,17 @@
 #ifndef jscntxt_h
 #define jscntxt_h
 
 #include "mozilla/MemoryReporting.h"
 
 #include "js/CharacterEncoding.h"
 #include "js/GCVector.h"
 #include "js/Result.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
@@ -1289,16 +1289,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
@@ -701,16 +701,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
@@ -11,16 +11,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().