1896186 - Assertion failure: contents.isAligned(byteLength), at /builds/worker/checkouts/gecko/js/src/vm/ArrayBufferObject.h:634

Status: VERIFIED FIXED

(Core :: Graphics: WebGPU, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20240425-ec5140ea1b13 (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework --upgrade
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

Assertion failure: contents.isAligned(byteLength), at /builds/worker/checkouts/gecko/js/src/vm/ArrayBufferObject.h:634

#0 0x726308ab4959 in js::ArrayBufferObject::initialize(unsigned long, js::ArrayBufferObject::BufferContents) /builds/worker/checkouts/gecko/js/src/vm/ArrayBufferObject.h:634:5
#1 0x726308ab50f6 in js::ArrayBufferObject::createForContents(JSContext*, unsigned long, js::ArrayBufferObject::BufferContents) /builds/worker/checkouts/gecko/js/src/vm/ArrayBufferObject.cpp:1855:11
#2 0x726308abb6b6 in JS::NewExternalArrayBuffer(JSContext*, unsigned long, mozilla::UniquePtr<void, JS::BufferContentsDeleter>) /builds/worker/checkouts/gecko/js/src/vm/ArrayBufferObject.cpp:3091:7
#3 0x72630489c34b in mozilla::webgpu::Buffer::GetMappedRange(JSContext*, unsigned long, mozilla::dom::Optional<unsigned long> const&, JS::Rooted<JSObject*>*, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/webgpu/Buffer.cpp:298:12
#4 0x726303dd3dfd in mozilla::dom::GPUBuffer_Binding::getMappedRange(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/./WebGPUBinding.cpp:14179:24
#5 0x7263043d7e47 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3268:13
#6 0x7263089da564 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:480:13
#7 0x7263089d9e7d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:574:12
#8 0x7263089e9b7d in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:646:10
#9 0x7263089e9b7d in js::Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3071:16
#10 0x7263089d9442 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:452:13
#11 0x7263089d9e99 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:606:13
#12 0x7263089db347 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:673:8
#13 0x726308d8d007 in js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/SelfHosting.cpp:1592:10
#14 0x726309755366 in js::jit::InterpretResume(JSContext*, JS::Handle<JSObject*>, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jit/VMFunctions.cpp:1139:10
#15 0x1a6e226bc8d9  ([anon:js-executable-memory]+0x118d9)

Comment 1

[Bugmon [:jkratzer for issues]]

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

Start: fd7d96cffddb9a76a1ecc148c3b284e8003fe6b9 (20240424075615)
End: e0fbdb526ceb24d82b3b8997b5de98935626c6a2 (20240424092214)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fd7d96cffddb9a76a1ecc148c3b284e8003fe6b9&tochange=e0fbdb526ceb24d82b3b8997b5de98935626c6a2

Comment 2

[Donal Meehan [:dmeehan]]

Bug 1891195 from the pushlog in Comment 1 looks likely. Please correct if needed.

Comment 3

[Lee Salzman [:lsalzman]]

Seems like an interaction between bug 1891195 and WebGPU's usage of changes therein.

Comment 4

[André Bargull [:anba]]

JS::NewExternalArrayBuffer should be passed aligned memory, because TypedArrays expect their underlying buffer points to aligned memory. Can we change the WebGPU code to only pass aligned memory?

Comment 5

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

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

Comment 6

[Haik Aftandilian [:haik]]

@Andre, did you mean to needinfo someone in particular with your question on comment 4?

Comment 7

[André Bargull [:anba]]

I don't know who can take a closer look at this code. It looks like the code dates back to bug 1622846, but I :kvark is no longer available.

The getMappedRange spec requires offset to be a multiple of eight. The test case from this bug has offset = 13763, which is clearly not a multiple of eight. Is this a bug in our implementation? Fixing this could be enough to resolve this bug.

Comment 8

[Daniel Holbert [:dholbert]]

Followup from regression triage; we forgot why this was marked as disabled for 127; after looking closer, it seems this doesn't affect any builds beyond Nightly since WebGPU is disabled-by-default beyond Nightly builds. (behind dom.webgpu.enabled, which is only on for Nightly since bug 1746245.

Comment 9

[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?).
:jimb, could you consider increasing the severity?

For more information, please visit BugBot documentation.

Comment 10

[Jim Blandy :jimb]

We're simply not applying WebGPU's own validation to getMappedRanges arguments. Fixing that would address ArrayBufferObjects concerns.

Comment 11

[Jim Blandy :jimb]

Attachment: WIP: Bug 1896186: Properly validate arguments to `GPUBuffer.getMappedRange`. r!#webgpu-reviewers

Align validation in dom::webgpu::Buffer::GetMappedRange to
correspond with that given in the WebGPU specification. Change
variables to more closely resemble the terms used in the
specification.

Change dom::webgpu::Buffer::mArrayBuffers to an nsTArray of a new
struct MappedView, which carries enough information to properly
validate requests for mapped ranges. Update all uses.

Add a new mochitest, test_buffer_mapping_overlapping_views.html.

Comment 12

[Jim Blandy :jimb]

Attachment: Bug 1896186: Properly validate arguments to `GPUBuffer.getMappedRange`. r?#webgpu-reviewers!

Align validation in dom::webgpu::Buffer::GetMappedRange to
correspond with that given in the WebGPU specification. Change
variables to more closely resemble the terms used in the
specification.

Change dom::webgpu::Buffer::mArrayBuffers to an nsTArray of a new
struct MappedView, which carries enough information to properly
validate requests for mapped ranges. Update all uses.

Add a new mochitest, test_buffer_mapping_overlapping_views.html.

Comment 13

[Jim Blandy :jimb]

My patch fails to build on CI (but not locally, grr):

In file included from RegisterWorkerBindings.cpp:1:
In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/AbortControllerBinding.h:9:
In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/BindingDeclarations.h:25:
/builds/worker/workspace/obj-build/dist/include/nsTArray.h:808:34: error: Cannot instantiate 'nsTArray_RelocationStrategy<mozilla::webgpu::MappedView>' with non-memmovable template argument 'mozilla::webgpu::MappedView'
  808 | struct MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray_RelocationStrategy {
      |                                  ^
/builds/worker/workspace/obj-build/dist/include/nsTArray.h:996:37: note: instantiation of 'nsTArray_RelocationStrategy<mozilla::webgpu::MappedView>' requested here
  996 |                            typename nsTArray_RelocationStrategy<E>::Type>,
      |                                     ^
/builds/worker/workspace/obj-build/dist/include/mozilla/webgpu/Buffer.h:36:23: note: 'mozilla::webgpu::MappedView' is a non-memmove()able type because member 'mArrayBuffer' is a non-memmove()able type 'JS::Heap<JSObject *>'
   36 |   JS::Heap<JSObject*> mArrayBuffer;
      |                       ^
1 error generated.
gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:676: RegisterWorkerBindings.o] Error 1

Comment 14

[Jim Blandy :jimb]

I've described the fix in a comment I'm adding, but I'm getting strange build failures:

Give nsTArray some advice on how to handle MappedInfo::mViews.

JS::Heap is marked MOZ_NON_MEMMOVABLE, meaning that it cannot
be safely relocated using memmove. nsTArray.h specializes
nsTArray_RelocationStrategy to indicate that JS::Heap can be
moved using its move constructor.

Since MappedView::mArrayBuffer is a JS::Heap, it is
automatically MOZ_NON_MEMMOVABLE. Since MappedInfo::mViews
holds an nsTArray<MappedView>, we must spell out MappedView's
relocation strategy as well.

Comment 15

[Jim Blandy :jimb]

Patch revised. New try push.

Comment 16

[Jim Blandy :jimb]

New try push.

Comment 17

[Erich Gubler [:ErichDonGubler]]

:jimb: Looks like that new Try push fixed the remaining build issues, but persists with tier 2 failures in WebGPU CTS across all platforms. I'm guessing determining the root cause is in scope for this bug?

Comment 18

[Jim Blandy :jimb]

New try push

Comment 19

[Jim Blandy :jimb]

Looks like that new Try push fixed the remaining build issues, but persists with tier 2 failures in WebGPU CTS across all platforms. I'm guessing determining the root cause is in scope for this bug?

Ideally we should always be doing this.

Comment 20

[Jim Blandy :jimb]

Most of the Tier 3 oranges are "expected timeout, got pass", which I don't think merits updating the expectations. I would think there would be some unexpected passes in api/operation/buffers/map and api/validation/buffer/mapping.

Comment 21

[Jim Blandy :jimb]

That last try push looks good for tiers 1 and 2, so once the patch can get a re-review (requested in chat), we can land this.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47b0daca7135
Properly validate arguments to `GPUBuffer.getMappedRange`. r=webgpu-reviewers,nical,ErichDonGubler

Comment 23

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/47b0daca7135

Comment 24

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20240703213305-6b21071006a1.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.