Closed Bug 2045576 Opened 5 months ago Closed 5 months ago

Assertion failure: atomIsMarked(zone, symbol), at js/src/gc/AtomMarking-inl.h:101 (UnmarkGrayTracer traces weak WeakRef→Symbol edges into the per-zone atom-marking bitmap)

Categories

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

Product:
Core
Component:
JavaScript: GC
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
154 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox-esr140 --- unaffected
firefox151 --- wontfix
firefox152 + fixed
firefox153 + fixed
firefox154 + fixed

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 2 open bugs, Regression)

Details

(6 keywords, Whiteboard: [adv-main152.0.4+r])

Attachments

(2 files, 4 external files)

poc.js — testcase (run with --fuzzing-safe)
3.52 KB, text/plain
Details
crash-debug.txt — full assertion + backtrace log
7.72 KB, text/plain
Details

Summary

The gray-unmarking traversal (js::gc::UnmarkGrayTracer) traces weak edges (it only sets WeakMapTraceAction::Skip, not JS::WeakEdgeTraceAction::Skip), while regular GC marking does not. When unmark-gray walks a gray object that holds a WeakRef whose target is an atoms-zone JS::Symbol, WeakRefObject::trace (js/src/builtin/WeakRefObject.cpp:125) feeds that weak symbol edge to UnmarkGrayTracer::onChild<JS::Symbol> (js/src/gc/Marking.cpp:3224), which unconditionally calls

AtomMarkingRuntime::maybeUnmarkGrayAtomically(sourceZone, symbol)

That primitive is repair-only: it requires the symbol to already be present in the source zone's per-zone atom-marking bitmap (MOZ_ASSERT(atomIsMarked(zone, symbol)), js/src/gc/AtomMarking-inl.h:101) and then does zone->markedAtoms().atomicSetExistingBit(blackBit).

Because the WeakRef target edge is weak, GC marking never adds the symbol to the reading zone's bitmap; after a zone GC, refineZoneBitmapsForCollectedZones refines that bitmap entry to white (the symbol stays alive via another, uncollected zone). The precondition is then violated:

  • debug builds: deterministic Assertion failure: atomIsMarked(zone, symbol), at js/src/gc/AtomMarking-inl.h:101 (a plain MOZ_ASSERT, compiled out in release).
  • release builds: SparseBitmap::atomicSetExistingBit() (js/src/ds/Bitmap.h) performs __atomic_fetch_or through a BitBlock* that is only debug-asserted non-null — a null-relative write when the zone has no bitmap block for that arena, or, when the block exists, a spurious black bit for a symbol the zone does not strongly reference, corrupting the GC's per-zone atom liveness bookkeeping from pure content JS.

The PoC reaches this through the hand-written Ion JIT read barrier for inlined WeakMap.prototype.get: MacroAssembler::emitWeapMapBarrierFastPath detects the gray mark bit on the returned value and calls the out-of-line js::jit::WeakMapValueReadBarrier (js/src/jit/VMFunctions.cpp:3462), which runs gc::UnmarkGrayGCThingRecursively(cell) directly from JIT code with no VM-call validation layer in between. Note, however, that any gray-unmark entry point reaches the same bug; the root cause is in the tracer, not the JIT barrier.

Reproduction

Testcase: poc.js (attached).

m-c-asan-debug/dist/bin/js --fuzzing-safe poc.js

Deterministic (3/3 runs), no GC zeal needed. Shell version: product_version = 20260603-5cffec5992d3 (m-c 20260603, rev 5cffec5992d3, Linux x86_64 ASan debug build).

The testcase: (1) creates a second global/zone g that strongly holds Symbol("t"); (2) in the main zone, builds gray root → WeakMap → entry value V = { wr: new WeakRef(g.s) } so V is marked gray; (3) runs schedulezone(this); schedulezone('atoms'); gc('zone') so the main zone's atom bitmap entry for the symbol is refined to white while the symbol survives via g; (4) calls a warmed Ion-compiled m.get(k) on the weak map, whose barrier fast path gray-unmarks V and walks the weak WeakRef edge into maybeUnmarkGrayAtomically on the white bitmap entry.

The shell's grayRoot() stands in for the gray cross-compartment wrappers the cycle collector creates routinely in the browser, and schedulezone/gc('zone') for ordinary zonal GC scheduling; the ingredients (multiple globals, WeakMap, WeakRef, Symbol) are all standard content JS.

Crash signature

Full log in crash-debug.txt:

Assertion failure: atomIsMarked(zone, symbol), at js/src/gc/AtomMarking-inl.h:101
#1  js::gc::AtomMarkingRuntime::maybeUnmarkGrayAtomically(JS::Zone*, JS::Symbol*) gc/AtomMarking-inl.h:101
#2  js::gc::UnmarkGrayTracer<1u>::onChild<JS::Symbol>(JS::Symbol*)                gc/Marking.cpp:3224
#11 js::WeakRefObject::trace(JSTracer*, JSObject*)                                builtin/WeakRefObject.cpp:125
#18 js::gc::UnmarkGrayTracer<1u>::unmark(JS::GCCellPtr)                           gc/Marking.cpp:3285
#24 JS::UnmarkGrayGCThingRecursively(JS::GCCellPtr)                               gc/Marking.cpp:3322
#25 js::jit::WeakMapValueReadBarrier(js::gc::TenuredCell*, JS::Zone*)             jit/VMFunctions.cpp:3462
#26 ([anon:js-executable-memory]+0x2d9b)   <- Ion-compiled WeakMap.get fast path

