Out of bounds memory access in TestJSArrayForAllocationSiteInfo.

Out of bounds memory access in TestJSArrayForAllocationSiteInfo.

The function intended to check the map pointer of an AllocationSiteInfo object, but neglected to
subtract an offset to do so.

BUG=169928
Committed: https://code.google.com/p/v8/source/detail?r=13444

[mvstanton, 2013-01-17 16:19:32]

Hi guys, here is the fix.
The regression test just makes sure to go down a code path where the stub that
failed will be called.

[mvstanton, 2013-01-18 15:59:34]

Okay, here is the fix with a regression test:
1) the golem run had no performance issues
2) Michael, you should have a look at the regression test, thx for the help with
that today.

Thanks guys,
--Michael

[Michael Starzinger, 2013-01-21 09:54:26]

Drive-by-comments on the regression test.

https://chromiumcodereview.appspot.com/11931037/diff/14001/test/cctest/test-h...
File test/cctest/test-heap.cc (right):

https://chromiumcodereview.appspot.com/11931037/diff/14001/test/cctest/test-h...
test/cctest/test-heap.cc:2749: ASSERT(limit != *limit_addr);
I think we can drop the assertion whether the limit changed after a GC.

https://chromiumcodereview.appspot.com/11931037/diff/14001/test/cctest/test-h...
test/cctest/test-heap.cc:2757: JSArray::kSize + AllocationSiteInfo::kSize +
Wouldn't it be better to just allocate the array completely at the end of
new-space without any allocation-site info and trailing pointer? This should
still trigger the crash and on top of that make sure that the stub doesn't read
a single pointer out of bounds. Also it would make the test case much simpler.

[ulan_google, 2013-01-21 12:21:07]

LGTM

https://codereview.chromium.org/11931037/diff/20001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/11931037/diff/20001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:2744: Address* limit_addr =
HEAP->new_space()->allocation_limit_address();
Now that the assertion is removed, this code seems to unused.

[mvstanton, 2013-01-21 12:25:39]

Thanks for the comments and help!

https://codereview.chromium.org/11931037/diff/14001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/11931037/diff/14001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:2749: ASSERT(limit != *limit_addr);
On 2013/01/21 09:54:26, Michael Starzinger wrote:
> I think we can drop the assertion whether the limit changed after a GC.

Done.

https://codereview.chromium.org/11931037/diff/14001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:2757: JSArray::kSize + AllocationSiteInfo::kSize +
On 2013/01/21 09:54:26, Michael Starzinger wrote:
> Wouldn't it be better to just allocate the array completely at the end of
> new-space without any allocation-site info and trailing pointer? This should
> still trigger the crash and on top of that make sure that the stub doesn't
read
> a single pointer out of bounds. Also it would make the test case much simpler.

Actually in this case we do need it. The crash will only happen if we have
<ARRAY> <AllocationSiteInfo> <kPointerSize filler>. There is a guard check in
place, and without the extra bytes after the array, we won't enter the failure
case.

https://codereview.chromium.org/11931037/diff/20001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/11931037/diff/20001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:2744: Address* limit_addr =
HEAP->new_space()->allocation_limit_address();
On 2013/01/21 12:21:07, ulan1 wrote:
> Now that the assertion is removed, this code seems to unused.

Oh indeed, thanks for catching that Ulan!

[ulan, 2013-01-21 12:27:25]

LGTM

[danno, 2013-01-21 12:38:18]

lgtm with some nits

https://codereview.chromium.org/11931037/diff/10001/test/mjsunit/allocation-s...
File test/mjsunit/allocation-site-info.js (right):

https://codereview.chromium.org/11931037/diff/10001/test/mjsunit/allocation-s...
test/mjsunit/allocation-site-info.js:84: function get_standard_literal() {
nit: maybe just get_literal()

https://codereview.chromium.org/11931037/diff/14001/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/11931037/diff/14001/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:3058: 
nit: discard the whitespace change

https://codereview.chromium.org/11931037/diff/14001/test/mjsunit/allocation-s...
File test/mjsunit/allocation-site-info.js (right):

https://codereview.chromium.org/11931037/diff/14001/test/mjsunit/allocation-s...
test/mjsunit/allocation-site-info.js:78: function fastliteralcase(literal,
value) {
2 space indent for the "then" body of this function

https://codereview.chromium.org/11931037/diff/14001/test/mjsunit/allocation-s...
test/mjsunit/allocation-site-info.js:79: // var literal = [1, 2, 3];
nit: remove the commented-out line above

https://codereview.chromium.org/11931037/diff/14001/test/mjsunit/allocation-s...
test/mjsunit/allocation-site-info.js:81: return literal;
2 space indent above.

https://codereview.chromium.org/11931037/diff/14001/test/mjsunit/allocation-s...
test/mjsunit/allocation-site-info.js:85: var literal = [1, 2, 3];
2-space indent here and below

https://codereview.chromium.org/11931037/diff/14001/test/mjsunit/allocation-s...
test/mjsunit/allocation-site-info.js:115: var literal = [1, 2, 3, 4];
nit: 2-space indent here too