[csa] Bailout to the runtime for ToInteger conversion in Array.p.indexOf.

[csa] Bailout to the runtime for ToInteger conversion in Array.p.indexOf.

The fast-path for Array.prototype.indexOf first checks whether the
receiver is a fast-mode JSArray (and there are no elements in the
prototype chain in case of holey arrays), then loads the known
JSArray::length, and afterwards calls ToInteger on the fromIndex.

But this ToInteger(fromIndex) call can cause arbitrary side effects if
the fromIndex is a JSReceiver, in particular it can invalidate the
assumptions about the fast-mode of the receiver and the length. In the
worst case this leads to OOB memory access.

Quick-fix is to bailout to the runtime if the fromIndex is neither a Smi
nor undefined, which represents the common cases.

R=jarin@chromium.org
BUG=chromium:702058
Review-Url: https://codereview.chromium.org/2756663002
Cr-Commit-Position: refs/heads/master@{#43843}
Committed: https://chromium.googlesource.com/v8/v8/+/9224d5d1bc61c5054ff79b9c5c700052b85c56d3

[Benedikt Meurer, 2017-03-16 05:44:49]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 05:44:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2017-03-16 05:45:34]

Hey Jaro,

Here's a quickfix for an OOB access in Array.p.indexOf.
Please take a look.

Thanks,
Benedikt

[Benedikt Meurer, 2017-03-16 05:51:25]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 05:51:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jarin, 2017-03-16 05:55:02]

lgtm

https://codereview.chromium.org/2756663002/diff/1/src/builtins/builtins-array.cc
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2756663002/diff/1/src/builtins/builtins-array...
src/builtins/builtins-array.cc:2054: Branch(TaggedIsSmi(start_from),
&init_k_smi, &init_k_other);
Please explain in a comment what you are doing here.

[Benedikt Meurer, 2017-03-16 05:55:31]

https://codereview.chromium.org/2756663002/diff/1/src/builtins/builtins-array.cc
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2756663002/diff/1/src/builtins/builtins-array...
src/builtins/builtins-array.cc:2054: Branch(TaggedIsSmi(start_from),
&init_k_smi, &init_k_other);
On 2017/03/16 05:55:02, Jarin wrote:
> Please explain in a comment what you are doing here.

Done.

[Benedikt Meurer, 2017-03-16 05:55:36]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-03-16 05:55:39]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2017-03-16 05:55:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-03-16 06:03:47]

On 2017/03/16 05:55:47, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Does this need to handle Array.includes() as well?

[Benedikt Meurer, 2017-03-16 06:04:20]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-03-16 06:07:59]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2017-03-16 06:08:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2017-03-16 06:08:13]

On 2017/03/16 06:03:47, dcheng wrote:
> On 2017/03/16 05:55:47, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Does this need to handle Array.includes() as well?

Array.p.includes doesn't have this bug.

[Benedikt Meurer, 2017-03-16 06:10:06]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-03-16 06:29:40]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 06:29:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2017-03-16 06:43:39]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-03-16 06:43:45]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-03-16 06:43:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2756663002/#ps40001
(title: "Also fix FastDoubleElementsAccessor::IndexOfValueImpl")

[commit-bot: I haz the power, 2017-03-16 06:43:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 06:53:10]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489646625037090,
"parent_rev": "f76966ae720955e2492435233b043acb7930eda3", "commit_rev":
"9224d5d1bc61c5054ff79b9c5c700052b85c56d3"}

[commit-bot: I haz the power, 2017-03-16 06:53:15]

Description was changed from

==========
[csa] Bailout to the runtime for ToInteger conversion in Array.p.indexOf.

The fast-path for Array.prototype.indexOf first checks whether the
receiver is a fast-mode JSArray (and there are no elements in the
prototype chain in case of holey arrays), then loads the known
JSArray::length, and afterwards calls ToInteger on the fromIndex.

But this ToInteger(fromIndex) call can cause arbitrary side effects if
the fromIndex is a JSReceiver, in particular it can invalidate the
assumptions about the fast-mode of the receiver and the length. In the
worst case this leads to OOB memory access.

Quick-fix is to bailout to the runtime if the fromIndex is neither a Smi
nor undefined, which represents the common cases.

R=jarin@chromium.org
BUG=chromium:702058
==========

to

==========
[csa] Bailout to the runtime for ToInteger conversion in Array.p.indexOf.

The fast-path for Array.prototype.indexOf first checks whether the
receiver is a fast-mode JSArray (and there are no elements in the
prototype chain in case of holey arrays), then loads the known
JSArray::length, and afterwards calls ToInteger on the fromIndex.

But this ToInteger(fromIndex) call can cause arbitrary side effects if
the fromIndex is a JSReceiver, in particular it can invalidate the
assumptions about the fast-mode of the receiver and the length. In the
worst case this leads to OOB memory access.

Quick-fix is to bailout to the runtime if the fromIndex is neither a Smi
nor undefined, which represents the common cases.

R=jarin@chromium.org
BUG=chromium:702058

Review-Url: https://codereview.chromium.org/2756663002
Cr-Commit-Position: refs/heads/master@{#43843}
Committed:
https://chromium.googlesource.com/v8/v8/+/9224d5d1bc61c5054ff79b9c5c700052b85...
==========

[commit-bot: I haz the power, 2017-03-16 06:53:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/9224d5d1bc61c5054ff79b9c5c700052b85...