WIP: refactor(core)!: factor out new buffer_region_overrun module

Author: ErichDonGubler

wgpu-core/src/buffer_region_overrun.rs

@@ -0,0 +1,95 @@
+use thiserror::Error;
+use wgt::BufferAddress;
+
+use crate::{
+    command::{CopySide, TransferError},
+    resource::BufferAccessError,
+};
+
+/// Error encountered while checking offsets against a buffer.
+#[derive(Clone, Debug, Error)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+#[non_exhaustive]
+pub enum BufferRegionOverrunError {
+    #[error("start offset ({offset}) is out-of-bounds for buffer of size {buffer_size}")]
+    StartOffset {
+        offset: BufferAddress,
+        buffer_size: BufferAddress,
+    },
+    #[error(
+        "end offset (start at {} + size of {}) is out-of-bounds for buffer of size {}",
+        offset,
+        size,
+        buffer_size
+    )]
+    EndOffset {
+        offset: BufferAddress,
+        size: BufferAddress,
+        buffer_size: BufferAddress,
+    },
+}
+
+impl BufferRegionOverrunError {
+    /// Returns the end offset if everything is validated.
+    ///
+    /// TODO: more
+    pub(crate) fn check(
+        start_offset: BufferAddress,
+        size: BufferAddress,
+        buffer_size: BufferAddress,
+    ) -> Result<BufferAddress, Self> {
+        Self::check_start(start_offset, buffer_size)?.check_end(size)
+    }
+
+    pub(crate) fn check_start(
+        start_offset: BufferAddress,
+        buffer_size: BufferAddress,
+    ) -> Result<BufferOverrunEndOffsetChecker, Self> {
+        if start_offset >= buffer_size {
+            return Err(Self::StartOffset {
+                offset: start_offset,
+                buffer_size,
+            });
+        }
+        Ok(BufferOverrunEndOffsetChecker {
+            start_offset,
+            buffer_size,
+        })
+    }
+}
+
+/// A
+#[derive(Clone, Copy, Debug)]
+pub(crate) struct BufferOverrunEndOffsetChecker {
+    start_offset: BufferAddress,
+    buffer_size: BufferAddress,
+}
+
+impl BufferOverrunEndOffsetChecker {
+    pub(crate) fn check_end(
+        self,
+        size: BufferAddress,
+    ) -> Result<BufferAddress, BufferRegionOverrunError> {
+        let Self {
+            start_offset,
+            buffer_size,
+        } = self;
+
+        // NOTE: Should never underflow because of our earlier check.
+        if size > buffer_size - start_offset {
+            return Err(BufferRegionOverrunError::EndOffset {
+                offset: start_offset,
+                size,
+                buffer_size,
+            });
+        }
+        // NOTE: Should never overflow because of our earlier check.
+        Ok(start_offset + size)
+    }
+}
+
+impl BufferRegionOverrunError {
+    pub(crate) fn to_transfer_error(&self, side: CopySide) -> TransferError {
+        todo!()
+    }
+}

wgpu-core/src/command/transfer.rs

