1847397 - (CVE-2023-4577) Assertion failure: kind == JS::TracerKind::Tenuring || kind == JS::TracerKind::MinorSweeping || kind == JS::TracerKind::Moving, at gc/Marking.cpp:137

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

Steps to reproduce:

On git commit 0816653c3ab851fa0e362eaec48c643fb764eaf4 the attached (flaky) sample crashes with one of two assertion violation.
I observed js::Mutex::assertOwnedByCurrentThread at js/src/threading/Mutex.cpp:73 when reproducing with rr --chaos and
Assertion failure: kind == JS::TracerKind::Tenuring || kind == JS::TracerKind::MinorSweeping || kind == JS::TracerKind::Moving, at gc/Marking.cpp:137 when running without an attached debugger.
The sample is invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --baseline-warmup-threshold=10 --ion-warmup-threshold=100 --fuzzing-safe --small-function-length=512 --inlining-entry-threshold=16 --gc-zeal=10 crash.js
The crash is quite flaky; it takes a couple of dozens of execution until the crash manifests. I failed to further minimize the sample, sorry about that.
Furthermore, I only observed the crash in an optimized build; the mozconfig file is attached.
If you cannot reproduce the crash locally I'll upload a pernosco session.

#0  js::Mutex::assertOwnedByCurrentThread (this=0x551e58e26c68)
    at js/src/threading/Mutex.cpp:73
#1  0x000056358f6a4536 in js::gc::StoreBuffer::unput<js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::CellPtrEdge<JSString> >, js::gc::StoreBuffer::CellPtrEdge<JSString> > (buffer=...,
    edge=..., this=<optimized out>) at js/src/gc/StoreBuffer.h:409
#2  js::gc::StoreBuffer::unputCell (strp=0x551e58e539e8, this=<optimized out>)
    at js/src/gc/StoreBuffer.h:481
#3  js::gc::PostWriteBarrierImpl<JSString> (cellp=<optimized out>, prev=<optimized out>,
    next=<optimized out>) at js/src/gc/StoreBuffer.h:636
#4  js::gc::PostWriteBarrier<JSString> (vp=<optimized out>, prev=<optimized out>,
    next=<optimized out>) at js/src/gc/StoreBuffer.h:647
#5  js::InternalBarrierMethods<JSString*, void>::postBarrier (vp=<optimized out>,
    prev=<optimized out>, next=<optimized out>)
    at js/src/gc/Barrier.h:349
#6  js::WriteBarriered<JSString*>::post (this=<optimized out>, prev=<optimized out>,
    next=<optimized out>) at js/src/gc/Barrier.h:509
#7  js::HeapPtr<JSString*>::~HeapPtr (this=0x551e58e539e8)
    at js/src/gc/Barrier.h:703
#8  js::RegExpStatics::~RegExpStatics (this=0x551e58e53940)
    at js/src/vm/RegExpStatics.h:17
#9  0x000056358f67ea2d in js_delete<js::RegExpStatics> (p=0x551e58e53940)
    at obj-x86_64-pc-linux-gnu/dist/include/js/Utility.h:566
#10 JS::DeletePolicy<js::RegExpStatics>::operator() (this=<optimized out>, ptr=0x551e58e53940)
    at obj-x86_64-pc-linux-gnu/dist/include/js/Utility.h:639
#11 mozilla::UniquePtr<js::RegExpStatics, JS::DeletePolicy<js::RegExpStatics> >::reset (
    this=0x551e58ec3ed0, aPtr=0x0)
    at obj-x86_64-pc-linux-gnu/dist/include/mozilla/UniquePtr.h:301
#12 mozilla::UniquePtr<js::RegExpStatics, JS::DeletePolicy<js::RegExpStatics> >::~UniquePtr (
    this=0x551e58ec3ed0)
    at obj-x86_64-pc-linux-gnu/dist/include/mozilla/UniquePtr.h:249
#13 js::GlobalObjectData::~GlobalObjectData (this=0x551e58ec3800)
    at js/src/vm/GlobalObject.cpp:1002
#14 JS::GCContext::delete_<js::GlobalObjectData> (this=0x551e58e23740, cell=0x1d29829ab240, 
    p=0x551e58ec3800, nbytes=1808, use=js::MemoryUse::GlobalObjectData)
    at js/src/gc/GCContext.h:163
#15 JS::GCContext::delete_<js::GlobalObjectData> (this=0x551e58e23740, cell=0x1d29829ab240,
    p=0x551e58ec3800, use=js::MemoryUse::GlobalObjectData)
    at js/src/gc/GCContext.h:151
#16 js::GlobalObject::releaseData (this=0x1d29829ab240, gcx=0x551e58e23740)
    at js/src/vm/GlobalObject.cpp:997
#17 0x000056358fecc1ef in js::gc::GCRuntime::sweepRealmGlobals (this=0x551e58e23728)
    at js/src/gc/Sweeping.cpp:1233
#18 js::gc::GCRuntime::beginSweepingSweepGroup (this=0x551e58e23728, gcx=0x551e58e23740,
    budget=...) at js/src/gc/Sweeping.cpp:1570
#19 0x000056358fef1481 in sweepaction::SweepActionSequence::run (this=0x551e58e06970, args=...)
    at js/src/gc/Sweeping.cpp:2165
#20 0x000056358feeca00 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run (this=0x551e58e1a100, args=...) at js/src/gc/Sweeping.cpp:2200
#21 0x000056358fed4cda in js::gc::GCRuntime::performSweepActions (this=0x551e58e23728,
    budget=...) at js/src/gc/Sweeping.cpp:2342
#22 0x000056358fe13dd2 in js::gc::GCRuntime::incrementalSlice (this=0x551e58e23728, budget=...,
    reason=JS::GCReason::DESTROY_RUNTIME, budgetWasIncreased=<optimized out>)
    at js/src/gc/GC.cpp:3670
