⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 242991 in webkit


Ignore:
Timestamp:
Mar 14, 2019, 10:56:24 PM (7 years ago)
Author:
ysuzuki@apple.com
Message:

[JSC] Retain PrivateName of Symbol before passing it to operations potentially incurring GC
​https://bugs.webkit.org/show_bug.cgi?id=195791
<rdar://problem/48806130>

Reviewed by Mark Lam.

JSTests:

  • stress/symbol-is-destructed-before-refing-underlying-symbol-impl.js: Added.

(foo):

Source/JavaScriptCore:

Consider the following example:

void putByVal(JSObject*, PropertyName propertyName, ...);

putByVal(object, symbol->privateName(), ...);

PropertyName does not retain the passed UniquedStringImpl*. It just holds the pointer to UniquedStringImpl*.
It means that since Symbol::privateName() returns const PrivateName& instead of PrivateName, putByVal
and its caller does not retain UniquedStringImpl* held in PropertyName. The problem happens when the putByVal
incurs GC, and when the symbol is missing in the conservative GC scan. The underlying UniquedStringImpl* of
PropertyName can be accidentally destroyed in the middle of the putByVal operation. We should retain PrivateName
before passing it to operations which takes it as PropertyName.

  1. We use the code pattern like this.

auto propertyName = symbol->privateName();
someOperation(..., propertyName);

This pattern is well aligned to existing JSValue::toPropertyKey(exec) and JSString::toIdentifier(exec) code patterns.

auto propertyName = value.toPropertyKey(exec);
RETURN_IF_EXCEPTION(scope, { });
someOperation(..., propertyName);

  1. We change Symbol::privateName() to returning PrivateName instead of const PrivateName& to avoid potential dangerous use cases. This is OK because the code using Symbol::privateName() is not a critical path, and they typically need to retain PrivateName.
  1. We audit similar functions toPropertyKey(exec) and toIdentifier(exec) for needed but missing exception checks. BTW, these functions are safe to the problem fixed in this patch since they return Identifier instead of const Identifier&.

Mark and Robin investigated and offered important data to understand what went wrong. And figured out the reason behind
the mysterious behavior shown in the data, and now, we confirm that this is the right fix for this bug.

  • dfg/DFGOperations.cpp:
  • jit/JITOperations.cpp:

(JSC::tryGetByValOptimize):

  • runtime/JSFunction.cpp:

(JSC::JSFunction::setFunctionName):

  • runtime/JSModuleLoader.cpp:

(JSC::printableModuleKey):

  • runtime/JSONObject.cpp:

(JSC::Stringifier::Stringifier):

  • runtime/Symbol.cpp:

(JSC::Symbol::descriptiveString const):
(JSC::Symbol::description const):

  • runtime/Symbol.h:
  • runtime/SymbolConstructor.cpp:

(JSC::symbolConstructorKeyFor):

  • tools/JSDollarVM.cpp:

(JSC::functionGetGetterSetter):

Source/WebCore:

  • bindings/js/ScriptController.cpp:

(WebCore::ScriptController::setupModuleScriptHandlers):