@@ -10,6 +10,7 @@ use wgt::{
 
 use crate::{
     api_log,
+    buffer_region_overrun::BufferRegionOverrunError,
     command::{
         clear_texture, encoder::EncodingState, ArcCommand, CommandEncoderError, EncoderStateError,
     },
@@ -47,24 +48,8 @@ pub enum TransferError {
     MissingBufferUsage(#[from] MissingBufferUsageError),
     #[error(transparent)]
     MissingTextureUsage(#[from] MissingTextureUsageError),
-    #[error("Start offset ({offset}) is out-of-bounds for buffer of size {buffer_size}")]
-    BufferStartOffsetOverrun {
-        offset: BufferAddress,
-        buffer_size: BufferAddress,
-        side: CopySide,
-    },
-    #[error(
-        "End offset (start at {} + size of {}) is out-of-bounds for buffer of size {}",
-        offset,
-        size,
-        buffer_size
-    )]
-    BufferEndOffsetOverrun {
-        offset: BufferAddress,
-        size: BufferAddress,
-        buffer_size: BufferAddress,
-        side: CopySide,
-    },
+    #[error("Transfer {_0}")]
+    BufferOverrun(BufferRegionOverrunError),
     #[error("Copy of {dimension:?} {start_offset}..{end_offset} would end up overrunning the bounds of the {side:?} texture of {dimension:?} size {texture_size}")]
     TextureOverrun {
         start_offset: u32,
@@ -192,9 +177,8 @@ impl WebGpuError for TransferError {
             Self::MissingTextureUsage(e) => e,
             Self::MemoryInitFailure(e) => e,
 
-            Self::BufferEndOffsetOverrun { .. }
+            Self::BufferOverrun { .. }
             | Self::TextureOverrun { .. }
-            | Self::BufferStartOffsetOverrun { .. }
             | Self::UnsupportedPartialTransfer { .. }
             | Self::InvalidCopyWithinSameTexture { .. }
             | Self::InvalidTextureAspect { .. }
@@ -352,22 +336,9 @@ pub(crate) fn validate_linear_texture_data(
         return Err(TransferError::UnspecifiedRowsPerImage);
     };
 
-    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::BufferEndOffsetOverrun {
-            offset,
-            size: bytes_in_copy,
-            buffer_size,
-            side: buffer_side,
-        });
-    }
+    // Avoid underflow in the subtraction by checking bytes_in_copy against buffer_size first.
+    let _ = BufferRegionOverrunError::check(offset, bytes_in_copy, buffer_size)
+        .map_err(|e| e.to_transfer_error(buffer_side))?;
 
     let is_contiguous = (row_stride_bytes == row_bytes_dense || !requires_multiple_rows)
         && (image_stride_bytes == image_bytes_dense || !requires_multiple_images);
@@ -1000,15 +971,10 @@ 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));
 
-    // 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 {
-            offset: destination_offset,
-            buffer_size: dst_buffer.size,
-            side: CopySide::Destination,
-        }
-        .into());
-    }
+    let src_end_offset_checker =
+        BufferRegionOverrunError::check_start(source_offset, src_buffer.size)
+            .map_err(|e| e.to_transfer_error(CopySide::Source))?;
+
     let size = size.unwrap_or_else(|| {
         // NOTE: Should never underflow because of our earlier check.
         src_buffer.size - source_offset
@@ -1045,40 +1011,13 @@ pub(super) fn copy_buffer_to_buffer(
         }
     }
 
-    if size > src_buffer.size - source_offset {
-        return Err(TransferError::BufferEndOffsetOverrun {
-            offset: source_offset,
-            size,
-            buffer_size: src_buffer.size,
-            side: CopySide::Source,
-        }
-        .into());
-    }
-    // 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 {
-            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 {
-            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;
+    let source_end_offset = src_end_offset_checker
+        .check_end(size)
+        .map_err(|e| e.to_transfer_error(CopySide::Source))?;
+
+    let destination_end_offset =
+        BufferRegionOverrunError::check(destination_offset, size, dst_buffer.size)
+            .map_err(|e| e.to_transfer_error(CopySide::Destination))?;
 
     // This must happen after parameter validation (so that errors are reported
     // as required by the spec), but before any side effects.

wgpu-core/src/device/queue.rs

@@ -18,6 +18,7 @@ use super::{life::LifetimeTracker, Device};
 use crate::device::trace::{Action, IntoTrace};
 use crate::{
     api_log,
+    buffer_region_overrun::BufferRegionOverrunError,
     command::{
         extract_texture_selector, validate_linear_texture_data, validate_texture_buffer_copy,
         validate_texture_copy_dst_format, validate_texture_copy_range, ClearError,
@@ -661,21 +662,8 @@ impl Queue {
             return Err(TransferError::UnalignedBufferOffset(buffer_offset));
         }
 
-        if buffer_offset > buffer.size {
-            return Err(TransferError::BufferStartOffsetOverrun {
-                offset: buffer_offset,
-                buffer_size: buffer_size.get(),
-                side: CopySide::Destination,
-            });
-        }
-        if buffer_size.get() > buffer.size - buffer_offset {
-            return Err(TransferError::BufferEndOffsetOverrun {
-                offset: buffer_offset,
-                size: buffer_size.get(),
-                buffer_size: buffer.size,
-                side: CopySide::Destination,
-            });
-        }
+        let _ = BufferRegionOverrunError::check(buffer_offset, buffer_size.get(), buffer.size)
+            .map_err(|e| e.to_transfer_error(CopySide::Destination))?;
 
         Ok(())
     }

wgpu-core/src/lib.rs

@@ -67,6 +67,7 @@ extern crate wgpu_types as wgt;
 
 mod as_hal;
 pub mod binding_model;
+pub(crate) mod buffer_region_overrun;
 pub mod command;
 mod conv;
 pub mod device;

wgpu-core/src/resource.rs

@@ -18,6 +18,7 @@ use wgt::{
 use crate::device::trace;
 use crate::{
     binding_model::{BindGroup, BindingError},
+    buffer_region_overrun::BufferRegionOverrunError,
     device::{
         queue, resource::DeferredDestroy, BufferMapPendingClosure, Device, DeviceError,
         DeviceMismatch, HostMap, MissingDownlevelFlags, MissingFeatures,
@@ -304,22 +305,8 @@ 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,
-    },
+    #[error("Map {_0}")]
+    MapOverrun(BufferRegionOverrunError),
 }
 
 impl WebGpuError for BufferAccessError {
@@ -341,8 +328,7 @@ impl WebGpuError for BufferAccessError {
             | Self::OutOfBoundsStartOffsetOverrun { .. }
             | Self::OutOfBoundsEndOffsetOverrun { .. }
             | Self::MapAborted
-            | Self::MapStartOffsetOverrun { .. }
-            | Self::MapEndOffsetOverrun { .. } => return ErrorType::Validation,
+            | Self::MapOverrun { .. } => return ErrorType::Validation,
         };
         e.webgpu_error_type()
     }
@@ -613,26 +599,12 @@ 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,
-                },
-            ));
-        }
+        let end_offset = match BufferRegionOverrunError::check(offset, range_size, self.size)
+            .map_err(BufferAccessError::MapOverrun)
+        {
+            Ok(ok) => ok,
+            Err(e) => return Err((op, e)),
+        };
 
         if !offset.is_multiple_of(wgt::MAP_ALIGNMENT)
             || !end_offset.is_multiple_of(wgt::COPY_BUFFER_ALIGNMENT)
@@ -671,8 +643,8 @@ impl Buffer {
                     return Err((op, BufferAccessError::MapAlreadyPending));
                 }
                 BufferMapState::Idle => BufferMapState::Waiting(BufferPendingMapping {
-                   // NOTE: Should never overflow because of our earlier check.
-                   range: offset..end_offset,
+                    // NOTE: Should never overflow because of our earlier check.
+                    range: offset..end_offset,
                     op,
                     _parent_buffer: self.clone(),
                 }),
@@ -726,20 +698,8 @@ 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,
-                    });
-                }
+                let _ = BufferRegionOverrunError::check(offset, range_size, self.size)
+                    .map_err(BufferAccessError::MapOverrun)?;
                 let ptr = unsafe { staging_buffer.ptr() };
                 let ptr = unsafe { NonNull::new_unchecked(ptr.as_ptr().offset(offset as isize)) };
                 Ok((ptr, range_size))