1841682 - Assertion failure: this->flags() == 0, at gc/Cell.h:832

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20230629-c93a9e0ad90d (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --ion-warmup-threshold=0 --fast-warmup --blinterp-warmup-threshold=1):

gczeal(12,9);
function clone_object_check(b, desc) {
  function ownProperties(obj) {
    return Object.getOwnPropertyNames(obj).
      map(function (p) { return [p, Object.getOwnPropertyDescriptor(obj, p)]; });
  }
  function isArrayLength(obj, pair) {
    return Array.isArray(obj) && pair[0] == "length";
  }
  function isCloneable(obj, pair) {
    return isArrayLength(obj, pair) || (typeof pair[0] === 'string' && pair[1].enumerable);
  }
  function assertIsCloneOf(a, b, path) {
    var pb = ownProperties(b).filter(function(element) {
      return isCloneable(b, element);
    });
    var pa = ownProperties(a);
    for (var i = 0; i < pa.length; i++) {
      var da = pa[i][1];
      var va = da.value;
      var vb = b[pb[i][0]];
      queue.push([va, vb]);
    }
  }
  var a = deserialize(serialize(b));
  var queue = [[a, b]];
  while (queue.length) {
    var triple = queue.shift();
    assertIsCloneOf(triple[0], triple[1], triple[2]);
  }
}
gczeal(19, 5);
var allTypedArraySamples = [
    { value: new Int8Array(1), expected: true },
    { value: new Uint8ClampedArray(1), expected: true }
];
clone_object_check(allTypedArraySamples);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555577bed3d in CheckForCompartmentMismatch(JSObject*, JSObject*) ()
#1  0x00005555577a48e7 in bool js::GCMarker::processMarkStackTop<4u>(js::SliceBudget&) ()
#2  0x000055555779ee02 in bool js::GCMarker::doMarking<4u>(js::SliceBudget&, js::gc::ShouldReportMarkTime) ()
#3  0x000055555779ec8a in js::GCMarker::markUntilBudgetExhausted(js::SliceBudget&, js::gc::ShouldReportMarkTime) ()
#4  0x0000555557757d4a in js::gc::GCRuntime::markUntilBudgetExhausted(js::SliceBudget&, js::gc::GCRuntime::ParallelMarking, js::gc::ShouldReportMarkTime) ()
#5  0x000055555775c1b4 in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, JS::GCReason, bool) ()
#6  0x000055555775faae in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, JS::GCReason) ()
#7  0x0000555557761434 in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, JS::GCReason) ()
#8  0x0000555557729e16 in js::gc::GCRuntime::runDebugGC() ()
#9  0x000055555772848c in bool js::gc::CellAllocator::PreAllocChecks<(js::AllowGC)1>(JS::RootingContext*, js::gc::AllocKind) ()
#10 0x00005555577293a5 in void* js::gc::CellAllocator::AllocTenuredCell<(js::AllowGC)1>(JS::RootingContext*, js::gc::AllocKind, unsigned long) ()
#11 0x00005555571bf6e8 in js::SharedShape::getInitialShape(JSContext*, JSClass const*, JS::Realm*, js::TaggedProto, unsigned long, js::EnumFlags<js::ObjectFlag>) ()
#12 0x0000555556fff4b9 in NewObject(JSContext*, JSClass const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind) ()
#13 0x0000555556fff836 in js::NewObjectWithClassProto(JSContext*, JSClass const*, JS::Handle<JSObject*>, js::gc::AllocKind, js::NewObjectKind) ()
#14 0x0000555556e427e7 in js::BooleanObject::create(JSContext*, bool, JS::Handle<JSObject*>) ()
#15 0x0000555556ea630c in obj_getOwnPropertyNames(JSContext*, unsigned int, JS::Value*) ()
#16 0x000001ffb972374e in ?? ()
#17 0x0000000000000000 in ?? ()
rax	0x5555557edf6f	93824994959215
rbx	0x3ed9ec000198	69105688248728
rcx	0x5555585b1378	93825042944888
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffbe40	140737488338496
rsp	0x7fffffffbe20	140737488338464
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9a840	140737353721920
r10	0x2	2
r11	0x0	0
r12	0x1	1
r13	0x7ffff3e3b080	140737285173376
r14	0x2	2
r15	0x7ffff39c5f20	140737280499488
rip	0x5555577bed3d <CheckForCompartmentMismatch(JSObject*, JSObject*)+237>
=> 0x5555577bed3d <_ZL27CheckForCompartmentMismatchP8JSObjectS0_+237>:	movl   $0x340,0x0
   0x5555577bed48 <_ZL27CheckForCompartmentMismatchP8JSObjectS0_+248>:	callq  0x555556cac07f <abort>

