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

Status: VERIFIED FIXED

(Core :: JavaScript: GC, defect)

Description

[Christian Holler (:decoder)]

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*/");
}
gc();
gcslice(1000000);



Backtrace:

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.

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Christian Holler (:decoder)]

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.

Comment 3

[Andrew McCreight [:mccr8]]

Jon, maybe this is something you would know about?

Comment 4

[Andrew McCreight [:mccr8]]

A similar crash signature has been showing up on TBPL in bug 1105123.

Comment 5

[Jon Coppeard (:jonco)]

Thanks, investigating.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1108836-sweepBackgroundThings

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.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8534477 [details] [diff] [review]
bug1108836-sweepBackgroundThings

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

\o/

Comment 8

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-high

Comment 9

[Jon Coppeard (:jonco)]

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

Comment 10

[Andrew McCreight [:mccr8]]

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.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Jon Coppeard (:jonco)]

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

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 8534477 [details] [diff] [review]
bug1108836-sweepBackgroundThings

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

Aurora.

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.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8534477 [details] [diff] [review]
bug1108836-sweepBackgroundThings

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?

Comment 15

[Jon Coppeard (:jonco)]

Attachment: bug1108836-aurora-patch

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

Comment 16

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/856e6399952a

Comment 17

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx36