Bug 151501 – JSC Builtins should use safe array methods

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

[youenn fablet]

Currently builtins are using push and shift directly, which can be modified by user scripts.

[youenn fablet]

Created attachment 265971 [details]
Patch

[youenn fablet]

Created attachment 266355 [details]
Adding tests

[Xabier Rodríguez Calvar]

Comment on attachment 266355 [details]
Adding tests

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

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:100
> -    JSC_NATIVE_INTRINSIC_FUNCTION("push", arrayProtoFuncPush, DontEnum, 1, ArrayPushIntrinsic);
> +    JSC_NATIVE_INTRINSIC_FUNCTION(vm.propertyNames->builtinNames().pushPublicName(), arrayProtoFuncPush, DontEnum, 1, ArrayPushIntrinsic);
> +    JSC_NATIVE_INTRINSIC_FUNCTION(vm.propertyNames->builtinNames().pushPrivateName(), arrayProtoFuncPush, DontEnum | DontDelete | ReadOnly, 1, ArrayPushIntrinsic);

This makes me think even harder that we need to find a proper solution for this, as adding internal slots with the original function seems like a workaround and I am almost sure we are forgetting to fix something.

> LayoutTests/streams/streams-promises.html:160
> +    const ArrayPushBackup = Array.prototype.push;
> +    const ArrayShiftBackup = Array.prototype.shift;
> +
> +    function cleanTest() {
> +        Array.prototype.push = ArrayPushBackup;
> +        Array.prototype.shift = ArrayShiftBackup;
> +    }

I guess at some point it would be interesting to implement a setup and tear down for the tests.

[Geoffrey Garen]

Comment on attachment 266355 [details]
Adding tests

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

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:103
> +    JSC_NATIVE_FUNCTION(vm.propertyNames->builtinNames().shiftPublicName(), arrayProtoFuncShift, DontEnum, 0);
> +    JSC_NATIVE_FUNCTION(vm.propertyNames->builtinNames().shiftPrivateName(), arrayProtoFuncShift, DontEnum | DontDelete | ReadOnly, 0);

Wrong function here.

[youenn fablet]

Comment on attachment 266355 [details]
Adding tests

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:100
>> +    JSC_NATIVE_INTRINSIC_FUNCTION(vm.propertyNames->builtinNames().pushPrivateName(), arrayProtoFuncPush, DontEnum | DontDelete | ReadOnly, 1, ArrayPushIntrinsic);
> 
> This makes me think even harder that we need to find a proper solution for this, as adding internal slots with the original function seems like a workaround and I am almost sure we are forgetting to fix something.

The latter is indeed worrying.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:103
>> +    JSC_NATIVE_FUNCTION(vm.propertyNames->builtinNames().shiftPrivateName(), arrayProtoFuncShift, DontEnum | DontDelete | ReadOnly, 0);
> 
> Wrong function here.

Can you be more explicit about what is wrong here?

[Geoffrey Garen]

Comment on attachment 266355 [details]
Adding tests

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

>>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:103
>>> +    JSC_NATIVE_FUNCTION(vm.propertyNames->builtinNames().shiftPrivateName(), arrayProtoFuncShift, DontEnum | DontDelete | ReadOnly, 0);
>> 
>> Wrong function here.
> 
> Can you be more explicit about what is wrong here?

Oops. I take it back.

[youenn fablet]

Created attachment 266516 [details]
Improving test

[youenn fablet]

> > LayoutTests/streams/streams-promises.html:160
> > +    const ArrayPushBackup = Array.prototype.push;
> > +    const ArrayShiftBackup = Array.prototype.shift;
> > +
> > +    function cleanTest() {
> > +        Array.prototype.push = ArrayPushBackup;
> > +        Array.prototype.shift = ArrayShiftBackup;
> > +    }
> 
> I guess at some point it would be interesting to implement a setup and tear
> down for the tests.

Right.
I improved a bit the test as modifying Array.prototype.push is making testharness.js unhappy when cleanTest is not called or not called early enough.
Current version will mark test as failing if an assertion does not pass, which is better than timing out.

[WebKit Commit Bot]

Comment on attachment 266516 [details]
Improving test

Clearing flags on attachment: 266516

Committed r193899: <http://trac.webkit.org/changeset/193899>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.