Marking s-s due to GC assert.

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 20230704214905-bb6a5e451dac.
Unable to bisect testcase (Unable to launch the start build!):

Start: fc7fbf3a78e0776b0a0ff21cf27151e6b6aa5970 (20220706094022)
End: c93a9e0ad90d7eca1077e5b52a84b577549bc02e (20230629085424)
BuildFlags: BuildFlags(asan=False, tsan=False, debug=True, fuzzing=False, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Comment 4

[Andrew McCreight [:mccr8]]

This is a compartment mismatch during GC tracing, so I'll assume this is sec-high. Feel free to clear it or whatever if it is some kind of weird shell function.

Comment 5

[Steven DeTar [:sdetar]]

Jon, can you take a quick look at this sec-high GC bug? Or it makes sense I can also ask Steve

Comment 6

[Jon Coppeard (:jonco) (PTO until Monday 30th)]

This looks like a missing postbarrier in JIT code. We hit the JS_FRESH_NURSERY_PATTERN poison pattern when tracing JSObject elements. The object is in the tenured heap and the element value was for an object in the nursery, with the write happening in generated code.

Comment 7

[Jon Coppeard (:jonco) (PTO until Monday 30th)]

I don't really know what's going wrong here. JIT spew tells me the write is part of an ArrayPush operation, but we do emit a postbarrier in WarpCacheIRTranspiler::emitArrayPush. There is code generated for the barrier although it is some way before the store itself.

Jan do you have any ideas on how to track this down?

Comment 8

[Jan de Mooij [:jandem]]

I'm looking into this...

Comment 9

[Jan de Mooij [:jandem]]

What's happening is something like this:

1. We call jit::PostWriteElementBarrier before pushing an element and because of gczeal 12 (= ElementsBarrier) we add a single-element entry for index 5 to the slots store buffer. The array has 1 shifted element at this point so the entry is for unshifted index 6.
2. We then call NativeObject::addDenseElementPure before we store the new element and this gets rid of the shifted element in NativeObject::moveShiftedElements. This does trigger barriers, but we haven't stored the new element yet.
3. We store the new element.
4. We do a nursery GC. Because of (2) the new element stored in (3) is not covered by the post barrier entry added in (1).

This might be a regression from bug 1834038. I think with Baseline ICs this works because we actually do the post barrier after the store, but because in Ion we do the post barrier first there's a window between the barrier and the store where we mess with the dense elements and that's causing this bug.

I'm not sure what's the best fix for this.

Here's a shorter test case that fails with --ion-eager --no-threads:

function f(a) {
  var vals = Object.getOwnPropertyNames(a).map(p => a[p]);
  for (var i = 0; i < vals.length; i++) {
    var v = vals[i];
    queue.push([v]);
  }
}

gczeal(12);
gczeal(19, 5);

var a = [{x:{0:0}}, {x:{0:0}}];
var queue = [a];

while (queue.length) {
  var arr = queue.shift();
  f(arr);
}

Comment 10

[Jan de Mooij [:jandem]]

Actually it doesn't require MArrayPush, the same thing applies to MStoreElementHole.

function f(a) {
  var vals = Object.getOwnPropertyNames(a).map(p => a[p]);
  for (var i = 0; i < vals.length; i++) {
    var v = vals[i];
    queue[queue.length] = [v];
  }
}

gczeal(12);
gczeal(19, 5);

var a = [{x:{0:0}}, {x:{0:0}}];
var queue = [a];

while (queue.length) {
  var arr = queue.shift();
  f(arr);
}

Comment 11

[Jan de Mooij [:jandem]]

I think eventually we want to replace MPostWriteBarrier and MPostWriteElementBarrier with a store barrier that's part of the MIR instruction similar to what we do with the pre-barrier and with ICs. This would require adding a post barrier to the following instructions:

• To remove MPostWriteElementBarrier
  • MStoreElement
  • MStoreElementHole
  • MArrayPush
• To remove MPostWriteBarrier
  • MStoreElement
  • MSetArgumentsObjectArg
  • MStoreFixedSlot
  • MStoreDynamicSlot
  • MInitHomeObject
  • MAddAndStoreSlot
  • MAllocateAndStoreSlot
  • MWasmStoreFieldRefKA

This would also fix some inconsistency we have today where sometimes we add the post-barrier MIR instruction before the store, and sometimes we add it after. The latter is a bit of a risk because if an instruction that can bail out is moved between the store and the barrier we have another security bug.

For this bug we only care about MPostWriteElementBarrier.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1841682 - Simplify post barrier for MStoreElementHole and MArrayPush. r?iain!

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1841682 - Add tests. r?iain!

Depends on D183583

Comment 14

[Jan de Mooij [:jandem]]

I posted a patch that fixes MStoreElementHole and MArrayPush. I'm pretty happy with how it turned out. It also removes the IndexInBounds enum/template because we now always call PostWriteElementBarrier with an index value that's in bounds.

Longer-term it might make sense to fix MStoreElement and the other instructions in comment 11 too, but that's not something we want to uplift and this fixes the most complicated case.

Comment 15

[Jan de Mooij [:jandem]]

Before bug 1797486, MStoreElementHole called jit::SetDenseElement which did the store in C++ if we had to resize the elements. For MArrayPush that was bug 1834038.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 9343900 [details]
Bug 1841682 - Simplify post barrier for MStoreElementHole and MArrayPush. r?iain!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not very easy but it's possible.
• 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?: 115+
• If not all supported branches, which bug introduced the flaw?: Bug 1797486
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should apply or be easy to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Medium risk: the patch is not trivial but testing and fuzzing on Nightly should be sufficient.
• Is Android affected?: Yes

Comment 17

[Dianna Smith [:diannaS]]

As a reminder we only one beta left to land this for 116

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Dianna Smith [:diannaS] from comment #17)

As a reminder we only one beta left to land this for 116

This is waiting for sec-approval. Forwarding to dveditz since Tom is OOTO.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 9343900 [details]
Bug 1841682 - Simplify post barrier for MStoreElementHole and MArrayPush. r?iain!

sec-approval+ = dveditz (land the tests after shipping, as usual)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0de156207253
Simplify post barrier for MStoreElementHole and MArrayPush. r=iain

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/0de156207253

Comment 22

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

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

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

For more information, please visit BugBot documentation.

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 9343900 [details]
Bug 1841682 - Simplify post barrier for MStoreElementHole and MArrayPush. r?iain!

Beta/Release Uplift Approval Request

• User impact if declined: Security bugs, maybe crashes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This code has pretty good test coverage and isn't high risk, but unfortunately the fix isn't trivial either.
• String changes made/needed:
• Is Android affected?: Yes

Comment 24

[Dianna Smith [:diannaS]]

Comment on attachment 9343900 [details]
Bug 1841682 - Simplify post barrier for MStoreElementHole and MArrayPush. r?iain!

Approved for 116.0b8

Comment 25

[Pulsebot]

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

Comment 26

[Dianna Smith [:diannaS]]

Comment on attachment 9343900 [details]
Bug 1841682 - Simplify post barrier for MStoreElementHole and MArrayPush. r?iain!

Approved for 115.1esr

Comment 27

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1841682 using build mozilla-central 20230629085424-c93a9e0ad90d. 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 28

[Dianna Smith [:diannaS]]

ty! unfortunately this doesnt graft cleanly to esr115, can you submit a rebased patch?

Comment 29

[Jan de Mooij [:jandem]]

Attachment: Bug 1841682 - (ESR115) Simplify post barrier for MStoreElementHole and MArrayPush. r=iain!, a=dsmith

Comment 30

[Jan de Mooij [:jandem]]

(In reply to Dianna Smith [:diannaS] from comment #28)

ty! unfortunately this doesnt graft cleanly to esr115, can you submit a rebased patch?

I posted a separate patch for ESR115. It's smaller because bug 1834038 landed in 116 so I took out the ArrayPush changes and some of the other improvements we can't land as a result.

I also verified the store-element test case is fixed by this patch. The array-push test doesn't fail with or without the patch, as expected.

Comment 31

[Dianna Smith [:diannaS]]

Comment on attachment 9345019 [details]
Bug 1841682 - (ESR115) Simplify post barrier for MStoreElementHole and MArrayPush. r=iain!, a=dsmith

Moving flag to rebased patch

Comment 32

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/cef9812a2701

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b43805a4a9c9
Add tests. r=iain

Comment 34

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/b43805a4a9c9