diff --git a/cts_runner/test.lst b/cts_runner/test.lst index 3d68ea9a4..163944db9 100644 --- a/cts_runner/test.lst +++ b/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:* diff --git a/wgpu-core/src/command/transfer.rs b/wgpu-core/src/command/transfer.rs index 35a0c0265..dcb1347b0 100644 --- a/wgpu-core/src/command/transfer.rs +++ b/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 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( copy_texture: &wgt::TexelCopyTextureInfo, 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); }; diff --git a/wgpu-types/src/transfers.rs b/wgpu-types/src/transfers.rs index 8ca32ce3b..2e40958c1 100644 --- a/wgpu-types/src/transfers.rs +++ b/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; + ) -> Result { + 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 = format.block_copy_size(Some(aspect)).unwrap() as BufferAddress; + 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, + }; - BufferTextureCopyInfo { + // 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))? + }; + + 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 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, } 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();