1918978 - Assertion failure: isDouble(), at dist/include/js/Value.h:1070

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

Attachment: debug stack

var x = new ArrayBuffer(1, {
  maxByteLength: 1,
});
function f() {
  new Uint8Array(x);
  new Uint8Array(x);
  oomTest(f);
}
f();
fullcompartmentchecks(true);

(gdb) bt
#0  JS::Value::toPrivate (this=0x121ad093e2e0) at /home/yksubu/shell-cache/js-dbg-64-linux-x86_64-a51b9d3e7251/objdir-js/dist/include/js/Value.h:1070
#1  js::ArrayBufferViewObject::initialByteOffsetValue (this=this@entry=0x121ad093e298)
    at /home/yksubu/trees/mozilla-central/js/src/vm/ArrayBufferViewObject.h:279
#2  0x00005555573d711e in js::ArrayBufferViewObject::dataPointerOffset (this=this@entry=0x121ad093e298)
    at /home/yksubu/trees/mozilla-central/js/src/vm/ArrayBufferViewObject.cpp:306
#3  0x00005555573d6cbb in js::ArrayBufferViewObject::trace (trc=<optimized out>, obj=<optimized out>)
    at /home/yksubu/trees/mozilla-central/js/src/vm/ArrayBufferViewObject.cpp:44
#4  0x00005555575398ce in JSClass::doTrace (trc=0x7fffffffcef8, obj=0x121ad093e298, this=<optimized out>)
    at /home/yksubu/shell-cache/js-dbg-64-linux-x86_64-a51b9d3e7251/objdir-js/dist/include/js/Class.h:660
#5  JSObject::traceChildren (this=0x121ad093e298, trc=0x7fffffffcef8) at /home/yksubu/trees/mozilla-central/js/src/vm/JSObject.cpp:3385
/snip

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/641a1d71cfa8
user:        Daniel Minor
date:        Wed Jun 05 16:11:18 2024 +0000
summary:     Bug 1884150 - Ship growable and resizeable ArrayBuffers; r=sfink

Run with --fuzzing-safe --no-threads --no-baseline --no-ion --no-ggc, 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 a51b9d3e7251.

Setting s-s just in case. I'm going to guess bug 1884150 merely flipped a switch, so hoping Jan can take a quick look as a start because of a scary stack.

I think I can go back further when I additionally use --enable-arraybuffer-resizable, I'll see what I can get.

Comment 1

[BugBot [:suhaib / :marco]]

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

Comment 2

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

I think I can go back further when I additionally use --enable-arraybuffer-resizable, I'll see what I can get.

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/bf6be80ff787
user:        André Bargull
date:        Tue Feb 27 13:27:46 2024 +0000
summary:     Bug 1842999 - Part 9: Change length and byteOffset tracking for easier JIT integration. r=jandem

Andre, would you possibly also be the right person to take a look? Please also feel free to tweak the "Regressed by" field.

Comment 3

[André Bargull [:anba]]

Bug 1842999 is the correct regressor.

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1918978: Initialise other slots first before calling fallible init. r=jandem!

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1918978: Add test case. r=jandem!

Comment 6

[André Bargull [:anba]]

I don't think this bug can lead to security-issues, because when we OOM during initialisation of an ArrayBufferView object, the object isn't yet accessible to user-code. And AFAICS even if ArrayBufferViewObject::trace sets the data-pointer to an invalid value through ArrayBufferViewObject::notifyBufferMoved, there aren't any callers from GC code which dereference the bogus data-pointer. (Code other than GC code can be ignored, because, as mentioned above, this issue only arises when there's an OOM during initialisation, so user-code can't get a reference to the ArrayBufferView object in the first place.)

Comment 7

[Jan de Mooij [:jandem]]

jonco and I discussed this and we agree it likely isn't security sensitive, but let's keep it hidden for now.

Comment 8

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #7)

jonco and I discussed this and we agree it likely isn't security sensitive, but let's keep it hidden for now.

Do you think it makes sense to backport to ESR?

Comment 9

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #8)

Do you think it makes sense to backport to ESR?

Yes please request beta/ESR uplift. It's a small and simple patch that should apply cleanly.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Too late to uplift to Beta for Fx131 at this point, but an ESR approval request would still be good.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this ready to land?

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1918978: Initialise other slots first before calling fallible init. r=jandem!

Original Revision: https://phabricator.services.mozilla.com/D222267

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3780b8ae3881
Initialise other slots first before calling fallible init. r=jandem

Comment 14

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Crashes when assertions are enabled. GC can read bogus values, but doesn't appear to use the bogus values, so there shouldn't be any immediate risk for users.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low risk
• Explanation of risk level: Low risk because only reorders initialisation of fixed slots.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/3780b8ae3881

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/7088e8b3253f

Comment 17

[Daniel Veditz [:dveditz]]

It looks like the sec-moderate was a mistake since it was added during a conversation where three people agreed this didn't have a security impact on users (comment 6 and comment 7)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f785d9ee965
Add test case. r=jandem

Comment 19

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/7f785d9ee965