Changeset 260343 in webkit


Ignore:
Timestamp:
Apr 19, 2020, 2:18:07 PM (5 years ago)
Author:
mark.lam@apple.com
Message:

Fix missing exception checks and handling in JSC APIs.
​https://bugs.webkit.org/show_bug.cgi?id=210715
<rdar://problem/61599658>

Reviewed by Saam Barati.

  • API/APICallbackFunction.h:

(JSC::APICallbackFunction::call):

  • We should return early if an exception was thrown. We should not be using the result in any way since we cannot rely on it having any sane value.

(JSC::APICallbackFunction::construct):

  • For consistency, also return an undefined here when an exception was thrown.
  • API/JSCallbackObjectFunctions.h:

(JSC::JSCallbackObject<Parent>::construct):
(JSC::JSCallbackObject<Parent>::call):

  • Return an undefined if an exception was thrown. Don't return the potentially garbage result value. Who knows what the client code will do with it. Returning an undefined here makes the code more robust.
  • API/JSObjectRef.cpp:

(JSObjectGetProperty):
(JSObjectHasPropertyForKey):
(JSObjectGetPropertyForKey):
(JSObjectDeletePropertyForKey):
(JSObjectGetPropertyAtIndex):
(JSObjectDeleteProperty):

  • Explicitly return a nullptr if an exception was thrown. The toRef() on the result that follows the exception check may or may not return a nullptr (also see toRef(JSC::VM& vm, JSC::JSValue v) for !CPU(ADDRESS64)).
  • API/JSValueRef.cpp:

(JSValueIsEqual):
(JSValueIsInstanceOfConstructor):

  • For consistency, make these return false if an exception is thrown.
  • API/ObjCCallbackFunction.mm:

(JSC::objCCallbackFunctionCallAsFunction):
(JSC::objCCallbackFunctionCallAsConstructor):
(JSC::ObjCCallbackFunctionImpl::call):

  • Add some assertions and return early if an exception was thrown.
