Closed Bug 1350844 Opened 3 years ago Closed 3 years ago

Assertion failure: zone->gcSweepGroupEdges().empty(), at js/src/jsgc.cpp:4568 with nukeCCW

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 53+ fixed
firefox52 --- unaffected
firefox-esr52 53+ fixed
firefox53 + verified
firefox54 + verified
firefox55 + verified

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main53+][adv-esr52.1+][adv-esr45.9+])

Attachments

(4 files)

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.
We just need to check whether a zone is marking before adding to its edge set.
Assignee: nobody → jcoppeard
Attachment #8851578 - Flags: review?(sphink)
Attachment #8851578 - Flags: review?(sphink) → review+
Blocks: 1343261
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.
Attachment #8851578 - Flags: sec-approval?
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.
Attachment #8851578 - Flags: sec-approval? → sec-approval+
bug 1343261 made it to both esr branches as well, does this one not affect them for some reason?
https://hg.mozilla.org/mozilla-central/rev/773f4dd4aa02
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(In reply to Julien Cristau [:jcristau] from comment #4)
Yes, those branches are affected.
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.
Attachment #8853509 - Flags: approval-mozilla-aurora?
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.
Attachment #8853510 - Flags: approval-mozilla-beta?
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.
Attachment #8853510 - Flags: approval-mozilla-esr52?
Attachment #8853510 - Flags: approval-mozilla-esr45?
Comment on attachment 8853509 [details] [diff] [review]
bug1350844-aurora

Fix a sec-high. Aurora54+
Attachment #8853509 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8853510 [details] [diff] [review]
bug1350844-backport

Fix a sec-high. Beta53+.
Attachment #8853510 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8853510 [details] [diff] [review]
bug1350844-backport

sec-high fix, esr45+/esr52+
Attachment #8853510 - Flags: approval-mozilla-esr52?
Attachment #8853510 - Flags: approval-mozilla-esr52+
Attachment #8853510 - Flags: approval-mozilla-esr45?
Attachment #8853510 - Flags: approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-esr52/rev/a76e626ae6db

Steve, this doesn't graft cleanly to ESR45. Can you please take a look?
Flags: needinfo?(sphink)
Flags: needinfo?(sphink)
Comment on attachment 8854573 [details] [diff] [review]
bug 1350844 backport to esr45

let's get this sec-high fix on esr45
Attachment #8854573 - Flags: approval-mozilla-esr45+
Attachment #8853510 - Flags: approval-mozilla-esr45+
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main53+][adv-esr52.1+][adv-esr45.9+]
Setting qe-verify- based on Jon's assessment on manual testing needs (see Comment 8) and the fact that this fix has automated coverage.
Flags: qe-verify-
(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
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
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.