1858678 - Assertion failure: !buffer->isPreparedForAsmJS(), at vm/ArrayBufferObject.cpp:1960

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20231009-d5fd5e481ff2 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

function foo(glob, imp, b) {
  "use asm";
  var arr=new glob.Int8Array(b);
  return {};
}
a = new ArrayBuffer(64 * 1024);
foo(this, null, a);
function f(h, g) {
  ensureNonInline(g);
}
f(Float64Array, a);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555576d1c68 in js::ArrayBufferObject::ensureNonInline(JSContext*, JS::Handle<js::ArrayBufferObject*>) ()
#1  0x000055555717747c in JS::EnsureNonInlineArrayBufferOrView(JSContext*, JSObject*) ()
#2  0x00005555575c13dc in EnsureNonInline(JSContext*, unsigned int, JS::Value*) ()
#3  0x0000555557068575 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#14 0x0000555556e90bd9 in main ()
rax	0x5555558ca337	93824995861303
rbx	0x7fffffffcfa8	140737488342952
rcx	0x55555897a5b8	93825046914488
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffcf80	140737488342912
rsp	0x7fffffffced0	140737488342736
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f92840	140737353689152
r10	0x2	2
r11	0x0	0
r12	0xfff8800000000000	-2111062325329920
r13	0xfff8800000000000	-2111062325329920
r14	0xe0588067040	15416919748672
r15	0x7ffff3d35600	140737284101632
rip	0x5555576d1c68 <js::ArrayBufferObject::ensureNonInline(JSContext*, JS::Handle<js::ArrayBufferObject*>)+1176>
=> 0x5555576d1c68 <_ZN2js17ArrayBufferObject15ensureNonInlineEP9JSContextN2JS6HandleIPS0_EE+1176>:	movl   $0x7a8,0x0
   0x5555576d1c73 <_ZN2js17ArrayBufferObject15ensureNonInlineEP9JSContextN2JS6HandleIPS0_EE+1187>:	callq  0x555556f31cc0 <abort>

Marking s-s until triaged since I can't tell if this is an issue with the helper function or not.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1858678 using build mozilla-central 20231009093538-d5fd5e481ff2. 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 4

[Iain Ireland [:iain]]

Steve, can you take a quick look at this?

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

I did some rummaging around this bug. It seems like there's certainly something being highlighted here. We have a pre-existing invariant that an AsmJS used array buffer cannot be detached, which is what the assert is trying to avoid.

Normally, I'd fix this by just modifying ArrayBufferObject::ensureNonInline to throw if we had a detach key set... The problem however is that the only non-test caller of JS::EnsureNonInlineArrayBufferOrView can't actually handle failures, and is actively being investigated for some odd crash behaviour (Bug 1856672).

I'll have to leave final determination here to Steve, as this may be dependent of PeterV figuring out how to handle errors in dom::TypedArray_base<T>::ProcessFixedData.

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Oh: And I'm not 100% sure of the implications security wise-- it seems like any buffer that can make it to ProcessFixedData may have its invariants potentially violated, which seems... suboptimal.

Comment 7

[Steve Fink [:sfink] [:s:]]

Hm, my original expectation here is that ProcessFixedData would never be called on such a buffer, but did not reflect that in either the documentation or the test function. (As in, the test function that crashes here.)

But in looking at it now, I'm thinking that the right fix is just to silently succeed: ensureNonInline(asm-prepared ArrayBuffer) is a safe no-op, since asm.js already forces the data to be out of line. And non-detachable, for that matter.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1858678 - AsmJS ArrayBuffers are happy ArrayBuffers. Happy ArrayBuffers are already out-of-line.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8af4873509e1
AsmJS ArrayBuffers are happy ArrayBuffers. Happy ArrayBuffers are already out-of-line. r=mgaudet

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/8af4873509e1

Comment 11

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

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 12

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

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

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

For more information, please visit BugBot documentation.

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9366631 [details]
Bug 1858678 - AsmJS ArrayBuffers are happy ArrayBuffers. Happy ArrayBuffers are already out-of-line.

Beta/Release Uplift Approval Request

• User impact if declined: None expected. I don't expect asm.js buffers to reach relevant code in real-world cases. The reason for landing would be fuzzing—it seems like this is not hard to hit with the test function.
• 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 just removing an unnecessary assertion.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9366631 [details]
Bug 1858678 - AsmJS ArrayBuffers are happy ArrayBuffers. Happy ArrayBuffers are already out-of-line.

Approved for 121.0b9.

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/7b8fe08a599a

Comment 16

[Jason Kratzer [:jkratzer]]

NI'ing myself to investigate why bugmon was unable to reproduce this issue.

Comment 17

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20231205090844-775ade8b04da.