[heap] fix crash during the scavenge of ArrayBuffer

[heap] fix crash during the scavenge of ArrayBuffer

Scavenger should not attempt to visit ArrayBuffer's storage, it is a
user-supplied pointer that may have any alignment. Visiting it, may
result in a crash.

BUG=
R=jochen

Committed: https://crrev.com/8d6a228819d2a45a20e1dd5982ad53bf23145667
Cr-Commit-Position: refs/heads/master@{#31611}

[fedor.indutny, 2015-10-17 15:52:30]

Hello!

This is one more fix for ArrayBuffer's with unaligned pointer in contents. We
haven't discovered it previously, because it is very hard to trigger. I tried to
write a test, but it requires some serious twiddling with the pointers to
deceive that Scavenger.

Not sure if branching is appropriate at DoScavenge loop, but moving pointer to
the very end of the object does not seem to be that straightforward either.
However, if you want - I could explore this more throughly.

Thank you!

[fedor.indutny, 2015-10-19 22:57:53]

Description was changed from

==========
[heap] fix crash during the scavenge of ArrayBuffer

Scavenger should not attempt to visit ArrayBuffer's storage, it is a
user-supplied pointer that may have any alignment. Visiting it, may
result in a crash.

BUG=
R=jochen
==========

to

==========
[heap] fix crash during the scavenge of ArrayBuffer

Scavenger should not attempt to visit ArrayBuffer's storage, it is a
user-supplied pointer that may have any alignment. Visiting it, may
result in a crash.

BUG=
R=jochen
==========

[fedor.indutny, 2015-10-19 22:58:32]

Michael,

Added you to CC, just in case.

Thanks!

[jochen (gone - plz use gerrit), 2015-10-20 08:28:00]

jochen@chromium.org changed reviewers:
+ hpayer@chromium.org

[jochen (gone - plz use gerrit), 2015-10-20 08:28:00]

+hpayer

where exactly does this crash?

if this is indeed an issue, we should probably use HeapObject::ContentType to
filter out raw / mixed objects

[fedor.indutny, 2015-10-20 15:00:36]

Jochen,

Sorry, I forgot to include the stack trace:

   frame #0: 0x000000010033c675
node`v8::internal::Heap::IterateAndMarkPointersToFromSpace(bool, unsigned char*,
unsigned char*, void (*)(v8::internal::HeapObject**, v8::internal::HeapObject*))
+ 437
 * frame #1: 0x000000010033b662
node`v8::internal::Heap::DoScavenge(v8::internal::ObjectVisitor*, unsigned
char*) + 226
   frame #2: 0x000000010033a044 node`v8::internal::Heap::Scavenge() + 1236
   frame #3: 0x00000001003388ae
node`v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector,
v8::GCCallbackFlags) + 894
   frame #4: 0x000000010033823f
node`v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector, char
const*, char const*, v8::GCCallbackFlags) + 703

Do you want me to duplicate the code in store-buffer.cc ?

Thank you,
Fedor.

[fedor.indutny, 2015-10-20 16:07:43]

On 2015/10/20 15:00:36, fedor.indutny wrote:
> Jochen,
> 
> Sorry, I forgot to include the stack trace:
> 
>    frame #0: 0x000000010033c675
> node`v8::internal::Heap::IterateAndMarkPointersToFromSpace(bool, unsigned
char*,
> unsigned char*, void (*)(v8::internal::HeapObject**,
v8::internal::HeapObject*))
> + 437
>  * frame #1: 0x000000010033b662
> node`v8::internal::Heap::DoScavenge(v8::internal::ObjectVisitor*, unsigned
> char*) + 226
>    frame #2: 0x000000010033a044 node`v8::internal::Heap::Scavenge() + 1236
>    frame #3: 0x00000001003388ae
>
node`v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector,
> v8::GCCallbackFlags) + 894
>    frame #4: 0x000000010033823f
> node`v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector, char
> const*, char const*, v8::GCCallbackFlags) + 703
> 
> Do you want me to duplicate the code in store-buffer.cc ?
> 
> Thank you,
> Fedor.

Anyway, just pushed an update. Hopefully, this is what you meant.

Cheers!

[Hannes Payer (out of office), 2015-10-21 13:41:16]

What happens is that the ArrayBuffer gets added to the promotion queue, which it
should not since it never contains JS heap pointers.

Changing
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/scaven...
to DATA_OBJECT should fix the bug.

