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 312595 747085a1cd93
parent 312594 79a31d8153bf
child 312596 f0329536734b
push id387
push userryanvm@gmail.com
push dateWed, 18 Jan 2017 02:39:30 +0000
treeherdermozilla-esr45@f0329536734b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem, lizzard
bugs1330769
milestone45.7.0
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
@@ -73,24 +73,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
@@ -7,16 +7,17 @@
 /* JS execution context. */
 
 #ifndef jscntxt_h
 #define jscntxt_h
 
 #include "mozilla/MemoryReporting.h"
 
 #include "js/TraceableVector.h"
+#include "js/Utility.h"
 #include "js/Vector.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
@@ -1174,16 +1174,23 @@ JSCompartment::addTelemetry(const char* 
 {
     // Only report telemetry for web content and add-ons, not chrome JS.
     if (isSystem_ || (!addonId && (!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
@@ -609,16 +609,18 @@ struct JSCompartment
 
     // Initialize randomNumberGenerator if needed.
     void ensureRandomNumberGenerator();
 
   private:
     mozilla::non_crypto::XorShift128PlusRNG randomKeyGenerator_;
 
   public:
+    js::HashNumber randomHashCode();
+
     mozilla::HashCodeScrambler randomHashCodeScrambler();
 
   private:
     JSCompartment* thisForCtor() { return this; }
 
   public:
     //
     // The Debugger observes execution on a frame-by-frame basis. The
--- 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
@@ -7,16 +7,17 @@
 #include "vm/ObjectGroup.h"
 
 #include "jshashutil.h"
 #include "jsobj.h"
 
 #include "gc/StoreBuffer.h"
 #include "gc/Zone.h"
 #include "vm/ArrayObject.h"
+#include "vm/Shape.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
@@ -25,16 +25,17 @@
 #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
 
@@ -512,21 +513,43 @@ 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));
+}
+
+struct JsidHasher
+{
+    typedef jsid Lookup;
+    static HashNumber hash(jsid id) {
+        return HashId(id);
+    }
+    static bool match(jsid id1, jsid id2) {
+        return id1 == id2;
+    }
+};
+
 typedef HashSet<ReadBarriered<UnownedBaseShape*>,
                 StackBaseShape,
                 SystemAllocPolicy> BaseShapeSet;
 
-
 class Shape : public gc::TenuredCell
 {
     friend class ::JSObject;
     friend class ::JSFunction;
     friend class Bindings;
     friend class NativeObject;
     friend class PropertyTree;
     friend class StaticBlockObject;
--- a/js/src/vm/String.h
+++ b/js/src/vm/String.h
@@ -1261,35 +1261,16 @@ template <js::AllowGC allowGC>
 inline JSFlatString*
 NewStringCopyZ(js::ExclusiveContext* cx, const char* s)
 {
     return NewStringCopyN<allowGC>(cx, s, strlen(s));
 }
 
 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));
-}
-
-struct JsidHasher
-{
-    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)
 {
     MOZ_ASSERT(cx->compartment() == cx->atomsCompartment());
     MOZ_ASSERT(cx->atomsCompartment()->runtimeFromAnyThread()->currentThreadHasExclusiveAccess());
 
     // 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)
 {
     RootedAtom atom(cx);
     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());
-    return newInternal(cx, code, atom);
+    return newInternal(cx, code, cx->compartment()->randomHashCode(), atom);
 }
 
 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();
     SymbolRegistry::AddPtr p = registry.lookupForAdd(atom);
     if (p)
         return *p;
 
     AutoCompartment ac(cx, cx->atomsCompartment());
-    Symbol* sym = newInternal(cx, SymbolCode::InSymbolRegistry, atom);
+    Symbol* sym = newInternal(cx, SymbolCode::InSymbolRegistry, atom->hash(), atom);
     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,53 +11,61 @@
 
 #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 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);
+    newInternal(js::ExclusiveContext* cx, SymbolCode code, js::HashNumber hash,
+                JSAtom* description);
 
   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 inline js::ThingRootKind rootKind() { return js::THING_ROOT_SYMBOL; }
     inline void traceChildren(JSTracer* trc) {
         if (description_)
             js::TraceManuallyBarrieredEdge(trc, &description_, "description");
     }
@@ -83,17 +91,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().