Replace &mut [u8] access to mapped buffers with WriteOnly.

When accessing a buffer for writing, do not expose (or internally
create) any `&'a mut [u8]` references to that memory, so as to avoid
exposing write-combining memory that might visibly misbehave in atomic
operations. Instead, expose the new pointer type `WriteOnly<'a, [u8]>`
which offers only write operations. This can be sliced and reborrowed,
allowing it to be used in most ways which `&mut [u8]` would be,
hopefully with near-zero overhead.

This pointer type might also allow users to opt in to uninitialized
memory for performance, while confining the consequences to GPU UB
rather than Rust UB. However, that is a future possibility and is not
implemented in this commit other than as a consideration in defining the
API of the `WriteOnly` type.

This change is not a complete solution to the problem of write-combining
memory because it is still possible to read after writing by calling
`get_mapped_range()` after `get_mapped_range_mut()`. However, that is
solvable separately, perhaps with a barrier or a validation change,
and is not particularly coupled with this part of the solution.

Author: kpreid

CHANGELOG.md

@@ -175,6 +175,7 @@ depth_stencil: Some(wgpu::DepthStencilState::stencil(
   - Split the `TransferError::BufferOverrun` variant into new `BufferStartOffsetOverrun` and `BufferEndOffsetOverrun` variants.
 - The various "max resources per stage" limits are now capped at 100, so that their total remains below `max_bindings_per_bind_group`, as required by WebGPU. By @andyleiserson in [#9118](https://github.com/gfx-rs/wgpu/pull/9118).
 - The `max_uniform_buffer_binding_size` and `max_storage_buffer_binding_size` limits are now `u64` instead of `u32`, to match WebGPU. By @wingertge in [#9146](https://github.com/gfx-rs/wgpu/pull/9146).
+- To ensure memory safety when accessing mapped memory, `MAP_WRITE` buffer mappings are no longer exposed as Rust `&mut [u8]`, but the new type `WriteOnly<[u8]>`, which does not allow reading. Similar methods are provided where possible, but changes to your code will likely be needed, particularly including replacing `view[start..end]` with `view.slice(start..end)`. By @kpreid in [#9042](https://github.com/gfx-rs/wgpu/pull/9042).
 
 #### Metal
 

tests/tests/wgpu-gpu/binding_array/buffers.rs

@@ -158,7 +158,10 @@ async fn binding_array_buffers(
             size: 16,
             mapped_at_creation: true,
         });
-        buffer.slice(..).get_mapped_range_mut()[0..4].copy_from_slice(&data.0);
+        buffer
+            .get_mapped_range_mut(..)
+            .slice(0..4)
+            .copy_from_slice(&data.0);
         buffer.unmap();
         buffers.push(buffer);
     }

tests/tests/wgpu-gpu/buffer.rs

@@ -89,7 +89,7 @@ async fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label:
 
     {
         let view = b1.slice(0..0).get_mapped_range_mut();
-        assert!(view.is_empty());
+        assert_eq!(view.len(), 0);
     }
 
     b1.unmap();
@@ -148,8 +148,8 @@ static MAP_OFFSET: GpuTestConfiguration = GpuTestConfiguration::new()
         {
             let slice = write_buf.slice(32..48);
             let mut view = slice.get_mapped_range_mut();
-            for byte in &mut view[..] {
-                *byte = 2;
+            for byte in view.slice(..) {
+                byte.write(2);
             }
         }
 

tests/tests/wgpu-validation/api/buffer_mapping.rs

@@ -1,8 +1,27 @@
-fn mapping_is_zeroed(array: &[u8]) {
-    for (i, &byte) in array.iter().enumerate() {
+// FIXME: Now that MAP_WRITE mappings are write-only,
+// the “mut” and “immutable” terminology is incorrect.
+
+fn read_mapping_is_zeroed(slice: &[u8]) {
+    for (i, &byte) in slice.iter().enumerate() {
         assert_eq!(byte, 0, "Byte at index {i} is not zero");
     }
 }
+fn write_mapping_is_zeroed(mut slice: wgpu::WriteOnly<'_, [u8]>) {
+    let ptr = slice.as_raw_ptr().cast::<u8>();
+    for i in 0..slice.len() {
+        assert_eq!(
+            // SAFETY: it is not, in general, safe to read from a write mapping, but our goal here
+            // is specifically to verify the internally provided zeroedness.
+            //
+            // FIXME: Is the goal of these tests to ensure that zeroes are what is exposed to Rust,
+            // and not to ensure that zeroes get into the GPU buffer? If so, then we can delete
+            // them, or perhaps replace them with tests of mapping without writing, then reading.
+            unsafe { ptr.add(i).read() },
+            0,
+            "Byte at index {i} is not zero"
+        );
+    }
+}
 
 // Ensure that a simple immutable mapping works and it is zeroed.
 #[test]
@@ -21,7 +40,7 @@ fn full_immutable_binding() {
 
     let mapping = buffer.slice(..).get_mapped_range();
 
-    mapping_is_zeroed(&mapping);
+    read_mapping_is_zeroed(&mapping);
 
     drop(mapping);
 
@@ -40,9 +59,9 @@ fn full_mut_binding() {
         mapped_at_creation: true,
     });
 
-    let mapping = buffer.slice(..).get_mapped_range_mut();
+    let mut mapping = buffer.slice(..).get_mapped_range_mut();
 
-    mapping_is_zeroed(&mapping);
+    write_mapping_is_zeroed(mapping.slice(..));
 
     drop(mapping);
 
@@ -67,8 +86,8 @@ fn split_immutable_binding() {
     let mapping0 = buffer.slice(0..512).get_mapped_range();
     let mapping1 = buffer.slice(512..1024).get_mapped_range();
 
-    mapping_is_zeroed(&mapping0);
-    mapping_is_zeroed(&mapping1);
+    read_mapping_is_zeroed(&mapping0);
+    read_mapping_is_zeroed(&mapping1);
 
     drop(mapping0);
     drop(mapping1);
@@ -88,11 +107,11 @@ fn split_mut_binding() {
         mapped_at_creation: true,
     });
 
-    let mapping0 = buffer.slice(0..512).get_mapped_range_mut();
-    let mapping1 = buffer.slice(512..1024).get_mapped_range_mut();
+    let mut mapping0 = buffer.slice(0..512).get_mapped_range_mut();
+    let mut mapping1 = buffer.slice(512..1024).get_mapped_range_mut();
 
-    mapping_is_zeroed(&mapping0);
-    mapping_is_zeroed(&mapping1);
+    write_mapping_is_zeroed(mapping0.slice(..));
+    write_mapping_is_zeroed(mapping1.slice(..));
 
     drop(mapping0);
     drop(mapping1);

tests/tests/wgpu-validation/util.rs

@@ -31,7 +31,8 @@ fn staging_belt_random_test() {
                 offset,
                 wgpu::BufferSize::new(size).unwrap(),
             );
-            slice[0] = 1; // token amount of actual writing, just in case it makes a difference
+            // token amount of actual writing, just in case it makes a difference
+            slice.slice(..1).copy_from_slice(&[1]);
         }
 
         belt.finish();

wgpu/src/api/buffer.rs

@@ -1,7 +1,7 @@
 use alloc::{boxed::Box, sync::Arc, vec::Vec};
 use core::{
     error, fmt,
-    ops::{Bound, Deref, DerefMut, Range, RangeBounds},
+    ops::{Bound, Deref, Range, RangeBounds},
 };
 
 use crate::util::Mutex;
@@ -140,8 +140,8 @@ use crate::*;
 /// buffer.map_async(wgpu::MapMode::Write, .., move |result| {
 ///     if result.is_ok() {
 ///         let mut view = capturable.get_mapped_range_mut(..);
-///         let floats: &mut [f32] = bytemuck::cast_slice_mut(&mut view);
-///         floats.fill(42.0);
+///         let mut floats: wgpu::WriteOnly<[[u8; 4]]> = view.slice(..).into_chunks::<4>().0;
+///         floats.fill(42.0f32.to_ne_bytes());
 ///         drop(view);
 ///         capturable.unmap();
 ///     }
@@ -613,7 +613,7 @@ impl<'a> BufferSlice<'a> {
         }
     }
 
-    /// Gain write access to the bytes of a [mapped] [`Buffer`].
+    /// Gain write-only access to the bytes of a [mapped] [`Buffer`].
     ///
     /// Returns a [`BufferViewMut`] referring to the buffer range represented by
     /// `self`. See the documentation for [`BufferViewMut`] for more details.
@@ -825,7 +825,7 @@ impl MapContext {
     ///
     /// This panics if the given range does not exactly match one previously
     /// passed to [`MapContext::validate_and_add`].
-    fn remove(&mut self, offset: BufferAddress, size: BufferSize) {
+    pub(crate) fn remove(&mut self, offset: BufferAddress, size: BufferSize) {
         let end = offset + size.get();
 
         let index = self
@@ -894,43 +894,16 @@ pub struct BufferView {
     inner: dispatch::DispatchBufferMappedRange,
 }
 
-#[cfg(webgpu)]
-impl BufferView {
-    /// Provides the same data as dereferencing the view, but as a `Uint8Array` in js.
-    /// This can be MUCH faster than dereferencing the view which copies the data into
-    /// the Rust / wasm heap.
-    pub fn as_uint8array(&self) -> &js_sys::Uint8Array {
-        self.inner.as_uint8array()
-    }
-}
-
-impl core::ops::Deref for BufferView {
-    type Target = [u8];
-
-    #[inline]
-    fn deref(&self) -> &[u8] {
-        self.inner.slice()
-    }
-}
-
-impl AsRef<[u8]> for BufferView {
-    #[inline]
-    fn as_ref(&self) -> &[u8] {
-        self.inner.slice()
-    }
-}
-
 /// A write-only view of a mapped buffer's bytes.
 ///
 /// To get a `BufferViewMut`, first [map] the buffer, and then
 /// call `buffer.slice(range).get_mapped_range_mut()`.
 ///
-/// `BufferViewMut` dereferences to `&mut [u8]`, so you can use all the usual
-/// Rust slice methods to access the buffer's contents. It also implements
-/// `AsMut<[u8]>`, if that's more convenient.
-///
-/// It is possible to read the buffer using this view, but doing so is not
-/// recommended, as it is likely to be slow.
+/// Because Rust has no write-only reference type
+/// (`&[u8]` is read-only and `&mut [u8]` is read-write),
+/// this type does not dereference to a slice in the way that [`BufferView`] does.
+/// Instead, [`.slice()`][BufferViewMut::slice] returns a special [`WriteOnly`] pointer type,
+/// and there are also a few convenience methods such as [`BufferViewMut::copy_from_slice()`].
 ///
 /// Before the buffer can be unmapped, all `BufferViewMut`s observing it
 /// must be dropped. Otherwise, the call to [`Buffer::unmap`] will panic.
@@ -947,24 +920,25 @@ pub struct BufferViewMut {
     inner: dispatch::DispatchBufferMappedRange,
 }
 
-impl AsMut<[u8]> for BufferViewMut {
-    #[inline]
-    fn as_mut(&mut self) -> &mut [u8] {
-        self.inner.slice_mut()
-    }
-}
+// `BufferView` simply dereferences. `BufferViewMut` cannot, because mapped memory may be
+// write-combining memory <https://en.wikipedia.org/wiki/Write_combining>,
+// and not support the expected behavior of atomic accesses.
+// Further context: <https://github.com/gfx-rs/wgpu/issues/8897>
 
-impl Deref for BufferViewMut {
+impl core::ops::Deref for BufferView {
     type Target = [u8];
 
-    fn deref(&self) -> &Self::Target {
-        self.inner.slice()
+    #[inline]
+    fn deref(&self) -> &[u8] {
+        // SAFETY: this is a read mapping
+        unsafe { self.inner.read_slice() }
     }
 }
 
-impl DerefMut for BufferViewMut {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        self.inner.slice_mut()
+impl AsRef<[u8]> for BufferView {
+    #[inline]
+    fn as_ref(&self) -> &[u8] {
+        self
     }
 }
 
@@ -986,6 +960,50 @@ impl Drop for BufferViewMut {
     }
 }
 
+#[cfg(webgpu)]
+impl BufferView {
+    /// Provides the same data as dereferencing the view, but as a `Uint8Array` in js.
+    /// This can be MUCH faster than dereferencing the view which copies the data into
+    /// the Rust / wasm heap.
+    pub fn as_uint8array(&self) -> &js_sys::Uint8Array {
+        self.inner.as_uint8array()
+    }
+}
+
+/// These methods are equivalent to the methods of the same names on [`WriteOnly`].
+impl BufferViewMut {
+    /// Returns the length of this view; the number of bytes to be written.
+    pub fn len(&self) -> usize {
+        // cannot fail because we can't actually map more than isize::MAX bytes
+        usize::try_from(self.size.get()).unwrap()
+    }
+
+    /// Returns `true` if the view has a length of 0.
+    ///
+    /// Note that this is currently impossible.
+    pub fn is_empty(&self) -> bool {
+        self.len() == 0
+    }
+
+    /// Returns a [`WriteOnly`] reference to a portion of this.
+    ///
+    /// `.slice(..)` can be used to access the whole data.
+    pub fn slice<'a, S: RangeBounds<usize>>(&'a mut self, bounds: S) -> WriteOnly<'a, [u8]> {
+        // SAFETY: this is a write mapping
+        unsafe { self.inner.write_slice() }.into_slice(bounds)
+    }
+
+    /// Copies all elements from src into `self`.
+    ///
+    /// The length of `src` must be the same as `self`.
+    ///
+    /// This method is equivalent to
+    /// [`self.slice(..).copy_from_slice(src)`][WriteOnly::copy_from_slice].
+    pub fn copy_from_slice(&mut self, src: &[u8]) {
+        self.slice(..).copy_from_slice(src)
+    }
+}
+
 #[track_caller]
 fn check_buffer_bounds(
     buffer_size: BufferAddress,

wgpu/src/api/queue.rs

@@ -1,5 +1,5 @@
 use alloc::boxed::Box;
-use core::ops::{Deref, DerefMut};
+use core::ops::{Deref, RangeBounds};
 
 use crate::{api::DeferredCommandBufferActions, *};
 
@@ -55,9 +55,7 @@ static_assertions::assert_impl_all!(PollType: Send, Sync);
 
 /// A write-only view into a staging buffer.
 ///
-/// Reading into this buffer won't yield the contents of the buffer from the
-/// GPU and is likely to be slow. Because of this, although [`AsMut`] is
-/// implemented for this type, [`AsRef`] is not.
+/// This type is what [`Queue::write_buffer_with()`] returns.
 pub struct QueueWriteBufferView {
     queue: Queue,
     buffer: Buffer,
@@ -75,31 +73,44 @@ impl QueueWriteBufferView {
     }
 }
 
-impl Deref for QueueWriteBufferView {
-    type Target = [u8];
-
-    fn deref(&self) -> &Self::Target {
-        self.inner.slice()
+impl Drop for QueueWriteBufferView {
+    fn drop(&mut self) {
+        self.queue
+            .inner
+            .write_staging_buffer(&self.buffer.inner, self.offset, &self.inner);
     }
 }
 
-impl DerefMut for QueueWriteBufferView {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        self.inner.slice_mut()
+/// These methods are equivalent to the methods of the same names on [`WriteOnly`].
+impl QueueWriteBufferView {
+    /// Returns the length of this view; the number of bytes to be written.
+    pub fn len(&self) -> usize {
+        self.inner.len()
     }
-}
 
-impl AsMut<[u8]> for QueueWriteBufferView {
-    fn as_mut(&mut self) -> &mut [u8] {
-        self.inner.slice_mut()
+    /// Returns `true` if the view has a length of 0.
+    pub fn is_empty(&self) -> bool {
+        self.len() == 0
     }
-}
 
-impl Drop for QueueWriteBufferView {
-    fn drop(&mut self) {
-        self.queue
-            .inner
-            .write_staging_buffer(&self.buffer.inner, self.offset, &self.inner);
+    /// Returns a [`WriteOnly`] reference to a portion of this.
+    ///
+    /// `.slice(..)` can be used to access the whole data.
+    pub fn slice<'a, S: RangeBounds<usize>>(&'a mut self, bounds: S) -> WriteOnly<'a, [u8]> {
+        // SAFETY:
+        // * this is a write mapping
+        // * function signature ensures no aliasing
+        unsafe { self.inner.write_slice() }.into_slice(bounds)
+    }
+
+    /// Copies all elements from src into `self`.
+    ///
+    /// The length of `src` must be the same as `self`.
+    ///
+    /// This method is equivalent to
+    /// [`self.slice(..).copy_from_slice(src)`][WriteOnly::copy_from_slice].
+    pub fn copy_from_slice(&mut self, src: &[u8]) {
+        self.slice(..).copy_from_slice(src)
     }
 }