A test case would be nice, in that case a cctest should not be too difficult:
(1) Allocate a byte array.
Store at an unaligned location in the byte array, a regular (aligned) pointer
that will point somewhere in new
(2) Allocate an ArrayBuffer, reference with a handle that keeps it alive.
Point the buffer to the unaligned location.
(3) Call Scavenge 2 times to get the ArrayBuffer Promoted.
You should observe that the array buffer gets added to the promotion queue and
it should crash when the promotion queue gets processed.

[fedor.indutny, 2015-10-21 14:43:55]

On 2015/10/21 13:41:16, Hannes Payer wrote:
> What happens is that the ArrayBuffer gets added to the promotion queue, which
it
> should not since it never contains JS heap pointers.
> 
> Changing
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/scaven...
> to DATA_OBJECT should fix the bug.
> 
> A test case would be nice, in that case a cctest should not be too difficult:
> (1) Allocate a byte array.
> Store at an unaligned location in the byte array, a regular (aligned) pointer
> that will point somewhere in new
> (2) Allocate an ArrayBuffer, reference with a handle that keeps it alive.
> Point the buffer to the unaligned location.
> (3) Call Scavenge 2 times to get the ArrayBuffer Promoted.
> You should observe that the array buffer gets added to the promotion queue and
> it should crash when the promotion queue gets processed.

Fixed, please review it if you are still online by any chance! ;) This is a bit
critical for some node.js users, and it would be great to include it in our next
release.

Thanks!

[Hannes Payer (out of office), 2015-10-21 14:54:43]

Cool thanks! Lgtm with comment nits.

https://codereview.chromium.org/1406133003/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1406133003/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:14211: // Should not crash
, i.e. backing store pointer should not be treated as a heap object pointer

https://codereview.chromium.org/1406133003/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:14215: // Just use the `ab` to silence compiler warning
Use `ab` to...

[fedor.indutny, 2015-10-21 15:00:36]

On 2015/10/21 14:54:43, Hannes Payer wrote:
> Cool thanks! Lgtm with comment nits.
> 
> https://codereview.chromium.org/1406133003/diff/40001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/1406133003/diff/40001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:14211: // Should not crash
> , i.e. backing store pointer should not be treated as a heap object pointer
> 
>
https://codereview.chromium.org/1406133003/diff/40001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:14215: // Just use the `ab` to silence compiler
warning
> Use `ab` to...

Thank you so much for prompt review! All fixed, clicking the button ;)

[fedor.indutny, 2015-10-21 15:00:46]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-10-21 15:00:46]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1406133003/#ps60001
(title: "fixes")

[commit-bot: I haz the power, 2015-10-21 15:00:50]

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

[fedor.indutny, 2015-10-21 15:28:06]

On 2015/10/21 15:00:50, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1406133003/60001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1406133003/60001

Hannes,

Looks like it still has some fields that have pointers in them. Here is a crash
log:

  * frame #0: 0x000000010001733c
d8`v8::internal::Map::instance_type(this=0x1beefdad0beefdad) + 12 at
objects-inl.h:4580
    frame #1: 0x000000010001ccd6
d8`v8::internal::Object::IsFixedArray(this=0x000014e3ed055f99) const + 70 at
objects-inl.h:702
    frame #2: 0x000000010001cc3a
d8`v8::internal::FixedArray::cast(object=0x000014e3ed055f99) + 42 at
objects-inl.h:3276
    frame #3: 0x0000000100255009
d8`v8::internal::JSObject::properties(this=0x000004ff9dd5fcb1) const + 25 at
objects-inl.h:1649
    frame #4: 0x0000000100730b4f
d8`v8::internal::JSObject::JSObjectVerify(this=0x000004ff9dd5fcb1) + 31 at
objects-debug.cc:266
    frame #5: 0x0000000100733765
d8`v8::internal::JSArrayBuffer::JSArrayBufferVerify(this=0x000004ff9dd5fcb1) +
101 at objects-debug.cc:834
    frame #6: 0x000000010072f2ea
d8`v8::internal::HeapObject::HeapObjectVerify(this=0x000004ff9dd5fcb1) + 1290 at
objects-debug.cc:179
    frame #7: 0x000000010072ec6d
d8`v8::internal::Object::ObjectVerify(this=0x000004ff9dd5fcb1) + 77 at
objects-debug.cc:23
    frame #8: 0x00000001005c3905