Location:
trunk/Source/JavaScriptCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/API/APICallbackFunction.h

    r259676 r260343  
    11/*
    2  * Copyright (C) 2013, 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013-2020 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    6363        result = jsCast<T*>(toJS(functionRef))->functionCallback()(execRef, functionRef, thisObjRef, argumentCount, arguments.data(), &exception);
    6464    }
    65     if (exception)
     65    if (exception) {
    6666        throwException(globalObject, scope, toJS(globalObject, exception));
     67        return JSValue::encode(jsUndefined());
     68    }
    6769
    6870    // result must be a valid JSValue.
    … …  
    98100        if (exception) {
    99101            throwException(globalObject, scope, toJS(globalObject, exception));
    100             return JSValue::encode(toJS(globalObject, exception));
     102            return JSValue::encode(jsUndefined());
    101103        }
    102104        // result must be a valid JSValue.

  • trunk/Source/JavaScriptCore/API/JSCallbackObjectFunctions.h

    r259676 r260343  
    466466                result = toJS(callAsConstructor(execRef, constructorRef, argumentCount, arguments.data(), &exception));
    467467            }
    468             if (exception)
     468            if (exception) {
    469469                throwException(globalObject, scope, toJS(globalObject, exception));
     470                return JSValue::encode(jsUndefined());
     471            }
    470472            return JSValue::encode(result);
    471473        }
    … …  
    539541                result = toJS(globalObject, callAsFunction(execRef, functionRef, thisObjRef, argumentCount, arguments.data(), &exception));
    540542            }
    541             if (exception)
     543            if (exception) {
    542544                throwException(globalObject, scope, toJS(globalObject, exception));
     545                return JSValue::encode(jsUndefined());
     546            }
    543547            return JSValue::encode(result);
    544548        }

  • trunk/Source/JavaScriptCore/API/JSObjectRef.cpp

    r257529 r260343  
    349349    if (!ctx || !object) {
    350350        ASSERT_NOT_REACHED();
    351         return 0;
     351        return nullptr;
    352352    }
    353353    JSGlobalObject* globalObject = toJS(ctx);
    … …  
    359359
    360360    JSValue jsValue = jsObject->get(globalObject, propertyName->identifier(&vm));
    361     handleExceptionIfNeeded(scope, ctx, exception);
     361    if (handleExceptionIfNeeded(scope, ctx, exception) == ExceptionStatus::DidThrow)
     362        return nullptr;
    362363    return toRef(globalObject, jsValue);
    363364}
    … …  
    408409
    409410    bool result = jsObject->hasProperty(globalObject, ident);
    410     handleExceptionIfNeeded(scope, ctx, exception);
     411    if (handleExceptionIfNeeded(scope, ctx, exception) == ExceptionStatus::DidThrow)
     412        return false;
    411413    return result;
    412414}
    … …  
    429431
    430432    JSValue jsValue = jsObject->get(globalObject, ident);
    431     handleExceptionIfNeeded(scope, ctx, exception);
     433    if (handleExceptionIfNeeded(scope, ctx, exception) == ExceptionStatus::DidThrow)
     434        return nullptr;
    432435    return toRef(globalObject, jsValue);
    433436}
    … …  
    481484
    482485    bool result = JSCell::deleteProperty(jsObject, globalObject, ident);
    483     handleExceptionIfNeeded(scope, ctx, exception);
     486    if (handleExceptionIfNeeded(scope, ctx, exception) == ExceptionStatus::DidThrow)
     487        return false;
    484488    return result;
    485489}
    … …  
    489493    if (!ctx) {
    490494        ASSERT_NOT_REACHED();
    491         return 0;
     495        return nullptr;
    492496    }
    493497    JSGlobalObject* globalObject = toJS(ctx);
    … …  
    499503
    500504    JSValue jsValue = jsObject->get(globalObject, propertyIndex);
    501     handleExceptionIfNeeded(scope, ctx, exception);
     505    if (handleExceptionIfNeeded(scope, ctx, exception) == ExceptionStatus::DidThrow)
     506        return nullptr;
    502507    return toRef(globalObject, jsValue);
    503508}
    … …  
    536541
    537542    bool result = JSCell::deleteProperty(jsObject, globalObject, propertyName->identifier(&vm));
    538     handleExceptionIfNeeded(scope, ctx, exception);
     543    if (handleExceptionIfNeeded(scope, ctx, exception) == ExceptionStatus::DidThrow)
     544        return false;
    539545    return result;
    540546}

  • trunk/Source/JavaScriptCore/API/JSValueRef.cpp

    r253365 r260343  
    11/*
    2  * Copyright (C) 2006-2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2006-2020 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    236236
    237237    bool result = JSValue::equal(globalObject, jsA, jsB); // false if an exception is thrown
    238     handleExceptionIfNeeded(scope, ctx, exception);
     238    if (handleExceptionIfNeeded(scope, ctx, exception) == ExceptionStatus::DidThrow)
     239        return false;
    239240   
    240241    return result;
    … …  
    273274        return false;
    274275    bool result = jsConstructor->hasInstance(globalObject, jsValue); // false if an exception is thrown
    275     handleExceptionIfNeeded(scope, ctx, exception);
     276    if (handleExceptionIfNeeded(scope, ctx, exception) == ExceptionStatus::DidThrow)
     277        return false;
    276278    return result;
    277279}

  • trunk/Source/JavaScriptCore/API/ObjCCallbackFunction.mm

    r251425 r260343  
    11/*
    2  * Copyright (C) 2013, 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013-2020 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    6868    void set(NSInvocation *invocation, NSInteger argumentNumber, JSContext *context, JSValueRef argument, JSValueRef* exception) override
    6969    {
     70        ASSERT(exception && !*exception);
    7071        T value = (T)JSC::toInt32(JSValueToNumber([context JSGlobalContextRef], argument, exception));
     72        if (*exception)
     73            return;
    7174        [invocation setArgument:&value atIndex:argumentNumber];
    7275    }
    … …  
    7780    void set(NSInvocation *invocation, NSInteger argumentNumber, JSContext *context, JSValueRef argument, JSValueRef* exception) override
    7881    {
     82        ASSERT(exception && !*exception);
    7983        T value = (T)JSValueToNumber([context JSGlobalContextRef], argument, exception);
     84        if (*exception)
     85            return;
    8086        [invocation setArgument:&value atIndex:argumentNumber];
    8187    }
    … …  
    108114    void set(NSInvocation *invocation, NSInteger argumentNumber, JSContext *context, JSValueRef argument, JSValueRef* exception) override
    109115    {
     116        ASSERT(exception && !*exception);
    110117        JSGlobalContextRef contextRef = [context JSGlobalContextRef];
    111118
    … …  
    131138    void set(NSInvocation *invocation, NSInteger argumentNumber, JSContext *context, JSValueRef argument, JSValueRef* exception) override
    132139    {
     140        ASSERT(exception && !*exception);
    133141        id value = valueToNumber([context JSGlobalContextRef], argument, exception);
     142        if (*exception)
     143            return;
    134144        [invocation setArgument:&value atIndex:argumentNumber];
    135145    }
    … …  
    139149    void set(NSInvocation *invocation, NSInteger argumentNumber, JSContext *context, JSValueRef argument, JSValueRef* exception) override
    140150    {
     151        ASSERT(exception && !*exception);
    141152        id value = valueToString([context JSGlobalContextRef], argument, exception);
     153        if (*exception)
     154            return;
    142155        [invocation setArgument:&value atIndex:argumentNumber];
    143156    }
    … …  
    147160    void set(NSInvocation *invocation, NSInteger argumentNumber, JSContext *context, JSValueRef argument, JSValueRef* exception) override
    148161    {
     162        ASSERT(exception && !*exception);
    149163        id value = valueToDate([context JSGlobalContextRef], argument, exception);
     164        if (*exception)
     165            return;
    150166        [invocation setArgument:&value atIndex:argumentNumber];
    151167    }
    … …  
    155171    void set(NSInvocation *invocation, NSInteger argumentNumber, JSContext *context, JSValueRef argument, JSValueRef* exception) override
    156172    {
     173        ASSERT(exception && !*exception);
    157174        id value = valueToArray([context JSGlobalContextRef], argument, exception);
     175        if (*exception)
     176            return;
    158177        [invocation setArgument:&value atIndex:argumentNumber];
    159178    }
    … …  
    163182    void set(NSInvocation *invocation, NSInteger argumentNumber, JSContext *context, JSValueRef argument, JSValueRef* exception) override
    164183    {
     184        ASSERT(exception && !*exception);
    165185        id value = valueToDictionary([context JSGlobalContextRef], argument, exception);
     186        if (*exception)
     187            return;
    166188        [invocation setArgument:&value atIndex:argumentNumber];
    167189    }
    … …  
    448470static JSValueRef objCCallbackFunctionCallAsFunction(JSContextRef callerContext, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
    449471{
     472    ASSERT(exception && !*exception);
     473
    450474    // Retake the API lock - we need this for a few reasons:
    451475    // (1) We don't want to support the C-API's confusing drops-locks-once policy - should only drop locks if we can do so recursively.
    … …  
    461485        JSGlobalContextRef contextRef = [context JSGlobalContextRef];
    462486        *exception = toRef(JSC::createTypeError(toJS(contextRef), "Cannot call a class constructor without |new|"_s));
     487        if (*exception)
     488            return nullptr;
    463489        return JSValueMakeUndefined(contextRef);
    464490    }
    … …  
    473499        [context endCallbackWithData:&callbackData];
    474500    }
     501    if (*exception)
     502        return nullptr;
    475503    return result;
    476504}
    … …  
    478506static JSObjectRef objCCallbackFunctionCallAsConstructor(JSContextRef callerContext, JSObjectRef constructor, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
    479507{
     508    ASSERT(exception && !*exception);
    480509    JSC::JSLockHolder locker(toJS(callerContext));
    481510
    … …  
    493522        [context endCallbackWithData:&callbackData];
    494523    }
     524    if (*exception)
     525        return nullptr;
    495526
    496527    JSGlobalContextRef contextRef = [context JSGlobalContextRef];
    497     if (*exception)
    498         return nullptr;
    499 
    500528    if (!JSValueIsObject(contextRef, result)) {
    501529        *exception = toRef(JSC::createTypeError(toJS(contextRef), "Objective-C blocks called as constructors must return an object."_s));
    502530        return nullptr;
    503531    }
     532    ASSERT(!*exception);
    504533    return const_cast<JSObjectRef>(result);
    505534}
    … …  
    541570JSValueRef ObjCCallbackFunctionImpl::call(JSContext *context, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
    542571{
     572    ASSERT(exception && !*exception);
    543573    JSGlobalContextRef contextRef = [context JSGlobalContextRef];
    544574
    … …  
    551581        if (!target || ![target isKindOfClass:m_instanceClass.get()]) {
    552582            *exception = toRef(JSC::createTypeError(toJS(contextRef), "self type check failed for Objective-C instance method"_s));
     583            if (*exception)
     584                return nullptr;
    553585            return JSValueMakeUndefined(contextRef);
    554586        }
    … …  
    561593        if (!target || ![target isKindOfClass:m_instanceClass.get()]) {
    562594            *exception = toRef(JSC::createTypeError(toJS(contextRef), "self type check failed for Objective-C instance method"_s));
     595            if (*exception)
     596                return nullptr;
    563597            return JSValueMakeUndefined(contextRef);
    564598        }
    … …  
    579613        argument->set(m_invocation.get(), argumentNumber + firstArgument, context, value, exception);
    580614        if (*exception)
    581             return JSValueMakeUndefined(contextRef);
     615            return nullptr;
    582616        ++argumentNumber;
    583617    }
    … …  
    586620
    587621    JSValueRef result = m_result->get(m_invocation.get(), context, exception);
     622    if (*exception)
     623        return nullptr;
    588624
    589625    // Balance our call to -alloc with a call to -autorelease. We have to do this after calling -init

  • trunk/Source/JavaScriptCore/ChangeLog

    r260333 r260343  
     12020-04-19  Mark Lam  <mark.lam@apple.com>
     2
     3        Fix missing exception checks and handling in JSC APIs.
     4        https://bugs.webkit.org/show_bug.cgi?id=210715
     5        <rdar://problem/61599658>
     6
     7        Reviewed by Saam Barati.
     8
     9        * API/APICallbackFunction.h:
     10        (JSC::APICallbackFunction::call):
     11        - We should return early if an exception was thrown.  We should not be using the
     12          result in any way since we cannot rely on it having any sane value.
     13        (JSC::APICallbackFunction::construct):
     14        - For consistency, also return an undefined here when an exception was thrown.
     15
     16        * API/JSCallbackObjectFunctions.h:
     17        (JSC::JSCallbackObject<Parent>::construct):
     18        (JSC::JSCallbackObject<Parent>::call):
     19        - Return an undefined if an exception was thrown.  Don't return the potentially
     20          garbage result value.  Who knows what the client code will do with it.  Returning
     21          an undefined here makes the code more robust.
     22
     23        * API/JSObjectRef.cpp:
     24        (JSObjectGetProperty):
     25        (JSObjectHasPropertyForKey):
     26        (JSObjectGetPropertyForKey):
     27        (JSObjectDeletePropertyForKey):
     28        (JSObjectGetPropertyAtIndex):
     29        (JSObjectDeleteProperty):
     30        - Explicitly return a nullptr if an exception was thrown.  The toRef() on the
     31          result that follows the exception check may or may not return a nullptr
     32          (also see toRef(JSC::VM& vm, JSC::JSValue v) for !CPU(ADDRESS64)).
     33
     34        * API/JSValueRef.cpp:
     35        (JSValueIsEqual):
     36        (JSValueIsInstanceOfConstructor):
     37        - For consistency, make these return false if an exception is thrown.
     38
     39        * API/ObjCCallbackFunction.mm:
     40        (JSC::objCCallbackFunctionCallAsFunction):
     41        (JSC::objCCallbackFunctionCallAsConstructor):
     42        (JSC::ObjCCallbackFunctionImpl::call):
     43        - Add some assertions and return early if an exception was thrown.
     44
    1452020-04-18  Keith Miller  <keith_miller@apple.com>
    246
Note: See TracChangeset for help on using the changeset viewer.