1830921 - Assertion failure: (value & RESERVED_MASK) == 0, at js/src/gc/Cell.h:147

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20230429-8339bdf8fcc8 (debug build, run with --fuzzing-safe --differential-testing --wasm-compiler=baseline+optimizing --no-asmjs --no-sse4 --ion-full-warmup-threshold=100 --ion-instruction-reordering=on --ion-extra-checks --ion-warmup-threshold=100 --ion-scalar-replacement=off --ion-check-range-analysis --ion-range-analysis=off --ion-osr=off --fast-warmup --blinterp --blinterp-warmup-threshold=1 --more-compartments --ion-offthread-compile=off --gc-zeal=4,230 --baseline-warmup-threshold=100 --no-threads):

See attachment.

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555570eb0b4 in JSLinearString* JSRope::flattenInternal<(JSRope::UsingBarrier)0, unsigned char>(JSRope*) ()
#1  0x00005555570d7cf8 in JSRope::flatten(JSContext*) ()
#2  0x0000555556ca1d85 in ExecuteRegExp(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSString*>, int, js::VectorMatchPairs*) ()
#3  0x0000555556ca07c1 in RegExpMatcherImpl(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSString*>, int, JS::MutableHandle<JS::Value>) ()
#4  0x0000317399ef44c7 in ?? ()
[...]
#14 0x0000000000000000 in ?? ()
rax	0x55555575b496	93824994358422
rbx	0xfffe2f2f2f2f2f2f	-511070251831505
rcx	0x5555582e7d38	93825040022840
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffc3c0	140737488339904
rsp	0x7fffffffc340	140737488339776
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9a840	140737353721920
r10	0x2	2
r11	0x0	0
r12	0x7ffff3e4e580	140737285252480
r13	0x7ffff3e26280	140737285087872
r14	0x3cc359ae2118	66809720873240
r15	0x3cc359ae30d0	66809720877264
rip	0x5555570eb0b4 <JSLinearString* JSRope::flattenInternal<(JSRope::UsingBarrier)0, unsigned char>(JSRope*)+2164>
=> 0x5555570eb0b4 <_ZN6JSRope15flattenInternalILNS_12UsingBarrierE0EhEEP14JSLinearStringPS_+2164>:	movl   $0x93,0x0
   0x5555570eb0bf <_ZN6JSRope15flattenInternalILNS_12UsingBarrierE0EhEEP14JSLinearStringPS_+2175>:	callq  0x555556c08957 <abort>

This report is rather unreduced, I didn't have more time to reduce further (and it didn't easily auto-reduce).

S-s because this is a GC assert.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Nicolas B. Pierron [:nbp]]

Iain, would you be the right person to investigate this issue, could it be related to the regexp integration?

Comment 4

[Christian Holler (:decoder)]

This is also a fuzzblocker with this any other GC related asserts all over the place.

Comment 5

[Iain Ireland [:iain]]

This seems to be a bad interaction between the StringConcat stub and the VerifierPre gczeal mode. It looks like what happens is:

1. While running in the C++ interpreter, we trigger a VerifyPreTracer run.
2. This temporarily disables the nursery.
3. This is an incremental trace, so at some point we return to the interpreter.
4. A script tries to enter the baseline interpreter, so we initialize the JitRealm.
5. At this point, the nursery is not enabled, so we set the initialStringHeap to gc::TenuredHeap.
6. When we generate the StringConcat stub, we allocate a new rope in initialStringHeap, with a comment saying that we don't need post-barriers if that's the default heap. But in this case, it's the tenured heap, even though the nursery was only temporarily disabled.
7. We concatenate a nursery string. The new rope is allocated in the tenured space. There's no prebarrier. The nursery string is eventually collected, and everything explodes.

My initial instinct is that the problem is in step 6, and we should generate post-barriers if initialStringHeap is the tenured heap. I also think this is unlikely to be a real security issue, because AFAICT it should only happen with GC zeal enabled. But I'd appreciate a second pair of eyes. Jon, any thoughts?

