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

Co-authored-by: Jim Blandy <[email protected]>

Author: ErichDonGubler

CHANGELOG.md

@@ -91,7 +91,11 @@ By @cwfitzgerald in [#8999](https://github.com/gfx-rs/wgpu/pull/8999).
 
 #### General
 
-- Replace `TransferError::BufferOverrun::end_offset` with a `size` field. By @ErichDonGubler in [9073](https://github.com/gfx-rs/wgpu/pull/9073).
+- Several error APIs were changed by @ErichDonGubler in [#9073](https://github.com/gfx-rs/wgpu/pull/9073):
+    - `BufferAccessError`:
+        - 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.
 
 #### Metal
 

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,10 +47,18 @@ 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}"
     )]
-    BufferOverrun {
+    BufferEndOffsetOverrun {
         start_offset: BufferAddress,
         size: BufferAddress,
         buffer_size: BufferAddress,
@@ -183,8 +191,9 @@ impl WebGpuError for TransferError {
             Self::MissingTextureUsage(e) => e,
             Self::MemoryInitFailure(e) => e,
 
-            Self::BufferOverrun { .. }
+            Self::BufferEndOffsetOverrun { .. }
             | Self::TextureOverrun { .. }
+            | Self::BufferStartOffsetOverrun { .. }
             | Self::UnsupportedPartialTransfer { .. }
             | Self::InvalidCopyWithinSameTexture { .. }
             | Self::InvalidTextureAspect { .. }
@@ -342,9 +351,16 @@ 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 {
-        return Err(TransferError::BufferOverrun {
+    if offset > buffer_size {
+        return Err(TransferError::BufferStartOffsetOverrun {
+            start_offset: offset,
+            buffer_size,
+            side: buffer_side,
+        });
+    }
+    // NOTE: Should never underflow because of our earlier check.
+    if bytes_in_copy > buffer_size - offset {
+        return Err(TransferError::BufferEndOffsetOverrun {
             start_offset: offset,
             size: bytes_in_copy,
             buffer_size,
@@ -983,10 +999,18 @@ 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),
-    };
+    if source_offset > src_buffer.size {
+        return Err(TransferError::BufferStartOffsetOverrun {
+            start_offset: source_offset,
+            buffer_size: src_buffer.size,
+            side: CopySide::Source,
+        }
+        .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,25 +1043,38 @@ pub(super) fn copy_buffer_to_buffer(
         }
     }
 
-    let destination_end_offset = destination_offset + size;
-    if source_end_offset > src_buffer.size {
-        return Err(TransferError::BufferOverrun {
+    if size > src_buffer.size - source_offset {
+        return Err(TransferError::BufferEndOffsetOverrun {
             start_offset: source_offset,
             size,
             buffer_size: src_buffer.size,
             side: CopySide::Source,
         }
         .into());
     }
-    if destination_end_offset > dst_buffer.size {
-        return Err(TransferError::BufferOverrun {
+    // NOTE: Should never overflow because of our earlier check.
+    let source_end_offset = source_offset + size;
+
+    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::BufferEndOffsetOverrun {
             start_offset: destination_offset,
             size,
             buffer_size: dst_buffer.size,
             side: CopySide::Destination,
         }
         .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 +1088,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,8 +660,16 @@ 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 {
-            return Err(TransferError::BufferOverrun {
+
+        if buffer_offset > buffer.size {
+            return Err(TransferError::BufferStartOffsetOverrun {
+                start_offset: buffer_offset,
+                buffer_size: buffer.size,
+                side: CopySide::Destination,
+            });
+        }
+        if buffer_size.get() > buffer.size - buffer_offset {
+            return Err(TransferError::BufferEndOffsetOverrun {
                 start_offset: buffer_offset,
                 size: buffer_size.get(),
                 buffer_size: buffer.size,

wgpu-core/src/resource.rs

@@ -281,26 +281,45 @@ pub enum BufferAccessError {
     #[error("Buffer range size invalid: range_size {range_size} must be multiple of 4")]
     UnalignedRangeSize { range_size: wgt::BufferAddress },
     #[error("Buffer access out of bounds: index {index} would underrun the buffer (limit: {min})")]
-    OutOfBoundsUnderrun {
+    OutOfBoundsStartOffsetUnderrun {
         index: wgt::BufferAddress,
         min: wgt::BufferAddress,
     },
     #[error(
-        "Buffer access out of bounds: last index {index} would overrun the buffer (limit: {max})"
+        "Buffer access out of bounds: start offset {index} would overrun the buffer (limit: {max})"
     )]
-    OutOfBoundsOverrun {
+    OutOfBoundsStartOffsetOverrun {
         index: wgt::BufferAddress,
         max: wgt::BufferAddress,
     },
-    #[error("Buffer map range start {start} is greater than end {end}")]
-    NegativeRange {
-        start: wgt::BufferAddress,
-        end: wgt::BufferAddress,
+    #[error(
+        "Buffer access out of bounds: start offset {index} + size {size} would overrun the buffer (limit: {max})"
+    )]
+    OutOfBoundsEndOffsetOverrun {
+        index: wgt::BufferAddress,
+        size: wgt::BufferAddress,
+        max: wgt::BufferAddress,
     },
     #[error("Buffer map aborted")]
     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 {
@@ -318,10 +337,12 @@ impl WebGpuError for BufferAccessError {
             | Self::UnalignedRange
             | Self::UnalignedOffset { .. }
             | Self::UnalignedRangeSize { .. }
-            | Self::OutOfBoundsUnderrun { .. }
-            | Self::OutOfBoundsOverrun { .. }
-            | Self::NegativeRange { .. }
-            | Self::MapAborted => return ErrorType::Validation,
+            | Self::OutOfBoundsStartOffsetUnderrun { .. }
+            | Self::OutOfBoundsStartOffsetOverrun { .. }
+            | Self::OutOfBoundsEndOffsetOverrun { .. }
+            | Self::MapAborted
+            | Self::MapStartOffsetOverrun { .. }
+            | Self::MapEndOffsetOverrun { .. } => return ErrorType::Validation,
         };
         e.webgpu_error_type()
     }
@@ -592,10 +613,30 @@ impl Buffer {
             return Err((op, BufferAccessError::UnalignedRangeSize { range_size }));
         }
 
-        let range = offset..(offset + 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,
+                },
+            ));
+        }
+        let end_offset = offset + range_size;
 
-        if !range.start.is_multiple_of(wgt::MAP_ALIGNMENT)
-            || !range.end.is_multiple_of(wgt::COPY_BUFFER_ALIGNMENT)
+        if !offset.is_multiple_of(wgt::MAP_ALIGNMENT)
+            || !end_offset.is_multiple_of(wgt::COPY_BUFFER_ALIGNMENT)
         {
             return Err((op, BufferAccessError::UnalignedRange));
         }
@@ -609,25 +650,6 @@ impl Buffer {
             return Err((op, e.into()));
         }
 
-        if range.start > range.end {
-            return Err((
-                op,
-                BufferAccessError::NegativeRange {
-                    start: range.start,
-                    end: range.end,
-                },
-            ));
-        }
-        if range.end > self.size {
-            return Err((
-                op,
-                BufferAccessError::OutOfBoundsOverrun {
-                    index: range.end,
-                    max: self.size,
-                },
-            ));
-        }
-
         let device = &self.device;
         if let Err(e) = device.check_is_valid() {
             return Err((op, e.into()));
@@ -650,7 +672,8 @@ impl Buffer {
                     return Err((op, BufferAccessError::MapAlreadyPending));
                 }
                 BufferMapState::Idle => BufferMapState::Waiting(BufferPendingMapping {
-                    range,
+                    // NOTE: Should never overflow because of our earlier check.
+                    range: offset..end_offset,
                     op,
                     _parent_buffer: self.clone(),
                 }),
@@ -704,11 +727,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() };
@@ -720,15 +750,22 @@ impl Buffer {
                 ref range,
                 ..
             } => {
+                if offset > range.end {
+                    return Err(BufferAccessError::OutOfBoundsStartOffsetOverrun {
+                        index: offset,
+                        max: range.end,
+                    });
+                }
                 if offset < range.start {
-                    return Err(BufferAccessError::OutOfBoundsUnderrun {
+                    return Err(BufferAccessError::OutOfBoundsStartOffsetUnderrun {
                         index: offset,
                         min: range.start,
                     });
                 }
-                if offset + range_size > range.end {
-                    return Err(BufferAccessError::OutOfBoundsOverrun {
-                        index: offset + range_size - 1,
+                if range_size > range.end - offset {
+                    return Err(BufferAccessError::OutOfBoundsEndOffsetOverrun {
+                        index: offset,
+                        size: range_size,
                         max: range.end,
                     });
                 }