Bug 187211 – Builtins and host functions should get their own structures.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

<rdar://problem/41646336>

[Saam Barati]

Why?

[Mark Lam]

(In reply to Saam Barati from comment #1)
> Why?

Because normal JSFunctions can start out with the same structure as a builtin.  When in strict mode, the structure for a builtin function will be the same as a normal empty function.

If the program invokes hasOwnProperty("prototype") on the builtin first and we cache the result, we will end up caching a false value.  However, when the program subsequently invokes hasOwnProperty("prototype") on a normal empty function (e.g. foo = function() {}), it will see the same structure and return the false result in the cache.  However, the expected result in this case should be true.

[Saam Barati]

(In reply to Mark Lam from comment #2)
> (In reply to Saam Barati from comment #1)
> > Why?
> 
> Because normal JSFunctions can start out with the same structure as a
> builtin.  When in strict mode, the structure for a builtin function will be
> the same as a normal empty function.
> 
> If the program invokes hasOwnProperty("prototype") on the builtin first and
> we cache the result, we will end up caching a false value.  However, when
> the program subsequently invokes hasOwnProperty("prototype") on a normal
> empty function (e.g. foo = function() {}), it will see the same structure
> and return the false result in the cache.  However, the expected result in
> this case should be true.

they shouldn't have the same structure then.

[Saam Barati]

What you're describing is a bug that is not just in this cache, but all our ICs

[Mark Lam]

(In reply to Saam Barati from comment #4)
> What you're describing is a bug that is not just in this cache, but all our
> ICs

I'll investigate further.

[Mark Lam]

Created attachment 344016 [details]
proposed patch.

[Saam Barati]

Comment on attachment 344016 [details]
proposed patch.

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

Nice. r=me

> JSTests/stress/regress-187211.js:76
> +}

Can we also add a test for getById ICs? I’m guessing they will run into the same problem of producing the wrong result

> Source/JavaScriptCore/runtime/JSFunction.cpp:73
> +    if (executable->isBuiltinFunction()) {

Might be worth an assert above that arrow functions aren’t Builtins? Or can they be if they’re nested in a builtin? Or maybe it’s moot because they don’t have a “prototype” property?

[Mark Lam]

(In reply to Saam Barati from comment #7)
> Comment on attachment 344016 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344016&action=review
> 
> Nice. r=me
> 
> > JSTests/stress/regress-187211.js:76
> > +}
> 
> Can we also add a test for getById ICs? I’m guessing they will run into the
> same problem of producing the wrong result

I think getById is not vulnerable, and I have not been able to write a test to get it to fail.  Looking at the code, getById caches an offset only if PropertySlot.isCacheable() is true.  In the case of the builtin function's prototype property, isCacheable() is false.

However, the HasOwnPropertyCache caches a result value if !(!slot.isCacheable() && !slot.isUnset()) i.e. it caches if slot.isCacheable() || slot.isUnset().  The second condition is what allow the HasOwnProperty result to be cached for the builtin function, and this is the source of the discrepancy that led to this bug.


> > Source/JavaScriptCore/runtime/JSFunction.cpp:73
> > +    if (executable->isBuiltinFunction()) {
> 
> Might be worth an assert above that arrow functions aren’t Builtins? Or can
> they be if they’re nested in a builtin? Or maybe it’s moot because they
> don’t have a “prototype” property?

My testing shows that there are builtin arrow functions.  I think you are right that arrow functions not having the "prototype" property makes the immediate issue moot.  However, I think it is fragile to leave it in this state: if anyone adds another lazily reified property, there's a chance that it can become an issue.

Hence, I've re-written the code to give builtin arrow functions a different structure as well.  I'll upload a patch shortly.

[Mark Lam]

Created attachment 344079 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 344079 [details]
proposed patch.

Attachment 344079 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8409883

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 344085 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 344079 [details]
proposed patch.

Attachment 344079 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8410411

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 344091 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Saam Barati]

Comment on attachment 344016 [details]
proposed patch.

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

>>> JSTests/stress/regress-187211.js:76
>>> +}
>> 
>> Can we also add a test for getById ICs? I’m guessing they will run into the same problem of producing the wrong result
> 
> I think getById is not vulnerable, and I have not been able to write a test to get it to fail.  Looking at the code, getById caches an offset only if PropertySlot.isCacheable() is true.  In the case of the builtin function's prototype property, isCacheable() is false.
> 
> However, the HasOwnPropertyCache caches a result value if !(!slot.isCacheable() && !slot.isUnset()) i.e. it caches if slot.isCacheable() || slot.isUnset().  The second condition is what allow the HasOwnProperty result to be cached for the builtin function, and this is the source of the discrepancy that led to this bug.

Do Builtins have a prototype property or not? It seems like the GetById cache would break when given an unset slot

This is what GetById does, which I believe is the same as the own property cache

if (!slot.isCacheable() && !slot.isUnset())
                return GiveUpOnCache;

[Mark Lam]

(In reply to Saam Barati from comment #14)
> Comment on attachment 344016 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344016&action=review
> 
> >>> JSTests/stress/regress-187211.js:76
> >>> +}
> >> 
> >> Can we also add a test for getById ICs? I’m guessing they will run into the same problem of producing the wrong result
> > 
> > I think getById is not vulnerable, and I have not been able to write a test to get it to fail.  Looking at the code, getById caches an offset only if PropertySlot.isCacheable() is true.  In the case of the builtin function's prototype property, isCacheable() is false.
> > 
> > However, the HasOwnPropertyCache caches a result value if !(!slot.isCacheable() && !slot.isUnset()) i.e. it caches if slot.isCacheable() || slot.isUnset().  The second condition is what allow the HasOwnProperty result to be cached for the builtin function, and this is the source of the discrepancy that led to this bug.
> 
> Do Builtins have a prototype property or not? It seems like the GetById
> cache would break when given an unset slot
> 
> This is what GetById does, which I believe is the same as the own property
> cache
> 
> if (!slot.isCacheable() && !slot.isUnset())
>                 return GiveUpOnCache;

Thanks for the review.  I'll dig further to see what I was missing when I attempted to make the GetById test and failed.  Since we know that this patch is fundamentally correct, I'll move forward and land it first.  If I can come up with a test for GetById, I'll land that in a follow up.  Thanks.

[Mark Lam]

Fix landed in r233426: <http://trac.webkit.org/r233426>.