1911288 - (CVE-2024-8384) Assertion failure: kind == JS::TracerKind::Tenuring || kind == JS::TracerKind::MinorSweeping || kind == JS::TracerKind::Moving, at js/src/gc/Marking.cpp:141 with OOM and Debugger

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20240730-c756f74154bf (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

Object.defineProperty(this, "x", {});
try {
  oomTest(function() {
      let m = parseModule(``);
  });
  var dbg = new Debugger;
  dbg.onNewGlobalObject = function (global) {}
  let t39 = {};
  addMarkObservers([t39]);
  let g33 = newGlobal({newCompartment: true});
  g33.t39 = t39;
  g33.eval("grayRoot().push(t);");
} catch (exc) {}
gcparam("allocationThreshold", 1 /* MB */);
gcparam("incrementalGCEnabled", false);
oomTest(function() {
  var lfGlobal = newGlobal({sameZoneAs: this});
});

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x000055555795e1f6 in void js::CheckTracedThing<JSObject>(JSTracer*, JSObject*) ()
#1  0x000055555797ffe3 in _ZN2js15MapGCThingTypedIZ18TraceTaggedPtrEdgeIN2JS5ValueEEbP8JSTracerPT_PKcEUlS6_E_EEDaRKS3_OS6_ ()
#2  0x00005555579622ab in void js::TraceManuallyBarrieredCrossCompartmentEdge<JS::Value>(JSTracer*, JSObject*, JS::Value*, char const*) ()
#3  0x00005555571ad748 in JS::Compartment::traceWrapperTargetsInCollectedZones(JSTracer*, JS::Compartment::EdgeSelector) ()
#4  0x00005555571ae1b0 in JS::Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(JSTracer*, JS::Compartment::EdgeSelector) ()
#5  0x00005555579b09fb in js::gc::GCRuntime::traceRuntimeForMajorGC(JSTracer*, js::gc::AutoGCSession&) ()
#6  0x0000555557932c63 in js::gc::GCRuntime::beginMarkPhase(js::gc::AutoGCSession&) ()
#7  0x0000555557938888 in js::gc::GCRuntime::incrementalSlice(JS::SliceBudget&, JS::GCReason, bool) ()
#8  0x000055555793be9e in js::gc::GCRuntime::gcCycle(bool, JS::SliceBudget const&, JS::GCReason) ()
#9  0x000055555793d7f4 in js::gc::GCRuntime::collect(bool, JS::SliceBudget const&, JS::GCReason) ()
#10 0x000055555793e844 in js::gc::GCRuntime::startGC(JS::GCOptions, JS::GCReason, JS::SliceBudget const&) ()
#11 0x00005555579277e6 in js::gc::GCRuntime::gcIfRequestedImpl(bool) ()
#12 0x000055555790251b in void* js::gc::CellAllocator::TryNewTenuredCell<(js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long) ()
#13 0x000055555721a87f in js::GetterSetter::create(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) ()
#14 0x000055555731901d in bool AddOrChangeProperty<(IsAddOrChange)0>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::PropertyDescriptor>, js::PropertyResult*) ()
#15 0x0000555557317c90 in js::NativeDefineProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::PropertyDescriptor>, JS::ObjectOpResult&) ()
#16 0x00005555572a2913 in js::DefineAccessorProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int, JS::ObjectOpResult&) ()
#17 0x00005555572a2f18 in js::DefineAccessorProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int) ()
#18 0x00005555573a9494 in DefineAccessorPropertyById(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int) ()
#19 0x00005555573a91eb in DefineAccessorPropertyById(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JSNativeWrapper const&, JSNativeWrapper const&, unsigned int) ()
#20 0x00005555573af640 in JS_DefineProperties(JSContext*, JS::Handle<JSObject*>, JSPropertySpec const*) ()
#21 0x000055555721b19c in js::GlobalObject::resolveConstructor(JSContext*, JS::Handle<js::GlobalObject*>, JSProtoKey, js::GlobalObject::IfClassIsDisabled) ()
#22 0x00005555571214fa in CreateObjectConstructor(JSContext*, JSProtoKey) ()
#23 0x000055555721afae in js::GlobalObject::resolveConstructor(JSContext*, JS::Handle<js::GlobalObject*>, JSProtoKey, js::GlobalObject::IfClassIsDisabled) ()
#24 0x000055555721dd63 in js::GlobalObject::new_(JSContext*, JSClass const*, JSPrincipals*, JS::OnNewGlobalHookOption, JS::RealmOptions const&) ()
#25 0x0000555556e9f442 in NewGlobalObject(JSContext*, JS::RealmOptions&, JSPrincipals*, ShellGlobalKind, bool) ()
#26 0x0000555556eb5d2b in NewGlobal(JSContext*, unsigned int, JS::Value*) ()
#27 0x00003daa04b62e23 in ?? ()
#28 0x000000000000004f in ?? ()
#29 0x00007fffffffc650 in ?? ()
#30 0x0000000000000000 in ?? ()
rax	0x5555558c5726	93824995841830
rbx	0x195c44849ac0	27884077226688
rcx	0x555558867008	93825045786632
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb170	140737488335216
rsp	0x7fffffffb140	140737488335168
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f92840	140737353689152
r10	0x0	0
r11	0x0	0
r12	0x7fffffffb220	140737488335392
r13	0x7fffffffb280	140737488335488
r14	0x7ffff3d42d40	140737284156736
r15	0x7ffff3d42d40	140737284156736
rip	0x55555795e1f6 <void js::CheckTracedThing<JSObject>(JSTracer*, JSObject*)+1734>
=> 0x55555795e1f6 <_ZN2js16CheckTracedThingI8JSObjectEEvP8JSTracerPT_+1734>:	movl   $0x8d,0x0
   0x55555795e201 <_ZN2js16CheckTracedThingI8JSObjectEEvP8JSTracerPT_+1745>:	callq  0x555556f0ee80 <abort>

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240802153712-c38029641964.
The bug appears to have been introduced in the following build range:

Start: 55c28baccbfda1bef4e3983689b5d955fb030a57 (20240508094125)
End: ed7b6f65910afcd687ef5aaa25605666805796bb (20240508130913)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=55c28baccbfda1bef4e3983689b5d955fb030a57&tochange=ed7b6f65910afcd687ef5aaa25605666805796bb

Comment 4

[Iain Ireland [:iain]]

I took a quick look at this. I don't fully understand what's going on, but maybe my observations can make a good starting point.

I haven't managed to correlate rr events back to the original testcase very well, but it appears that we have a situation where we create a cross-zone wrapper, then start a major GC with the wrapper marked gray. (The target seems to be a PlainObject that is not marked when we start the GC.) We call traceWrapperTargetsInCollectedZones once to mark non-gray edges, then a second time to mark gray edges. However, in between those two calls, the wrapper is marked black by the UnmarkGrayTracer. As a result, we don't mark the wrapper's target either time. Later, we do another GC. This time, traceWrapperTargetsInCollectedZones is not confused by gray bits, and it tries to trace the target only to crash because it's already been freed.

At some point in UnmarkGray tracing (before the wrapper is marked black, if it matters), an oomTest-induced failure here caused us to skip adding something to the stack: specifically, a JSFunction. I haven't worked out how the JSFunction is connected to the wrapper, but I do see that searchfox doesn't think we have any coverage of that oom branch, so it seems plausible to me that our OOM-handling here is just broken. It looks like we give up when we see an OOM and require that a full GC be completed before the next CC, but again there's no test coverage.

In principle I would support slapping an AutoEnterOOMUnsafeRegion on the failing allocation and calling it a day, but maybe somebody who knows this code a little better should take a peek to make sure we're not papering over a real bug.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Iain Ireland [:iain] from comment #4)
Thanks for looking into this.

We call traceWrapperTargetsInCollectedZones once to mark non-gray edges, then a second time to mark gray edges. However, in between those two calls, the wrapper is marked black by the UnmarkGrayTracer. As a result, we don't mark the wrapper's target either time.

This seems to be the problem. We need to mark the target black in this case.

I do see that searchfox doesn't think we have any coverage of that oom branch, so it seems plausible to me that our OOM-handling here is just broken.

Code coverage only runs on opt builds and oomTest is only present in debug builds. As a result a lot of our OOM handling paths appear to lack coverage but that is not necessarily the case.

Comment 6

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

Based on comment #3, this bug contains a bisection range found by bugmon. However, the Regressed by field is still not filled.

:jonco, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit BugBot documentation.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1911288 - Mark cross compartment edges from uncollected zones during gray marking that have been marked black since black marking happened r?sfink

The problem here is that currently we mark incoming cross compartment edges
from uncollected compartments in two phases depending on the color of the
wrapper, but that color can change in between due to gray unmarking.

The patch does the simple thing of rescanning the incoming edges for black
wrappers after marking gray ones. This involves a little juggling of the
current mark color to ensure we can't mark black after marking gray.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #6)

:jonco, if possible, could you fill the Regressed by field and investigate this regression?

This bug has been around since forever.

Comment 9

[Iain Ireland [:iain]]

(In reply to Jon Coppeard (:jonco) from comment #5)

Code coverage only runs on opt builds and oomTest is only present in debug builds. As a result a lot of our OOM handling paths appear to lack coverage but that is not necessarily the case.

Today I learned!

Comment 10

[Andrew McCreight (out of office until 8/21) [:mccr8]]

The test case involves the debugger, and it sounds like an OOM failure is involved, but the actual fix doesn't seem explicitly about that, so I'll mark it sec-high. Maybe that's too high.

Comment 11

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

The severity field for this bug is set to S4. 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.

Comment 12

[Jon Coppeard (:jonco)]

I'll raise this to S3 since it is a potential security vulnerability but the fact that it's gone unnoticed for so long indicates that this unlikely to be having much impact on users.

Comment 13

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I guess I'll bump it down to sec-moderate. The test case is rather intricate and involves the debugger, so maybe it is difficult or impossible for regular web content to trigger the right conditions.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2faef4807b80
Mark cross compartment edges from uncollected zones during gray marking that have been marked black since black marking happened r=sfink

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/2faef4807b80

Comment 16

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20240815213144-35292ed5ccd3.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 17

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

The patch landed in nightly and beta is affected.
:jonco, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox130 to wontfix.

For more information, please visit BugBot documentation.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

FYI, this grafts cleanly to Beta but will need rebased patches for ESR128 and ESR115. Though comment 13 makes it sound like maybe ESR115 isn't as crucial?

Comment 19

[Jon Coppeard (:jonco)]

Comment on attachment 9417910 [details]
Bug 1911288 - Mark cross compartment edges from uncollected zones during gray marking that have been marked black since black marking happened r?sfink

Beta/Release Uplift Approval Request

• User impact if declined: Possible crash / security vulnerability
• 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): This is a relatively straightforward change to mark edges from black wrappers into collecting zones. It's low risk because it only adds extra marking.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9417910 [details]
Bug 1911288 - Mark cross compartment edges from uncollected zones during gray marking that have been marked black since black marking happened r?sfink

Approved for 130.0b8.

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1b98f29764d

Comment 22

[Jon Coppeard (:jonco)]

Attachment: Bug 1911288 - Mark cross compartment edges from uncollected zones during gray marking that have been marked black since black marking happened (ESR115)

The problem here is that currently we mark incoming cross compartment edges
from uncollected compartments in two phases depending on the color of the
wrapper, but that color can change in between due to gray unmarking.

The patch does the simple thing of rescanning the incoming edges for black
wrappers after marking gray ones. This involves a little juggling of the
current mark color to ensure we can't mark black after marking gray.

Comment 23

[Jon Coppeard (:jonco)]

Attachment: Bug 1911288 - Mark cross compartment edges from uncollected zones during gray marking that have been marked black since black marking happened (ESR128)

The problem here is that currently we mark incoming cross compartment edges
from uncollected compartments in two phases depending on the color of the
wrapper, but that color can change in between due to gray unmarking.

The patch does the simple thing of rescanning the incoming edges for black
wrappers after marking gray ones. This involves a little juggling of the
current mark color to ensure we can't mark black after marking gray.

Comment 24

[Jon Coppeard (:jonco)]

Comment on attachment 9420076 [details]
Bug 1911288 - Mark cross compartment edges from uncollected zones during gray marking that have been marked black since black marking happened (ESR115)

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is security bug that could potentially cause a vulnerability.
• User impact if declined: Possible crash / security vulnerability.
• Fix Landed on Version: 131
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a relatively straightforward change to mark edges from black wrappers into collecting zones. It has been on central for five days without issue.

Comment 25

[Jon Coppeard (:jonco)]

Comment on attachment 9420077 [details]
Bug 1911288 - Mark cross compartment edges from uncollected zones during gray marking that have been marked black since black marking happened (ESR128)

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is security bug that could potentially cause a vulnerability.
• User impact if declined: Possible crash / security vulnerability.
• Fix Landed on Version: 131
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a relatively straightforward change to mark edges from black wrappers into collecting zones. It has been on central for five days without issue.

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/7ee6dc774d92

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/85af7e4f93f9

Comment 28

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt