Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1431503002:
Avoid creating indexed elements at index maxUint32 (Closed)
|
Created:
5 years, 1 month ago by Dan Ehrenberg Modified:
5 years, 1 month ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAvoid creating indexed elements at index maxUint32
The maximum indexed element size is maxUint32-1, not maxUint32, because
the maximum length of elements is maxUint32. This patch tweaks the limit
to switch to named properties as appropriate.
BUG=v8:4516
LOG=Y
R=adamk
Committed: https://crrev.com/755fcc5fc63dc48a5bf18dee5c3d59de79471a11
Cr-Commit-Position: refs/heads/master@{#31809}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use Jakob's suggestion for a more robust check #
Messages
Total messages: 21 (6 generated)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431503002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431503002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Avoid creating indexed elements at index maxUint32
The maximum indexed element size is maxUint32-1, not maxUint32, because
the maximum length of elements is maxUint32. This patch tweaks the limit
to switch to named properties as appropriate.
BUG=v8:4516
LOG=Y
R=adamk
TEST=Array.prototype.slice.call({length: Math.pow(2, 32)}, 0, Math.pow(2, 32)})
throws a RangeError rather than crashing or "illegal access", even in debug
mode.
==========
to
==========
Avoid creating indexed elements at index maxUint32
The maximum indexed element size is maxUint32-1, not maxUint32, because
the maximum length of elements is maxUint32. This patch tweaks the limit
to switch to named properties as appropriate.
BUG=v8:4516
LOG=Y
R=adamk
==========
DBC. https://codereview.chromium.org/1431503002/diff/1/src/js/runtime.js File src/js/runtime.js (right): https://codereview.chromium.org/1431503002/diff/1/src/js/runtime.js#newcode213 src/js/runtime.js:213: if (index < kMaxUint32) { How do you guarantee that index >= 0?
On 2015/10/30 at 09:38:05, jkummerow wrote: > DBC. > > https://codereview.chromium.org/1431503002/diff/1/src/js/runtime.js > File src/js/runtime.js (right): > > https://codereview.chromium.org/1431503002/diff/1/src/js/runtime.js#newcode213 > src/js/runtime.js:213: if (index < kMaxUint32) { > How do you guarantee that index >= 0? That's just guaranteed by the calling code, same as before this patch.
On 2015/11/02 18:15:52, Dan Ehrenberg wrote: > same as before this patch. Nope, that's my point -- for index == -1, "index === TO_UINT32(index)" is false, but "index < kMaxUint32" is true. So previously this code would do the right thing even if call sites were careless. Elsewhere, we use the pattern "index === TO_UINT32(index) && index != kMaxUint32", see e.g. https://chromium.googlesource.com/v8/v8/+/master/src/objects-inl.h#2395
On 2015/11/03 at 10:17:22, jkummerow wrote: > On 2015/11/02 18:15:52, Dan Ehrenberg wrote: > > same as before this patch. > > Nope, that's my point -- for index == -1, "index === TO_UINT32(index)" is false, but "index < kMaxUint32" is true. So previously this code would do the right thing even if call sites were careless. > > Elsewhere, we use the pattern "index === TO_UINT32(index) && index != kMaxUint32", see e.g. https://chromium.googlesource.com/v8/v8/+/master/src/objects-inl.h#2395 Previously, callers just called out to %AddElement directly. How did that handle careless callsites? I think it would just check-fail there. I can use this pattern but I don't see what the problem is exactly.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431503002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431503002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/11/04 at 17:18:19, Dan Ehrenberg wrote: > On 2015/11/03 at 10:17:22, jkummerow wrote: > > On 2015/11/02 18:15:52, Dan Ehrenberg wrote: > > > same as before this patch. > > > > Nope, that's my point -- for index == -1, "index === TO_UINT32(index)" is false, but "index < kMaxUint32" is true. So previously this code would do the right thing even if call sites were careless. > > > > Elsewhere, we use the pattern "index === TO_UINT32(index) && index != kMaxUint32", see e.g. https://chromium.googlesource.com/v8/v8/+/master/src/objects-inl.h#2395 > > Previously, callers just called out to %AddElement directly. How did that handle careless callsites? I think it would just check-fail there. I can use this pattern but I don't see what the problem is exactly. Made this change, PTAL.
lgtm given that you've done exactly what jkummerow asked for
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431503002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431503002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/755fcc5fc63dc48a5bf18dee5c3d59de79471a11 Cr-Commit-Position: refs/heads/master@{#31809}
Message was sent while issue was closed.
LGTM, thanks. On 2015/11/04 17:18:19, Dan Ehrenberg wrote: > Previously, callers just called out to %AddElement directly. How did that handle > careless callsites? I think it would just check-fail there. I can use this > pattern but I don't see what the problem is exactly. What I meant was: before this CL and with patch set 2, when someone calls AddIndexedProperty with index == -1 (or any other negative integer value, really), the %AddNamedProperty path will be taken, which is correct. With patch set 1, %AddElement would have been called, which in turn would crash in "CHECK(key->ToArrayIndex(&index));". |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
