Bug 1896186: Properly validate arguments to `GPUBuffer.getMappedRange`. r=webgpu-reviewers,nical,ErichDonGubler

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. Provide an
`nsTArray_RelocationStrategy` specialization for `MappedView`.

Add a new mochitest, `test_buffer_mapping_overlapping_views.html`.

Differential Revision: https://phabricator.services.mozilla.com/D215442

dom/webgpu/Buffer.cpp

@@ -34,9 +34,9 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(Buffer)
   NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
   if (tmp->mMapped) {
-    for (uint32_t i = 0; i < tmp->mMapped->mArrayBuffers.Length(); ++i) {
+    for (uint32_t i = 0; i < tmp->mMapped->mViews.Length(); ++i) {
       NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(
-          mMapped->mArrayBuffers[i])
+          mMapped->mViews[i].mArrayBuffer)
     }
   }
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
@@ -144,7 +144,7 @@ void Buffer::Cleanup() {
 
   AbortMapRequest();
 
-  if (mMapped && !mMapped->mArrayBuffers.IsEmpty()) {
+  if (mMapped && !mMapped->mViews.IsEmpty()) {
     // The array buffers could live longer than us and our shmem, so make sure
     // we clear the external buffer bindings.
     dom::AutoJSAPI jsapi;
@@ -268,55 +268,132 @@ static void ExternalBufferFreeCallback(void* aContents, void* aUserData) {
 void Buffer::GetMappedRange(JSContext* aCx, uint64_t aOffset,
                             const dom::Optional<uint64_t>& aSize,
                             JS::Rooted<JSObject*>* aObject, ErrorResult& aRv) {
+  // The WebGPU spec spells out the validation we must perform, but
+  // use `CheckedInt<uint64_t>` anyway to catch our mistakes. Except
+  // where we explicitly say otherwise, invalid `CheckedInt` values
+  // should only arise when we have a bug, so just calling
+  // `CheckedInt::value` where needed should be fine (it checks with
+  // `MOZ_DIAGNOSTIC_ASSERT`).
+
+  // https://gpuweb.github.io/gpuweb/#dom-gpubuffer-getmappedrange
+  //
+  // Content timeline steps:
+  //
+  // 1. If `size` is missing:
+  //    1. Let `rangeSize` be `max(0, this.size - offset)`.
+  //    Otherwise, let `rangeSize` be `size`.
+  const auto offset = CheckedInt<uint64_t>(aOffset);
+  CheckedInt<uint64_t> rangeSize;
+  if (aSize.WasPassed()) {
+    rangeSize = aSize.Value();
+  } else {
+    const auto bufferSize = CheckedInt<uint64_t>(mSize);
+    // Use `CheckInt`'s underflow detection for `max(0, ...)`.
+    rangeSize = bufferSize - offset;
+    if (!rangeSize.isValid()) {
+      rangeSize = 0;
+    }
+  }
+
+  // 2. If any of the following conditions are unsatisfied, throw an
+  //    `OperationError` and stop.
+  //
+  //     - `this.[[mapping]]` is not `null`.
   if (!mMapped) {
-    aRv.ThrowInvalidStateError("Buffer is not mapped");
+    aRv.ThrowOperationError("Buffer is not mapped");
     return;
   }
 
-  const auto checkedOffset = CheckedInt<size_t>(aOffset);
-  const auto checkedSize = aSize.WasPassed()
-                               ? CheckedInt<size_t>(aSize.Value())
-                               : CheckedInt<size_t>(mSize) - aOffset;
-  const auto checkedMinBufferSize = checkedOffset + checkedSize;
-
-  if (!checkedOffset.isValid() || !checkedSize.isValid() ||
-      !checkedMinBufferSize.isValid() || aOffset < mMapped->mOffset ||
-      checkedMinBufferSize.value() > mMapped->mOffset + mMapped->mSize) {
-    aRv.ThrowRangeError("Invalid range");
+  //     - `offset` is a multiple of 8.
+  //
+  // (`operator!=` is not available on `CheckedInt`.)
+  if (offset.value() % 8 != 0) {
+    aRv.ThrowOperationError("GetMappedRange offset is not a multiple of 8");
     return;
   }
 
-  auto offset = checkedOffset.value();
-  auto size = checkedSize.value();
-  auto span = mShmem->Bytes().Subspan(offset, size);
+  //     - `rangeSize` is a multiple of `4`.
+  if (rangeSize.value() % 4 != 0) {
+    aRv.ThrowOperationError("GetMappedRange size is not a multiple of 4");
+    return;
+  }
 
-  std::shared_ptr<ipc::WritableSharedMemoryMapping>* userData =
+  //     - `offset ≥ this.[[mapping]].range[0]`.
+  if (offset.value() < mMapped->mOffset) {
+    aRv.ThrowOperationError(
+        "GetMappedRange offset starts before buffer's mapped range");
+    return;
+  }
+
+  //     - `offset + rangeSize ≤ this.[[mapping]].range[1]`.
+  //
+  // Perform the addition in `CheckedInt`, treating overflow as a validation
+  // error.
+  const auto rangeEndChecked = offset + rangeSize;
+  if (!rangeEndChecked.isValid() ||
+      rangeEndChecked.value() > mMapped->mOffset + mMapped->mSize) {
+    aRv.ThrowOperationError(
+        "GetMappedRange range extends beyond buffer's mapped range");
+    return;
+  }
+
+  //     - `[offset, offset + rangeSize)` does not overlap another range
+  //       in `this.[[mapping]].views`.
+  const uint64_t rangeEnd = rangeEndChecked.value();
+  for (const auto& view : mMapped->mViews) {
+    if (view.mOffset < rangeEnd && offset.value() < view.mRangeEnd) {
+      aRv.ThrowOperationError(
+          "GetMappedRange range overlaps with existing buffer view");
+      return;
+    }
+  }
+
+  // 3. Let `data` be `this.[[mapping]].data`.
+  //
+  // The creation of a *pointer to* a `shared_ptr` here seems redundant but is
+  // unfortunately necessary: `JS::BufferContentsDeleter` requires that its
+  // `userData` be a `void*`, and while `shared_ptr` can't be inter-converted
+  // with `void*` (it's actually two pointers), `shared_ptr*` obviously can.
+  std::shared_ptr<ipc::WritableSharedMemoryMapping>* data =
       new std::shared_ptr<ipc::WritableSharedMemoryMapping>(mShmem);
-  UniquePtr<void, JS::BufferContentsDeleter> dataPtr{
-      span.data(), {&ExternalBufferFreeCallback, userData}};
-  JS::Rooted<JSObject*> arrayBuffer(
-      aCx, JS::NewExternalArrayBuffer(aCx, size, std::move(dataPtr)));
-  if (!arrayBuffer) {
+
+  // 4. Let `view` be (potentially fallible operation follows) create an
+  //    `ArrayBuffer` of size `rangeSize`, but with its pointer mutably
+  //    referencing the content of `data` at offset `(offset -
+  //    [[mapping]].range[0])`.
+  //
+  // Since `size_t` may not be the same as `uint64_t`, check, convert, and check
+  // again. `CheckedInt<size_t>(x)` produces an invalid value if `x` is not in
+  // range for `size_t` before any conversion is performed.
+  const auto checkedSize = CheckedInt<size_t>(rangeSize.value()).value();
+  const auto checkedOffset = CheckedInt<size_t>(offset.value()).value();
+  const auto span = (*data)->Bytes().Subspan(checkedOffset, checkedSize);
+  UniquePtr<void, JS::BufferContentsDeleter> contents{
+      span.data(), {&ExternalBufferFreeCallback, data}};
+  JS::Rooted<JSObject*> view(
+      aCx, JS::NewExternalArrayBuffer(aCx, checkedSize, std::move(contents)));
+  if (!view) {
     aRv.NoteJSContextException(aCx);
     return;
   }
 
-  aObject->set(arrayBuffer);
-  mMapped->mArrayBuffers.AppendElement(*aObject);
+  aObject->set(view);
+  mMapped->mViews.AppendElement(
+      MappedView({checkedOffset, rangeEnd, *aObject}));
 }
 
 void Buffer::UnmapArrayBuffers(JSContext* aCx, ErrorResult& aRv) {
   MOZ_ASSERT(mMapped);
 
   bool detachedArrayBuffers = true;
-  for (const auto& arrayBuffer : mMapped->mArrayBuffers) {
-    JS::Rooted<JSObject*> rooted(aCx, arrayBuffer);
+  for (const auto& view : mMapped->mViews) {
+    JS::Rooted<JSObject*> rooted(aCx, view.mArrayBuffer);
     if (!JS::DetachArrayBuffer(aCx, rooted)) {
       detachedArrayBuffers = false;
     }
   };
 
-  mMapped->mArrayBuffers.Clear();
+  mMapped->mViews.Clear();
 
   AbortMapRequest();
 

dom/webgpu/Buffer.h

@@ -14,6 +14,40 @@
 #include "mozilla/ipc/RawShmem.h"
 #include <memory>
 
+namespace mozilla {
+namespace webgpu {
+struct MappedView;
+}  // namespace webgpu
+}  // namespace mozilla
+
+// Give `nsTArray` some advice on how to handle `MappedInfo::mViews`.
+//
+// In the `mozilla::webgpu` namespace, `MappedInfo::mViews` is an
+// `nsTArray<MappedView>`, and `MappedView::mArrayBuffer` is a `JS::Heap`
+// pointer. This arrangement requires special handling.
+//
+// Normally, `nsTArray` wants its element type to be movable with simple byte
+// copies, so that an `nsTArray` can efficiently resize its element buffer.
+// However, `JS::Heap` is marked `MOZ_NON_MEMMOVABLE`, meaning that it cannot be
+// safely moved by a simple byte-by-byte copy. Normally, this would cause
+// `nsTArray` to reject `JS::Heap` as an element type, but `nsTArray.h`
+// specializes `nsTArray_RelocationStrategy` to indicate that `JS::Heap` can be
+// moved safely using its move constructor. This causes `nsTArray<JS::Heap<T>>`
+// to perform element buffer moves using element-by-element move constructor
+// application: slower, but safe for `JS::Heap`.
+//
+// However, while `MappedView` is automatically marked `MOZ_NON_MEMMOVABLE`
+// because of its `mArrayBuffer` member, the `nsTArray_RelocationStrategy`
+// specialization is not somehow similarly magically carried over from
+// `JS::Heap` to `MappedView`. To use `MappedView` in `nsTArray`, we must spell
+// out a relocation strategy for it.
+template <>
+struct nsTArray_RelocationStrategy<mozilla::webgpu::MappedView> {
+  // The default move constructors are fine for MappedView.
+  using Type =
+      nsTArray_RelocateUsingMoveConstructor<mozilla::webgpu::MappedView>;
+};
+
 namespace mozilla {
 class ErrorResult;
 
@@ -28,11 +62,23 @@ namespace webgpu {
 
 class Device;
 
+// A portion of the current mapped buffer range that is currently
+// visible to JS as an ArrayBuffer.
+struct MappedView {
+  BufferAddress mOffset;
+  BufferAddress mRangeEnd;
+  JS::Heap<JSObject*> mArrayBuffer;
+
+  MappedView(BufferAddress aOffset, BufferAddress aRangeEnd,
+             JSObject* aArrayBuffer)
+      : mOffset(aOffset), mRangeEnd(aRangeEnd), mArrayBuffer(aArrayBuffer) {}
+};
+
 struct MappedInfo {
   // True if mapping is requested for writing.
   bool mWritable = false;
   // Populated by `GetMappedRange`.
-  nsTArray<JS::Heap<JSObject*>> mArrayBuffers;
+  nsTArray<MappedView> mViews;
   BufferAddress mOffset;
   BufferAddress mSize;
   MappedInfo() = default;
@@ -84,8 +130,22 @@ class Buffer final : public ObjectBase, public ChildOf<Device> {
   // Information about the currently active mapping.
   Maybe<MappedInfo> mMapped;
   RefPtr<dom::Promise> mMapRequest;
-  // mShmem does not point to a shared memory segment if the buffer is not
-  // mappable.
+
+  // A shared memory mapping for the entire buffer, or a zero-length
+  // mapping.
+  //
+  // If `mUsage` contains `MAP_READ` or `MAP_WRITE`, this mapping is
+  // created at `Buffer` construction, and destroyed at `Buffer`
+  // destruction.
+  //
+  // If `mUsage` contains neither of those flags, but `this` is mapped
+  // at creation, this mapping is created at `Buffer` construction,
+  // and destroyed when we first unmap the buffer, by clearing this
+  // `shared_ptr`.
+  //
+  // Otherwise, this points to `WritableSharedMemoryMapping()` (the
+  // default constructor), a zero-length mapping that doesn't point to
+  // any shared memory.
   std::shared_ptr<ipc::WritableSharedMemoryMapping> mShmem;
 };
 

dom/webgpu/mochitest/mochitest.toml

@@ -24,6 +24,8 @@ scheme = "https"
 
 ["test_buffer_mapping_invalid_device.html"]
 
+["test_buffer_mapping_overlapping_views.html"]
+
 ["test_command_buffer_creation.html"]
 
 ["test_compilation_message_pos.html"]

dom/webgpu/mochitest/test_buffer_mapping_overlapping_views.html

@@ -0,0 +1,57 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <script src="/tests/SimpleTest/SimpleTest.js"></script>
+    <link rel="stylesheet" href="/tests/SimpleTest/test.css" />
+  </head>
+  <body>
+    <script>
+      ok(
+        SpecialPowers.getBoolPref("dom.webgpu.enabled"),
+        "Pref should be enabled."
+      );
+
+      async function testBody() {
+        const adapter = await navigator.gpu.requestAdapter();
+        const device = await adapter.requestDevice();
+
+        let buffer = device.createBuffer({
+          size: 48,
+          usage: GPUBufferUsage.MAP_READ,
+          mappedAtCreation: true,
+        });
+        let view1 = buffer.getMappedRange(16, 24);
+
+        function bad_overlap(start, size, complaint) {
+          SimpleTest.doesThrow(() => {
+            buffer.getMappedRange(start, size);
+          }, complaint);
+        }
+
+        bad_overlap(8, 16, "overlap start of prior");
+        bad_overlap(32, 16, "overlap end of prior");
+        bad_overlap(8, 40, "enclose prior");
+        bad_overlap(24, 8, "enclosed by prior");
+
+        // Not really an overlap, but this checks for a mistake in
+        // an early revision of the patch that introduces this test.
+        SimpleTest.doesThrow(() => {
+          buffer.getMappedRange(64, 8);
+        }, "offset beyond mapped range");
+
+        // should be fine
+        ok(
+          typeof buffer.getMappedRange(0, 16) == "object",
+          "no overlap before"
+        );
+        ok(typeof buffer.getMappedRange(40, 8) == "object", "no overlap after");
+      }
+
+      SimpleTest.waitForExplicitFinish();
+      testBody()
+        .catch(e => ok(false, "Unhandled exception " + e))
+        .finally(() => SimpleTest.finish());
+    </script>
+  </body>
+</html>