Add WriteOnly type to replace giving &mut [u8] access to mapped buffers.

Fixes <#8897>.

Author: kpreid

.github/workflows/ci.yml

@@ -743,6 +743,41 @@ jobs:
 
           cargo --locked test --doc
 
+  miri:
+    # TODO: note typical run time
+    timeout-minutes: 10
+
+    name: Miri (limited scope)
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout repo
+        uses: actions/checkout@v6
+
+      # Miri is *only* available on nightly.
+      - name: Install nightly toolchain
+      
+        run: |
+          rustup toolchain install nightly --no-self-update --profile=minimal --component miri
+          rustup override set nightly
+          cargo -V
+
+      - name: Caching
+        uses: Swatinem/rust-cache@v2
+        with:
+          key: miri-${{ env.CACHE_SUFFIX }}
+
+      - name: Run Miri on selected tests
+        shell: bash
+        run: |
+          set -e
+
+          # Note: this is a *smaller* set of tests than `cargo xtask miri` runs;
+          # it is ones that are both important and cheap.
+          # Consider expanding it to include some API tests with the noop backend.
+          cargo miri test --package=wgpu --no-default-features -- write_only
+
+
   fmt:
     # runtime is normally 15 seconds
     timeout-minutes: 2

CHANGELOG.md

@@ -98,6 +98,7 @@ By @cwfitzgerald in [#8999](https://github.com/gfx-rs/wgpu/pull/8999).
         - Split the `OutOfBoundsOverrun` variant into new `OutOfBoundsStartOffsetOverrun` and `OutOfBoundsEndOffsetOverrun` variants. 
         - Removed the `NegativeRange` variant in favor of new `MapStartOffsetUnderrun` and `MapStartOffsetOverrun` variants.
     - Split the `TransferError::BufferOverrun` variant into new `BufferStartOffsetOverrun` and `BufferEndOffsetOverrun` variants.
+- To ensure memory safety in all circumstances, `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 will likely be needed, particularly including replacing `view[start..end]` with `view.slice(start..end)`. 
 
 #### 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,