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

The following testcase crashes on mozilla-central revision f5e214144799 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --no-threads):

function testSweep() {
    var wrapper = evaluate("({a: 15, b: {c: 42}})", { global: newGlobal({}) });
    nukeCCW(wrapper);
}
testSweep();
var a = ['a', 'b', 'c'];
for (var i = 0; i < 1000000; i++)
  a += i;



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000099e518 in js::gc::GCRuntime::groupZonesForSweeping (this=this@entry=0x7ffff695e650, lock=...) at js/src/jsgc.cpp:4568
#0  0x000000000099e518 in js::gc::GCRuntime::groupZonesForSweeping (this=this@entry=0x7ffff695e650, lock=...) at js/src/jsgc.cpp:4568
#1  0x00000000009b886c in js::gc::GCRuntime::beginSweepPhase (this=this@entry=0x7ffff695e650, destroyingRuntime=destroyingRuntime@entry=false, lock=...) at js/src/jsgc.cpp:5302
#2  0x00000000009b94f9 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff695e650, budget=..., reason=reason@entry=JS::gcreason::ALLOC_TRIGGER, lock=...) at js/src/jsgc.cpp:6058
#3  0x00000000009ba3f4 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff695e650, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::ALLOC_TRIGGER) at js/src/jsgc.cpp:6357
#4  0x00000000009bace8 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff695e650, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::ALLOC_TRIGGER) at js/src/jsgc.cpp:6506
#5  0x00000000009bb07a in js::gc::GCRuntime::startGC (this=0x7ffff695e650, gckind=GC_NORMAL, reason=JS::gcreason::ALLOC_TRIGGER, millis=0) at js/src/jsgc.cpp:6584
#6  0x00000000009bb3ea in js::gc::GCRuntime::gcIfRequested (this=0x7ffff695e650) at js/src/jsgc.cpp:6782
#7  0x0000000000b97dc6 in InvokeInterruptCallback (cx=0x7ffff6948000) at js/src/vm/Runtime.cpp:495
#8  0x000031d5ec920aee in ?? ()
[...]
rax	0x0	0
rbx	0x7ffff5b17000	140737315434496
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffbca0	140737488338080
rsp	0x7fffffffbbd0	140737488337872
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7fffffffbc20	140737488337952
r13	0x7ffff695e650	140737330406992
r14	0x7ffff6960810	140737330415632
r15	0x0	0
rip	0x99e518 <js::gc::GCRuntime::groupZonesForSweeping(js::AutoLockForExclusiveAccess&)+472>
=> 0x99e518 <js::gc::GCRuntime::groupZonesForSweeping(js::AutoLockForExclusiveAccess&)+472>:	movl   $0x0,0x0
   0x99e523 <js::gc::GCRuntime::groupZonesForSweeping(js::AutoLockForExclusiveAccess&)+483>:	ud2    



Marking s-s because this is a GC assert and the test uses nukeCCW.

	Jon Coppeard (:jonco) (PTO until 24th June) Assignee
	Comment 1 • 7 years ago

We just need to check whether a zone is marking before adding to its edge set.

	Jon Coppeard (:jonco) (PTO until 24th June) Assignee
	Comment 2 • 7 years ago

Comment on attachment 8851578 [details] [diff] [review]
bug1350844-dead-proxy-edges

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very hard.

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? Everything back to 53.

If not all supported branches, which bug introduced the flaw? Bug 1343261.

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

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

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

Making this a sec-high and giving sec-approval+ for trunk. We'll want this on Beta and Aurora (so please nominate patches) so we don't ship the problem.

Ideally, you wouldn't include a test in this because what happens if we take this, with the test, on Trunk and/or Aurora but it doesn't make it to Beta (53) and we then ship? We'll have zero dayed ourselves then with our own test.

	Julien Cristau [:jcristau]
	Comment 4 • 7 years ago

bug 1343261 made it to both esr branches as well, does this one not affect them for some reason?

	Carsten Book [:Tomcat]
	Comment 5 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/773f4dd4aa02

	Jon Coppeard (:jonco) (PTO until 24th June) Assignee
	Comment 6 • 7 years ago

(In reply to Julien Cristau [:jcristau] from comment #4)
Yes, those branches are affected.

	Jon Coppeard (:jonco) (PTO until 24th June) Assignee
	Comment 7 • 7 years ago

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1343261.
[User impact if declined]: Possible crash / security vulnerability.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[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?]: No.
[Why is the change risky/not risky?]: This is a simple change to filter out zones that are not being marked.
[String changes made/needed]: None.

	Jon Coppeard (:jonco) (PTO until 24th June) Assignee
	Comment 8 • 7 years ago

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1343261.
[User impact if declined]: Possible crash / security vulnerability.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[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?]: No.
[Why is the change risky/not risky?]: This is a simple change to filter out zones that are not being marked.
[String changes made/needed]: None.

	Jon Coppeard (:jonco) (PTO until 24th June) Assignee
	Comment 9 • 7 years ago

Comment on attachment 8853510 [details] [diff] [review]
bug1350844-backport

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug.
User impact if declined: Possible crash / security vulnerability.
Fix Landed on Version: 55.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

	Gerry Chang [:gchang]
	Comment 10 • 7 years ago

Comment on attachment 8853509 [details] [diff] [review]
bug1350844-aurora

Fix a sec-high. Aurora54+

	Gerry Chang [:gchang]
	Comment 11 • 7 years ago

Comment on attachment 8853510 [details] [diff] [review]
bug1350844-backport

Fix a sec-high. Beta53+.

	Randell Jesup [:jesup] (needinfo me)
	Comment 12 • 7 years ago
uplift

 https://hg.mozilla.org/releases/mozilla-aurora/rev/f26e2606488b9e01e63bdb8670d22bb8da77b4d2
 https://hg.mozilla.org/releases/mozilla-beta/rev/b7220fe2a194b39e03e0e2156109fd418d317327

	Julien Cristau [:jcristau]
	Comment 13 • 7 years ago

Comment on attachment 8853510 [details] [diff] [review]
bug1350844-backport

sec-high fix, esr45+/esr52+

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

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

Steve, this doesn't graft cleanly to ESR45. Can you please take a look?

	Julien Cristau [:jcristau]
	Comment 16 • 7 years ago

Comment on attachment 8854573 [details] [diff] [review]
bug 1350844 backport to esr45

let's get this sec-high fix on esr45

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

https://hg.mozilla.org/releases/mozilla-esr45/rev/c071fab59d05

Small typo fix per discussion with sfink:
https://hg.mozilla.org/releases/mozilla-aurora/rev/c82173e61331
https://hg.mozilla.org/releases/mozilla-beta/rev/115218aeda4a
https://hg.mozilla.org/releases/mozilla-esr52/rev/0ce4196ab86e

	Andrei Vaida [:avaida]
	Comment 18 • 7 years ago

Setting qe-verify- based on Jon's assessment on manual testing needs (see Comment 8) and the fact that this fix has automated coverage.

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Small typo fix per discussion with sfink:

Same for m-c.
https://hg.mozilla.org/mozilla-central/rev/ca8440c0eeb3

	Fuzzing Team
	Comment 20 • 7 years ago

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx53
JSBugMon: This bug has been automatically verified fixed on Fx54