Check the math for buffer-texture copies

Author: andyleiserson

cts_runner/test.lst

@@ -43,6 +43,7 @@ webgpu:api,validation,encoding,programmable,pipeline_bind_group_compat:bind_grou
 webgpu:api,validation,encoding,programmable,pipeline_bind_group_compat:bind_groups_and_pipeline_layout_mismatch:encoderType="render%20pass";*
 webgpu:api,validation,encoding,programmable,pipeline_bind_group_compat:buffer_binding,render_pipeline:*
 webgpu:api,validation,encoding,programmable,pipeline_bind_group_compat:sampler_binding,render_pipeline:*
+webgpu:api,validation,image_copy,layout_related:copy_end_overflows_u64:*
 webgpu:api,validation,image_copy,texture_related:format:dimension="1d";*
 webgpu:api,validation,queue,submit:command_buffer,device_mismatch:*
 webgpu:api,validation,queue,submit:command_buffer,duplicate_buffers:*

wgpu-core/src/command/transfer.rs

@@ -4,7 +4,8 @@ use arrayvec::ArrayVec;
 use thiserror::Error;
 use wgt::{
     error::{ErrorType, WebGpuError},
-    BufferAddress, BufferUsages, Extent3d, TextureSelector, TextureUsages,
+    BufferAddress, BufferTextureCopyInfoError, BufferUsages, Extent3d, TextureSelector,
+    TextureUsages,
 };
 
 #[cfg(feature = "trace")]
