Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 7 years ago

x = new Uint32Array(5);
try {
    Math.max(Uint32Array.prototype)();
} catch (e) {}
x[3] = -1;
print(x);


$ ./js-64-dm-linux-19f368b1267d --fuzzing-safe --no-threads --ion-eager testcase.js 
0,0,0,-2097152,0

$ ./js-64-dm-linux-19f368b1267d --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js 
0,0,0,4294967295,0

Tested this on m-c rev 19f368b1267d.

My configure flags are:

AR=ar sh ./configure --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 19f368b1267d

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b7cb15d415ee
user:        Jan de Mooij
date:        Tue Oct 04 10:19:41 2016 +0200
summary:     Bug 1306626 - Don't attach an Ion GetDenseElement stub if we're not accessing a dense element. r=h4writer

Jan, is bug 1306626 a likely regressor?

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 1 • 7 years ago

Actually, turning this s-s since this involves typed arrays.

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 7 years ago

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Jan, is bug 1306626 a likely regressor?

No, but I'll take a look.

	Jan de Mooij [:jandem] Assignee
	Comment 3 • 7 years ago

This is actually a pretty bad TI bug. Patches coming up.

	Jan de Mooij [:jandem] Assignee
	Comment 4 • 7 years ago

PropertyReadNeedsTypeBarrier was returning false for typed array elements. Then we added an IC and infallibly unboxed the return value to int32.

	Jan de Mooij [:jandem] Assignee
	Comment 5 • 7 years ago

This emits debug checks in JIT code for infallible unbox operations. If we had done this sooner it would probably have caught this bug a long time ago.

	Jan de Mooij [:jandem] Assignee
	Comment 6 • 7 years ago

Comment on attachment 8914331 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not too easy. Maybe with some work.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.

	Naveed Ihsanullah [:naveed]
	Comment 7 • 7 years ago

Does this need to be lifted to FF57? If so let's get that approval asap.

	Al Billings [:abillings - ex-MoCo]
	Comment 8 • 7 years ago

Sec-approval+ for trunk. This seems fairly safe to take on ESR52 and Beta. Can you please nominate patches for there as well? I don't think this is won't fix for 57 unless you think there is a reason not explained why we wouldn't take it there.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 9 • 7 years ago

Yes, this should be fine for 57 and ESR, please nominate patches for uplift (without tests). It can make it into tomorrow's beta 12 build if we get it landed by Thursday morning.

	Jan de Mooij [:jandem] Assignee
	Comment 10 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/74d76655c9192b20dc69ddfa40c38e65fa9a47e3

	Jan de Mooij [:jandem] Assignee
	Comment 11 • 7 years ago

Comment on attachment 8914331 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Some risk.
[Why is the change risky/not risky?]: Probably low risk but a bit hard to say.
[String changes made/needed]: None.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 12 • 7 years ago

Comment on attachment 8914331 [details] [diff] [review]
Part 1 - Fix

Fix for sec-high issue, let's uplift for beta 12 and for ESR.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/00e60f48b78d

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/dd068f4e132a

	Jan de Mooij [:jandem] Assignee
	Comment 15 • 7 years ago

NI myself to land part 2 later.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/74d76655c919

	Jan de Mooij [:jandem] Assignee
	Comment 17 • 7 years ago

Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bba4a6269557ba7214a09c9c260f83d9a84885e8

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 18 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/bba4a6269557