Suggested fix

Either make the gray-unmark traversal skip weak edges (set JS::WeakEdgeTraceAction::Skip for UnmarkGrayTracer, matching the marking tracer), or make UnmarkGrayTracer::onChild<JS::Symbol> / maybeUnmarkGrayAtomically tolerate (skip) symbols that are not present in the source zone's bitmap. The invariant of the per-zone atom-marking bitmap ([SMDOC] in gc/AtomMarking.cpp) is that it over-approximates atoms reachable via edges that GC marking traces — gray-unmarking must not assume weak edges are in it.

Same family as the previously fixed Bug 2034837 / Bug 2042465 (atoms-zone symbol handling in the WeakMap read barrier), but a distinct defect in the tracer itself.

Assignee: nobody → jcoppeard
Attached file (secure) —

Unmarking a cell shouldn't unmark things weakly referenced from it. This change
makes the unmarking tracer work the same as the mark marking tracer which also
doesn't mark through weak edges.

Blocks: sm-security, GC.stability
Severity: -- → S3
Priority: -- → P1

Setting csectype-wildptr and set-high for possible relative write based on null pointer in release builds as per description.

Keywords: csectype-wildptr, sec-high
Keywords: regression
Regressed by: 1994871

Comment on attachment 9595457 [details]
(secure)

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Very difficult.
  • 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All back to 146.
  • If not all supported branches, which bug introduced the flaw?: Bug 1994871
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: Should be trivial.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely. This is a very simple change that matches the gray unmarking tracer to the same logic as the main marking tracer.
  • Is the patch ready to land after security approval is given?: Yes
  • Is Android affected?: Yes
Attachment #9595457 - Flags: sec-approval?

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

status-firefox151: --- → affected
status-firefox152: --- → affected
status-firefox153: --- → affected
status-firefox-esr140: --- → unaffected

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:jonco, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Flags: needinfo?(jcoppeard)

The bug is marked as tracked for firefox152 (beta) and tracked for firefox153 (nightly). However, the bug still has low severity.

:sdetar, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Flags: needinfo?(sdetar)
Severity: S3 → S2
Flags: needinfo?(sdetar)
Flags: needinfo?(jcoppeard)

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

status-firefox154: --- → affected
Attachment #9595457 - Flags: sec-approval? → sec-approval+

https://hg-edge.mozilla.org/mozilla-central/rev/193503f0a452

Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 5 months ago
status-firefox154: affected → fixed
tracking-firefox154: --- → +
Resolution: --- → FIXED
Target Milestone: --- → 154 Branch

Please nominate this for Beta and Release uplift.

Flags: needinfo?(jcoppeard)
Flags: needinfo?(jcoppeard)

firefox-beta Uplift Approval Request

  • User impact if declined/Reason for urgency: Possible crash / security vulnerability.
  • Code covered by automated testing?: yes
  • Fix verified in Nightly?: yes
  • Needs manual QE testing?: no
  • Steps to reproduce for manual QE testing:
  • Risk associated with taking this patch: low
  • Explanation of risk level: This is a small and simple fix that stops gray unmarking progressing through weak edges as intended.
  • String changes made/needed?: None
  • Is Android affected?: yes
Attachment #9599276 - Flags: approval-mozilla-beta?
Attached file (secure) —

Unmarking a cell shouldn't unmark things weakly referenced from it. This change
makes the unmarking tracer work the same as the mark marking tracer which also
doesn't mark through weak edges.

Original Revision: https://phabricator.services.mozilla.com/D305484

firefox-release Uplift Approval Request

  • User impact if declined/Reason for urgency: Possible crash / security vulnerability.
  • Code covered by automated testing?: yes
  • Fix verified in Nightly?: yes
  • Needs manual QE testing?: no
  • Steps to reproduce for manual QE testing:
  • Risk associated with taking this patch: low
  • Explanation of risk level: This is a small and simple fix that stops gray unmarking progressing through weak edges as intended.
  • String changes made/needed?: None
  • Is Android affected?: yes
Attachment #9599277 - Flags: approval-mozilla-release?
Attached file (secure) —

Unmarking a cell shouldn't unmark things weakly referenced from it. This change
makes the unmarking tracer work the same as the mark marking tracer which also
doesn't mark through weak edges.

Original Revision: https://phabricator.services.mozilla.com/D305484

Attachment #9599276 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [sec] [uplift] [qa-triage-done-c154/b153]
Attachment #9599277 - Flags: approval-mozilla-release? → approval-mozilla-release+
Whiteboard: [adv-main152.0.4+r]
Group: core-security-release
Attachment #9595458 - Attachment description: (secure) → Bug 2045576 - Add testcase r?sfink
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: