Check that array length stays a safe integer in Array.prototype.push

Check that array length stays a safe integer in Array.prototype.push

This patch adds a check in Array.prototype.push to assert that the new
length does not become greater than 2**53-1. Such a length would be
dangerous because integer arithmetic becomes imprecise after the
boundary. The check is also required by a test262 test.

R=adamk
LOG=Y
BUG=v8:3087
Committed: https://crrev.com/e68adf4548dd101dc08fcbff14444152fb1b7fe7
Cr-Commit-Position: refs/heads/master@{#31588}

[Dan Ehrenberg, 2015-10-26 20:02:26]

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

[commit-bot: I haz the power, 2015-10-26 20:02:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428483002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428483002/1

[Dan Ehrenberg, 2015-10-26 20:02:44]

Description was changed from

==========
Check that array length stays a safe integer in Array.prototype.push

This patch adds a check in Array.prototype.push to assert that the new
length does not become greater than 2**53-1. Such a length would be
dangerous because integer arithmetic becomes imprecise after the
boundary.

R=adamk
LOG=Y
BUG=v8:3087
==========

to

==========
Check that array length stays a safe integer in Array.prototype.push

This patch adds a check in Array.prototype.push to assert that the new
length does not become greater than 2**53-1. Such a length would be
dangerous because integer arithmetic becomes imprecise after the
boundary. The check is also required by a test262 test.

R=adamk
LOG=Y
BUG=v8:3087
==========

[commit-bot: I haz the power, 2015-10-26 20:13:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-26 20:13:06]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[adamk, 2015-10-26 21:21:03]

https://codereview.chromium.org/1428483002/diff/20001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/1428483002/diff/20001/src/js/array.js#newcode520
src/js/array.js:520: // The length of arguments can never be more than 2 ** 32,
so
It looks to me like it can't be more than 2**16, actually...

https://codereview.chromium.org/1428483002/diff/20001/src/js/array.js#newcode522
src/js/array.js:522: // the length in order to avoid integer overflow.
Is this math necessary to get the right answer or is it an optimization? If the
latter, did this actually show up in some benchmark? It's definitely a little
funny-looking for a the "slow" path.

https://codereview.chromium.org/1428483002/diff/20001/src/js/array.js#newcode524
src/js/array.js:524: throw MakeTypeError(kPushPastSafeLength, m, n);
Nit: indentation off, needs one more leading space.

[Dan Ehrenberg, 2015-10-26 22:59:32]

https://codereview.chromium.org/1428483002/diff/20001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/1428483002/diff/20001/src/js/array.js#newcode520
src/js/array.js:520: // The length of arguments can never be more than 2 ** 32,
so
On 2015/10/26 at 21:21:02, adamk wrote:
> It looks to me like it can't be more than 2**16, actually...

We talked about this offline. Apparently it can be more...

https://codereview.chromium.org/1428483002/diff/20001/src/js/array.js#newcode522
src/js/array.js:522: // the length in order to avoid integer overflow.
On 2015/10/26 at 21:21:03, adamk wrote:
> Is this math necessary to get the right answer or is it an optimization? If
the latter, did this actually show up in some benchmark? It's definitely a
little funny-looking for a the "slow" path.

It's necessary for the answer. Without that change, it was just not detecting a
single element push.

https://codereview.chromium.org/1428483002/diff/20001/src/js/array.js#newcode524
src/js/array.js:524: throw MakeTypeError(kPushPastSafeLength, m, n);
On 2015/10/26 at 21:21:02, adamk wrote:
> Nit: indentation off, needs one more leading space.

Fixed

[Dan Ehrenberg, 2015-10-26 22:59:36]

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

[commit-bot: I haz the power, 2015-10-26 22:59:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428483002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428483002/40001

[commit-bot: I haz the power, 2015-10-26 23:09:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-26 23:09:53]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/11144)

[Dan Ehrenberg, 2015-10-26 23:13:20]

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

[commit-bot: I haz the power, 2015-10-26 23:13:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428483002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428483002/60001

[adamk, 2015-10-26 23:14:28]

https://codereview.chromium.org/1428483002/diff/40001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/1428483002/diff/40001/src/js/array.js#newcode525
src/js/array.js:525: if (m > 1 << 32 || (n - (1 << 32)) + m > kMaxSafeInteger -
(1 << 32)) {
"1 << 32" isn't doing what you want. It returns 1...

[Dan Ehrenberg, 2015-10-26 23:26:36]

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

[commit-bot: I haz the power, 2015-10-26 23:26:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428483002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428483002/80001

[commit-bot: I haz the power, 2015-10-26 23:36:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-26 23:36:25]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/11146)

[Dan Ehrenberg, 2015-10-26 23:56:20]

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

[commit-bot: I haz the power, 2015-10-26 23:56:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428483002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428483002/100001

[commit-bot: I haz the power, 2015-10-27 00:05:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 00:05:13]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[Dan Ehrenberg, 2015-10-27 00:31:52]

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

[commit-bot: I haz the power, 2015-10-27 00:32:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428483002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428483002/120001

[Dan Ehrenberg, 2015-10-27 00:47:36]

https://codereview.chromium.org/1428483002/diff/40001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/1428483002/diff/40001/src/js/array.js#newcode525
src/js/array.js:525: if (m > 1 << 32 || (n - (1 << 32)) + m > kMaxSafeInteger -
(1 << 32)) {
On 2015/10/26 at 23:14:28, adamk wrote:
> "1 << 32" isn't doing what you want. It returns 1...

Right. I finally settled on something that works, WDYT?

[commit-bot: I haz the power, 2015-10-27 00:57:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 00:57:25]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-10-27 01:11:43]

lgtm

[Dan Ehrenberg, 2015-10-27 01:13:09]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2015-10-27 01:13:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428483002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428483002/120001

[commit-bot: I haz the power, 2015-10-27 04:39:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-27 04:39:34]

Patchset 7 (id:??) landed as
https://crrev.com/e68adf4548dd101dc08fcbff14444152fb1b7fe7
Cr-Commit-Position: refs/heads/master@{#31588}

[Dan Ehrenberg, 2015-10-27 05:45:28]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1418093007/ by littledan@chromium.org.

The reason for reverting is: Caused for-in-opt test to fail.