Christian Holler (:decoder) Reporter
	Description • 7 years ago

The following testcase crashes on mozilla-central revision 5801aa478de1 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with taskset -c 1 js --fuzzing-safe --thread-count=2 --baseline-eager --ion-extra-checks --ion-check-range-analysis --ion-eager test.js):

var lfLogBuffer = `
function f(arr) {
    var x;
    for (var i=0; i<100; i++) {
        x = arr.pop();
    }
    return x;
}
var arr = [];
for (var i=0; i<130; i++) {
    arr.push({i: i});
}
assertEq(f(arr).i, 30);
assertEq(f(arr), undefined);
`;
loadFile(lfLogBuffer);
function loadFile(lfVarx) {
  try {
    oomTest(function() { eval(lfVarx); });
  } catch (lfVare) {}
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x4f84481e in ?? ()
#0  0x4f84481e in ?? ()
eax	0xf4ec5800	-185837568
ebx	0xf6263040	-165269440
ecx	0xffffff8c	-116
edx	0xf4e32010	-186441712
esi	0x0	0
edi	0xf5245460	-182168480
ebp	0x0	0
esp	0xffffa53c	4294944060
eip	0x4f84481e	1334069278
=> 0x4f84481e:	mov    0x0(%ebp),%eax
   0x4f844821:	cmp    $0xf523b550,%eax


This testcase has a very interesting behavior: It only reproduces when run with "taskset -c 1" in conjunction with "--thread-count=2" (but then 100% of the time).  Running without taskset, reducing the thread count to 1 or running with --no-threads makes the test not reproduce. I'm marking this s-s because it could be some kind of thread race and also the crash is on the heap, maybe in jitted code.

	Andrew McCreight [:mccr8]
	Comment 1 • 7 years ago

Jan, could you triage this? Thanks.

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

Old TI OOM bug in FinishCompilation: we shouldn't set the hasFreezeConstraints flag if we failed to generate these freeze constraints.

Like decoder, I could repro reliably with taskset but not with rr or anything else. Not sure what's up with that, could be just to get OOM triggered in the right place.

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

This goes back years. We should probably fix it everywhere as the patch is simple and it's pretty bad.

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

Can you request sec-approval? Thanks.

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

Comment on attachment 8880354 [details] [diff] [review]
Patch

Not sure if Brian still has bugzilla login issues, so requesting an additional review from nbp.

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

Comment on attachment 8880354 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Pretty difficult, probably not impossible.

> 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 apply or be easy to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely. OOM here should be rare and the patch is simple.

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

Comment on attachment 8880354 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Very old bug.
[User impact if declined]: Security bugs, crashes.
[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?]: Low risk.
[Why is the change risky/not risky?]: Very local change, OOM here should be unlikely.
[String changes made/needed]: None.

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

sec-approval+ for trunk.

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

Comment on attachment 8880354 [details] [diff] [review]
Patch

Giving branch approvals as well.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2624217100407efb198dafb417a8d538f5d2b754

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

https://hg.mozilla.org/releases/mozilla-beta/rev/583d168dbe90
https://hg.mozilla.org/releases/mozilla-esr52/rev/f9bc084fbb8a

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 7 years ago

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