1715471 - Assertion failure: !detail::CellHasStoreBuffer(reinterpret_cast<const Cell*>(cell)), at js/HeapAPI.h:581

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210608-abe5cbf9daec (debug build, run with --fuzzing-safe --no-threads):

See attachment.

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x000055555752d6ef in bool js::GCMarker::mark<JS::Symbol>(JS::Symbol*) ()
#1  0x00005555574f2887 in js::GCMarker::markEphemeronEdges(mozilla::Vector<js::gc::EphemeronEdge, 2ul, js::SystemAllocPolicy>&) ()
#2  0x00005555574fae80 in JS::Zone::enterWeakMarkingMode(js::GCMarker*, js::SliceBudget&) ()
#3  0x0000555557499c2e in js::gc::IncrementalProgress js::gc::GCRuntime::markWeakReferences<js::gc::SweepGroupZonesIter>(js::SliceBudget&) ()
#4  0x000055555749fc03 in js::gc::GCRuntime::endMarkingSweepGroup(JSFreeOp*, js::SliceBudget&) ()
#5  0x00005555574e5a31 in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#6  0x00005555574d4f35 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#7  0x00005555574a72af in js::gc::GCRuntime::performSweepActions(js::SliceBudget&) ()
#8  0x00005555574ad5f0 in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason) ()
#9  0x00005555574b0363 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason) ()
#10 0x00005555574b16db in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason) ()
#11 0x00005555574b6fee in js::gc::GCRuntime::runDebugGC() ()
#12 0x0000555557463b36 in bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) ()
#13 0x000055555746397c in JSObject* js::AllocateObject<(js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, JSClass const*, js::gc::AllocSite*) ()
#14 0x0000555556b6b89f in js::NativeObject::create(JSContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, js::gc::AllocSite*) ()
#15 0x0000555556da3d6e in js::LexicalEnvironmentObject::createTemplateObject(JSContext*, JS::Handle<js::Shape*>, JS::Handle<JSObject*>, js::gc::InitialHeap) ()
#16 0x0000555556da40e7 in js::BlockLexicalEnvironmentObject::create(JSContext*, JS::Handle<js::LexicalScope*>, JS::Handle<JSObject*>, js::gc::InitialHeap) ()
#17 0x0000555556da4f37 in js::BlockLexicalEnvironmentObject::clone(JSContext*, JS::Handle<js::BlockLexicalEnvironmentObject*>) ()
#18 0x00005555576aac31 in js::jit::BaselineFrame::freshenLexicalEnvironment(JSContext*) ()
#19 0x000015c0fecfe4d5 in ?? ()
#20 0x0000555558064760 in js::jit::tailCallVMFunctions ()
#21 0x00007fffffffbbe0 in ?? ()
#22 0x0000555558063540 in js::jit::vmFunctions ()
#23 0x000015c0fed22fa9 in ?? ()
#24 0x0000000000005821 in ?? ()
#25 0x00007fffffffbc00 in ?? ()
#26 0x0000000000000000 in ?? ()
rax	0x55555573e0c9	93824994238665
rbx	0x51be9d0e188	5617445036424
rcx	0x5555580a73c8	93825037661128
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb280	140737488335488
rsp	0x7fffffffb260	140737488335456
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99840	140737353717824
r10	0x0	0
r11	0x0	0
r12	0x7ffff602c3e0	140737320764384
r13	0x7ffff5efe7c0	140737319528384
r14	0x7ffff5efe7d0	140737319528400
r15	0x51be9d00000	5617444978688
rip	0x55555752d6ef <bool js::GCMarker::mark<JS::Symbol>(JS::Symbol*)+223>
=> 0x55555752d6ef <_ZN2js8GCMarker4markIN2JS6SymbolEEEbPT_+223>:	movl   $0x245,0x0
   0x55555752d6fa <_ZN2js8GCMarker4markIN2JS6SymbolEEEbPT_+234>:	callq  0x555556ad037a <abort>

This test was fairly complex to reduce as it seems to require somewhat precise GC (I had to adjust zeal params dynamically while reducing in order to get this down to a reasonable size). Marking s-s because this is a GC assert with potential memory corruption.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Andrew McCreight [:mccr8]]

Needinfoing sfink because it involves weakmap marking.

Comment 3

[Steve Fink [:sfink] [:s:]]

First reaction: wait, we haven't implemented that yet! (Using Symbols as keys.)

Great bug. I'll take a look.

Comment 4

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210609111307-662aa4502f55.
The bug appears to have been introduced in the following build range:

Start: 89f7028049edb2eca8e5f8731450278b33f84c27 (20210511140347)
End: f7b553bd9c0b77b0130d377f9a9c696c8c611a01 (20210511142103)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=89f7028049edb2eca8e5f8731450278b33f84c27&tochange=f7b553bd9c0b77b0130d377f9a9c696c8c611a01

Comment 5

[Daniel Veditz [:dveditz]]

Only one patch in the range above, bug 1694538

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1694538

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1715471 - Small refactoring of Zone::sweepEphemeronTablesAfterMinorGC

Comment 8

[Phabricator Automation]

Comment on attachment 9228248 [details]
Bug 1715471 - Small refactoring of Zone::sweepEphemeronTablesAfterMinorGC

Revision D118432 was moved to bug 1717565. Setting attachment 9228248 [details] to obsolete.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1715471 - Conservatively mark WeakMap nursery values

Comment 10

[Steve Fink [:sfink] [:s:]]

No, it's not the unimplemented thing sneaking in; the value here is claiming to be a Symbol, not the key.

The basic problem: after bug 1694538, key -> value edges are inserted into the gcEphemeronEdges table, which is not swept during a minor GC. Nursery keys are handled by having a separate gcNurseryEphemeronEdges, while during a minor GC gets moved to the tenured table and also sweeps through the entries (it's a little complicated, because it also has to sweep delegates for those nursery keys.) But nursery values were not handled.

I'm fixing by marking nursery values. They should be pretty rare, and sweeping would require sweeping the whole gcEphemeronEdges table on every minor GC.

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9228250 [details]
Bug 1715471 - Conservatively mark WeakMap nursery values

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Really, really difficult.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: 90
• If not all supported branches, which bug introduced the flaw?: Bug 1694538
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It'll be the same, since this code just recently changed in a way that introduced this problem.
• How likely is this patch to cause regressions; how much testing does it need?: Some chance of increasing memory usage temporarily, but not very high.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 9228250 [details]
Bug 1715471 - Conservatively mark WeakMap nursery values

sec-approval = dveditz

OK to land with the test if we're allowed to uplift to 90 (last beta today!)

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 9228250 [details]
Bug 1715471 - Conservatively mark WeakMap nursery values

approved for 90.0b12

Comment 14

[Julien Cristau [:jcristau]]

Conservatively mark WeakMap nursery values r=jonco
https://hg.mozilla.org/integration/autoland/rev/56254c833970bcb6bfd860d8c28a210033f1ddc7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Follow-up test fix:
https://hg.mozilla.org/integration/autoland/rev/5b9012813f110eb5c181b7637d5696e7e5a6b3f6

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/179204da0641

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/56254c833970
https://hg.mozilla.org/mozilla-central/rev/5b9012813f11

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 19

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Unable to reproduce bug using build mozilla-central 20210608040833-abe5cbf9daec. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 20

[Steve Fink [:sfink] [:s:]]

sec-survey+

(I don't know if bugmon is ok with me editing the whiteboard tag yet.)