1502143 - Assertion failure: !ObjectMayHaveExtraIndexedProperties(obj->staticPrototype()), at js/src/vm/NativeObject.cpp:2546

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 3cc04ee79005 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

var code = wasmTextToBinary('(module (func (export "run") (result i32) i32.const 42))');
var b = [];
b[10000] = 1;
([].concat(b))[0];
var a2 = new Int32Array(2);
b.__proto__ = a2;
3 == ([].concat(b))[0]


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x000055555601a498 in js::GetSparseElementHelper (cx=<optimized out>, obj=..., int_id=0, result=...) at js/src/vm/NativeObject.cpp:2545
#0  0x000055555601a498 in js::GetSparseElementHelper (cx=<optimized out>, obj=..., int_id=0, result=...) at js/src/vm/NativeObject.cpp:2545
#1  0x00002d2b7adf06ab in ?? ()
[...]
#10 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffc518	140737488340248
rcx	0x7ffff6c1c2dd	140737333281501
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffc4d0	140737488340176
rsp	0x7fffffffc420	140737488340000
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x0	0
r13	0x7ffff48330a0	140737295626400
r14	0x7ffff4db8140	140737301414208
r15	0x7fffffffc6c0	140737488340672
rip	0x55555601a498 <js::GetSparseElementHelper(JSContext*, JS::Handle<js::ArrayObject*>, int, JS::MutableHandle<JS::Value>)+536>
=> 0x55555601a498 <js::GetSparseElementHelper(JSContext*, JS::Handle<js::ArrayObject*>, int, JS::MutableHandle<JS::Value>)+536>:	movl   $0x0,0x0
   0x55555601a4a3 <js::GetSparseElementHelper(JSContext*, JS::Handle<js::ArrayObject*>, int, JS::MutableHandle<JS::Value>)+547>:	ud2


Marking s-s as a start because this looks like fallout from the sparse array patch and we had s-s problems there already.

Comment 1

[Christian Holler (:decoder)]

I would assume this bug isn't directly related to WebAssembly but the call to wasmTextToBinary into self-hosted JS code does something with Arrays by coincidence, triggering the bug.

Comment 2

[Kannan Vijayan [:djvj]]

Ok, just took a look at this.  This seems to be a case of a poorly-constructed ASSERT.  Trying to verify for sure that's the case.

Comment 3

[Kannan Vijayan [:djvj]]

I actually think there may be a correctness issue here.  Probably not critical in terms of security, but I haven't figured out exactly how to fix it.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

It's unrelated to wasm; the following asserts too:

new Uint8Array(1024).buffer;
var b = [];
b[10000] = 1;
([].concat(b))[0];
var a2 = new Int32Array(2);
b.__proto__ = a2;
3 == ([].concat(b))[0]

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: fix-bug.patch

Fixes this bug as well as the fuzzbug in bug 1502709.  Both tests are included in patch.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: fix-bug.patch

Updated fix.  This patch addresses a similar problem in in the sparse SetElem code as well.

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 9021208 [details] [diff] [review]
fix-bug.patch

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

I think I need to have some more of this explained as to why this is the right choice. 

My gut feeling is that something is off here. The root cause of my concern is the definition of GeneratePrototypeHoleGuards [1], which will conditionally call GuardGroupProto. It's not clear why these cases need to specially, and unconditionally call GuardGroupProto. 

On other places GeneratePrototypeHoleGuards has been paired with TestMatchingNativeReceiver [2]. 

Testing the test case from this bug, I can confirm TestMatchingNativeReciever fixes it; but in either case, I think this is deserving of a justifying comment. This stuff is subtle, so it's good for us to be justifying. It may be that the correct response here is to actually move the call to TestMatchingNativeReciever inside GeneratePrototypeHoleGuards, if the justification means we can't see them actually being separate guards in reality. 

I have a similar question for the ShapeGuardProtoChain and whether or not TestMatchingNativeReciever is the better pre-req guard, and if not, why. 

[1]: https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/js/src/jit/CacheIR.cpp#820-825
[2]: https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/js/src/jit/CacheIR.cpp#2210

Comment 9

[Kannan Vijayan [:djvj]]

TestMatchingNativeReceiver will "fix" the problem, but not in the way that you want - for dictionary-fied objects, it will simply never use that stub again after a change has been made to the properties list.

The difference between these uses and others is that here we are optimizing against a sparse-property (i.e. potentially dictionary) case.  We cannot expect object shapes to be stable, and so we cannot guard on the shape of the receiver.

After reading your comment, I noticed one other place where this happens:

https://searchfox.org/mozilla-central/source/js/src/jit/CacheIR.cpp#3027

This usage only emits the guard if `hasUncacheableProto()` is false, to avoid a multiple proto guards in the case where `hasUncacheableProto()` being true causes the emission of an additional GuardGroupProto.

Let me take a different stab at this.

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: fix-bug.patch

Updated patch.  I've left the testcase addition off of this one to just focus on the code changes.

The main thing here is an additional argument to GenreatePrototypeHoleGuards, which indicates whether the receiver's prototype should be always guarded.

Found an existing place where the CacheIR generation logic can be cleaned up with this approach.

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 9021323 [details] [diff] [review]
fix-bug.patch

I like this better! Thanks for doing this.

Comment 12

[Ted Campbell [:tcampbell]]

Comment on attachment 9021323 [details] [diff] [review]
fix-bug.patch

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

Good find. The problem is that prototype chain guard helpers require you to have called TestNativeReceiver or equivalent (and I never documented this...).

The hasOwn code should have been wrapped up instead as a helper function, and that helper should be used for all HasProp/GetProp/SetProp spare element case instead of changing the prototype code. Perhaps |TestReceiverGroupProto|.

I want to avoid conflating receiver vs protochain and shape-proto vs group-proto.  I really should have added more comments when I last cleaned this up. I had intended to do Bug 1440554 so we can avoid so many awkward checks.

Comment 13

[Ted Campbell [:tcampbell]]

Comment on attachment 9021323 [details] [diff] [review]
fix-bug.patch

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

I've also decided I'm fine with adding the arg to GeneratePrototypeHoleGuards, but perhaps the name should be alwaysGuardFirstGroupProto.

::: js/src/jit/CacheIR.cpp
@@ +4181,5 @@
>  
>      // Shape guard the prototype chain to avoid shadowing indexes from appearing.
> +    // Guard the prototype of the receiver explicitly, because the receiver's
> +    // shape is not being guarded as a proxy for that.
> +    GuardGroupProto(writer, obj, objId);

I don't think this is needed. The ShapeGuardProtoChain directly calls loadProto and we don't need a shape or other check to imply prototype.

Comment 14

[Andrew McCreight [:mccr8]]

Can this bug be unhidden per comment 3? I don't understand the severity here.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb7f83b51bc6660980b54699ad97f1ebd4c67ed
https://hg.mozilla.org/mozilla-central/rev/7fb7f83b51bc

Comment 16

[Kannan Vijayan [:djvj]]

:mccr8 - I wanted to be sure there was no hidden security issue here before I unhid the bug.  Until I knew for sure that this was harmless in terms of exploitability I preferred it to be hidden.

Not sure how to make the bug public, though.