From 5ecccc4fbb96e63a8918dba1ea80950989d247bf Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Wed, 3 Jul 2024 16:40:17 +0000 Subject: [PATCH] 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 | 133 ++++++++++++++---- dom/webgpu/Buffer.h | 66 ++++++++- dom/webgpu/mochitest/mochitest.toml | 2 + ...test_buffer_mapping_overlapping_views.html | 57 ++++++++ 4 files changed, 227 insertions(+), 31 deletions(-) create mode 100644 dom/webgpu/mochitest/test_buffer_mapping_overlapping_views.html diff --git a/dom/webgpu/Buffer.cpp b/dom/webgpu/Buffer.cpp index 3bd1353e69b0..7814afe6b610 100644 --- a/dom/webgpu/Buffer.cpp +++ b/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& aSize, JS::Rooted* aObject, ErrorResult& aRv) { + // The WebGPU spec spells out the validation we must perform, but + // use `CheckedInt` 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(aOffset); + CheckedInt rangeSize; + if (aSize.WasPassed()) { + rangeSize = aSize.Value(); + } else { + const auto bufferSize = CheckedInt(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(aOffset); - const auto checkedSize = aSize.WasPassed() - ? CheckedInt(aSize.Value()) - : CheckedInt(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* 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* data = new std::shared_ptr(mShmem); - UniquePtr dataPtr{ - span.data(), {&ExternalBufferFreeCallback, userData}}; - JS::Rooted 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(x)` produces an invalid value if `x` is not in + // range for `size_t` before any conversion is performed. + const auto checkedSize = CheckedInt(rangeSize.value()).value(); + const auto checkedOffset = CheckedInt(offset.value()).value(); + const auto span = (*data)->Bytes().Subspan(checkedOffset, checkedSize); + UniquePtr contents{ + span.data(), {&ExternalBufferFreeCallback, data}}; + JS::Rooted 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 rooted(aCx, arrayBuffer); + for (const auto& view : mMapped->mViews) { + JS::Rooted rooted(aCx, view.mArrayBuffer); if (!JS::DetachArrayBuffer(aCx, rooted)) { detachedArrayBuffers = false; } }; - mMapped->mArrayBuffers.Clear(); + mMapped->mViews.Clear(); AbortMapRequest(); diff --git a/dom/webgpu/Buffer.h b/dom/webgpu/Buffer.h index fa3881f38acf..2c49d33f4c1f 100644 --- a/dom/webgpu/Buffer.h +++ b/dom/webgpu/Buffer.h @@ -14,6 +14,40 @@ #include "mozilla/ipc/RawShmem.h" #include +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`, 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>` +// 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 { + // The default move constructors are fine for MappedView. + using Type = + nsTArray_RelocateUsingMoveConstructor; +}; + 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 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> mArrayBuffers; + nsTArray mViews; BufferAddress mOffset; BufferAddress mSize; MappedInfo() = default; @@ -84,8 +130,22 @@ class Buffer final : public ObjectBase, public ChildOf { // Information about the currently active mapping. Maybe mMapped; RefPtr 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 mShmem; }; diff --git a/dom/webgpu/mochitest/mochitest.toml b/dom/webgpu/mochitest/mochitest.toml index d83dca28084d..b267b7c60b26 100644 --- a/dom/webgpu/mochitest/mochitest.toml +++ b/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"] diff --git a/dom/webgpu/mochitest/test_buffer_mapping_overlapping_views.html b/dom/webgpu/mochitest/test_buffer_mapping_overlapping_views.html new file mode 100644 index 000000000000..58432d30d0aa --- /dev/null +++ b/dom/webgpu/mochitest/test_buffer_mapping_overlapping_views.html @@ -0,0 +1,57 @@ + + + + + + + + + + +