fix(core): validate start + size for buffer ops in more places

Author: ErichDonGubler

player/tests/player/data/buffer-copy.ron

@@ -21,10 +21,8 @@
         WriteBuffer(
             id: PointerId(0x10),
             data: File("data1.bin"),
-            range: (
-                start: 0,
-                end: 16,
-            ),
+            offset: 0,
+            size: 16,
             queued: true,
         ),
         Submit(1, []),

player/tests/player/data/clear-buffer-texture.ron

@@ -74,10 +74,8 @@
         WriteBuffer(
             id: PointerId(0x110),
             data: File("data1.bin"),
-            range: (
-                start: 0,
-                end: 16,
-            ),
+            offset: 0,
+            size: 16,
             queued: true,
         ),
         Submit(1, [

player/tests/player/data/zero-init-buffer.ron

@@ -68,10 +68,8 @@
         WriteBuffer(
             id: PointerId(0x120),
             data: File("data1.bin"),
-            range: (
-                start: 4,
-                end: 20,
-            ),
+            offset: 4,
+            size: 20,
             queued: true,
         ),
         CreateShaderModule(

wgpu-core/src/command/transfer.rs

@@ -47,6 +47,14 @@ pub enum TransferError {
     MissingBufferUsage(#[from] MissingBufferUsageError),
     #[error(transparent)]
     MissingTextureUsage(#[from] MissingTextureUsageError),
+    #[error(
+        "Copy at offset {start_offset} bytes would end up overrunning the bounds of the {side:?} buffer of size {buffer_size}"
+    )]
+    BufferStartOffsetOverrun {
+        start_offset: BufferAddress,
+        buffer_size: BufferAddress,
+        side: CopySide,
+    },
     #[error(
         "Copy at offset {start_offset} for {size} bytes would end up overrunning the bounds of the {side:?} buffer of size {buffer_size}"
     )]
@@ -185,6 +193,7 @@ impl WebGpuError for TransferError {
 
             Self::BufferOverrun { .. }
             | Self::TextureOverrun { .. }
+            | Self::BufferStartOffsetOverrun { .. }
             | Self::UnsupportedPartialTransfer { .. }
             | Self::InvalidCopyWithinSameTexture { .. }
             | Self::InvalidTextureAspect { .. }
@@ -342,8 +351,15 @@ pub(crate) fn validate_linear_texture_data(
         return Err(TransferError::UnspecifiedRowsPerImage);
     };
 
-    // Avoid underflow in the subtraction by checking bytes_in_copy against buffer_size first.
-    if bytes_in_copy > buffer_size || offset > buffer_size - bytes_in_copy {
+    if offset > buffer_size {
+        return Err(TransferError::BufferStartOffsetOverrun {
+            start_offset: offset,
+            buffer_size: bytes_in_copy,
+            side: buffer_side,
+        });
+    }
+    // NOTE: Should never underflow because of our earlier check.
+    if bytes_in_copy > buffer_size - offset {
         return Err(TransferError::BufferOverrun {
             start_offset: offset,
             size: bytes_in_copy,
@@ -983,10 +999,19 @@ pub(super) fn copy_buffer_to_buffer(
         .map_err(TransferError::MissingBufferUsage)?;
     let dst_barrier = dst_pending.map(|pending| pending.into_hal(dst_buffer, state.snatch_guard));
 
-    let (size, source_end_offset) = match size {
-        Some(size) => (size, source_offset + size),
-        None => (src_buffer.size - source_offset, src_buffer.size),
-    };
+    // TODO: This check isn't part of the spec., but it looks like it should be.
+    if source_offset > src_buffer.size {
+        return Err(TransferError::BufferStartOffsetOverrun {
+            start_offset: destination_offset,
+            buffer_size: dst_buffer.size,
+            side: CopySide::Destination,
+        }
+        .into());
+    }
+    let size = size.unwrap_or_else(|| {
+        // NOTE: Should never underflow because of our earlier check.
+        src_buffer.size - source_offset
+    });
 
     if !size.is_multiple_of(wgt::COPY_BUFFER_ALIGNMENT) {
         return Err(TransferError::UnalignedCopySize(size).into());
@@ -1019,8 +1044,7 @@ pub(super) fn copy_buffer_to_buffer(
         }
     }
 
-    let destination_end_offset = destination_offset + size;
-    if source_end_offset > src_buffer.size {
+    if size > src_buffer.size - source_offset {
         return Err(TransferError::BufferOverrun {
             start_offset: source_offset,
             size,
@@ -1029,7 +1053,21 @@ pub(super) fn copy_buffer_to_buffer(
         }
         .into());
     }
-    if destination_end_offset > dst_buffer.size {
+    // NOTE: Should never overflow because of our earlier check.
+    let source_end_offset = source_offset + size;
+
+    // TODO: Checks for source buffer overrun don't quite match spec., but it seems important to do
+    // things in this order.
+    if destination_offset > dst_buffer.size {
+        return Err(TransferError::BufferStartOffsetOverrun {
+            start_offset: destination_offset,
+            buffer_size: dst_buffer.size,
+            side: CopySide::Destination,
+        }
+        .into());
+    }
+    // NOTE: Should never underflow because of our earlier check.
+    if size > dst_buffer.size - destination_offset {
         return Err(TransferError::BufferOverrun {
             start_offset: destination_offset,
             size,
@@ -1038,6 +1076,8 @@ pub(super) fn copy_buffer_to_buffer(
         }
         .into());
     }
+    // NOTE: Should never overflow because of our earlier check.
+    let destination_end_offset = destination_offset + size;
 
     // This must happen after parameter validation (so that errors are reported
     // as required by the spec), but before any side effects.
@@ -1051,14 +1091,14 @@ pub(super) fn copy_buffer_to_buffer(
         .buffer_memory_init_actions
         .extend(dst_buffer.initialization_status.read().create_action(
             dst_buffer,
-            destination_offset..(destination_offset + size),
+            destination_offset..destination_end_offset,
             MemoryInitKind::ImplicitlyInitialized,
         ));
     state
         .buffer_memory_init_actions
         .extend(src_buffer.initialization_status.read().create_action(
             src_buffer,
-            source_offset..(source_offset + size),
+            source_offset..source_end_offset,
             MemoryInitKind::NeedsInitializedMemory,
         ));
 

wgpu-core/src/device/queue.rs

@@ -660,7 +660,15 @@ impl Queue {
         if !buffer_offset.is_multiple_of(wgt::COPY_BUFFER_ALIGNMENT) {
             return Err(TransferError::UnalignedBufferOffset(buffer_offset));
         }
-        if buffer_offset + buffer_size.get() > buffer.size {
+
+        if buffer_offset > buffer.size {
+            return Err(TransferError::BufferStartOffsetOverrun {
+                start_offset: buffer_offset,
+                buffer_size: buffer_size.get(),
+                side: CopySide::Destination,
+            });
+        }
+        if buffer_size.get() > buffer.size - buffer_offset {
             return Err(TransferError::BufferOverrun {
                 start_offset: buffer_offset,
                 size: buffer_size.get(),

wgpu-core/src/resource.rs

@@ -301,6 +301,22 @@ pub enum BufferAccessError {
     MapAborted,
     #[error(transparent)]
     InvalidResource(#[from] InvalidResourceError),
+    #[error("Map start offset ({offset}) is out-of-bounds for buffer of size {buffer_size}")]
+    MapStartOffsetOverrun {
+        offset: wgt::BufferAddress,
+        buffer_size: wgt::BufferAddress,
+    },
+    #[error(
+        "Map end offset (start at {} + size of {}) is out-of-bounds for buffer of size {}",
+        offset,
+        size,
+        buffer_size
+    )]
+    MapEndOffsetOverrun {
+        offset: wgt::BufferAddress,
+        size: wgt::BufferAddress,
+        buffer_size: wgt::BufferAddress,
+    },
 }
 
 impl WebGpuError for BufferAccessError {
@@ -321,7 +337,9 @@ impl WebGpuError for BufferAccessError {
             | Self::OutOfBoundsUnderrun { .. }
             | Self::OutOfBoundsOverrun { .. }
             | Self::NegativeRange { .. }
-            | Self::MapAborted => return ErrorType::Validation,
+            | Self::MapAborted
+            | Self::MapStartOffsetOverrun { .. }
+            | Self::MapEndOffsetOverrun { .. } => return ErrorType::Validation,
         };
         e.webgpu_error_type()
     }
@@ -592,6 +610,27 @@ impl Buffer {
             return Err((op, BufferAccessError::UnalignedRangeSize { range_size }));
         }
 
+        if offset > self.size {
+            return Err((
+                op,
+                BufferAccessError::MapStartOffsetOverrun {
+                    offset,
+                    buffer_size: self.size,
+                },
+            ));
+        }
+        // NOTE: Should never underflow because of our earlier check.
+        if range_size > self.size - offset {
+            return Err((
+                op,
+                BufferAccessError::MapEndOffsetOverrun {
+                    offset,
+                    size: range_size,
+                    buffer_size: self.size,
+                },
+            ));
+        }
+        // NOTE: Should never overflow because of our earlier check.
         let range = offset..(offset + range_size);
 
         if !range.start.is_multiple_of(wgt::MAP_ALIGNMENT)
@@ -704,11 +743,18 @@ impl Buffer {
         let map_state = &*self.map_state.lock();
         match *map_state {
             BufferMapState::Init { ref staging_buffer } => {
-                // offset (u64) can not be < 0, so no need to validate the lower bound
-                if offset + range_size > self.size {
-                    return Err(BufferAccessError::OutOfBoundsOverrun {
-                        index: offset + range_size - 1,
-                        max: self.size,
+                if offset > self.size {
+                    return Err(BufferAccessError::MapStartOffsetOverrun {
+                        offset,
+                        buffer_size: self.size,
+                    });
+                }
+                // NOTE: Should never underflow because of our earlier check.
+                if range_size > self.size - offset {
+                    return Err(BufferAccessError::MapEndOffsetOverrun {
+                        offset,
+                        size: range_size,
+                        buffer_size: self.size,
                     });
                 }
                 let ptr = unsafe { staging_buffer.ptr() };