Crash [@ js::gc::GCRuntime::sweepBackgroundThings]

VERIFIED FIXED in Firefox 36, Firefox OS v2.2



JavaScript: GC
3 years ago
2 years ago


(Reporter: decoder, Assigned: jonco)


(Blocks: 1 bug, 4 keywords)

crash, regression, sec-high, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox35 unaffected, firefox36+ verified, firefox37+ verified, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.2 fixed)


(Whiteboard: [jsbugmon:update,bisect,ignore][fuzzblocker][b2g-adv-main2.2-], crash signature)


(2 attachments)



3 years ago
The following testcase crashes on mozilla-central revision 035a951fc24a (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-debug, run with --fuzzing-safe --thread-count=2):

const root = newGlobal();
var g = newGlobal();
for (var indexI = 0; indexI <= 65535; indexI++) {
    eval("/*var " + String.fromCharCode(indexI) + "xx = 1*/");


Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff7fe6700 (LWP 21844)]
0x00000000007d87e8 in js::gc::GCRuntime::sweepBackgroundThings (this=0x16ce338, zones=..., threadType=<optimized out>) at js/src/jsgc.cpp:3468
3468	                ArenaHeader *arenas = zone->allocator.arenas.arenaListsToSweep[kind];
#0  0x00000000007d87e8 in js::gc::GCRuntime::sweepBackgroundThings (this=0x16ce338, zones=..., threadType=<optimized out>) at js/src/jsgc.cpp:3468
#1  0x00000000007f16cf in doSweep (lock=..., this=<optimized out>) at js/src/jsgc.cpp:3713
#2  js::GCHelperState::work (this=0x16d5180) at js/src/jsgc.cpp:3599
#3  0x0000000000870726 in handleGCHelperWorkload (this=0x16e0830) at js/src/vm/HelperThreads.cpp:1309
#4  js::HelperThread::threadLoop (this=0x16e0830) at js/src/vm/HelperThreads.cpp:1367
#5  0x00000000008cd379 in nspr::Thread::ThreadRoutine (arg=0x16db350) at js/src/vm/PosixNSPR.cpp:45
#6  0x00007ffff7bc4e9a in start_thread (arg=0x7ffff7fe6700) at pthread_create.c:308
#7  0x00007ffff6cc131d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#8  0x0000000000000000 in ?? ()
rax	0x1	1
rbx	0x7ffff7fe5e30	140737354030640
rcx	0xa80	2688
rdx	0x1	1
rsi	0x0	0
rdi	0x7ffff7fe5e30	140737354030640
rbp	0x1652928	23406888
rsp	0x7ffff7fe5d30	140737354030384
r8	0x16db2e0	23966432
r9	0x5554	21844
r10	0x0	0
r11	0x1	1
r12	0x16ce338	23913272
r13	0x0	0
r14	0x6	6
r15	0xb1d690	11654800
rip	0x7d87e8 <js::gc::GCRuntime::sweepBackgroundThings(js::gc::ZoneList&, js::ThreadType)+152>
=> 0x7d87e8 <js::gc::GCRuntime::sweepBackgroundThings(js::gc::ZoneList&, js::ThreadType)+152>:	mov    0x378(%rdx,%rax,8),%rsi
   0x7d87f0 <js::gc::GCRuntime::sweepBackgroundThings(js::gc::ZoneList&, js::ThreadType)+160>:	test   %rsi,%rsi

Marking s-s due to crash instruction offset and GC-relatedness.


3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect,ignore]

Comment 1

3 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision f1f48ccb2d4e).

Comment 2

3 years ago
I'm seeing more GC crashes with newGlobal and gc, and they're all hard to reproduce/intermittent. Marking this as a fuzzblocker so we can figure out quickly, how many distinct issues we're dealing with here.

Needinfo from jandem for triage for now.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update,bisect,ignore] → [jsbugmon:update,bisect,ignore][fuzzblocker]
Jon, maybe this is something you would know about?
Flags: needinfo?(jcoppeard)
Keywords: sec-high
A similar crash signature has been showing up on TBPL in bug 1105123.

Comment 5

3 years ago
Thanks, investigating.
Assignee: nobody → jcoppeard
Flags: needinfo?(jdemooij)
Flags: needinfo?(jcoppeard)

Comment 6

3 years ago
Created attachment 8534477 [details] [diff] [review]

This is a race when starting the background thread while it's currently inside doSweep() and was introduced by my changes in bug 989390.

This affects debug builds but is hard to reproduce there because of timing differences.  I managed to come up with a test that fails 90% of the time in opt builds though.

I also added a single release assert that catches the problem, where a zone that is queued for background sweeping is re-added the list.
Attachment #8534477 - Flags: review?(terrence)
Comment on attachment 8534477 [details] [diff] [review]

Review of attachment 8534477 [details] [diff] [review]:

Attachment #8534477 - Flags: review?(terrence) → review+
[Tracking Requested - why for this release]: sec-high
Blocks: 989390, 1105123
status-firefox35: --- → unaffected
status-firefox36: --- → affected
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?
Component: JavaScript Engine → JavaScript: GC
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → affected
status-firefox-esr31: --- → unaffected
tracking-firefox36: ? → +
tracking-firefox37: ? → +

Comment 9

3 years ago
As a sec-high or sec-critical bug that affects more than trunk this should have gotten sec-approval prior to landing.  Jon, can you confirm that this is a regression from bug 989390, or does it need to be backported further?  Also, please be sure to request Aurora approval at some point.
Flags: needinfo?(jcoppeard)
Last Resolved: 3 years ago
status-firefox37: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

Comment 12

3 years ago
(In reply to Andrew McCreight [:mccr8] from comment #10)

I'm sorry for landing without approval (again!).

Yes, it's a regression from bug 989390.

I'm not sure exactly how the security classification works, but I don't think this is exploitable because the crash is always at a fixed address that is not under control of the user.
Flags: needinfo?(jcoppeard)

Comment 13

3 years ago
Comment on attachment 8534477 [details] [diff] [review]

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

Certinaly non-trivial.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The patch points to the existence of a race condition but not how to trigger or exploit it.

Which older supported branches are affected by this flaw?


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

Bug 989390.

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

This patch should apply cleanly.

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

Unlikely but we should let it bake on central for a few days.
Attachment #8534477 - Flags: sec-approval?
Comment on attachment 8534477 [details] [diff] [review]

sec-approval+ for trunk. Can you create and nominate an Aurora patch so we can get it in there too after some baking on trunk?
Attachment #8534477 - Flags: sec-approval? → sec-approval+

Comment 15

3 years ago
Created attachment 8536467 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: Bug 989390
[User impact if declined]: Possible intermittent crashes
[Describe test coverage new/current, TBPL]: On central since 12/12
[Risks and why]: Low
[String/UUID change made/needed]: None
Attachment #8536467 - Flags: approval-mozilla-aurora?
Attachment #8536467 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 16

3 years ago
status-b2g-v2.2: affected → fixed
status-firefox36: affected → fixed


3 years ago
status-firefox36: fixed → verified
status-firefox37: fixed → verified

Comment 17

3 years ago
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx36
Group: core-security
Whiteboard: [jsbugmon:update,bisect,ignore][fuzzblocker] → [jsbugmon:update,bisect,ignore][fuzzblocker][b2g-adv-main2.2-]
You need to log in before you can comment on or make changes to this bug.