850080 - Crash [@ js::gc::Cell::compartment] with --enable-root-analysis, --enable-more-deterministic and gczeal(6)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

try {
    x = {}
    x.o = {}
    gczeal(6)
    eval("")
} catch (e) {}
for (v of x) {}

crashes js debug shell (compiled with --enable-root-analysis) on m-c changeset e6215e0357fa without any CLI arguments at js::gc::Cell::compartment

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: possible not-so-small regression window

Comment 2

[Terrence Cole [:terrence]]

I was unable to reproduce on either tip or e6215e0357fa.

I configured with:

../configure --enable-debug --enable-optimize --enable-gczeal --enable-rooting-analysis --disable-threadsafe

And built with cl.exe:

Microsoft (R) C/C++ Optimizing Compiler Version 17.00.50727.1 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

What exact configure line and compiler did you use?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I've sent my configuration parameters to Terrence and he is now able to reproduce the crash.

Comment 4

[Terrence Cole [:terrence]]

The corrupted value is v0 in obj_toSource:

&v0: 0x0028e5e8

Then in CheckStackRoots:

rooters.begin()[2].rooter: 0x0028e5e8

Looking good so far: the right rooter is in the filtered list and it is less than stackEnd.

However:

((JS::Rooted<void*>    *)(rooters.begin()[2].rooter))->address(): 0x0028e5f4
((JS::Rooted<JS::Value>*)(rooters.begin()[2].rooter))->address(): 0x0028e5f8

The address the conservative scanner is going to see is the beginning of the Value, 0x0028e5f8, but the address the rooter is reporting is 4 bytes shorter because void* is 4 bytes shorter than a Value on 32 bit. The fix is simple: we need to specialize, like we already do for Bindings.

Patch coming in a moment.

Comment 5

[Terrence Cole [:terrence]]

Attachment: v0

Great find, Gary! Sorry it took me so long to get to it: now that ggc is landed I have a bit of time to spend on exact rooting again.

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 755663 [details] [diff] [review]
v0

Review of attachment 755663 [details] [diff] [review]:
-----------------------------------------------------------------

I still don't entirely understand why sizeof(Value) != sizeof(void*) would shift the ptr field around, unless the 8-byte Value on 32-bit gets 8-byte aligned (.stack @ 0, .prev @ 4, .scanned @ 8, so .ptr normally @ 12 but perhaps for Value it gets padded to be @ 16?).  If so, an alternate fix would be to pad it and add a JS_ASSERT(&ptr == &((Rooted<void*>*)this)->ptr). But this is probably better.

Hmm... would it be worth adding a checkStructLayout() that asserts that all of the 'this' fields are in the same place when cast to a Rooted<void*>*, unless kind is in {THING_ROOT_PROPERTY_DESCRIPTOR, THING_ROOT_BINDINGS, THING_ROOT_VALUE} (the special-cased ones?) That would catch both the 'scanned' field ordering problem and this one.

Comment 7

[Terrence Cole [:terrence]]

So after ruminating on this a bit more, I think I've got a good picture of what exactly is going wrong here: the Value is little endian on 8 bytes, but the stack scanner is little endian on 4 bytes. On the stack we have (individually) little endian int32s from low to high: stack*, prev, scanned (in an int32 for alignment), [A] value (high half), [B] value (low half). In this setup, Rooted<Value>::address() is going to be at [A], but the conservative scanner finds the pointer at [B] because the endianness has swapped the Value's halves.

Comment 8

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b00795492e9

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b00795492e9