d8`v8::internal::PagedSpace::Verify(this=0x0000000102d02f50,
visitor=0x00007fff5fbfdc28) + 789 at spaces.cc:1205
    frame #9: 0x000000010054ed85
d8`v8::internal::Heap::Verify(this=0x0000000104001020) + 293 at heap.cc:4413
    frame #10: 0x000000010054f718
d8`v8::internal::Heap::GarbageCollectionEpilogue(this=0x0000000104001020) + 88
at heap.cc:624
    frame #11: 0x000000010055128f
d8`v8::internal::Heap::CollectGarbage(this=0x0000000104001020,
collector=SCAVENGER, gc_reason="allocation failure",
collector_reason=0x0000000000000000, gc_callback_flags=kNoGCCallbackFlags) + 703
at heap.cc:991
    frame #12: 0x000000010023a613
d8`v8::internal::Heap::CollectGarbage(this=0x0000000104001020, space=NEW_SPACE,
gc_reason="allocation failure", callbackFlags=kNoGCCallbackFlags) + 83 at
heap-inl.h:531
    frame #13: 0x00000001004ecfb7
d8`v8::internal::Factory::NewFixedArray(this=0x0000000104001000, size=259,
pretenure=NOT_TENURED) + 391 at factory.cc:128
    frame #14: 0x00000001007a9650
d8`v8::internal::HashTable<v8::internal::CodeCacheHashTable,
v8::internal::CodeCacheHashTableShape,
v8::internal::HashTableKey*>::New(isolate=0x0000000104001000,
at_least_space_for=64, capacity_option=USE_DEFAULT_MINIMUM_CAPACITY,
pretenure=NOT_TENURED) + 288 at objects.cc:15105
    frame #15: 0x00000001007741e9
d8`v8::internal::CodeCache::Update(code_cache=Handle<v8::internal::CodeCache> @
0x00007fff5fbfe098, name=Handle<v8::internal::Name> @ 0x00007fff5fbfe090,
code=Handle<v8::internal::Code> @ 0x00007fff5fbfe088) + 121 at objects.cc:8831
    frame #16: 0x00000001007638da
d8`v8::internal::Map::UpdateCodeCache(map=Handle<v8::internal::Map> @
0x00007fff5fbfe118, name=Handle<v8::internal::Name> @ 0x00007fff5fbfe110,
code=Handle<v8::internal::Code> @ 0x00007fff5fbfe108) + 250 at objects.cc:8791
    frame #17: 0x00000001006ce6ed
d8`v8::internal::PropertyICCompiler::ComputeMonomorphic(kind=KEYED_STORE_IC,
name=Handle<v8::internal::Name> @ 0x00007fff5fbfe330,
map=Handle<v8::internal::Map> @ 0x00007fff5fbfe328,
handler=Handle<v8::internal::Code> @ 0x00007fff5fbfe320, extra_ic_state=64) +
813 at ic-compiler.cc:85
    frame #18: 0x00000001006ba13c
d8`v8::internal::IC::UpdateMonomorphicIC(this=0x00007fff5fbfea40,
handler=Handle<v8::internal::Code> @ 0x00007fff5fbfe3a8,
name=Handle<v8::internal::Name> @ 0x00007fff5fbfe3a0) + 252 at ic.cc:856
    frame #19: 0x00000001006b8fbc
d8`v8::internal::IC::PatchCache(this=0x00007fff5fbfea40,
name=Handle<v8::internal::Name> @ 0x00007fff5fbfe438,
code=Handle<v8::internal::Code> @ 0x00007fff5fbfe430) + 108 at ic.cc:893
    frame #20: 0x00000001006bdbbc
d8`v8::internal::StoreIC::UpdateCaches(this=0x00007fff5fbfea40,
lookup=0x00007fff5fbfe5a8, value=Handle<v8::internal::Object> @
0x00007fff5fbfe4e8, store_mode=MAY_BE_STORE_FROM_KEYED) + 460 at ic.cc:1709
    frame #21: 0x00000001006bd8c8
d8`v8::internal::StoreIC::Store(this=0x00007fff5fbfea40,
object=Handle<v8::internal::Object> @ 0x00007fff5fbfe7c8,
name=Handle<v8::internal::Name> @ 0x00007fff5fbfe7c0,
value=Handle<v8::internal::Object> @ 0x00007fff5fbfe7b8,
store_mode=MAY_BE_STORE_FROM_KEYED) + 2552 at ic.cc:1590
    frame #22: 0x00000001006c08d4
