Bug 142922 – Insert exception check around toPropertyKey call

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

In some places (such as operationPutByValInternal), exception check is missing after toPropertyKey.
However, since it calls toString and it could raise exceptions, we should perform exception check.

[Darin Adler]

We should also write a regression test for this. I think it can be done.

[Yusuke Suzuki]

(In reply to comment #1)
> We should also write a regression test for this. I think it can be done.

Seeing the code, I found that we cannot observe this regression. Since

+ Missing exception checks are placed in GetByVal codes
+ Workaround exception check was inserted in callGetter (http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/GetterSetter.cpp#L77)

[Yusuke Suzuki]

Created attachment 249171 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 249171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249171&action=review

Added comments.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:575
> +    ASSERT(!exec->hadException());

Since property is always JSString.

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:91
> +    return JSValue::encode(jsBoolean(thisValue.toObject(exec)->hasOwnProperty(exec, propertyName)));

When performing toObject onto null/undefined, it returnes JSNotAObject. This can allow us to defer exception checks live the above code.

[Darin Adler]

Comment on attachment 249171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249171&action=review

> Source/JavaScriptCore/ChangeLog:15
> +        But since callGetter has conservative (defensive) exception guard,
> +        missing exception checks in GetByVal of JIT/DFG/LLInt cannot be observed.

If they can’t be observed, the checks aren’t needed.

[Darin Adler]

Comment on attachment 249171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249171&action=review

Code generally looks good. Please don’t land the changes that are not testable/observable, though.

>> Source/JavaScriptCore/ChangeLog:15
>> +        missing exception checks in GetByVal of JIT/DFG/LLInt cannot be observed.
> 
> If they can’t be observed, the checks aren’t needed.

Any that can’t be observed aren’t actually bugs!

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:576
> +    auto propertyName = property.toPropertyKey(exec);
> +    ASSERT(!exec->hadException());
> +    RETURN(baseValue.get(exec, propertyName));

I would write this:

    RETURN(baseValue.get(exec, asString(property)->toIdentifier(exec));

I don’t see any reason to call a function that is then going to check if property is a string. Nor do I see a need to split this out into multiple lines of code just to assert there is no exception.

[Yusuke Suzuki]

Comment on attachment 249171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249171&action=review

I've found one more issues. I'll fix it with subsequent patch.

>>> Source/JavaScriptCore/ChangeLog:15
>>> +        missing exception checks in GetByVal of JIT/DFG/LLInt cannot be observed.
>> 
>> If they can’t be observed, the checks aren’t needed.
> 
> Any that can’t be observed aren’t actually bugs!

After investigating the code, I've found that the current code works fine and my change breaks it.
I'll write it in LLIntSlowPaths.cpp.
And I'll fix my patch with explicit comments.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:751
>      return baseValue.get(exec, property);

Reaching here, `baseValue` may be null or undefined. So baseValue.get also may cause errors.
There's several source of exceptions.

1. baseValue.get can cause errors if it's null or undefined.
2. toPropertyKey can cause errors

Seeing the spec, VM first checks (1) and perfoms toPropertyKey and checks (2).
So the above fix cause observable incorrect behavior.

var object = null;
var property = {
  toString() {
    throw new Error("out");
  }
}

object[property];    // We should see TypeError (since object is null), but with the above fix, we'll see new Error("out");

I think it's observable. I'll update the patch.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:576
>> +    RETURN(baseValue.get(exec, propertyName));
> 
> I would write this:
> 
>     RETURN(baseValue.get(exec, asString(property)->toIdentifier(exec));
> 
> I don’t see any reason to call a function that is then going to check if property is a string. Nor do I see a need to split this out into multiple lines of code just to assert there is no exception.

Looks nice. I'll fix it.

[Yusuke Suzuki]

(In reply to comment #7)

Typo: subsequent => updated.

[Yusuke Suzuki]

Created attachment 249194 [details]
Patch

[Geoffrey Garen]

Comment on attachment 249194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249194&action=review

> Source/JavaScriptCore/dfg/DFGOperations.cpp:300
> +    if (baseValue.isUndefinedOrNull())
> +        return JSValue::encode(vm.throwException(exec, createNotAnObjectError(exec, baseValue)));

Won't JSValue::get do this check for us?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:331
> +    ASSERT(!JSValue(base).isUndefinedOrNull());

I don't think this ASSERT makes sense because a cell will never be undefined or null, by virtue of it being a cell.

> Source/JavaScriptCore/jit/JITOperations.cpp:1425
> +    if (baseValue.isUndefinedOrNull())
> +        return exec->vm().throwException(exec, createNotAnObjectError(exec, baseValue));

I think get will do this for us. You should review other places and find out if these checks are really needed. I guess I'm echoing Darin's comments a bit.

[Yusuke Suzuki]

Comment on attachment 249194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249194&action=review

Thank you for your review!

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:300
>> +        return JSValue::encode(vm.throwException(exec, createNotAnObjectError(exec, baseValue)));
> 
> Won't JSValue::get do this check for us?

Yes, but we canot use this here.

JSValue::get checks null/undefined and raise an exception if |this| is null or undefined.
But, in this case, I don't use it because of the following 3 reasons.

(1)
We need to check whether the baseValue is object coercible before executing JSValue::toPropertyKey, 
Consider the following script,

var object = null;
var called = false;
var property = {
    toString() {
        called = true;
        retrun "value";
    }
};
try {
    object[property];
} catch (e) {
}
assert(!called);

toPropertyKey can cause toString call. So that is observable.
If we use the following implementation,

    auto propertyName = property.toPropertyKey(exec);
    if (exec->hadException())
        return JSValue::encode(jsUndefined());
    return JSValue::encode(baseValue.get(exec, propertyName));

The above script causes an assertion failure.

(2)
I think we should not use baseValue.toObject here like the following,

    baseValue.toObject(exec);  // Check object coercible by using toObject.
    if (exec->hadException())
        return JSValue::encode(jsUndefined());
    auto propertyName = property.toPropertyKey(exec);
    if (exec->hadException())
        return JSValue::encode(jsUndefined());
    return JSValue::encode(baseValue.get(exec, propertyName));

toObject converts primitive types into wrapper objects.
But it is not efficient since wrapper objects are not necessary if we look up methods from primitive values's prototype. (using synthesizePrototype is better).

(3)
And we need to take care that we use JSValue::get in the last sequence instead of JSObject::get.
If we use JSObject that is converted by toObject, getter will be called by using this JSObject as |this|.

    JSObject* baseObject = baseValue.toObject(exec);
    if (exec->hadException())
        return JSValue::encode(jsUndefined());
    auto propertyName = property.toPropertyKey(exec);
    if (exec->hadException())
        return JSValue::encode(jsUndefined());
    return JSValue::encode(baseObject->get(exec, propertyName));    // Using baseObject. This is not correct to the spec.

It is not correct since getter should be called with the original |this| value that may be primitive types.


So, in the updated patch, I checks whether baseValue is object coercible explicitly before calling toPropertyKey.

But, you're right, using

   if (baseValue.isUndefinedOrNull())
        return JSValue::encode(vm.throwException(exec, createNotAnObjectError(exec, baseValue)));

explicitly is not good.

Instead, I'll propose that introduce `JSValue::requireObjectCoercible`.
The implementation has one on one corresponding to RequireObjectCoercible in the spec.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-requireobjectcoercible

And the implementation will be the following.

    baseValue.requireObjectCoercible(exec);
    if (exec->hadException())
        return JSValue::encode(jsUndefined());
    auto propertyName = property.toPropertyKey(exec);
    if (exec->hadException())
        return JSValue::encode(jsUndefined());
    return JSValue::encode(baseValue.get(exec, propertyName));

What do you think of?

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:331
>> +    ASSERT(!JSValue(base).isUndefinedOrNull());
> 
> I don't think this ASSERT makes sense because a cell will never be undefined or null, by virtue of it being a cell.

Thanks. OK, I'll drop this.

>> Source/JavaScriptCore/jit/JITOperations.cpp:1425
>> +        return exec->vm().throwException(exec, createNotAnObjectError(exec, baseValue));
> 
> I think get will do this for us. You should review other places and find out if these checks are really needed. I guess I'm echoing Darin's comments a bit.

I've commented the reason why we use this check explicitly.

[Yusuke Suzuki]

Created attachment 249316 [details]
Patch

Updated patch. Adding JSValue::requireObjectCoercible and use it intead of explicit isUndefinedOrNull checks

[Yusuke Suzuki]

Created attachment 249317 [details]
Patch

Updated patch. Adding JSValue::requireObjectCoercible and use it intead of explicit isUndefinedOrNull checks

[Geoffrey Garen]

Comment on attachment 249317 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 249317 [details]
Patch

Clearing flags on attachment: 249317

Committed r182057: <http://trac.webkit.org/changeset/182057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.