Closed Bug 1338383 Opened 3 years ago Closed 3 years ago

Assertion failure: zone->gcZoneGroupEdges().empty(), at js/src/jsgc.cpp:4511

Categories

(Core :: JavaScript: GC, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox-esr45 52+ fixed
firefox51 --- wontfix
firefox52 + verified
firefox-esr52 52+ fixed
firefox53 + verified
firefox54 + verified

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [jsbugmon:update][adv-main52+][adv-esr45.8+])

Attachments

(3 files)

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

var lfOffThreadGlobal = newGlobal();
enableShellAllocationMetadataBuilder()
lfOffThreadGlobal.offThreadCompileScript(`
    gczeal(8, 1)
    function recurse(x) {
      recurse(x + 1);
    };
  recurse(0);
`);
lfOffThreadGlobal.runOffThreadScript();



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000099d88f in js::gc::GCRuntime::findZoneGroups (this=this@entry=0x7ffff695e598, lock=...) at js/src/jsgc.cpp:4511
#0  0x000000000099d88f in js::gc::GCRuntime::findZoneGroups (this=this@entry=0x7ffff695e598, lock=...) at js/src/jsgc.cpp:4511
#1  0x00000000009a7153 in js::gc::GCRuntime::beginSweepPhase (this=this@entry=0x7ffff695e598, destroyingRuntime=destroyingRuntime@entry=false, lock=...) at js/src/jsgc.cpp:5239
#2  0x00000000009b2753 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff695e598, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC, lock=...) at js/src/jsgc.cpp:5938
#3  0x00000000009b3bf6 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff695e598, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6241
#4  0x00000000009b4428 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff695e598, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6385
#5  0x00000000009b5fe0 in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ffff695e598) at js/src/jsgc.cpp:6843
#6  0x0000000000d172f5 in js::gc::GCRuntime::gcIfNeededPerAllocation (this=0x7ffff695e598, cx=cx@entry=0x7ffff6921000) at js/src/gc/Allocator.cpp:230
#7  0x0000000000d222a8 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=<optimized out>, cx=0x7ffff6921000, kind=js::gc::AllocKind::STRING) at js/src/gc/Allocator.cpp:191
#8  0x0000000000d254da in js::Allocate<JSString, (js::AllowGC)1> (cx=0x7ffff6921000) at js/src/gc/Allocator.cpp:142
#9  0x0000000000bddc4b in JSThinInlineString::new_<(js::AllowGC)1> (cx=<optimized out>) at js/src/vm/String-inl.h:258
#10 js::AllocateInlineString<(js::AllowGC)1, unsigned char> (cx=<optimized out>, len=8, chars=0x7fffffdfdb40) at js/src/vm/String-inl.h:31
#11 0x0000000000bdde06 in js::NewInlineString<(js::AllowGC)1, unsigned char> (cx=cx@entry=0x7ffff6921000, chars=...) at js/src/vm/String-inl.h:57
#12 0x0000000000bf5cfb in js::NewStringCopyNDontDeflate<(js::AllowGC)1, unsigned char> (cx=0x7ffff6921000, s=0x7ffff69201e0 "<string>", n=8) at js/src/vm/String.cpp:1311
#13 0x00000000008cfda5 in js::NewStringCopyN<(js::AllowGC)1> (n=<optimized out>, s=0x7ffff69201e0 "<string>", cx=0x7ffff6921000) at js/src/vm/String.h:1273
#14 js::NewStringCopyZ<(js::AllowGC)1> (s=0x7ffff69201e0 "<string>", cx=0x7ffff6921000) at js/src/vm/String.h:1293
#15 JS_NewStringCopyZ (cx=cx@entry=0x7ffff6921000, s=0x7ffff69201e0 "<string>") at js/src/jsapi.cpp:5038
#16 0x000000000094726f in js::ErrorToException (cx=0x7ffff6921000, reportp=0x7fffffdfddb0, callback=<optimized out>, userRef=<optimized out>) at js/src/jsexn.cpp:613
#17 0x000000000094da7c in js::ReportErrorNumberVA (cx=cx@entry=0x7ffff6921000, flags=flags@entry=0, callback=callback@entry=0x93a910 <js::GetErrorMessage(void*, unsigned int)>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=112, argumentsType=argumentsType@entry=js::ArgumentsAreASCII, ap=0x7fffffdfde90) at js/src/jscntxt.cpp:743
#18 0x00000000008d27b8 in JS_ReportErrorNumberASCIIVA (cx=0x7ffff6921000, errorCallback=0x93a910 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=112, ap=ap@entry=0x7fffffdfde90) at js/src/jsapi.cpp:5692
#19 0x00000000008d2868 in JS_ReportErrorNumberASCII (cx=cx@entry=0x7ffff6921000, errorCallback=errorCallback@entry=0x93a910 <js::GetErrorMessage(void*, unsigned int)>, userRef=userRef@entry=0x0, errorNumber=<optimized out>) at js/src/jsapi.cpp:5681
#20 0x000000000093fa3e in js::ReportOverRecursed (maybecx=0x7ffff6921000, errorNumber=<optimized out>) at js/src/jscntxt.cpp:321
#21 0x000000000081cc00 in js::jit::CheckOverRecursed (cx=0x7ffff6921000) at js/src/jit/VMFunctions.cpp:136
#22 0x000027c38c4b7d4e in ?? ()
#23 0x00000000000000ff in ?? ()
#24 0x00007fffffdfdfd8 in ?? ()
#25 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffdfd4c0	140737486247104
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffdfd550	140737486247248
rsp	0x7fffffdfd490	140737486247056
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff0325000	140737223217152
r13	0x7ffff6960748	140737330415432
r14	0x0	0
r15	0x7ffff69a3000	140737330688000
rip	0x99d88f <js::gc::GCRuntime::findZoneGroups(js::AutoLockForExclusiveAccess&)+1407>
=> 0x99d88f <js::gc::GCRuntime::findZoneGroups(js::AutoLockForExclusiveAccess&)+1407>:	movl   $0x0,0x0
   0x99d89a <js::gc::GCRuntime::findZoneGroups(js::AutoLockForExclusiveAccess&)+1418>:	ud2
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9f2ed9ef1d89
parent:      341222:b37fc0d40d06
user:        Jon Coppeard
date:        Tue Feb 07 17:37:45 2017 +0000
summary:     Bug 1330687 - Fix computation of zone edges for weakmap key delgates r=sfink a=abillings

This iteration took 261.146 seconds to run.
Jon, is bug 1330687 a likely regressor?
Flags: needinfo?(jcoppeard)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
That just added the assert, the bug is likely pre-existing.
Assignee: nobody → jcoppeard
Group: javascript-core-security
Flags: needinfo?(jcoppeard)
The gcZoneGroupEdges set currently gets cleared in Zone::findOutgoingEdges, but that doesn't get called if the ComponentFinder code bails out due to hitting the stack limit.

The simplest fix is to just always clear this list for collected zones.
Attachment #8835975 - Flags: review?(sphink)
How far back does this go? And can you please suggest a rating? :)
Flags: needinfo?(jcoppeard)
This goes back to bug 982561.  I guess it's a sec-high.
Blocks: 982561
No longer blocks: CVE-2017-5410
Flags: needinfo?(jcoppeard)
Attachment #8835975 - Flags: review?(sphink) → review+
Comment on attachment 8835975 [details] [diff] [review]
bug1338383-zone-group-edges

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

Very difficult.

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 32.

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

Bug 982561.

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

Should be trivial to backport.

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

Unlikely to cause regressions.
Attachment #8835975 - Flags: sec-approval?
tracking for 52/53/54 as sec-high.
sec-approval+ for trunk.
Let's get Aurora, Beta, and ESR45 patches nominated as well.
Attachment #8835975 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/baaf5d85129f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Please request Aurora/Beta/ESR45 approval on this when you get a chance.
Flags: needinfo?(jcoppeard)
Approval Request Comment
[Feature/Bug causing the regression]: Bug 982561.
[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 always clear the gcZoneGroupEdges set in GCRuntime::findZoneGroups rather than depending on it being cleared by a called method.
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8838086 - Flags: approval-mozilla-beta?
Attachment #8838086 - Flags: approval-mozilla-aurora?
Attached patch bug1338383-esr45Splinter Review
[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: 54.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #8838087 - Flags: approval-mozilla-esr45?
Comment on attachment 8838086 [details] [diff] [review]
bug1338383-backport

sec-high fix for aurora53, beta52
Attachment #8838086 - Flags: approval-mozilla-beta?
Attachment #8838086 - Flags: approval-mozilla-beta+
Attachment #8838086 - Flags: approval-mozilla-aurora?
Attachment #8838086 - Flags: approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed on Fx52
JSBugMon: This bug has been automatically verified fixed on Fx53
Component: JavaScript Engine → JavaScript: GC
Comment on attachment 8838087 [details] [diff] [review]
bug1338383-esr45

Fix a sec-high. ESR45+.
Attachment #8838087 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Duplicate of this bug: 1341046
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main52+][adv-esr45.8+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.