PS: I didn't spend any time reducing the testcase, but I did reduce the list of shell flags to --differential-testing --ion-osr=off --fast-warmup --blinterp-warmup-threshold=1 --gc-zeal=4,230 --baseline-warmup-threshold=100 --no-threads.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Iain Ireland [:iain] from comment #5)
Thank you for the detailed analysis.

What's supposed to happen in the concat stub is that if initialStringHeap is the tenured heap then the input strings should always be tenured too. We evict the nursery when we disable it, so this works when then nursery is disabled and it works whether the JIT realm is created before or after that happens.

What doesn't work is when the JIT realm is created after the nursery is disabled and it is then re-enabled. This bakes initialStringHeap into the concat staub. When we enable or disable allocation of particular kinds in the nursery we discard all JIT code but that doesn't happen when we enable or disable the nursery as a whole. That would be the safest thing to do here. Disabling the nursery is not something that happens in normal execution so I don't think there should be a problem with this.

This issue can occur by use of AutoDisableGenerationGC, but it's only used by the pre-barrier verifier and once in the shell in a way that doesn't cause this problem.

Comment 7

[Bugmon [:jkratzer for issues]]

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

Start: 8eabe4c96864d5ecaa73e1e4a1c91aad1b2000a6 (20230427102259)
End: 3bbc0a622f412fd9f601cdb5ee6133985e2499fb (20230427104925)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8eabe4c96864d5ecaa73e1e4a1c91aad1b2000a6&tochange=3bbc0a622f412fd9f601cdb5ee6133985e2499fb

Comment 8

[Daniel Veditz [:dveditz]]

Tagging this sec-other for now, but if comment 6 is right then we could unhide this. If it's a security bug then it's probably sec-high.

Comment 9

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

Setting Regressed by field after analyzing regression range found by bugmon in comment #7.

Comment 10

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

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

Comment 12

[Jon Coppeard (:jonco)]

Attachment: Bug 1830921 - Discard JIT code when disabling or enabling the nursery r?jandem

This makes it so we always discard JIT code when the kinds of GC thing we can
allocate in a zone changes. Previously this happened in two separate places for
pretenuring, and didn't happen when we enabled/disabled the nursery.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Christian Holler (:decoder) from comment #0)

(and it didn't easily auto-reduce).

Hah, I'm surprised at this - it was really super straightforward for me for most or even all of them. The shell flags too!

In any case, I'm glad the reduced testcase in bug 1831232 helped Jon!

Comment 14

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

This bug prevents fuzzing from making progress; however, it has low severity. It is important for fuzz blocker bugs to be addressed in a timely manner (see here why?).
:jonco, could you consider increasing the severity?

For more information, please visit BugBot documentation.

Comment 15

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

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

Comment 16

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

Discard JIT code when disabling or enabling the nursery r=jandem
https://hg.mozilla.org/integration/autoland/rev/04005f8322707e3dc71ee5b2115ad42d5a958a8d
https://hg.mozilla.org/mozilla-central/rev/04005f832270

Comment 17

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230509151822-a5468e749653.

Comment 18

[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-firefox114 to wontfix.

For more information, please visit BugBot documentation.

Comment 19

[Jon Coppeard (:jonco)]

Comment on attachment 9331842 [details]
Bug 1830921 - Discard JIT code when disabling or enabling the nursery r?jandem

Beta/Release Uplift Approval Request

• User impact if declined: Possible crash / security vulnerability, although it's not clear whether this is possible to trigger in a release build.
• 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: Bug 1831072
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a simplification of the existing code that is more obviously correct. This has baked on central for 6 days.
• String changes made/needed:
• Is Android affected?: Yes

Comment 20

[Daniel Veditz [:dveditz]]

re-rating sec-moderate based on the uncertainty expressed in comment 19

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9331842 [details]
Bug 1830921 - Discard JIT code when disabling or enabling the nursery r?jandem

Approved for 114.0b7.

Comment 22

[Donal Meehan [:dmeehan]]

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