From eb1f048c43f779aead4b1bc0605d2968eb400d27 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Mon, 25 Nov 2013 11:26:10 +0000 Subject: [PATCH] Bug 939993 - Check that AddPtrs are used only with matching Lookup values r=sfink --- js/public/HashTable.h | 1 + js/src/jshashutil.h | 66 +++++++++++++++++++++++++++++++++++++++ js/src/jsinfer.cpp | 35 +++++++++------------ js/src/vm/Debugger.cpp | 20 ++++++------ js/src/vm/Debugger.h | 11 +++++++ js/src/vm/SelfHosting.cpp | 12 ++++--- js/src/vm/Shape.cpp | 17 +++++----- 7 files changed, 118 insertions(+), 44 deletions(-) create mode 100644 js/src/jshashutil.h diff --git a/js/public/HashTable.h b/js/public/HashTable.h index 03265f5d1b8..2855f78b185 100644 --- a/js/public/HashTable.h +++ b/js/public/HashTable.h @@ -1511,6 +1511,7 @@ class HashTable : private AllocPolicy p.mutationCount = mutationCount; { mozilla::ReentrancyGuard g(*this); + JS_ASSERT(prepareHash(l) == p.keyHash); // l has not been destroyed p.entry_ = &lookup(l, p.keyHash, sCollisionBit); } return p.found() || add(p, mozilla::Forward(u)); diff --git a/js/src/jshashutil.h b/js/src/jshashutil.h new file mode 100644 index 00000000000..6192061b648 --- /dev/null +++ b/js/src/jshashutil.h @@ -0,0 +1,66 @@ +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- + * vim: set ts=8 sts=4 et sw=4 tw=99: + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef jshashutil_h +#define jshashutil_h + +#include "jscntxt.h" + +namespace js { + +/* + * Used to add entries to a js::HashMap or HashSet where the key depends on a GC + * thing that may be moved by generational collection between the call to + * lookupForAdd() and relookupOrAdd(). + */ +template +struct DependentAddPtr +{ + typedef typename T::AddPtr AddPtr; + typedef typename T::Entry Entry; + + template + DependentAddPtr(const ExclusiveContext *cx, const T &table, const Lookup &lookup) + : addPtr(table.lookupForAdd(lookup)) +#ifdef JSGC_GENERATIONAL + , cx(cx) + , originalGcNumber(cx->zone()->gcNumber()) +#endif + {} + + template + bool add(T &table, const KeyInput &key, const ValueInput &value) { +#ifdef JSGC_GENERATIONAL + bool gcHappened = originalGcNumber != cx->zone()->gcNumber(); + if (gcHappened) + return table.putNew(key, value); +#endif + return table.relookupOrAdd(addPtr, key, value); + } + + typedef void (DependentAddPtr::* ConvertibleToBool)(); + void nonNull() {} + + bool found() const { return addPtr.found(); } + operator ConvertibleToBool() const { return found() ? &DependentAddPtr::nonNull : 0; } + const Entry &operator*() const { return *addPtr; } + const Entry *operator->() const { return &*addPtr; } + + private: + AddPtr addPtr ; +#ifdef JSGC_GENERATIONAL + const ExclusiveContext *cx; + const uint64_t originalGcNumber; +#endif + + DependentAddPtr() MOZ_DELETE; + DependentAddPtr(const DependentAddPtr&) MOZ_DELETE; + DependentAddPtr& operator=(const DependentAddPtr&) MOZ_DELETE; +}; + +} // namespace js + +#endif diff --git a/js/src/jsinfer.cpp b/js/src/jsinfer.cpp index 821aae1c948..d1e4e222e73 100644 --- a/js/src/jsinfer.cpp +++ b/js/src/jsinfer.cpp @@ -14,6 +14,7 @@ #include "jsautooplen.h" #include "jscntxt.h" #include "jsgc.h" +#include "jshashutil.h" #include "jsobj.h" #include "jsprf.h" #include "jsscript.h" @@ -2270,7 +2271,11 @@ struct types::ArrayTableKey : public DefaultHasher JSObject *proto; ArrayTableKey() - : type(Type::UndefinedType()), proto(nullptr) + : type(Type::UndefinedType()), proto(nullptr) + {} + + ArrayTableKey(Type type, JSObject *proto) + : type(type), proto(proto) {} static inline uint32_t hash(const ArrayTableKey &v) { @@ -2295,11 +2300,8 @@ TypeCompartment::setTypeToHomogenousArray(ExclusiveContext *cx, } } - ArrayTableKey key; - key.type = elementType; - key.proto = obj->getProto(); - ArrayTypeTable::AddPtr p = arrayTypeTable->lookupForAdd(key); - + ArrayTableKey key(elementType, obj->getProto()); + DependentAddPtr p(cx, *arrayTypeTable, key); if (p) { obj->setType(p->value); } else { @@ -2315,7 +2317,8 @@ TypeCompartment::setTypeToHomogenousArray(ExclusiveContext *cx, if (!objType->unknownProperties()) objType->addPropertyType(cx, JSID_VOID, elementType); - if (!arrayTypeTable->relookupOrAdd(p, key, objType)) { + key.proto = objProto; + if (!p.add(*arrayTypeTable, key, objType)) { cx->compartment()->types.setPendingNukeTypes(cx); return; } @@ -3747,9 +3750,9 @@ ExclusiveContext::getNewType(const Class *clasp, TaggedProto proto_, JSFunction if (!newTypeObjects.initialized() && !newTypeObjects.init()) return nullptr; - TypeObjectSet::AddPtr p = newTypeObjects.lookupForAdd(TypeObjectSet::Lookup(clasp, proto_)); + DependentAddPtr p(this, newTypeObjects, + TypeObjectSet::Lookup(clasp, proto_)); SkipRoot skipHash(this, &p); /* Prevent the hash from being poisoned. */ - uint64_t originalGcNumber = zone()->gcNumber(); if (p) { TypeObject *type = *p; JS_ASSERT(type->clasp == clasp); @@ -3787,15 +3790,7 @@ ExclusiveContext::getNewType(const Class *clasp, TaggedProto proto_, JSFunction if (!type) return nullptr; - /* - * If a GC has occured, then the hash we calculated may be invalid, as it - * is based on proto, which may have been moved. - */ - bool gcHappened = zone()->gcNumber() != originalGcNumber; - bool added = - gcHappened ? newTypeObjects.putNew(TypeObjectSet::Lookup(clasp, proto), type.get()) - : newTypeObjects.relookupOrAdd(p, TypeObjectSet::Lookup(clasp, proto), type.get()); - if (!added) + if (!p.add(newTypeObjects, TypeObjectSet::Lookup(clasp, proto), type.get())) return nullptr; #ifdef JSGC_GENERATIONAL @@ -3862,7 +3857,7 @@ ExclusiveContext::getLazyType(const Class *clasp, TaggedProto proto) if (!table.initialized() && !table.init()) return nullptr; - TypeObjectSet::AddPtr p = table.lookupForAdd(TypeObjectSet::Lookup(clasp, proto)); + DependentAddPtr p(this, table, TypeObjectSet::Lookup(clasp, proto)); if (p) { TypeObject *type = *p; JS_ASSERT(type->lazy()); @@ -3875,7 +3870,7 @@ ExclusiveContext::getLazyType(const Class *clasp, TaggedProto proto) if (!type) return nullptr; - if (!table.relookupOrAdd(p, TypeObjectSet::Lookup(clasp, protoRoot), type)) + if (!p.add(table, TypeObjectSet::Lookup(clasp, protoRoot), type)) return nullptr; type->singleton = (JSObject *) TypeObject::LAZY_SINGLETON; diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index e319a43419c..ded4f2b2753 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -8,6 +8,7 @@ #include "jscntxt.h" #include "jscompartment.h" +#include "jshashutil.h" #include "jsnum.h" #include "jsobj.h" #include "jswrapper.h" @@ -652,7 +653,7 @@ Debugger::wrapEnvironment(JSContext *cx, Handle env, MutableHandleValue rv JS_ASSERT(!env->is()); JSObject *envobj; - ObjectWeakMap::AddPtr p = environments.lookupForAdd(env); + DependentAddPtr p(cx, environments, env); if (p) { envobj = p->value; } else { @@ -663,7 +664,7 @@ Debugger::wrapEnvironment(JSContext *cx, Handle env, MutableHandleValue rv return false; envobj->setPrivateGCThing(env); envobj->setReservedSlot(JSSLOT_DEBUGENV_OWNER, ObjectValue(*object)); - if (!environments.relookupOrAdd(p, env, envobj)) { + if (!p.add(environments, env, envobj)) { js_ReportOutOfMemory(cx); return false; } @@ -687,7 +688,7 @@ Debugger::wrapDebuggeeValue(JSContext *cx, MutableHandleValue vp) if (vp.isObject()) { RootedObject obj(cx, &vp.toObject()); - ObjectWeakMap::AddPtr p = objects.lookupForAdd(obj); + DependentAddPtr p(cx, objects, obj); if (p) { vp.setObject(*p->value); } else { @@ -699,7 +700,8 @@ Debugger::wrapDebuggeeValue(JSContext *cx, MutableHandleValue vp) return false; dobj->setPrivateGCThing(obj); dobj->setReservedSlot(JSSLOT_DEBUGOBJECT_OWNER, ObjectValue(*object)); - if (!objects.relookupOrAdd(p, obj, dobj)) { + + if (!p.add(objects, obj, dobj)) { js_ReportOutOfMemory(cx); return false; } @@ -2757,14 +2759,13 @@ Debugger::wrapScript(JSContext *cx, HandleScript script) { assertSameCompartment(cx, object.get()); JS_ASSERT(cx->compartment() != script->compartment()); - ScriptWeakMap::AddPtr p = scripts.lookupForAdd(script); + DependentAddPtr p(cx, scripts, script); if (!p) { JSObject *scriptobj = newDebuggerScript(cx, script); if (!scriptobj) return nullptr; - /* The allocation may have caused a GC, which can remove table entries. */ - if (!scripts.relookupOrAdd(p, script, scriptobj)) { + if (!p.add(scripts, script, scriptobj)) { js_ReportOutOfMemory(cx); return nullptr; } @@ -3649,14 +3650,13 @@ Debugger::wrapSource(JSContext *cx, HandleScriptSource source) { assertSameCompartment(cx, object.get()); JS_ASSERT(cx->compartment() != source->compartment()); - SourceWeakMap::AddPtr p = sources.lookupForAdd(source); + DependentAddPtr p(cx, sources, source); if (!p) { JSObject *sourceobj = newDebuggerSource(cx, source); if (!sourceobj) return nullptr; - /* The allocation may have caused a GC, which can remove table entries. */ - if (!sources.relookupOrAdd(p, source, sourceobj)) { + if (!p.add(sources, source, sourceobj)) { js_ReportOutOfMemory(cx); return nullptr; } diff --git a/js/src/vm/Debugger.h b/js/src/vm/Debugger.h index 336d4100706..38996b80b05 100644 --- a/js/src/vm/Debugger.h +++ b/js/src/vm/Debugger.h @@ -75,6 +75,17 @@ class DebuggerWeakMap : private WeakMap > return Base::init(len) && zoneCounts.init(); } + template + bool putNew(const KeyInput &k, const ValueInput &v) { + JS_ASSERT(v->compartment() == Base::compartment); + if (!incZoneCount(k->zone())) + return false; + bool ok = Base::putNew(k, v); + if (!ok) + decZoneCount(k->zone()); + return ok; + } + template bool relookupOrAdd(AddPtr &p, const KeyInput &k, const ValueInput &v) { JS_ASSERT(v->compartment() == Base::compartment); diff --git a/js/src/vm/SelfHosting.cpp b/js/src/vm/SelfHosting.cpp index 48a2f44546e..c47050d7954 100644 --- a/js/src/vm/SelfHosting.cpp +++ b/js/src/vm/SelfHosting.cpp @@ -7,6 +7,7 @@ #include "jscntxt.h" #include "jscompartment.h" #include "jsfriendapi.h" +#include "jshashutil.h" #include "jsobj.h" #include "selfhosted.out.h" @@ -865,7 +866,7 @@ GetObjectAllocKindForClone(JSRuntime *rt, JSObject *obj) static JSObject * CloneObject(JSContext *cx, HandleObject srcObj, CloneMemory &clonedObjects) { - CloneMemory::AddPtr p = clonedObjects.lookupForAdd(srcObj.get()); + DependentAddPtr p(cx, clonedObjects, srcObj.get()); if (p) return p->value; RootedObject clone(cx); @@ -904,9 +905,12 @@ CloneObject(JSContext *cx, HandleObject srcObj, CloneMemory &clonedObjects) GetObjectAllocKindForClone(cx->runtime(), srcObj), SingletonObject); } - if (!clone || !clonedObjects.relookupOrAdd(p, srcObj.get(), clone.get()) || - !CloneProperties(cx, srcObj, clone, clonedObjects)) - { + if (!clone) + return nullptr; + if (!p.add(clonedObjects, srcObj, clone)) + return nullptr; + if (!CloneProperties(cx, srcObj, clone, clonedObjects)) { + clonedObjects.remove(srcObj); return nullptr; } return clone; diff --git a/js/src/vm/Shape.cpp b/js/src/vm/Shape.cpp index a86e483727d..ca0c98853a0 100644 --- a/js/src/vm/Shape.cpp +++ b/js/src/vm/Shape.cpp @@ -14,6 +14,7 @@ #include "jsatom.h" #include "jscntxt.h" +#include "jshashutil.h" #include "jsobj.h" #include "js/HashTable.h" @@ -1468,8 +1469,7 @@ BaseShape::getUnowned(ExclusiveContext *cx, const StackBaseShape &base) if (!table.initialized() && !table.init()) return nullptr; - BaseShapeSet::AddPtr p = table.lookupForAdd(&base); - + DependentAddPtr p(cx, table, &base); if (p) return *p; @@ -1483,7 +1483,7 @@ BaseShape::getUnowned(ExclusiveContext *cx, const StackBaseShape &base) UnownedBaseShape *nbase = static_cast(nbase_); - if (!table.relookupOrAdd(p, &base, nbase)) + if (!p.add(table, &base, nbase)) return nullptr; return nbase; @@ -1595,9 +1595,8 @@ EmptyShape::getInitialShape(ExclusiveContext *cx, const Class *clasp, TaggedProt return nullptr; typedef InitialShapeEntry::Lookup Lookup; - InitialShapeSet::AddPtr p = - table.lookupForAdd(Lookup(clasp, proto, parent, metadata, nfixed, objectFlags)); - + DependentAddPtr + p(cx, table, Lookup(clasp, proto, parent, metadata, nfixed, objectFlags)); if (p) return p->shape; @@ -1616,11 +1615,9 @@ EmptyShape::getInitialShape(ExclusiveContext *cx, const Class *clasp, TaggedProt return nullptr; new (shape) EmptyShape(nbase, nfixed); - if (!table.relookupOrAdd(p, Lookup(clasp, protoRoot, parentRoot, metadataRoot, nfixed, objectFlags), - InitialShapeEntry(shape, protoRoot))) - { + Lookup lookup(clasp, protoRoot, parentRoot, metadataRoot, nfixed, objectFlags); + if (!p.add(table, lookup, InitialShapeEntry(shape, protoRoot))) return nullptr; - } return shape; }