1866636 - Assertion failure: !t->isForwarded(), at js/src/gc/Marking-inl.h:97

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20231124-dbd1a6aef0a7 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

b = new WeakMap();
c = Symbol();
b.set(c);
c = gczeal(10);
for (i=0; i<1000; ++i) {
    try {
        x;
    } catch {}
}

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555557a221d0 in void js::CheckTracedThing<JS::Symbol>(JSTracer*, JS::Symbol*) ()
#1  0x0000555557a4a361 in _ZN2js15MapGCThingTypedIZ18TraceTaggedPtrEdgeIN2JS5ValueEEbP8JSTracerPT_PKcEUlS6_E_EEDaRKS3_OS6_ ()
#2  0x0000555557a25de4 in void js::TraceWeakMapKeyEdgeInternal<JS::Value>(JSTracer*, JS::Zone*, JS::Value*, char const*) ()
#3  0x00005555575e35e6 in js::WeakMap<js::HeapPtr<JS::Value>, js::HeapPtr<JS::Value> >::trace(JSTracer*) ()
#4  0x00005555572e7fce in JSObject::traceChildren(JSTracer*) ()
#5  0x00005555579f8e8f in void UpdateArenaPointersTyped<JSObject>(js::gc::MovingTracer*, js::gc::Arena*) ()
#6  0x00005555579c7d28 in UpdateArenaListSegmentPointers(js::gc::GCRuntime*, ArenaListSegment const&) ()
#7  0x0000555557a0191b in js::gc::ParallelWorker<ArenaListSegment, ArenasToUpdate>::run(js::AutoLockHelperThreadState&) ()
#8  0x0000555557a196b8 in js::GCParallelTask::runTask(JS::GCContext*, js::AutoLockHelperThreadState&) ()
#9  0x0000555557a1998c in js::GCParallelTask::runHelperThreadTask(js::AutoLockHelperThreadState&) ()
#10 0x000055555724bea8 in js::GlobalHelperThreadState::runTaskLocked(js::HelperThreadTask*, js::AutoLockHelperThreadState&) ()
#11 0x000055555724bc40 in js::GlobalHelperThreadState::runOneTask(js::AutoLockHelperThreadState&) ()
#12 0x00005555572611e2 in js::HelperThread::threadLoop(js::InternalThreadPool*) ()
#13 0x0000555557260f9c in js::HelperThread::ThreadMain(js::InternalThreadPool*, js::HelperThread*) ()
#14 0x0000555557269818 in js::detail::ThreadTrampoline<void (&)(js::InternalThreadPool*, js::HelperThread*), js::InternalThreadPool*&, js::HelperThread*>::Start(void*) ()
#15 0x0000555556f2ffed in set_alt_signal_stack_and_start(PthreadCreateParams*) ()
#16 0x00007ffff7bc16ba in start_thread (arg=0x7ffff3cff700) at pthread_create.c:333
#17 0x00007ffff6e4641d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
rax	0x5555557f4812	93824994986002
rbx	0x2bb20e766030	48043746811952
rcx	0x5555589892c8	93825046975176
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7ffff3cfe810	140737283876880
rsp	0x7ffff3cfe7e0	140737283876832
r8	0x7ffff7105770	140737338431344
r9	0x7ffff3cff700	140737283880704
r10	0x2	2
r11	0x0	0
r12	0x555555857c1f	93824995392543
r13	0xffffffffffffff	72057594037927935
r14	0x7ffff3cfe890	140737283877008
r15	0x7ffff3cfea88	140737283877512
rip	0x555557a221d0 <void js::CheckTracedThing<JS::Symbol>(JSTracer*, JS::Symbol*)+672>
=> 0x555557a221d0 <_ZN2js16CheckTracedThingIN2JS6SymbolEEEvP8JSTracerPT_+672>:	movl   $0x61,0x0
   0x555557a221db <_ZN2js16CheckTracedThingIN2JS6SymbolEEEvP8JSTracerPT_+683>:	callq  0x555556f2f8f0 <abort>

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1866636 using build mozilla-central 20231124092418-dbd1a6aef0a7. 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 4

[Jon Coppeard (:jonco)]

From the testcase it's likely this is fallout from implementing symbols as weakmap keys (bug 1828144). Yoshi, can you take a look?

Comment 5

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

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

Comment 6

[Andrew McCreight [:mccr8]]

I'm assuming an assertion inside tracing code like this could lead to a UAF or something.

Comment 7

[Steven DeTar [:sdetar]]

Making this S2, P1 since this is sec-high bug.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

When sweeping the WeakMap, The weak map key (the Symbol object) isn't marked and should have been swept. But somehow the entry still exists after sweeping. Later the symbol got finalized, and caused the UAF problem when compacting the WeakMap entries.
I'm still investigating why the arena of the Symbol got finalized while the WeakMap still holds the key.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] from comment #8)

When sweeping the WeakMap, The weak map key (the Symbol object) isn't marked and should have been swept. But somehow the entry still exists after sweeping.

So the WeakMap key (the Symbol object) is in an atoms zone, so the WeakMap entry is still kept during sweeping.
Later when the atoms zone is swept, but the WeakMap isn't updated hence caused the UAF.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1866636 - Check if the cell is in Atoms Zone while sweeping.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1866636 - Test.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 9367494 [details]
Bug 1866636 - Check if the cell is in Atoms Zone while sweeping.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy. This patch clears the GC Cell if it's pointed to a Symbol which will be collected by GC.
• 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?: 121
• If not all supported branches, which bug introduced the flaw?: Bug 1828144
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: This method is simple, but there might be some small chances that this patch will cause some side effects, we will file another bug then.
• Is Android affected?: Yes

Comment 13

[Ryan VanderMeulen (PTO, back July 27)]

This is gated behind the javascript.options.experimental.symbols_as_weakmap_keys pref, which is off by default everywhere at the moment.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Remove [sec-high] as the pref is added in Bug 1869005.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, RyanVM,
I tried to remove the keyword [sec-high], but this bug says it still needs security approval to land,
can you check this? Thanks

Comment 16

[Christian Holler (:decoder)]

Sec-classification is independent of the state of the pref. Even if pref'ed off, we consider this a sec-high. If the feature is disabled even on Nightly, use the tracking flags to indicate that.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67fc2755b73d
Check if the cell is in Atoms Zone while sweeping. r=jonco
https://hg.mozilla.org/integration/autoland/rev/ec549fd10f6c
Test. r=jonco

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/67fc2755b73d
https://hg.mozilla.org/mozilla-central/rev/ec549fd10f6c

Comment 19

[Daniel Veditz [:dveditz]]

Making Firefox 122 security bugs public. [bugspam filter string: Pilgarlic-Towers]