Attachment 360061 Details for Bug 193800 – Patch

[patch] Patch

bug-193800-20190124173312.patch (text/plain), 14.53 KB, created by Yusuke Suzuki on 2019-01-24 17:33:13 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-24 17:33:13 PST

Size: 14.53 KB

patch

obsolete

>Subversion Revision: 240456
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 5faea625fdee34b38ba8d246c381485d88341e00..992a760f1f3f71cc7fe2c509a8a9269111a4c14f 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,44 @@
>+2019-01-24  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] ErrorConstructor should not have own IsoSubspace
>+        https://bugs.webkit.org/show_bug.cgi?id=193800
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Similar to r240456, sizeof(ErrorConstructor) != sizeof(InternalFunction), and that is why we have
>+        IsoSubspace errorConstructorSpace in VM. But it is allocated only one-per-JSGlobalObject, and it is
>+        too costly to have IsoSubspace which allocates 16KB. Since stackTraceLimit information is per
>+        JSGlobalObject information, we should have m_stackTraceLimit in JSGlobalObject instead and put
>+        ErrorConstructor in InternalFunction's IsoSubspace. As r230813 (moving InternalFunction and subclasses
>+        into IsoSubspaces) described,
>+
>+            "subclasses that are the same size as InternalFunction share its subspace. I did this because the subclasses
>+            appear to just override methods, which are called dynamically via the structure or class of the object.
>+            So, I don't see a type confusion risk if UAF is used to allocate one kind of InternalFunction over another."
>+
>+        Then, putting ErrorConstructor in InternalFunction IsoSubspace is fine since it meets the above condition.
>+        This patch removes m_stackTraceLimit in ErrorConstructor, and drops IsoSubspace for errorConstructorSpace.
>+        This reduces the memory usage.
>+
>+        * runtime/Error.cpp:
>+        (JSC::getStackTrace):
>+        * runtime/ErrorConstructor.cpp:
>+        (JSC::ErrorConstructor::finishCreation):
>+        (JSC::ErrorConstructor::put):
>+        (JSC::ErrorConstructor::deleteProperty):
>+        * runtime/ErrorConstructor.h:
>+        * runtime/JSGlobalObject.cpp:
>+        (JSC::JSGlobalObject::JSGlobalObject):
>+        (JSC::JSGlobalObject::init):
>+        (JSC::JSGlobalObject::visitChildren):
>+        * runtime/JSGlobalObject.h:
>+        (JSC::JSGlobalObject::stackTraceLimit const):
>+        (JSC::JSGlobalObject::setStackTraceLimit):
>+        (JSC::JSGlobalObject::errorConstructor const): Deleted.
>+        * runtime/VM.cpp:
>+        (JSC::VM::VM):
>+        * runtime/VM.h:
>+
> 2019-01-24  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] SharedArrayBufferConstructor and ArrayBufferConstructor should not have their own IsoSubspace
>diff --git a/Source/JavaScriptCore/runtime/Error.cpp b/Source/JavaScriptCore/runtime/Error.cpp
>index d93344097a13e0330a50c1410e5cc75bbe5ed46d..1b8e4c281de269710c7a3752a21d143acdd42c10 100644
>--- a/Source/JavaScriptCore/runtime/Error.cpp
>+++ b/Source/JavaScriptCore/runtime/Error.cpp
>@@ -161,13 +161,12 @@ class FindFirstCallerFrameWithCodeblockFunctor {
> std::unique_ptr<Vector<StackFrame>> getStackTrace(ExecState* exec, VM& vm, JSObject* obj, bool useCurrentFrame)
> {
>     JSGlobalObject* globalObject = obj->globalObject(vm);
>-    ErrorConstructor* errorConstructor = globalObject->errorConstructor();
>-    if (!errorConstructor->stackTraceLimit())
>+    if (!globalObject->stackTraceLimit())
>         return nullptr;
> 
>     size_t framesToSkip = useCurrentFrame ? 0 : 1;
>     std::unique_ptr<Vector<StackFrame>> stackTrace = std::make_unique<Vector<StackFrame>>();
>-    vm.interpreter->getStackTrace(obj, *stackTrace, framesToSkip, errorConstructor->stackTraceLimit().value());
>+    vm.interpreter->getStackTrace(obj, *stackTrace, framesToSkip, globalObject->stackTraceLimit().value());
>     if (!stackTrace->isEmpty())
>         ASSERT_UNUSED(exec, exec == vm.topCallFrame || exec->isGlobalExec());
>     return stackTrace;
>diff --git a/Source/JavaScriptCore/runtime/ErrorConstructor.cpp b/Source/JavaScriptCore/runtime/ErrorConstructor.cpp
>index b09c05870cd2195fcb7b15fcaa7bda8e31572dca..a911b07ac514ef5a0e91a8b64159bc28f67e79a9 100644
>--- a/Source/JavaScriptCore/runtime/ErrorConstructor.cpp
>+++ b/Source/JavaScriptCore/runtime/ErrorConstructor.cpp
>@@ -44,10 +44,7 @@ void ErrorConstructor::finishCreation(VM& vm, ErrorPrototype* errorPrototype)
>     // ECMA 15.11.3.1 Error.prototype
>     putDirectWithoutTransition(vm, vm.propertyNames->prototype, errorPrototype, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly);
>     putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(1), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);
>-
>-    unsigned defaultStackTraceLimit = Options::defaultErrorStackTraceLimit();
>-    m_stackTraceLimit = defaultStackTraceLimit;
>-    putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit, jsNumber(defaultStackTraceLimit), static_cast<unsigned>(PropertyAttribute::None));
>+    putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit, jsNumber(globalObject(vm)->stackTraceLimit().valueOr(Options::defaultErrorStackTraceLimit())), static_cast<unsigned>(PropertyAttribute::None));
> }
> 
> // ECMA 15.9.3
>@@ -79,9 +76,9 @@ bool ErrorConstructor::put(JSCell* cell, ExecState* exec, PropertyName propertyN
>             double effectiveLimit = value.asNumber();
>             effectiveLimit = std::max(0., effectiveLimit);
>             effectiveLimit = std::min(effectiveLimit, static_cast<double>(std::numeric_limits<unsigned>::max()));
>-            thisObject->m_stackTraceLimit = static_cast<unsigned>(effectiveLimit);
>+            thisObject->globalObject(vm)->setStackTraceLimit(static_cast<unsigned>(effectiveLimit));
>         } else
>-            thisObject->m_stackTraceLimit = { };
>+            thisObject->globalObject(vm)->setStackTraceLimit(WTF::nullopt);
>     }
> 
>     return Base::put(thisObject, exec, propertyName, value, slot);
>@@ -93,7 +90,7 @@ bool ErrorConstructor::deleteProperty(JSCell* cell, ExecState* exec, PropertyNam
>     ErrorConstructor* thisObject = jsCast<ErrorConstructor*>(cell);
> 
>     if (propertyName == vm.propertyNames->stackTraceLimit)
>-        thisObject->m_stackTraceLimit = { };
>+        thisObject->globalObject(vm)->setStackTraceLimit(WTF::nullopt);
> 
>     return Base::deleteProperty(thisObject, exec, propertyName);
> }
>diff --git a/Source/JavaScriptCore/runtime/ErrorConstructor.h b/Source/JavaScriptCore/runtime/ErrorConstructor.h
>index 857d31955cfe3719247a66d137f726b00110e07a..6edee730066f75baf5b6ac60739760e324b786bc 100644
>--- a/Source/JavaScriptCore/runtime/ErrorConstructor.h
>+++ b/Source/JavaScriptCore/runtime/ErrorConstructor.h
>@@ -29,13 +29,7 @@ class GetterSetter;
> 
> class ErrorConstructor final : public InternalFunction {
> public:
>-    typedef InternalFunction Base;
>-
>-    template<typename CellType>
>-    static IsoSubspace* subspaceFor(VM& vm)
>-    {
>-        return &vm.errorConstructorSpace;
>-    }
>+    using Base = InternalFunction;
> 
>     static ErrorConstructor* create(VM& vm, Structure* structure, ErrorPrototype* errorPrototype, GetterSetter*)
>     {
>@@ -51,8 +45,6 @@ class ErrorConstructor final : public InternalFunction {
>         return Structure::create(vm, globalObject, prototype, TypeInfo(InternalFunctionType, StructureFlags), info()); 
>     }
> 
>-    Optional<unsigned> stackTraceLimit() const { return m_stackTraceLimit; }
>-
> protected:
>     void finishCreation(VM&, ErrorPrototype*);
> 
>@@ -62,7 +54,8 @@ class ErrorConstructor final : public InternalFunction {
> private:
>     ErrorConstructor(VM&, Structure*);
> 
>-    Optional<unsigned> m_stackTraceLimit;
> };
> 
>+static_assert(sizeof(ErrorConstructor) == sizeof(InternalFunction), "");
>+
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>index cce56a21bbc0366b0ff89743b51e5c5244527117..0a0fbf1b3d0fbe21642bf2df36d914c558f154d9 100644
>--- a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>@@ -360,6 +360,7 @@ JSGlobalObject::JSGlobalObject(VM& vm, Structure* structure, const GlobalObjectM
>     , m_arraySpeciesWatchpoint(ClearWatchpoint)
>     , m_numberToStringWatchpoint(IsWatched)
>     , m_runtimeFlags()
>+    , m_stackTraceLimit(Options::defaultErrorStackTraceLimit())
>     , m_globalObjectMethodTable(globalObjectMethodTable ? globalObjectMethodTable : &s_globalObjectMethodTable)
> {
> }
>@@ -700,7 +701,6 @@ m_ ## lowerName ## Prototype->putDirectWithoutTransition(vm, vm.propertyNames->c
>     
> #undef CREATE_CONSTRUCTOR_FOR_SIMPLE_TYPE
> 
>-    m_errorConstructor.set(vm, this, errorConstructor);
>     m_promiseConstructor.set(vm, this, promiseConstructor);
>     m_internalPromiseConstructor.set(vm, this, internalPromiseConstructor);
>     
>@@ -878,7 +878,7 @@ putDirectWithoutTransition(vm, vm.propertyNames-> jsName, lowerName ## Construct
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().propertyIsEnumerablePrivateName(), privateFuncPropertyIsEnumerable, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().importModulePrivateName(), privateFuncImportModule, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().enqueueJobPrivateName(), JSFunction::create(vm, this, 0, String(), enqueueJob), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>-        GlobalPropertyInfo(vm.propertyNames->builtinNames().ErrorPrivateName(), m_errorConstructor.get(), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>+        GlobalPropertyInfo(vm.propertyNames->builtinNames().ErrorPrivateName(), errorConstructor, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().RangeErrorPrivateName(), m_rangeErrorConstructor.get(), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().TypeErrorPrivateName(), m_typeErrorConstructor.get(), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().typedArrayLengthPrivateName(), privateFuncTypedArrayLength, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>@@ -1548,7 +1548,6 @@ void JSGlobalObject::visitChildren(JSCell* cell, SlotVisitor& visitor)
>     visitor.append(thisObject->m_globalCallee);
>     visitor.append(thisObject->m_stackOverflowFrameCallee);
>     visitor.append(thisObject->m_regExpConstructor);
>-    visitor.append(thisObject->m_errorConstructor);
>     visitor.append(thisObject->m_nativeErrorPrototypeStructure);
>     visitor.append(thisObject->m_nativeErrorStructure);
>     thisObject->m_evalErrorConstructor.visit(visitor);
>diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.h b/Source/JavaScriptCore/runtime/JSGlobalObject.h
>index 15f0637922ab4bfdcbe0c06163240d7c364c78cf..4462ab1861d42caa91a7f1ca3501dc36477d0ec9 100644
>--- a/Source/JavaScriptCore/runtime/JSGlobalObject.h
>+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.h
>@@ -259,7 +259,6 @@ class JSGlobalObject : public JSSegmentedVariableObject {
>     WriteBarrier<JSCallee> m_globalCallee;
>     WriteBarrier<JSCallee> m_stackOverflowFrameCallee;
>     WriteBarrier<RegExpConstructor> m_regExpConstructor;
>-    WriteBarrier<ErrorConstructor> m_errorConstructor;
>     WriteBarrier<Structure> m_nativeErrorPrototypeStructure;
>     WriteBarrier<Structure> m_nativeErrorStructure;
>     LazyProperty<JSGlobalObject, NativeErrorConstructor> m_evalErrorConstructor;
>@@ -498,6 +497,7 @@ class JSGlobalObject : public JSSegmentedVariableObject {
>     String m_webAssemblyDisabledErrorMessage;
>     RuntimeFlags m_runtimeFlags;
>     ConsoleClient* m_consoleClient { nullptr };
>+    Optional<unsigned> m_stackTraceLimit;
> 
> #if !ASSERT_DISABLED
>     const ExecState* m_callFrameAtDebuggerEntry { nullptr };
>@@ -530,6 +530,9 @@ class JSGlobalObject : public JSSegmentedVariableObject {
>     WatchpointSet& ensureReferencedPropertyWatchpointSet(UniquedStringImpl*);
> #endif
> 
>+    Optional<unsigned> stackTraceLimit() const { return m_stackTraceLimit; }
>+    void setStackTraceLimit(Optional<unsigned> value) { m_stackTraceLimit = value; }
>+
> protected:
>     JS_EXPORT_PRIVATE explicit JSGlobalObject(VM&, Structure*, const GlobalObjectMethodTable* = nullptr);
> 
>@@ -573,7 +576,6 @@ class JSGlobalObject : public JSSegmentedVariableObject {
> 
>     RegExpConstructor* regExpConstructor() const { return m_regExpConstructor.get(); }
> 
>-    ErrorConstructor* errorConstructor() const { return m_errorConstructor.get(); }
>     ArrayConstructor* arrayConstructor() const { return m_arrayConstructor.get(); }
>     ObjectConstructor* objectConstructor() const { return m_objectConstructor.get(); }
>     JSPromiseConstructor* promiseConstructor() const { return m_promiseConstructor.get(); }
>diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp
>index c087ca8d6952fd78c974a773214ac478773e7964..cb2dc47cddd6429ecfd01e932ef8d9885ff10779 100644
>--- a/Source/JavaScriptCore/runtime/VM.cpp
>+++ b/Source/JavaScriptCore/runtime/VM.cpp
>@@ -297,7 +297,6 @@ VM::VM(VMType vmType, HeapType heapType)
>     , boundFunctionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), JSBoundFunction)
>     , callbackFunctionSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSCallbackFunction)
>     , customGetterSetterFunctionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), JSCustomGetterSetterFunction)
>-    , errorConstructorSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), ErrorConstructor)
>     , executableToCodeBlockEdgeSpace ISO_SUBSPACE_INIT(heap, cellDangerousBitsHeapCellType.get(), ExecutableToCodeBlockEdge)
>     , functionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), JSFunction)
>     , generatorFunctionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), JSGeneratorFunction)
>diff --git a/Source/JavaScriptCore/runtime/VM.h b/Source/JavaScriptCore/runtime/VM.h
>index ee0b3a66dde1e74fb52596dd39e0929443337c19..3f4accf358bf8cae99aaecdf6d9da105c1c4cd33 100644
>--- a/Source/JavaScriptCore/runtime/VM.h
>+++ b/Source/JavaScriptCore/runtime/VM.h
>@@ -372,7 +372,6 @@ class VM : public ThreadSafeRefCounted<VM>, public DoublyLinkedListNode<VM> {
>     IsoSubspace boundFunctionSpace;
>     IsoSubspace callbackFunctionSpace;
>     IsoSubspace customGetterSetterFunctionSpace;
>-    IsoSubspace errorConstructorSpace;
>     IsoSubspace executableToCodeBlockEdgeSpace;
>     IsoSubspace functionSpace;
>     IsoSubspace generatorFunctionSpace;

Actions: View | Formatted Diff | Diff

Attachments on bug 193800: 360061 | 360062 | 360066