d8`v8::internal::KeyedStoreIC::Store(this=0x00007fff5fbfea40,
object=Handle<v8::internal::Object> @ 0x00007fff5fbfe9e8,
key=Handle<v8::internal::Object> @ 0x00007fff5fbfe9e0,
value=Handle<v8::internal::Object> @ 0x00007fff5fbfe9d8) + 692 at ic.cc:2181
    frame #23: 0x00000001006c42b3
d8`v8::internal::__RT_impl_Runtime_KeyedStoreIC_Miss(args=Arguments @
0x00007fff5fbfebf8, isolate=0x0000000104001000) + 963 at ic.cc:2554
    frame #24: 0x00000001006c3ede
d8`v8::internal::Runtime_KeyedStoreIC_Miss(args_length=3,
args_object=0x00007fff5fbfeca8, isolate=0x0000000104001000) + 110 at ic.cc:2532

Do you want me to revert store-buffer changes?

[commit-bot: I haz the power, 2015-10-21 15:45:36]

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

[commit-bot: I haz the power, 2015-10-21 15:45:37]

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

[fedor.indutny, 2015-10-21 15:58:54]

On 2015/10/21 15:45:37, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux_dbg on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/9845)

It looks like the problem is related to the JSObject properties that
JSArrayBuffer has.

[Hannes Payer (out of office), 2015-10-22 08:48:20]

Arg, right. I was too fast, it is derived from JSObject. So you would have to
special case it in PromoteObject. Similar to JS_FUNCTION_TYPE, visit until the
last pointer field.
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/scaven...

[indutny, 2015-10-22 15:13:40]

On 2015/10/22 08:48:20, Hannes Payer wrote:
> Arg, right. I was too fast, it is derived from JSObject. So you would have to
> special case it in PromoteObject. Similar to JS_FUNCTION_TYPE, visit until the
> last pointer field.
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/scaven...

Hannes,

I just did the change. I'm slightly worried that we won't visit the fields after
the BackingStore, because we do visit it in all other places.

PTAL.

Thanks,
Fedor.

[Hannes Payer (out of office), 2015-10-23 11:33:14]

You are absolutely right, that would be fatal. The issue is the promotion queue
does not support non-pointer objects. The double unboxing was kind of hacked
into that system.

I was hoping for a simpler solution but there is no way around. Let's proceed
with patchset #2. Sorry for the detour. I will have a close look again at #2.

[Hannes Payer (out of office), 2015-10-23 11:41:19]

https://codereview.chromium.org/1406133003/diff/20001/src/heap/store-buffer.h
File src/heap/store-buffer.h (right):

https://codereview.chromium.org/1406133003/diff/20001/src/heap/store-buffer.h...
src/heap/store-buffer.h:66: void IteratePointersToFromSpace(HeapObject* target,
int size,
These methods should live in heap. They have nothing to do with the store buffer
(they can just create store buffer entries). Can you please move them to theap.

https://codereview.chromium.org/1406133003/diff/20001/src/heap/store-buffer.h...
src/heap/store-buffer.h:166: void IterateAndMarkPointersToFromSpace(HeapObject*
object, Address start,
Same same.

[indutny, 2015-10-23 15:17:08]

fedor.indutny@gmail.com changed reviewers:
+ fedor.indutny@gmail.com

[indutny, 2015-10-23 15:17:09]

Done! Thank you!

https://codereview.chromium.org/1406133003/diff/20001/src/heap/store-buffer.h
File src/heap/store-buffer.h (right):

https://codereview.chromium.org/1406133003/diff/20001/src/heap/store-buffer.h...
src/heap/store-buffer.h:66: void IteratePointersToFromSpace(HeapObject* target,
int size,
On 2015/10/23 11:41:19, Hannes Payer wrote:
> These methods should live in heap. They have nothing to do with the store
buffer
> (they can just create store buffer entries). Can you please move them to
theap.

Acknowledged.

https://codereview.chromium.org/1406133003/diff/20001/src/heap/store-buffer.h...
src/heap/store-buffer.h:166: void IterateAndMarkPointersToFromSpace(HeapObject*
object, Address start,
On 2015/10/23 11:41:19, Hannes Payer wrote:
> Same same.

Acknowledged.

[indutny, 2015-10-26 21:38:11]

Jochen,

Kindly reminding you about this.

Thanks!

[Hannes Payer (out of office), 2015-10-27 13:05:32]

lgtm

[fedor.indutny, 2015-10-27 15:58:58]

The CQ bit was checked by fedor@indutny.com

[commit-bot: I haz the power, 2015-10-27 15:59:06]

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

[commit-bot: I haz the power, 2015-10-27 15:59:06]

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

[commit-bot: I haz the power, 2015-10-27 15:59:07]

CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true
and NOPRESUBMIT=true in order for the CQ to process them

[fedor.indutny, 2015-10-27 16:06:48]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-10-27 16:06:48]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1406133003/#ps180001
(title: "fix ref")

[commit-bot: I haz the power, 2015-10-27 16:06:57]

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

[commit-bot: I haz the power, 2015-10-27 16:34:29]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-10-27 16:35:08]

Patchset 10 (id:??) landed as
https://crrev.com/8d6a228819d2a45a20e1dd5982ad53bf23145667
Cr-Commit-Position: refs/heads/master@{#31611}

[fedor.indutny, 2015-10-27 16:43:19]

On 2015/10/27 16:35:08, commit-bot: I haz the power wrote:
> Patchset 10 (id:??) landed as
> https://crrev.com/8d6a228819d2a45a20e1dd5982ad53bf23145667
> Cr-Commit-Position: refs/heads/master@{#31611}

Thank you, Hannes!

[fedor.indutny, 2015-10-27 16:54:05]

Hannes,

Do you think it may be useful to get it backported to the current stable version
of v8 (whatever it is)?

Thank you,
Fedor.

[Michael Starzinger, 2015-10-27 18:20:42]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-10-27 18:20:43]

https://codereview.chromium.org/1406133003/diff/180001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1406133003/diff/180001/src/heap/heap.h#newcod...
src/heap/heap.h:1254: void IteratePointersToFromSpace(HeapObject* target, int
size,
Really? Between the comment that talks about "object", "start" and "end" and the
function with the signature containing arguments called "object", "start" and
"end" seemed like the best place to put this?

[fedor.indutny, 2015-10-27 18:22:42]

https://codereview.chromium.org/1406133003/diff/180001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1406133003/diff/180001/src/heap/heap.h#newcod...
src/heap/heap.h:1254: void IteratePointersToFromSpace(HeapObject* target, int
size,
On 2015/10/27 18:20:43, Michael Starzinger wrote:
> Really? Between the comment that talks about "object", "start" and "end" and
the
> function with the signature containing arguments called "object", "start" and
> "end" seemed like the best place to put this?

Gosh, this was an awful place to put the function indeed, but I think it should
be around this place anyway.

[Hannes Payer (out of office), 2015-10-28 08:37:59]

On 2015/10/27 18:22:42, fedor.indutny wrote:
> https://codereview.chromium.org/1406133003/diff/180001/src/heap/heap.h
> File src/heap/heap.h (right):
> 
>
https://codereview.chromium.org/1406133003/diff/180001/src/heap/heap.h#newcod...
> src/heap/heap.h:1254: void IteratePointersToFromSpace(HeapObject* target, int
> size,
> On 2015/10/27 18:20:43, Michael Starzinger wrote:
> > Really? Between the comment that talks about "object", "start" and "end" and
> the
> > function with the signature containing arguments called "object", "start"
and
> > "end" seemed like the best place to put this?
> 
> Gosh, this was an awful place to put the function indeed, but I think it
should
> be around this place anyway.

Yes, we are going to merge it back to beta and stable. I will take care of that.

[fedor.indutny, 2015-10-28 08:59:49]

On 2015/10/28 08:37:59, Hannes Payer wrote:
> On 2015/10/27 18:22:42, fedor.indutny wrote:
> > https://codereview.chromium.org/1406133003/diff/180001/src/heap/heap.h
> > File src/heap/heap.h (right):
> > 
> >
>
https://codereview.chromium.org/1406133003/diff/180001/src/heap/heap.h#newcod...
> > src/heap/heap.h:1254: void IteratePointersToFromSpace(HeapObject* target,
int
> > size,
> > On 2015/10/27 18:20:43, Michael Starzinger wrote:
> > > Really? Between the comment that talks about "object", "start" and "end"
and
> > the
> > > function with the signature containing arguments called "object", "start"
> and
> > > "end" seemed like the best place to put this?
> > 
> > Gosh, this was an awful place to put the function indeed, but I think it
> should
> > be around this place anyway.
> 
> Yes, we are going to merge it back to beta and stable. I will take care of
that.

Thanks!