Closed Bug 1473523 Opened 2 years ago Closed 2 years ago

Differential Testing: Different output message involving __proto__ and TypedArrays

Categories

(Core :: JavaScript Engine: JIT, defect, P1, major)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: gkw, Assigned: evilpie)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase)

Attachments

(2 files, 3 obsolete files)

function f() {
    var x = [];
    x.push((function(){})(8, 0));
    print("FOO: " + x);
}
(new Array).__proto__.__proto__ = new Int8Array(4);
f();
f();


$ ./js-dbg-64-dm-linux-90be04d99fc7 --fuzzing-safe --no-threads --ion-eager testcase.js 
[0]
[(void 0)]

$ ./js-dbg-64-dm-linux-90be04d99fc7 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js 
[0]
[0]

Tested this on m-c rev 90be04d99fc7.

My configure flags are:

'AR=ar' 'sh' '/home/ubuntu/trees/mozilla-central/js/src/configure' '--enable-debug' '--enable-more-deterministic' '--with-ccache' '--enable-gczeal' '--enable-debug-symbols' '--disable-tests'

python -u -m funfuzz.js.compile_shell -b "--enable-debug --enable-more-deterministic" -r 90be04d99fc7

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/846e6b6678b6
user:        Tom Schuster
date:        Sun Jul 01 13:03:11 2018 +0200
summary:     Bug 1413794 - Typed array [[Set]] wrongly inspects the receiver when a canonical numeric string is passed as property name. r=anba

Tom, is bug 1413794 a likely regressor?

