1881417 - (CVE-2024-3860) Crash [@ js::NativeObject::setDenseInitializedLengthInternal]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: debug stack

for (let x = 0; x < 2; (function() { x++; })()) {};
function f() {
  var y = new (function () {})();
  (function () {
    Reflect.apply(y.toString, [], [0]);
  })();
}
f();
var z = [];
z.keepFailing = [];
oomTest(f, z);
dumpHeap();

(gdb) bt
#0  js::NativeObject::setDenseInitializedLengthInternal (this=0x8ce34b412f8, length=0) at /home/yksubu/trees/mozilla-central/js/src/vm/NativeObject.h:1477
#1  0x0000555557ee776a in js::NativeObject::setDenseInitializedLength (this=0x8ce34b412f8, length=0) at /home/yksubu/trees/mozilla-central/js/src/vm/NativeObject.h:1483
#2  js::jit::ShapeListObject::traceWeak (this=0x8ce34b412f8, trc=<optimized out>) at /home/yksubu/trees/mozilla-central/js/src/jit/BaselineCacheIRCompiler.cpp:2188
#3  0x000055555752369e in JSClass::doTrace (trc=0x7fffffffc780, obj=0x8ce34b412f8, this=<optimized out>) at /home/yksubu/shell-cache/js-dbg-64-linux-x86_64-f8dd4015fa59/objdir-js/dist/include/js/Class.h:653
#4  JSObject::traceChildren (this=0x8ce34b412f8, trc=0x7fffffffc780) at /home/yksubu/trees/mozilla-central/js/src/vm/JSObject.cpp:3343
#5  0x0000555557d687fb in JS::TraceChildren(JSTracer*, JS::GCCellPtr)::$_0::operator()<JSObject*>(JSObject*) const (t=0x8ce34b412f8, this=<optimized out>) at /home/yksubu/trees/mozilla-central/js/src/gc/Tracer.cpp:62
#6  JS::MapGCThingTyped<JS::TraceChildren(JSTracer*, JS::GCCellPtr)::$_0>(void*, JS::TraceKind, JS::TraceChildren(JSTracer*, JS::GCCellPtr)::$_0&&) (thing=0x8ce34b412f8, traceKind=<optimized out>, f=...) at /home/yksubu/shell-cache/js-dbg-64-linux-x86_64-f8dd4015fa59/objdir-js/dist/include/js/TraceKind.h:253
#7  JS::ApplyGCThingTyped<JS::TraceChildren(JSTracer*, JS::GCCellPtr)::$_0>(void*, JS::TraceKind, JS::TraceChildren(JSTracer*, JS::GCCellPtr)::$_0&&) (thing=0x8ce34b412f8, traceKind=<optimized out>, f=...) at /home/yksubu/shell-cache/js-dbg-64-linux-x86_64-f8dd4015fa59/objdir-js/dist/include/js/TraceKind.h:268
#8  JS::TraceChildren (trc=trc@entry=0x7fffffffc780, thing=...) at /home/yksubu/trees/mozilla-central/js/src/gc/Tracer.cpp:59
/snip

(Edit: see comment 2 for the real regressor)

Run with --fuzzing-safe --no-threads --ion-eager, compile with AR=ar sh ../configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev f8dd4015fa59.

Setting s-s to be safe. I'll set needinfo? from Jan as a start.

Comment 1

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

(Also not sure if this is a JIT or a GC issue, but I'll let Jan decide)

Comment 2

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

I've got something clearer now.

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9882c0d924b8
user:        Jon Coppeard
date:        Fri Jul 07 17:05:44 2023 +0000
summary:     Bug 1837620 - Part 6: Make edges for multiple shape guard weak too r=sfink

Jon, is bug 1837620 a likely regressor?

Comment 3

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

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

Comment 4

[Jon Coppeard (:jonco)]

I haven't had a change to look into this yet, but it probably depends on dumpHeap() touching otherwise unreachable objects.

Comment 5

[Jon Coppeard (:jonco)]

This crash requires dumping the heap. The testcase does this with the dumpHeap() function that's not present in release builds but it could probably also be done using the facilitles present in about:memory. I'm therefore marking this as sec-moderate.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1881417 - Skip tracing empty shape lists that may be present due to OOM r?jandem

The crash happens because we try to set the dense elements length of an object
that's using the empty elements header. We can skip tracing if there are no
elements.

Normally there we wouldn't create an empty shape list but this can happen due
to OOM as these are being initialized.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75e12d12d89c
Skip tracing empty shape lists that may be present due to OOM r=jandem

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/75e12d12d89c

Comment 9

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

For more information, please visit BugBot documentation.

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 9382249 [details]
Bug 1881417 - Skip tracing empty shape lists that may be present due to OOM 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 very simple change and has been on nightly for a week.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9382249 [details]
Bug 1881417 - Skip tracing empty shape lists that may be present due to OOM r?jandem

Approved for 124.0b8

Comment 12

[Pulsebot]

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

Comment 13

[Dianna Smith [:diannaS]]

Backing out of beta (for 124.0b8) for causing test failures

ReferenceError: oomTest is not defined
Failure log
Backout push

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 9382249 [details]
Bug 1881417 - Skip tracing empty shape lists that may be present due to OOM r?jandem

TBH this is not super important to uplift and the end of the train is close anyway. Cancelling uplift request.

Comment 15

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt