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

Author: ErichDonGubler

wgpu-core/src/command/transfer.rs

@@ -47,6 +47,14 @@ pub enum TransferError {
     MissingBufferUsage(#[from] MissingBufferUsageError),
     #[error(transparent)]
     MissingTextureUsage(#[from] MissingTextureUsageError),
+    #[error(
+        "Offset {start_offset} is outside 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 { .. }
@@ -983,10 +992,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 % wgt::COPY_BUFFER_ALIGNMENT != 0 {
         return Err(TransferError::UnalignedCopySize(size).into());
@@ -1019,8 +1037,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 +1046,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 +1069,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 +1084,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}) goes beyond bounds of buffer of size {buffer_size}")]
+    MapStartOffsetOverrun {
+        offset: wgt::BufferAddress,
+        buffer_size: wgt::BufferAddress,
+    },
+    #[error(
+        "Map end offset (start at {} + size of {}) goes beyond bounds of 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 % wgt::MAP_ALIGNMENT != 0 || range.end % wgt::COPY_BUFFER_ALIGNMENT != 0 {
@@ -702,10 +741,27 @@ impl Buffer {
         let map_state = &*self.map_state.lock();
         match *map_state {
             BufferMapState::Init { ref staging_buffer } => {
+                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,
+                    });
+                }
+                // NOTE: Should never overflow because of our earlier check.
+                let mapping_end_offset = offset + range_size;
+
                 // offset (u64) can not be < 0, so no need to validate the lower bound
-                if offset + range_size > self.size {
+                if mapping_end_offset > self.size {
                     return Err(BufferAccessError::OutOfBoundsOverrun {
-                        index: offset + range_size - 1,
+                        index: mapping_end_offset.checked_sub(1).unwrap(),
                         max: self.size,
                     });
                 }