#23 0x000056358fe19bfa in js::gc::GCRuntime::gcCycle (this=0x551e58e23728,
    nonincrementalByAPI=<optimized out>, budgetArg=..., reason=JS::GCReason::DESTROY_RUNTIME)
    at js/src/gc/GC.cpp:4181
#24 0x000056358fe1b670 in js::gc::GCRuntime::collect (this=0x551e58e23728,
    nonincrementalByAPI=<optimized out>, budget=..., reason=JS::GCReason::DESTROY_RUNTIME)
    at js/src/gc/GC.cpp:4372
#25 0x000056358fde06aa in js::gc::GCRuntime::gc (this=0x551e58e23728,
    options=JS::GCOptions::Shutdown, reason=JS::GCReason::DESTROY_RUNTIME)
    at js/src/gc/GC.cpp:4449
#26 0x000056358f846848 in JSRuntime::destroyRuntime (this=0x551e58e23000)
    at js/src/vm/Runtime.cpp:266
#27 0x000056358f6c4a91 in js::DestroyContext (cx=0x551e58e31500)
    at js/src/vm/JSContext.cpp:222
#28 0x000056358f3b01d4 in JS_DestroyContext (cx=0x551e58e31500)
    at js/src/jsapi.cpp:405
#29 main::$_2::operator() (this=<optimized out>)
    at js/src/shell/js.cpp:11283
#30 mozilla::ScopeExit<main::$_2>::~ScopeExit (this=<optimized out>)
    at obj-x86_64-pc-linux-gnu/dist/include/mozilla/ScopeExit.h:106
#31 main (argc=<optimized out>, argv=<optimized out>)
    at js/src/shell/js.cpp:11404

Comment 1

[lukas.bernhard]

Attachment: crash.js

Comment 2

[lukas.bernhard]

Attachment: mozconfig

Comment 3

[lukas.bernhard]

The sample might also here:

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 2883843.2883848]
js::gc::detail::CellHasStoreBuffer (cell=<optimized out>)
    at obj-x86_64-pc-linux-gnu/dist/include/js/HeapAPI.h:594
594	 return GetCellChunkBase(cell)->storeBuffer;

https://pernos.co/debug/mIhCOarsUcZqpgz3sIe14Q/index.html#f{m[KTk,AA_,t[Bg,LAEI_,f{e[KTM,Qw39_,s{aVb1CwPAA,bBQ,uBH0m0g,oBIA1vA___/

Comment 4

[Steven DeTar [:sdetar]]

:jonco, would you have time to take a quick look this bug?

Comment 5

[Jon Coppeard (:jonco)]

This looks like a missing postbarrier. I think what's going wrong is that we're using a stale version of initialStringHeap in UpdateRegExpStatics when writing pendingInputAddress.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1847397 - Get the string heap to use after possible GC when executing regular expressions (Beta) r?jandem

The problem here is that initialStringHeap can be stale by the time it's used
in UpdateRegExpStatics. GenerateRegExpMatchStubShared can allocate a shape
which can trigger GC which can re-enable previously disabled nursery string
allocation. This means we may incorrectly elide post barriers here.

The fix is to get the initial heap setting after potential GC. This also adds
an assertion that we don't see nursery GC things when we're eliding barriers.

I don't know how to make a reliable test case for this.

Comment 7

[Jon Coppeard (:jonco)]

This issue no longer affects central (it was fixed by bug 1847017), but is present on all other supported branches.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1847397 - Check no post barrier needed when eliding it in UpdateRegExpStatics r?jandem

The original issue was fixed bug 1847017. This patch adds some checks to ensure
things are working correctly.

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 9348979 [details]
Bug 1847397 - Check no post barrier needed when eliding it in UpdateRegExpStatics r?jandem

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very hard.
• 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?: All supported branches
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: The separate patch for beta should apply to other branches too. Note this patch contains a fix for this issue too (which has been fixed by other changes on central).
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. This patch adds assertions/checks only.
• Is Android affected?: Yes

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9348979 [details]
Bug 1847397 - Check no post barrier needed when eliding it in UpdateRegExpStatics r?jandem

Approved to land and uplift

Comment 11

[Ryan VanderMeulen [:RyanVM]]

The attached patch applies cleanly to Beta and ESR115, but ESR102 will need a rebased patch still.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5224ceefd4c
Check no post barrier needed when eliding it in UpdateRegExpStatics r=jandem

Comment 13

[Jon Coppeard (:jonco)]

On further investigation, this doesn't affect esr102.

It seems this was introduced in version 115 by bug 1832558. This created the static function GenerateRegExpMatchStubShared which takes the string to use as a parameter. Previously this would have been read from the JitRealm as needed and so would not be stale when used.

Comment 14

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

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR115.

Comment 16

[Jon Coppeard (:jonco)]

Comment on attachment 9348591 [details]
Bug 1847397 - Get the string heap to use after possible GC when executing regular expressions (Beta) r?jandem

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 simple change to get the heap to allocate in later, to prevent it being stale when used. An assertion ensures the value is valid for its lifetime.
• String changes made/needed:
• Is Android affected?: Yes

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9348591 [details]
Bug 1847397 - Get the string heap to use after possible GC when executing regular expressions (Beta) r?jandem

Approved for 117.0b9 and 115.2esr.

Comment 18

[Pulsebot]

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

Comment 19

[Pulsebot]

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

Comment 20

[June Wilde (she/her) [:jewilde]]

Attachment: advisory.txt

Comment 21

[Daniel Veditz [:dveditz]]

Although we had already planned to do the work in bug 1847017, it was not a "security bug". Without this report folks on ESR 115 would have remained vulnerable.