Setting s-s as a start because TypedArrays are involved.
Flags: needinfo?(evilpies)
Gary, can you try this under an ASAN build (assuming this wasn't ASAN) and give us the dump from it?
Flags: needinfo?(nth10sd)
I guess CanAttachAddElement() [1] just needs to be updated to handle TypedArrays on the prototype chain. But I wonder if there are more cases where we now need to check the prototype chain before attaching fast paths? 

[1] https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/js/src/jit/CacheIR.cpp#3568


Related test cases for normal [[Set]]:
---
// Expected: Prints "FOO: 1,2,3" twice
// Actual: Prints "FOO: 1,2,3" and then "FOO: 1,2,3,0" with --baseline-eager

function f() {
    var x = [1,2,3];
    x[3] = 0;
    print("FOO: " + x);
}
Object.setPrototypeOf(Array.prototype, new Int8Array(4));
f();
f();
---


---
// Expected: Prints "FOO: 1,2,3,4" twice
// Actual: Prints "FOO: 1,2,3,4,0" twice
// Also: V8 prints first "FOO: 1,2,3,4" and then "FOO: 1,2,3,4,0" !

function f() {
    var x = [1,2,3,4];
    x[4] = 0;
    print("FOO: " + x);
}
Object.setPrototypeOf(Array.prototype, new Int8Array(4));
f();
f();
---
(In reply to Al Billings [:abillings] from comment #1)
> Gary, can you try this under an ASAN build (assuming this wasn't ASAN) and
> give us the dump from it?

I don't understand. This isn't a crashing testcase nor an assertion failure, so I don't think it's necessary.
Flags: needinfo?(nth10sd)
Gary: the concern was maybe it was a UAF or some other pointer confusion that caused this, as opposed to a logic bug.
Yeah what André said, we need to verify all the ICs and maybe other set_prop optimization in Ion work correctly with this assumption. This doesn't need to be a security sensitive bug, this is purely different, but still safe behavior.
Flags: needinfo?(evilpies)
Opening up as per comment 5.
Group: javascript-core-security
Priority: -- → P1
Assignee: nobody → evilpies
I am really not sure if we need to modify something in TI/IonMonkey especially for the MStoreElementHole path?

For CacheIR the change to CanAttachDenseElement already covers SetDenseElement and SetDenseElementHole. SetTypedElement already handles OOB by just ignoring the set. These are the three SetElement cases in CacheIR.
Flags: needinfo?(jdemooij)
(In reply to Tom Schuster [:evilpie] from comment #7)
> I am really not sure if we need to modify something in TI/IonMonkey
> especially for the MStoreElementHole path?

We call jit::TypeCanHaveExtraIndexedProperties -> PrototypeHasIndexedProperty -> ClassCanHaveExtraProperties, and that returns true for TypedArray classes.

So in IonBuilder::initOrSetElemDense, hasExtraIndexedProperty will be true when we have a TypedArray on the proto chain and then we don't use MStoreElementHole or MFallibleStoreElement but fall back to MStoreElement bailing out on holes.

AFAICS IonBuilder::initOrSetElemDense doesn't need any changes.
Flags: needinfo?(jdemooij)
(In reply to André Bargull [:anba] from comment #2)
> 
> ---
> // Expected: Prints "FOO: 1,2,3,4" twice
> // Actual: Prints "FOO: 1,2,3,4,0" twice
> // Also: V8 prints first "FOO: 1,2,3,4" and then "FOO: 1,2,3,4,0" !
> 
> function f() {
>     var x = [1,2,3,4];
>     x[4] = 0;
>     print("FOO: " + x);
> }
> Object.setPrototypeOf(Array.prototype, new Int8Array(4));
> f();
> f();
> ---

So this test case also applies to the interpreter. I am investigating this.
I think |obj| here needs to be |pobj|: https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/js/src/vm/NativeObject.cpp#2827

|obj| is just the start of our search, which is usually |receiver| anyway.
SetNonexistentProperty actually seems to need the |obj| for the optimization with obj == receiver we are trying to perform. We might just need pobj and obj.
Comment on attachment 8994326 [details] [diff] [review]
Handle typed array [[Set]] more correctly in ICs and SetProperty

This seems to pass the tests. It's really annoying to make NativeSetPropert even more complicated. I plan on adding a followup that just handles typed arrays separately just like the spec.
Attachment #8994326 - Attachment description: WIP untested → Handle typed array [[Set]] more correctly in ICs and SetProperty
Attachment #8994326 - Flags: review?(andrebargull)
Attachment #8994326 - Flags: review?(andrebargull)
Actually implementing typed array [[Set]] seems pretty nice, let's do that instead.
Attachment #8994326 - Attachment is obsolete: true
Attachment #8994808 - Attachment is obsolete: true
Comment on attachment 8994981 [details] [diff] [review]
Implement typed array [[Set]] as a separate function

I am not sure what would be better for SetTypedArray, the enum return, or an additional bool* done parameter.
Attachment #8994981 - Flags: review?(jorendorff)
Attachment #8994982 - Flags: review?(andrebargull)
Comment on attachment 8994982 [details] [diff] [review]
Don't attach SetDenseElement IC when a typed array is on the proto chain

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

LGTM!
Attachment #8994982 - Flags: review?(andrebargull) → review+
(In reply to Tom Schuster [:evilpie] from comment #18)
> Comment on attachment 8994981 [details] [diff] [review]
> Implement typed array [[Set]] as a separate function
> 
> I am not sure what would be better for SetTypedArray, the enum return, or an
> additional bool* done parameter.

I could also just move the IsTypedArrayIndex call into NativeSetProperty.
New approach that is actually closer to the old code and is simpler.
Attachment #8994981 - Attachment is obsolete: true
Attachment #8994981 - Flags: review?(jorendorff)
Attachment #8996095 - Flags: review?(jorendorff)
Comment on attachment 8996095 [details] [diff] [review]
Implement typed array [[Set]] as a separate function

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

Thanks.
Attachment #8996095 - Flags: review?(jorendorff) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02beb82e0e1f
Implement typed array [[Set]] as a separate function. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4a266f45fd
Don't attach SetDenseElement IC when a typed array is on the proto chain. r=anba
https://hg.mozilla.org/mozilla-central/rev/02beb82e0e1f
https://hg.mozilla.org/mozilla-central/rev/8e4a266f45fd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1502889
You need to log in before you can comment on or make changes to this bug.