Location:
trunk
Files:
1 added
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r242989 r242991  
     12019-03-14  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Retain PrivateName of Symbol before passing it to operations potentially incurring GC
     4        https://bugs.webkit.org/show_bug.cgi?id=195791
     5        <rdar://problem/48806130>
     6
     7        Reviewed by Mark Lam.
     8
     9        * stress/symbol-is-destructed-before-refing-underlying-symbol-impl.js: Added.
     10        (foo):
     11
    1122019-03-14  Saam barati  <sbarati@apple.com>
    213

  • trunk/Source/JavaScriptCore/ChangeLog

    r242990 r242991  
     12019-03-14  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Retain PrivateName of Symbol before passing it to operations potentially incurring GC
     4        https://bugs.webkit.org/show_bug.cgi?id=195791
     5        <rdar://problem/48806130>
     6
     7        Reviewed by Mark Lam.
     8
     9        Consider the following example:
     10
     11            void putByVal(JSObject*, PropertyName propertyName, ...);
     12
     13            putByVal(object, symbol->privateName(), ...);
     14
     15        PropertyName does not retain the passed UniquedStringImpl*. It just holds the pointer to UniquedStringImpl*.
     16        It means that since `Symbol::privateName()` returns `const PrivateName&` instead of `PrivateName`, putByVal
     17        and its caller does not retain UniquedStringImpl* held in PropertyName. The problem happens when the putByVal
     18        incurs GC, and when the `symbol` is missing in the conservative GC scan. The underlying UniquedStringImpl* of
     19        PropertyName can be accidentally destroyed in the middle of the putByVal operation. We should retain PrivateName
     20        before passing it to operations which takes it as PropertyName.
     21
     22        1. We use the code pattern like this.
     23
     24            auto propertyName = symbol->privateName();
     25            someOperation(..., propertyName);
     26
     27        This pattern is well aligned to existing `JSValue::toPropertyKey(exec)` and `JSString::toIdentifier(exec)` code patterns.
     28
     29            auto propertyName = value.toPropertyKey(exec);
     30            RETURN_IF_EXCEPTION(scope, { });
     31            someOperation(..., propertyName);
     32
     33        2. We change `Symbol::privateName()` to returning `PrivateName` instead of `const PrivateName&` to avoid
     34           potential dangerous use cases. This is OK because the code using `Symbol::privateName()` is not a critical path,
     35           and they typically need to retain PrivateName.
     36
     37        3. We audit similar functions `toPropertyKey(exec)` and `toIdentifier(exec)` for needed but missing exception checks.
     38           BTW, these functions are safe to the problem fixed in this patch since they return `Identifier` instead
     39           of `const Identifier&`.
     40
     41        Mark and Robin investigated and offered important data to understand what went wrong. And figured out the reason behind
     42        the mysterious behavior shown in the data, and now, we confirm that this is the right fix for this bug.
     43
     44        * dfg/DFGOperations.cpp:
     45        * jit/JITOperations.cpp:
     46        (JSC::tryGetByValOptimize):
     47        * runtime/JSFunction.cpp:
     48        (JSC::JSFunction::setFunctionName):
     49        * runtime/JSModuleLoader.cpp:
     50        (JSC::printableModuleKey):
     51        * runtime/JSONObject.cpp:
     52        (JSC::Stringifier::Stringifier):
     53        * runtime/Symbol.cpp:
     54        (JSC::Symbol::descriptiveString const):
     55        (JSC::Symbol::description const):
     56        * runtime/Symbol.h:
     57        * runtime/SymbolConstructor.cpp:
     58        (JSC::symbolConstructorKeyFor):
     59        * tools/JSDollarVM.cpp:
     60        (JSC::functionGetGetterSetter):
     61
    1622019-03-14  Yusuke Suzuki  <ysuzuki@apple.com>
    263

  • trunk/Source/JavaScriptCore/dfg/DFGOperations.cpp

    r242715 r242991  
    771771    NativeCallFrameTracer tracer(&vm, exec);
    772772
    773     return JSValue::encode(getByValObject(exec, vm, asObject(base), asSymbol(symbol)->privateName()));
     773    auto propertyName = asSymbol(symbol)->privateName();
     774    return JSValue::encode(getByValObject(exec, vm, asObject(base), propertyName));
    774775}
    775776
    … …  
    827828    NativeCallFrameTracer tracer(&vm, exec);
    828829
    829     putByValCellInternal<true, false>(exec, vm, cell, asSymbol(symbol)->privateName(), JSValue::decode(encodedValue));
     830    auto propertyName = asSymbol(symbol)->privateName();
     831    putByValCellInternal<true, false>(exec, vm, cell, propertyName, JSValue::decode(encodedValue));
    830832}
    831833
    … …  
    835837    NativeCallFrameTracer tracer(&vm, exec);
    836838
    837     putByValCellInternal<false, false>(exec, vm, cell, asSymbol(symbol)->privateName(), JSValue::decode(encodedValue));
     839    auto propertyName = asSymbol(symbol)->privateName();
     840    putByValCellInternal<false, false>(exec, vm, cell, propertyName, JSValue::decode(encodedValue));
    838841}
    839842
    … …  
    987990    NativeCallFrameTracer tracer(&vm, exec);
    988991
    989     putByValCellInternal<true, true>(exec, vm, cell, asSymbol(symbol)->privateName(), JSValue::decode(encodedValue));
     992    auto propertyName = asSymbol(symbol)->privateName();
     993    putByValCellInternal<true, true>(exec, vm, cell, propertyName, JSValue::decode(encodedValue));
    990994}
    991995
    … …  
    995999    NativeCallFrameTracer tracer(&vm, exec);
    9961000
    997     putByValCellInternal<false, true>(exec, vm, cell, asSymbol(symbol)->privateName(), JSValue::decode(encodedValue));
     1001    auto propertyName = asSymbol(symbol)->privateName();
     1002    putByValCellInternal<false, true>(exec, vm, cell, propertyName, JSValue::decode(encodedValue));
    9981003}
    9991004
    … …  
    20552060}
    20562061
    2057 EncodedJSValue JIT_OPERATION operationHasGenericProperty(ExecState* exec, EncodedJSValue encodedBaseValue, JSCell* propertyName)
    2058 {
    2059     VM& vm = exec->vm();
    2060     NativeCallFrameTracer tracer(&vm, exec);
     2062EncodedJSValue JIT_OPERATION operationHasGenericProperty(ExecState* exec, EncodedJSValue encodedBaseValue, JSCell* property)
     2063{
     2064    VM& vm = exec->vm();
     2065    NativeCallFrameTracer tracer(&vm, exec);
     2066    auto scope = DECLARE_THROW_SCOPE(vm);
     2067
    20612068    JSValue baseValue = JSValue::decode(encodedBaseValue);
    20622069    if (baseValue.isUndefinedOrNull())
    … …  
    20662073    if (!base)
    20672074        return JSValue::encode(JSValue());
    2068     return JSValue::encode(jsBoolean(base->hasPropertyGeneric(exec, asString(propertyName)->toIdentifier(exec), PropertySlot::InternalMethodType::GetOwnProperty)));
     2075    auto propertyName = asString(property)->toIdentifier(exec);
     2076    RETURN_IF_EXCEPTION(scope, { });
     2077    RELEASE_AND_RETURN(scope, JSValue::encode(jsBoolean(base->hasPropertyGeneric(exec, propertyName, PropertySlot::InternalMethodType::GetOwnProperty))));
    20692078}
    20702079

  • trunk/Source/JavaScriptCore/jit/JITOperations.cpp

    r242596 r242991  
    721721
    722722    VM& vm = exec->vm();
     723    auto scope = DECLARE_THROW_SCOPE(vm);
    723724
    724725    if (baseValue.isObject() && isCopyOnWrite(baseValue.getObject()->indexingMode()))
    … …  
    751752    if (baseValue.isObject() && isStringOrSymbol(subscript)) {
    752753        const Identifier propertyName = subscript.toPropertyKey(exec);
     754        RETURN_IF_EXCEPTION(scope, OptimizationResult::GiveUp);
    753755        if (subscript.isSymbol() || !parseIndex(propertyName)) {
    754756            ASSERT(exec->bytecodeOffset());
    … …  
    791793    VM& vm = exec->vm();
    792794    NativeCallFrameTracer tracer(&vm, exec);
     795    auto scope = DECLARE_THROW_SCOPE(vm);
    793796
    794797    JSValue baseValue = JSValue::decode(encodedBaseValue);
    795798    JSValue subscript = JSValue::decode(encodedSubscript);
    796799    JSValue value = JSValue::decode(encodedValue);
    797     if (tryPutByValOptimize(exec, baseValue, subscript, byValInfo, ReturnAddressPtr(OUR_RETURN_ADDRESS)) == OptimizationResult::GiveUp) {
     800    OptimizationResult result = tryPutByValOptimize(exec, baseValue, subscript, byValInfo, ReturnAddressPtr(OUR_RETURN_ADDRESS));
     801    RETURN_IF_EXCEPTION(scope, void());
     802    if (result == OptimizationResult::GiveUp) {
    798803        // Don't ever try to optimize.
    799804        byValInfo->tookSlowPath = true;
    800805        ctiPatchCallByReturnAddress(ReturnAddressPtr(OUR_RETURN_ADDRESS), operationPutByValGeneric);
    801806    }
    802     putByVal(exec, baseValue, subscript, value, byValInfo);
     807    RELEASE_AND_RETURN(scope, putByVal(exec, baseValue, subscript, value, byValInfo));
    803808}
    804809
    … …  
    809814
    810815    VM& vm = exec->vm();
     816    auto scope = DECLARE_THROW_SCOPE(vm);
    811817
    812818    if (subscript.isInt32()) {
    … …  
    833839    } else if (isStringOrSymbol(subscript)) {
    834840        const Identifier propertyName = subscript.toPropertyKey(exec);
     841        RETURN_IF_EXCEPTION(scope, OptimizationResult::GiveUp);
    835842        if (subscript.isSymbol() || !parseIndex(propertyName)) {
    836843            ASSERT(exec->bytecodeOffset());
    … …  
    873880    VM& vm = exec->vm();
    874881    NativeCallFrameTracer tracer(&vm, exec);
     882    auto scope = DECLARE_THROW_SCOPE(vm);
    875883
    876884    JSValue baseValue = JSValue::decode(encodedBaseValue);
    … …  
    879887    RELEASE_ASSERT(baseValue.isObject());
    880888    JSObject* object = asObject(baseValue);
    881     if (tryDirectPutByValOptimize(exec, object, subscript, byValInfo, ReturnAddressPtr(OUR_RETURN_ADDRESS)) == OptimizationResult::GiveUp) {
     889    OptimizationResult result = tryDirectPutByValOptimize(exec, object, subscript, byValInfo, ReturnAddressPtr(OUR_RETURN_ADDRESS));
     890    RETURN_IF_EXCEPTION(scope, void());
     891    if (result == OptimizationResult::GiveUp) {
    882892        // Don't ever try to optimize.
    883893        byValInfo->tookSlowPath = true;
    … …  
    885895    }
    886896
    887     directPutByVal(exec, object, subscript, value, byValInfo);
     897    RELEASE_AND_RETURN(scope, directPutByVal(exec, object, subscript, value, byValInfo));
    888898}
    889899
    … …  
    18741884
    18751885    VM& vm = exec->vm();
     1886    auto scope = DECLARE_THROW_SCOPE(vm);
    18761887
    18771888    if (baseValue.isObject() && subscript.isInt32()) {
    … …  
    19041915    if (baseValue.isObject() && isStringOrSymbol(subscript)) {
    19051916        const Identifier propertyName = subscript.toPropertyKey(exec);
     1917        RETURN_IF_EXCEPTION(scope, OptimizationResult::GiveUp);
    19061918        if (subscript.isSymbol() || !parseIndex(propertyName)) {
    19071919            ASSERT(exec->bytecodeOffset());
    … …  
    19571969    VM& vm = exec->vm();
    19581970    NativeCallFrameTracer tracer(&vm, exec);
     1971    auto scope = DECLARE_THROW_SCOPE(vm);
    19591972
    19601973    JSValue baseValue = JSValue::decode(encodedBase);
    19611974    JSValue subscript = JSValue::decode(encodedSubscript);
    19621975    ReturnAddressPtr returnAddress = ReturnAddressPtr(OUR_RETURN_ADDRESS);
    1963     if (tryGetByValOptimize(exec, baseValue, subscript, byValInfo, returnAddress) == OptimizationResult::GiveUp) {
     1976    OptimizationResult result = tryGetByValOptimize(exec, baseValue, subscript, byValInfo, returnAddress);
     1977    RETURN_IF_EXCEPTION(scope, { });
     1978    if (result == OptimizationResult::GiveUp) {
    19641979        // Don't ever try to optimize.
    19651980        byValInfo->tookSlowPath = true;
    … …  
    19671982    }
    19681983
    1969     return JSValue::encode(getByVal(exec, baseValue, subscript, byValInfo, returnAddress));
     1984    RELEASE_AND_RETURN(scope, JSValue::encode(getByVal(exec, baseValue, subscript, byValInfo, returnAddress)));
    19701985}
    19711986

  • trunk/Source/JavaScriptCore/runtime/JSFunction.cpp

    r240796 r242991  
    670670    String name;
    671671    if (value.isSymbol()) {
    672         SymbolImpl& uid = asSymbol(value)->privateName().uid();
     672        PrivateName privateName = asSymbol(value)->privateName();
     673        SymbolImpl& uid = privateName.uid();
    673674        if (uid.isNullSymbol())
    674675            name = emptyString();

  • trunk/Source/JavaScriptCore/runtime/JSModuleLoader.cpp

    r239256 r242991  
    117117{
    118118    VM& vm = exec->vm();
    119     if (key.isString() || key.isSymbol())
    120         return key.toPropertyKey(exec).impl();
     119    auto scope = DECLARE_THROW_SCOPE(vm);
     120    if (key.isString() || key.isSymbol()) {
     121        auto propertyName = key.toPropertyKey(exec);
     122        scope.assertNoException(); // This is OK since this function is just for debugging purpose.
     123        return propertyName.impl();
     124    }
    121125    return vm.propertyNames->emptyIdentifier.impl();
    122126}

  • trunk/Source/JavaScriptCore/runtime/JSONObject.cpp

    r239544 r242991  
    248248                    } else if (!name.isNumber() && !name.isString())
    249249                        continue;
    250                     m_arrayReplacerPropertyNames.add(name.toString(exec)->toIdentifier(exec));
     250                    JSString* propertyNameString = name.toString(exec);
    251251                    RETURN_IF_EXCEPTION(scope, );
     252                    auto propertyName = propertyNameString->toIdentifier(exec);
     253                    RETURN_IF_EXCEPTION(scope, );
     254                    m_arrayReplacerPropertyNames.add(WTFMove(propertyName));
    252255                }
    253256            }

  • trunk/Source/JavaScriptCore/runtime/Symbol.cpp

    r235712 r242991  
    101101String Symbol::descriptiveString() const
    102102{
    103     return makeString("Symbol(", String(privateName().uid()), ')');
     103    return makeString("Symbol(", String(m_privateName.uid()), ')');
    104104}
    105105
    106106String Symbol::description() const
    107107{
    108     auto& uid = privateName().uid();
     108    auto& uid = m_privateName.uid();
    109109    return uid.isNullSymbol() ? String() : uid;
    110110}

  • trunk/Source/JavaScriptCore/runtime/Symbol.h

    r240766 r242991  
    5050    JS_EXPORT_PRIVATE static Symbol* create(VM&, SymbolImpl& uid);
    5151
    52     const PrivateName& privateName() const { return m_privateName; }
     52    PrivateName privateName() const { return m_privateName; }
    5353    String descriptiveString() const;
    5454    String description() const;

  • trunk/Source/JavaScriptCore/runtime/SymbolConstructor.cpp

    r242650 r242991  
    110110        return JSValue::encode(throwTypeError(exec, scope, SymbolKeyForTypeError));
    111111
    112     SymbolImpl& uid = asSymbol(symbolValue)->privateName().uid();
     112    PrivateName privateName = asSymbol(symbolValue)->privateName();
     113    SymbolImpl& uid = privateName.uid();
    113114    if (!uid.symbolRegistry())
    114115        return JSValue::encode(jsUndefined());

  • trunk/Source/JavaScriptCore/tools/JSDollarVM.cpp

    r242397 r242991  
    21042104static EncodedJSValue JSC_HOST_CALL functionGetGetterSetter(ExecState* exec)
    21052105{
     2106    VM& vm = exec->vm();
     2107    auto scope = DECLARE_THROW_SCOPE(vm);
     2108
    21062109    JSValue value = exec->argument(0);
    21072110    if (!value.isObject())
    … …  
    21122115        return JSValue::encode(jsUndefined());
    21132116
     2117    auto propertyName = asString(property)->toIdentifier(exec);
     2118    RETURN_IF_EXCEPTION(scope, { });
     2119
    21142120    PropertySlot slot(value, PropertySlot::InternalMethodType::VMInquiry);
    2115     value.getPropertySlot(exec, asString(property)->toIdentifier(exec), slot);
     2121    value.getPropertySlot(exec, propertyName, slot);
    21162122
    21172123    JSValue result;

  • trunk/Source/WebCore/ChangeLog

    r242988 r242991  
     12019-03-14  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Retain PrivateName of Symbol before passing it to operations potentially incurring GC
     4        https://bugs.webkit.org/show_bug.cgi?id=195791
     5        <rdar://problem/48806130>
     6
     7        Reviewed by Mark Lam.
     8
     9        * bindings/js/ScriptController.cpp:
     10        (WebCore::ScriptController::setupModuleScriptHandlers):
     11
    1122019-03-14  Brent Fulgham  <bfulgham@apple.com>
    213

  • trunk/Source/WebCore/bindings/js/ScriptController.cpp

    r240323 r242991  
    279279    RefPtr<LoadableModuleScript> moduleScript(&moduleScriptRef);
    280280
    281     auto& fulfillHandler = *JSNativeStdFunction::create(state.vm(), proxy.window(), 1, String(), [moduleScript](ExecState* exec) {
     281    auto& fulfillHandler = *JSNativeStdFunction::create(state.vm(), proxy.window(), 1, String(), [moduleScript](ExecState* exec) -> JSC::EncodedJSValue {
     282        VM& vm = exec->vm();
     283        auto scope = DECLARE_THROW_SCOPE(vm);
    282284        Identifier moduleKey = jsValueToModuleKey(exec, exec->argument(0));
     285        RETURN_IF_EXCEPTION(scope, { });
    283286        moduleScript->notifyLoadCompleted(*moduleKey.impl());
    284287        return JSValue::encode(jsUndefined());
Note: See TracChangeset for help on using the changeset viewer.