From 7d138e2e76c0d9bf1f1e573dbafb041bd533e76a Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Sun, 28 Aug 2022 17:07:04 -0700 Subject: [PATCH] Avoid overflow in check texture copy bounds. (#2963) --- .github/workflows/ci.yml | 2 + CHANGELOG.md | 1 + wgpu-core/src/command/transfer.rs | 75 ++++++++++++++++++------------- wgpu/tests/texture_bounds.rs | 65 ++++++++++++++++++++++++--- 4 files changed, 107 insertions(+), 36 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe0183342..6712b0339 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -225,6 +225,8 @@ jobs: for backend in ${{ matrix.backends }}; do echo "======= NATIVE TESTS $backend ======"; WGPU_BACKEND=$backend cargo nextest run -p wgpu --no-fail-fast + # Test that we catch overflows in `--release` builds too. + WGPU_BACKEND=$backend cargo nextest run --release -p wgpu --no-fail-fast done fmt: diff --git a/CHANGELOG.md b/CHANGELOG.md index 280e7092f..faf0cd74b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ the same every time it is rendered, we now warn if it is missing. #### General - Free `StagingBuffers` even when an error occurs in the operation that consumes them. By @jimblandy in [#2961](https://github.com/gfx-rs/wgpu/pull/2961) +- Avoid overflow when checking that texture copies fall within bounds. By @jimblandy in [#2963](https://github.com/gfx-rs/wgpu/pull/2963) - Improve the validation and error reporting of buffer mappings by @nical in [#2848](https://github.com/gfx-rs/wgpu/pull/2848) - Fix compilation errors when using wgpu-core in isolation while targetting `wasm32-unknown-unknown` by @Seamooo in [#2922](https://github.com/gfx-rs/wgpu/pull/2922) - Fixed opening of RenderDoc library by @abuffseagull in [#2930](https://github.com/gfx-rs/wgpu/pull/2930) diff --git a/wgpu-core/src/command/transfer.rs b/wgpu-core/src/command/transfer.rs index b9c6dd486..a1405009c 100644 --- a/wgpu-core/src/command/transfer.rs +++ b/wgpu-core/src/command/transfer.rs @@ -25,7 +25,7 @@ use std::iter; pub type ImageCopyBuffer = wgt::ImageCopyBuffer; pub type ImageCopyTexture = wgt::ImageCopyTexture; -#[derive(Clone, Debug)] +#[derive(Clone, Copy, Debug)] pub enum CopySide { Source, Destination, @@ -326,37 +326,52 @@ pub(crate) fn validate_texture_copy_range( _ => {} } - let x_copy_max = texture_copy_view.origin.x + copy_size.width; - if x_copy_max > extent.width { - return Err(TransferError::TextureOverrun { - start_offset: texture_copy_view.origin.x, - end_offset: x_copy_max, - texture_size: extent.width, - dimension: TextureErrorDimension::X, - side: texture_side, - }); - } - let y_copy_max = texture_copy_view.origin.y + copy_size.height; - if y_copy_max > extent.height { - return Err(TransferError::TextureOverrun { - start_offset: texture_copy_view.origin.y, - end_offset: y_copy_max, - texture_size: extent.height, - dimension: TextureErrorDimension::Y, - side: texture_side, - }); - } - let z_copy_max = texture_copy_view.origin.z + copy_size.depth_or_array_layers; - if z_copy_max > extent.depth_or_array_layers { - return Err(TransferError::TextureOverrun { - start_offset: texture_copy_view.origin.z, - end_offset: z_copy_max, - texture_size: extent.depth_or_array_layers, - dimension: TextureErrorDimension::Z, - side: texture_side, - }); + /// Return `Ok` if a run `size` texels long starting at `start_offset` falls + /// entirely within `texture_size`. Otherwise, return an appropriate a`Err`. + fn check_dimension( + dimension: TextureErrorDimension, + side: CopySide, + start_offset: u32, + size: u32, + texture_size: u32, + ) -> Result<(), TransferError> { + // Avoid underflow in the subtraction by checking start_offset against + // texture_size first. + if start_offset <= texture_size && size <= texture_size - start_offset { + Ok(()) + } else { + Err(TransferError::TextureOverrun { + start_offset, + end_offset: start_offset.wrapping_add(size), + texture_size, + dimension, + side, + }) + } } + check_dimension( + TextureErrorDimension::X, + texture_side, + texture_copy_view.origin.x, + copy_size.width, + extent.width, + )?; + check_dimension( + TextureErrorDimension::Y, + texture_side, + texture_copy_view.origin.y, + copy_size.height, + extent.height, + )?; + check_dimension( + TextureErrorDimension::Z, + texture_side, + texture_copy_view.origin.z, + copy_size.depth_or_array_layers, + extent.depth_or_array_layers, + )?; + if texture_copy_view.origin.x % block_width != 0 { return Err(TransferError::UnalignedCopyOriginX); } diff --git a/wgpu/tests/texture_bounds.rs b/wgpu/tests/texture_bounds.rs index 37fc3d82e..279681519 100644 --- a/wgpu/tests/texture_bounds.rs +++ b/wgpu/tests/texture_bounds.rs @@ -5,7 +5,7 @@ use std::num::NonZeroU32; #[test] fn bad_copy_origin() { - fn try_origin(origin: wgpu::Origin3d, should_panic: bool) { + fn try_origin(origin: wgpu::Origin3d, size: wgpu::Extent3d, should_panic: bool) { let mut parameters = TestParameters::default(); if should_panic { parameters = parameters.failure(); @@ -23,15 +23,68 @@ fn bad_copy_origin() { }, &data, BUFFER_COPY_LAYOUT, - TEXTURE_SIZE, + size, ); }); } - try_origin(wgpu::Origin3d { x: 0, y: 0, z: 0 }, false); - try_origin(wgpu::Origin3d { x: 1, y: 0, z: 0 }, true); - try_origin(wgpu::Origin3d { x: 0, y: 1, z: 0 }, true); - try_origin(wgpu::Origin3d { x: 0, y: 0, z: 1 }, true); + try_origin(wgpu::Origin3d { x: 0, y: 0, z: 0 }, TEXTURE_SIZE, false); + try_origin(wgpu::Origin3d { x: 1, y: 0, z: 0 }, TEXTURE_SIZE, true); + try_origin(wgpu::Origin3d { x: 0, y: 1, z: 0 }, TEXTURE_SIZE, true); + try_origin(wgpu::Origin3d { x: 0, y: 0, z: 1 }, TEXTURE_SIZE, true); + + try_origin( + wgpu::Origin3d { + x: TEXTURE_SIZE.width - 1, + y: TEXTURE_SIZE.height - 1, + z: TEXTURE_SIZE.depth_or_array_layers - 1, + }, + wgpu::Extent3d { + width: 1, + height: 1, + depth_or_array_layers: 1, + }, + false, + ); + try_origin( + wgpu::Origin3d { + x: u32::MAX, + y: 0, + z: 0, + }, + wgpu::Extent3d { + width: 1, + height: 1, + depth_or_array_layers: 1, + }, + true, + ); + try_origin( + wgpu::Origin3d { + x: u32::MAX, + y: 0, + z: 0, + }, + wgpu::Extent3d { + width: 1, + height: 1, + depth_or_array_layers: 1, + }, + true, + ); + try_origin( + wgpu::Origin3d { + x: u32::MAX, + y: 0, + z: 0, + }, + wgpu::Extent3d { + width: 1, + height: 1, + depth_or_array_layers: 1, + }, + true, + ); } const TEXTURE_SIZE: wgpu::Extent3d = wgpu::Extent3d {