Closed Bug 1597206 Opened 2 years ago Closed 2 years ago

Assertion failure: tag_ == TracerKindTag::WeakMarking, at js/src/gc/Marking.cpp:2618 with OOM


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




Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- unaffected
firefox72 - fixed
firefox73 --- fixed


(Reporter: decoder, Assigned: sfink)


(Blocks 1 open bug, Regression)


(5 keywords, Whiteboard: [jsbugmon:update,bisect][post-critsmash-triage])


(4 files)

The following testcase crashes on mozilla-central revision 4def8673359e (build with --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

enableShellAllocationMetadataBuilder(function() {});
try {
    for (var i73 = 0; i73 < 10; ++i73) {
        var f38 = function() {
            function g99() {
} catch (e44) {}


received signal SIGSEGV, Segmentation fault.
#0  js::GCMarker::leaveWeakMarkingMode (this=this@entry=0x7ffff5f2a788) at js/src/gc/Marking.cpp:2616
#1  0x0000555555e969a1 in js::GCMarker::abortLinearWeakMarking (this=0x7ffff5f2a788) at js/src/gc/GCMarker.h:290
#2  js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::addWeakEntry (marker=marker@entry=0x7ffff5f2a788, key=<optimized out>, markable=...) at js/src/gc/WeakMap-inl.h:214
#3  0x0000555555e96b1f in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::markEntries (this=0x7ffff4ddd580, marker=0x7ffff5f2a788) at js/src/gc/WeakMap-inl.h:245
#4  0x0000555555e92b45 in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::trace (this=0x7ffff4ddd580, trc=0x7ffff5f2a788) at js/src/gc/WeakMap-inl.h:175
#5  0x0000555555d05102 in js::ObjectRealm::trace (this=0x7ffff471d868, trc=0x7ffff5f2a788) at js/src/vm/Realm.cpp:284
#6  0x0000555555d05197 in JS::Realm::traceRoots (this=<optimized out>, trc=<optimized out>, traceOrMark=<optimized out>) at js/src/vm/Realm.cpp:323
#7  0x00005555561e63b9 in js::gc::GCRuntime::traceRuntimeCommon (this=this@entry=0x7ffff5f29700, trc=trc@entry=0x7ffff5f2a788, traceOrMark=traceOrMark@entry=js::gc::GCRuntime::MarkRuntime) at js/src/gc/RootMarking.cpp:398
#8  0x00005555561e67e8 in js::gc::GCRuntime::traceRuntimeForMajorGC (this=this@entry=0x7ffff5f29700, trc=trc@entry=0x7ffff5f2a788, session=...) at js/src/gc/RootMarking.cpp:290
#9  0x0000555556188202 in js::gc::GCRuntime::beginMarkPhase (this=0x7ffff5f29700, reason=<optimized out>, session=...) at js/src/gc/GC.cpp:4060
#10 0x0000555556195108 in js::gc::GCRuntime::incrementalSlice (this=this@entry=0x7ffff5f29700, budget=..., gckind=..., reason=reason@entry=JS::GCReason::API, session=...) at js/src/gc/GC.cpp:6758
#11 0x0000555556195cce in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f29700, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., gckind=..., reason=reason@entry=JS::GCReason::API) at js/src/gc/GC.cpp:7243
#12 0x000055555619629e in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f29700, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., gckindArg=..., reason=reason@entry=JS::GCReason::API) at js/src/gc/GC.cpp:7428
#13 0x00005555561968b5 in js::gc::GCRuntime::gc (this=0x7ffff5f29700, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::GCReason::API) at js/src/gc/GC.cpp:7510
#14 0x0000555556196a9f in JS::NonIncrementalGC (cx=cx@entry=0x7ffff5f27000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::GCReason::API) at js/src/gc/GC.cpp:8344
#15 0x0000555555e60ee9 in GC (cx=cx@entry=0x7ffff5f27000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:509
#16 0x00005555559ce6f0 in CallJSNative (cx=0x7ffff5f27000, native=native@entry=0x555555e60e60 <GC(JSContext*, unsigned int, JS::Value*)>, reason=reason@entry=js::CallReason::Call, args=...) at js/src/vm/Interpreter.cpp:456
#29 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11535
rax	0x5555580befa0	93825037758368
rbx	0x7ffff5f2a788	140737319708552
rcx	0x555556f60a30	93825019546160
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffba80	140737488337536
rsp	0x7fffffffba10	140737488337424
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x8	8
r13	0x7ffff5f2a788	140737319708552
r14	0x7fffffffbb10	140737488337680
r15	0x7ffff470e988	140737294428552
rip	0x5555561dce80 <js::GCMarker::leaveWeakMarkingMode()+592>
=> 0x5555561dce80 <js::GCMarker::leaveWeakMarkingMode()+592>:	movl   $0x0,0x0
   0x5555561dce8b <js::GCMarker::leaveWeakMarkingMode()+603>:	ud2

Marking s-s until investigated because this is GC related.

Attached file Testcase

Possibly related to recent weak map marking changes.

Flags: needinfo?(sphink)

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
user: Steve Fink
date: Fri Nov 15 16:40:44 2019 +0000
summary: Bug 1593399 - Rework how mark colors are handled in weakmap marking r=jonco

Guessing related to bug 1593399?

Regressed by: 1593399

There are two problems here. First, it is possible to OOM during regular marking (before entering weak marking mode) when updating the weak key table, and so we need to be able to cancel the weak marking stuff (and fall back to iterative marking) at any time.

Second, we shouldn't be doing anything with the weak key table during regular marking in the first place! I must have messed that up when I merged things together for the altered color handling in bug 1593399. So I will undo that for now -- but I'll still fix abortWeakMarking() so it can be called at any time, because I'll need that for incremental weak marking anyway.

Flags: needinfo?(sphink)
Assignee: nobody → sphink
Priority: -- → P1
Regressions: 1599972
Regressions: 1596943

Is the testcase from #c0 something suitable for landing in-tree? Also, is there a user impact which justifies Beta backport consideration for Fx72?

This should have gotten a security rating before landing. I'm marking it sec-high as it is looks like it is a couple of GC issues, and thus it should have also gotten sec-approval before landing.

Keywords: sec-high

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

Keywords: regression

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Is the testcase from #c0 something suitable for landing in-tree? Also, is there a user impact which justifies Beta backport consideration for Fx72?

I tried to make a more reliable test case out of #c0 and failed. That test reproduces the problem across a number of different versions, but it depends on an exact allocation count. I hoped that oomTest would remove that dependency, but I could not get it to reproduce that way.

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

This should have gotten a security rating before landing. I'm marking it sec-high as it is looks like it is a couple of GC issues, and thus it should have also gotten sec-approval before landing.

Oh, oops. I did explicitly read through the sec-approval rules before landing, but I somehow thought that this had made it into the 72 nightly (the regressing change landed in 72).

I would probably argue for sec-moderate. Although there are 2 issues fixed here, one of them should only result in a minor perf improvement. The other is only triggerable via an OOM at an unlikely point.

I have not requested backport yet because there is still a regression in bug 1596943 that I wanted to track down. This fix is probably fine to go in, but is unlikely to resolve that regression by itself. A further fix might require modifying the same code. If it would help to backport this, I'm fine with that. I don't think we'll be seeing any crashes in practice that this would fix, other than fuzz bugs. The bug this fixes would also be very hard to exploit (not just because of the OOM targeting problem.)

Not tracking based on the sec-moderate re-rating. I'll leave the 72 status as affected for now though in case we end up uplifting.

Comment on attachment 9111307 [details]
Bug 1597206 - Refactor enterWeakMarkingMode slightly

Beta/Release Uplift Approval Request

  • User impact if declined: Some misbehavior on OOM that might lead to UAF. Also, one of the changes here seems to have fixed a stability problem (bug 1597827) that would likely result in some number of crashes.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Although this is tricky code, the changes here mostly remove the possibility to encounter some tricky cases and at this point have been tested for quite a while in Nightly. (The code after these patches is much simpler as well.)
  • String changes made/needed: none
Flags: needinfo?(sphink)
Attachment #9111307 - Flags: approval-mozilla-beta?
Attachment #9111330 - Flags: approval-mozilla-beta?
Attachment #9112162 - Flags: approval-mozilla-beta?

Comment on attachment 9111307 [details]
Bug 1597206 - Refactor enterWeakMarkingMode slightly

gc fix, approved for 72.0b11

Attachment #9111307 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9111330 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9112162 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.