1138199 - Crash [@ js::ConstraintTypeSet::sweep] or Assertion failure: addr % CellSize == 0, at gc/Heap.h:1229 with runOffThreadScript

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Christian Holler (:decoder)]

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

var lfGlobal = newGlobal();
var src = "var obj = {";
for (var i = 0; i < 140; ++i) {
	src += "prop" + i + ": 2,";
}
src += "}; {";
lfGlobal.offThreadCompileScript(src);
lfGlobal.runOffThreadScript();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::ConstraintTypeSet::sweep (this=0x1b29dd8, zone=0x1b0e280, oom=...) at js/src/vm/TypeInference.cpp:3785
3785	            ObjectKey *key = oldArray[i];
#0  js::ConstraintTypeSet::sweep (this=0x1b29dd8, zone=0x1b0e280, oom=...) at js/src/vm/TypeInference.cpp:3785
#1  0x00000000006ea90b in js::ObjectGroup::maybeSweep (this=0x7ffff568e5e0, oom=oom@entry=0x7fffffffd290) at js/src/vm/TypeInference.cpp:3944
#2  0x0000000000a9a595 in SweepThing (group=<optimized out>, oom=0x7fffffffd290) at js/src/jsgc.cpp:5240
#3  SweepArenaList<js::ObjectGroup, js::AutoClearTypeInferenceStateOnOOM*> (sliceBudget=..., arenasToSweep=0x1b0e6e0) at js/src/jsgc.cpp:5249
#4  js::gc::GCRuntime::sweepPhase (this=this@entry=0x1a076c8, sliceBudget=...) at js/src/jsgc.cpp:5292
#5  0x0000000000aa572b in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x1a076c8, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:5941
#6  0x0000000000aa6456 in js::gc::GCRuntime::gcCycle (this=this@entry=0x1a076c8, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6116
#7  0x0000000000aa678d in js::gc::GCRuntime::collect (this=this@entry=0x1a076c8, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6228
#8  0x0000000000aa7a2a in js::gc::GCRuntime::gc (this=this@entry=0x1a076c8, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6289
#9  0x0000000000a26517 in js::DestroyContext (cx=0x1a29b50, mode=js::DCM_FORCE_GC) at js/src/jscntxt.cpp:185
#10 0x0000000000a26814 in JS_DestroyContext (cx=<optimized out>) at js/src/jsapi.cpp:576
#11 0x0000000000425efd in DestroyContext (withGC=true, cx=0x1a29b50) at js/src/shell/js.cpp:5649
#12 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6406
rax	0x1b0e7b8	28370872
rbx	0x1b29dd8	28483032
rcx	0x6	6
rdx	0x63614073	1667317875
rsi	0x1	1
rdi	0x1b	27
rbp	0x7fffffffd190	140737488343440
rsp	0x7fffffffd120	140737488343328
r8	0x8	8
r9	0x0	0
r10	0x1b29d50	28482896
r11	0x1b29d30	28482864
r12	0x200	512
r13	0x1b0e280	28369536
r14	0x0	0
r15	0x7ffff000ba10	140737219967504
rip	0x71b6df <js::ConstraintTypeSet::sweep(JS::Zone*, js::AutoClearTypeInferenceStateOnOOM&)+223>
=> 0x71b6df <js::ConstraintTypeSet::sweep(JS::Zone*, js::AutoClearTypeInferenceStateOnOOM&)+223>:	mov    (%r14),%r15
   0x71b6e2 <js::ConstraintTypeSet::sweep(JS::Zone*, js::AutoClearTypeInferenceStateOnOOM&)+226>:	test   %r15,%r15


Marking s-s, since this is a GC crash. The signatures vary a little.

Comment 1

[Andrew McCreight [:mccr8]]

Is this something you could look at, Jon?  This could be related to the type sweeping GC crashes we've been seeing.

Comment 2

[Jon Coppeard (:jonco)]

I've had a look but I can't see what's going wrong here.

We do an off-thread parse of a script that fails due to a syntax error in Parser::blockStatement().  The error is thrown on the main thread in GlobalHelperThreadState::finishParseTask().  When we GC as the context is destroyed we crash in ConstraintTypeSet::sweep() because we have oldCapacity of 64 but oldArray is nullptr.

The recent change to finishParseTask() in bug 1137341 doesn't seem implicated since it doesn't cause a GC (in fact there is no GC before the one where we crash).  My guess would be something to do with merging the object groups after off thread parsing.

Brian, do you have time to take a look at this as you're more familiar with this area?

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

This is an old problem which goes back to bug 909574 I think.  If we do an off thread parse and rejoin on the main thread and the main thread's zone's type LifoAlloc is completely empty, then we mess the pointers up a bit when merging in the off thread allocator's state.

I don't know if this can be triggered in the browser.  It requires the main thread zone's allocator to be completely empty, which would mean that no properties have been added to non-singleton objects, and that no code has run long enough to be baseline compiled.  Presumably something like that would happen for a content page to be able to trigger an off thread parse via a <script> tag, but I don't know enough about the browser machinery to say one way or the other.

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8573345 [details] [diff] [review]
patch

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

What a screwup! Thanks.

::: js/src/ds/LifoAlloc.h
@@ +194,4 @@
>  
>      // Append used chunks to the end of this LifoAlloc. We act as if all the
>      // chunks in |this| are used, even if they're not, so memory may be wasted.
>      void appendUsed(BumpChunk *start, BumpChunk *latest, BumpChunk *end) {

Can you rename this param to latest_ or something so it's a little less crazy? Maybe call the other params first_ and last_ too?

Comment 6

[Brian Hackett [Laid off!]]

Attachment: updated

Patch updated per comments.  I used other{First,Latest,Last} for even more bonus readability.

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 8574292 [details] [diff] [review]
updated

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

Not easily at all.  Exploitability of this bug is unknown.

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?

All.

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

Off thread parsing.

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

Trivial.

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

None.

Approval Request Comment
[Feature/regressing bug #]: Off thread parsing.
[User impact if declined]: Possible UAF, though exploitability is unknown.
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: None.

Comment 8

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

Comment on attachment 8574292 [details] [diff] [review]
updated

sec-approval+ and branch approvals.

Comment 9

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

Can we get an ESR31 patch for this as well?

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 8574292 [details] [diff] [review]
updated

This patch should apply fine to ESR branches.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8574292 [details] [diff] [review]
updated

ESR31+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1fb8a664e27b
https://hg.mozilla.org/releases/mozilla-beta/rev/46392e569cb6
https://hg.mozilla.org/releases/mozilla-esr31/rev/d30cf95d24c5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/46392e569cb6
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/19d5fb3ece4c
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/0e8ea792b0da
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/49d75d4d2846

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/19d5fb3ece4c
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/0e8ea792b0da