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

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

	Fuzzing Team
	Comment 1 • 8 years ago

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.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 2 • 8 years ago

Jon, is bug 1330687 a likely regressor?

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

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
That just added the assert, the bug is likely pre-existing.

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

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 8 years ago

How far back does this go? And can you please suggest a rating? :)

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

This goes back to bug 982561.  I guess it's a sec-high.

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

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.

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

tracking for 52/53/54 as sec-high.

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

sec-approval+ for trunk.
Let's get Aurora, Beta, and ESR45 patches nominated as well.

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

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

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

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

	Fuzzing Team
	Comment 12 • 7 years ago

JSBugMon: This bug has been automatically verified fixed.

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

Please request Aurora/Beta/ESR45 approval on this when you get a chance.

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

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.

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

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

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

Comment on attachment 8838086 [details] [diff] [review]
bug1338383-backport

sec-high fix for aurora53, beta52

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/7597080fcccb
https://hg.mozilla.org/releases/mozilla-beta/rev/1726fe19115e

	Fuzzing Team
	Comment 18 • 7 years ago

JSBugMon: This bug has been automatically verified fixed on Fx52
JSBugMon: This bug has been automatically verified fixed on Fx53

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

Comment on attachment 8838087 [details] [diff] [review]
bug1338383-esr45

Fix a sec-high. ESR45+.

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

https://hg.mozilla.org/releases/mozilla-esr52/rev/1726fe19115e
https://hg.mozilla.org/releases/mozilla-esr45/rev/ffca0f060bb4