@@ -95,6 +96,8 @@ pub enum TransferError {
     InvalidBytesPerRow,
     #[error("Number of rows per image is invalid")]
     InvalidRowsPerImage,
+    #[error("Overflow while computing the size of the copy")]
+    SizeOverflow,
     #[error("Copy source aspects must refer to all aspects of the source texture format")]
     CopySrcMissingAspects,
     #[error(
@@ -164,6 +167,7 @@ impl WebGpuError for TransferError {
             | Self::UnspecifiedRowsPerImage
             | Self::InvalidBytesPerRow
             | Self::InvalidRowsPerImage
+            | Self::SizeOverflow
             | Self::CopySrcMissingAspects
             | Self::CopyDstMissingAspects
             | Self::CopyAspectNotOne
@@ -181,6 +185,18 @@ impl WebGpuError for TransferError {
     }
 }
 
+impl From<BufferTextureCopyInfoError> for TransferError {
+    fn from(value: BufferTextureCopyInfoError) -> Self {
+        match value {
+            BufferTextureCopyInfoError::InvalidBytesPerRow => Self::InvalidBytesPerRow,
+            BufferTextureCopyInfoError::InvalidRowsPerImage => Self::InvalidRowsPerImage,
+            BufferTextureCopyInfoError::ImageStrideOverflow
+            | BufferTextureCopyInfoError::ImageBytesOverflow(_)
+            | BufferTextureCopyInfoError::ArraySizeOverflow(_) => Self::SizeOverflow,
+        }
+    }
+}
+
 pub(crate) fn extract_texture_selector<T>(
     copy_texture: &wgt::TexelCopyTextureInfo<T>,
     copy_size: &Extent3d,
@@ -254,7 +270,7 @@ pub(crate) fn validate_linear_texture_data(
         width_blocks: _,
         height_blocks,
 
-        row_bytes_dense,
+        row_bytes_dense: _,
         row_stride_bytes,
 
         image_stride_rows: _,
@@ -264,7 +280,7 @@ pub(crate) fn validate_linear_texture_data(
         image_bytes_dense: _,
 
         bytes_in_copy,
-    } = layout.get_buffer_texture_copy_info(format, aspect, copy_size);
+    } = layout.get_buffer_texture_copy_info(format, aspect, copy_size)?;
 
     if copy_width % block_width_texels != 0 {
         return Err(TransferError::UnalignedCopyWidth);
@@ -276,21 +292,15 @@ pub(crate) fn validate_linear_texture_data(
     let requires_multiple_rows = depth_or_array_layers > 1 || height_blocks > 1;
     let requires_multiple_images = depth_or_array_layers > 1;
 
-    if let Some(raw_bytes_per_row) = layout.bytes_per_row {
-        let raw_bytes_per_row = raw_bytes_per_row as BufferAddress;
-        if raw_bytes_per_row < row_bytes_dense {
-            return Err(TransferError::InvalidBytesPerRow);
-        }
-    } else if requires_multiple_rows {
+    // `get_buffer_texture_copy_info()` already proceeded with defaults if these
+    // were not specified, and ensured that the values satisfy the minima if
+    // they were, but now we enforce the WebGPU requirement that they be
+    // specified any time they apply.
+    if layout.bytes_per_row.is_none() && requires_multiple_rows {
         return Err(TransferError::UnspecifiedBytesPerRow);
     }
 
-    if let Some(raw_rows_per_image) = layout.rows_per_image {
-        let raw_rows_per_image = raw_rows_per_image as BufferAddress;
-        if raw_rows_per_image < height_blocks {
-            return Err(TransferError::InvalidRowsPerImage);
-        }
-    } else if requires_multiple_images {
+    if layout.rows_per_image.is_none() && requires_multiple_images {
         return Err(TransferError::UnspecifiedRowsPerImage);
     };
 

wgpu-types/src/transfers.rs

@@ -2,46 +2,81 @@ use crate::{BufferAddress, Extent3d, TexelCopyBufferLayout, TextureAspect, Textu
 
 impl TexelCopyBufferLayout {
     /// Extract a variety of information about the given copy operation.
+    ///
+    /// Returns an error if the size of the copy overflows a `u64`, or if the arguments are
+    /// not valid in conjunction with the `bytes_per_row` or `rows_per_image` parameters in
+    /// `self`.
+    ///
+    /// This is public for use by `wgpu-core` and `wgpu-hal`, it is not a stable API.
+    ///
+    /// Although WebGPU requires that `bytes_per_row` and `rows_per_image` be specified in
+    /// cases where they apply, we are more lenient here (although it's not clear if that is
+    /// necessary). Our caller, `validate_linear_texture_data`, enforces this and other
+    /// WebGPU requirements on the copy parameters that we do not check here.
+    #[doc(hidden)]
     #[inline(always)]
     pub fn get_buffer_texture_copy_info(
         &self,
         format: TextureFormat,
         aspect: TextureAspect,
         copy_size: &Extent3d,
-    ) -> BufferTextureCopyInfo {
-        // Convert all inputs to BufferAddress (u64) to avoid some of the overflow issues
-        // Note: u64 is not always enough to prevent overflow, especially when multiplying
-        // something with a potentially large depth value, so it is preferable to validate
-        // the copy size before calling this function (for example via `validate_texture_copy_range`).
-        let copy_width = copy_size.width as BufferAddress;
-        let copy_height = copy_size.height as BufferAddress;
-        let depth_or_array_layers = copy_size.depth_or_array_layers as BufferAddress;
-
-        let block_size_bytes = format.block_copy_size(Some(aspect)).unwrap() as BufferAddress;
+    ) -> Result<BufferTextureCopyInfo, Error> {
+        let copy_width = BufferAddress::from(copy_size.width);
+        let copy_height = BufferAddress::from(copy_size.height);
+        let depth_or_array_layers = BufferAddress::from(copy_size.depth_or_array_layers);
+
+        let block_size_bytes = BufferAddress::from(format.block_copy_size(Some(aspect)).unwrap());
         let (block_width, block_height) = format.block_dimensions();
-        let block_width_texels = block_width as BufferAddress;
-        let block_height_texels = block_height as BufferAddress;
+        let block_width_texels = BufferAddress::from(block_width);
+        let block_height_texels = BufferAddress::from(block_height);
 
         let width_blocks = copy_width.div_ceil(block_width_texels);
         let height_blocks = copy_height.div_ceil(block_height_texels);
 
+        // The spec calls this bytesInLastRow.
         let row_bytes_dense = width_blocks * block_size_bytes;
-
-        let row_stride_bytes = self.bytes_per_row.map_or(row_bytes_dense, u64::from);
-        let image_stride_rows = self.rows_per_image.map_or(height_blocks, u64::from);
-
-        let image_stride_bytes = row_stride_bytes * image_stride_rows;
+        let row_stride_bytes = match self.bytes_per_row.map(BufferAddress::from) {
+            Some(bytes_per_row) if bytes_per_row >= row_bytes_dense => bytes_per_row,
+            Some(_) => return Err(Error::InvalidBytesPerRow),
+            None => row_bytes_dense,
+        };
 
         let image_rows_dense = height_blocks;
-        let image_bytes_dense =
-            image_rows_dense.saturating_sub(1) * row_stride_bytes + row_bytes_dense;
+        let image_stride_rows = match self.rows_per_image.map(BufferAddress::from) {
+            Some(rows_per_image) if rows_per_image >= image_rows_dense => rows_per_image,
+            Some(_) => return Err(Error::InvalidRowsPerImage),
+            None => image_rows_dense,
+        };
 
-        let mut bytes_in_copy = image_stride_bytes * depth_or_array_layers.saturating_sub(1);
-        if height_blocks > 0 {
-            bytes_in_copy += row_stride_bytes * (height_blocks - 1) + row_bytes_dense;
-        }
+        let image_bytes_dense = match image_rows_dense.checked_sub(1) {
+            Some(rows_minus_one) => rows_minus_one
+                .checked_mul(row_stride_bytes)
+                .ok_or(Error::ImageBytesOverflow(false))?
+                .checked_add(row_bytes_dense)
+                .ok_or(Error::ImageBytesOverflow(true))?,
+            None => 0,
+        };
+
+        // It is possible that `image_stride_bytes` overflows, but the actual
+        // copy size does not, when the copy only has a single layer and
+        // `image_size_bytes` is not used. We don't worry about handling this
+        // gracefully because WebGPU texture size limits should keep things out
+        // of this realm entirely.
+        let image_stride_bytes = row_stride_bytes
+            .checked_mul(image_stride_rows)
+            .ok_or(Error::ImageStrideOverflow)?;
+
+        let bytes_in_copy = if depth_or_array_layers <= 1 {
+            depth_or_array_layers * image_bytes_dense
+        } else {
+            (depth_or_array_layers - 1)
+                .checked_mul(image_stride_bytes)
+                .ok_or(Error::ArraySizeOverflow(false))?
+                .checked_add(image_bytes_dense)
+                .ok_or(Error::ArraySizeOverflow(true))?
+        };
 
-        BufferTextureCopyInfo {
+        Ok(BufferTextureCopyInfo {
             copy_width,
             copy_height,
             depth_or_array_layers,
@@ -65,7 +100,7 @@ impl TexelCopyBufferLayout {
             image_bytes_dense,
 
             bytes_in_copy,
-        }
+        })
     }
 }
 
@@ -127,16 +162,55 @@ pub struct BufferTextureCopyInfo {
     pub bytes_in_copy: u64,
 }
 
+/// Errors that can occur while populating `BufferTextureCopyInfo`.
+//
+// We use the additional detail provided by these errors (over wgpu-core's
+// `TransferError`) to improve the reliability of the tests in this module. It
+// doesn't seem worth plumbing them upwards, because at the API level it
+// shouldn't be possible to exceed them without exceeding the WebGPU limits on
+// texture dimension. But the WebGPU limits are not currently enforced, so we
+// have to do something here to protect against overflows.
+//
+// Even when the WebGPU limits are enforced, it may still be useful to keep the
+// checks here as a failsafe if the correctness of the primary limit enforcement
+// is not immediately apparent.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum BufferTextureCopyInfoError {
+    /// The `bytes_per_row` is too small for the texture width.
+    InvalidBytesPerRow,
+    /// The `rows_per_image` is too small for the texture height.
+    InvalidRowsPerImage,
+    /// The image stride overflows a `u64`.
+    ImageStrideOverflow,
+    /// The last-layer byte size overflows a `u64`.
+    ///
+    /// The bool value indicates whether the multiplication (false) or the
+    /// addition (true) overflowed.
+    ImageBytesOverflow(bool),
+    /// The total size of the copy overflows a `u64`.
+    ///
+    /// The bool value indicates whether the multiplication (false) or the
+    /// addition (true) overflowed.
+    ArraySizeOverflow(bool),
+}
+type Error = BufferTextureCopyInfoError;
+
 #[cfg(test)]
 mod tests {
     use super::*;
 
+    #[derive(Clone)]
     struct LTDTest {
         layout: TexelCopyBufferLayout,
         format: TextureFormat,
         aspect: TextureAspect,
         copy_size: Extent3d,
         expected_result: BufferTextureCopyInfo,
+        // Normally a Result<BufferTextureCopyInfo, Error> would be make sense,
+        // but since the existing tests were written to mutate
+        // `LTDTest.expected_result`, keeping this separate avoids a bunch of
+        // `unwrap`s.
+        expected_error: Option<Error>,
     }
 
     impl LTDTest {
@@ -145,7 +219,11 @@ mod tests {
             let linear_texture_data =
                 self.layout
                     .get_buffer_texture_copy_info(self.format, self.aspect, &self.copy_size);
-            assert_eq!(linear_texture_data, self.expected_result);
+            let expected = match self.expected_error {
+                Some(err) => Err(err),
+                None => Ok(self.expected_result),
+            };
+            assert_eq!(linear_texture_data, expected);
         }
     }
 
@@ -182,6 +260,7 @@ mod tests {
                 image_bytes_dense: 16,
                 bytes_in_copy: 16,
             },
+            expected_error: None,
         };
 
         test.run();
@@ -211,7 +290,7 @@ mod tests {
 
     #[test]
     fn linear_texture_data_2d_3d_copy() {
-        let mut test = LTDTest {
+        let template = LTDTest {
             layout: TexelCopyBufferLayout {
                 offset: 0,
                 bytes_per_row: None,
@@ -242,8 +321,10 @@ mod tests {
                 image_bytes_dense: 4 * 7 * 12,
                 bytes_in_copy: 4 * 7 * 12,
             },
+            expected_error: None,
         };
 
+        let mut test = template.clone();
         test.run();
 
         // Changing bytes_per_row changes a number of other properties.
@@ -252,12 +333,71 @@ mod tests {
         test.expected_result.image_stride_bytes = 48 * 12;
         test.expected_result.image_bytes_dense = 48 * 11 + (4 * 7);
         test.expected_result.bytes_in_copy = 48 * 11 + (4 * 7);
+        test.run();
 
         // Making this a 3D copy only changes the depth_or_array_layers and the bytes_in_copy.
         test.copy_size.depth_or_array_layers = 4;
         test.expected_result.depth_or_array_layers = 4;
         test.expected_result.bytes_in_copy = 48 * 12 * 3 + 48 * 11 + (4 * 7); // 4 layers
+        test.run();
+
+        // Changing rows_per_image
+        test.layout.rows_per_image = Some(20);
+        test.expected_result.image_stride_rows = 20;
+        test.expected_result.image_stride_bytes = 20 * test.expected_result.row_stride_bytes;
+        test.expected_result.bytes_in_copy = 48 * 20 * 3 + 48 * 11 + (4 * 7); // 4 layers
+        test.run();
+
+        // Invalid because the row stride is too small.
+        let mut test = template.clone();
+        test.layout.bytes_per_row = Some(20);
+        test.expected_error = Some(Error::InvalidBytesPerRow);
+        test.run();
+
+        // Invalid because the image stride is too small.
+        let mut test = template.clone();
+        test.layout.rows_per_image = Some(8);
+        test.expected_error = Some(Error::InvalidRowsPerImage);
+        test.run();
+
+        // Invalid because width * height * texel_size_bytes overflows.
+        let mut test = template.clone();
+        test.copy_size.width = u32::MAX;
+        test.copy_size.height = u32::MAX;
+        test.expected_error = Some(Error::ImageBytesOverflow(false));
+        test.run();
+
+        // Invalid because the addition of row_bytes_dense overflows.
+        // (But the product rows_minus_one * row_stride_bytes does not overflow.)
+        let mut test = template.clone();
+        test.copy_size.width = 0x8000_0000;
+        test.copy_size.height = 0x8000_0000;
+        test.expected_error = Some(Error::ImageBytesOverflow(true));
+        test.run();
+
+        // Invalid because image_stride_bytes overflows.
+        let mut test = template.clone();
+        test.copy_size.width = 0x8000_0000;
+        test.layout.rows_per_image = Some(0x8000_0000);
+        test.expected_result.image_stride_rows = 0x8000_0000;
+        test.expected_error = Some(Error::ImageStrideOverflow);
+        test.run();
+
+        // Invalid because (layers - 1) * image_stride_bytes overflows.
+        let mut test = template.clone();
+        test.copy_size.depth_or_array_layers = 0x8000_0000;
+        test.copy_size.width = 0x1_0000;
+        test.copy_size.height = 0x1_0000;
+        test.expected_error = Some(Error::ArraySizeOverflow(false));
+        test.run();
 
+        // Invalid because the total size of the copy overflows (but the product
+        // (layers - 1) * image_stride_bytes does not overflow).
+        let mut test = template.clone();
+        test.copy_size.depth_or_array_layers = 0x3fff_8001;
+        test.copy_size.width = 0x1_0001;
+        test.copy_size.height = 0x1_0001;
+        test.expected_error = Some(Error::ArraySizeOverflow(true));
         test.run();
     }
 
@@ -294,6 +434,7 @@ mod tests {
                 image_bytes_dense: 8 * 2 * 4,
                 bytes_in_copy: 8 * 2 * 4,
             },
+            expected_error: None,
         };
 
         test.run();