From 41b3864ff5ba30c9d26b1c0eec53cb005b7c3298 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Mon, 10 Oct 2022 20:12:48 +0200 Subject: [PATCH 1/5] Add a test. --- wgpu/tests/root.rs | 1 + wgpu/tests/transfer.rs | 67 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 wgpu/tests/transfer.rs diff --git a/wgpu/tests/root.rs b/wgpu/tests/root.rs index 9f5068b723..d41032eb10 100644 --- a/wgpu/tests/root.rs +++ b/wgpu/tests/root.rs @@ -13,6 +13,7 @@ mod resource_descriptor_accessor; mod resource_error; mod shader_primitive_index; mod texture_bounds; +mod transfer; mod vertex_indices; mod write_texture; mod zero_init_texture_after_discard; diff --git a/wgpu/tests/transfer.rs b/wgpu/tests/transfer.rs new file mode 100644 index 0000000000..c18074bb2b --- /dev/null +++ b/wgpu/tests/transfer.rs @@ -0,0 +1,67 @@ +use crate::common::{initialize_test, TestParameters, fail}; + +#[test] +fn copy_overflow_z() { + // A simple crash test exercising validation that used to happen a bit too + // late, letting an integer overflow slip through. + initialize_test(TestParameters::default(), |ctx| { + let mut encoder = ctx + .device + .create_command_encoder(&wgpu::CommandEncoderDescriptor::default()); + + let t1 = ctx.device.create_texture(&wgpu::TextureDescriptor { + label: None, + dimension: wgpu::TextureDimension::D2, + size: wgpu::Extent3d { + width: 256, + height: 256, + depth_or_array_layers: 1, + }, + format: wgpu::TextureFormat::Rgba8Uint, + usage: wgpu::TextureUsages::COPY_DST, + mip_level_count: 1, + sample_count: 1, + }); + let t2 = ctx.device.create_texture(&wgpu::TextureDescriptor { + label: None, + dimension: wgpu::TextureDimension::D2, + size: wgpu::Extent3d { + width: 256, + height: 256, + depth_or_array_layers: 1, + }, + format: wgpu::TextureFormat::Rgba8Uint, + usage: wgpu::TextureUsages::COPY_DST, + mip_level_count: 1, + sample_count: 1, + }); + + fail(&ctx.device, || { + // Validation should catch the silly selected z layer range without panicking. + encoder.copy_texture_to_texture( + wgpu::ImageCopyTexture { + texture: &t1, + mip_level: 1, + origin: wgpu::Origin3d::ZERO, + aspect: wgpu::TextureAspect::All, + }, + wgpu::ImageCopyTexture { + texture: &t2, + mip_level: 1, + origin: wgpu::Origin3d { + x: 0, + y: 0, + z: 3824276442, + }, + aspect: wgpu::TextureAspect::All, + }, + wgpu::Extent3d { + width: 100, + height: 3, + depth_or_array_layers: 613286111, + }, + ); + ctx.queue.submit(Some(encoder.finish())); + }); + }) +} From 631ee7ddacdccae3c60257318c7e519ada36de53 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Mon, 10 Oct 2022 19:36:16 +0200 Subject: [PATCH 2/5] Validate texture copy ranges earlier. validate_texture_copy_range has a number of important checks on sizes that we want to make as early as possible to prevent integer overflows in other places such as transfer::extract_texture_selector which computes copy_texture.origin.z + copy_size.depth_or_array_layers (overflow found by fuzzers as always). The fix is to call validate_texture_copy_range earlier, in particular before extract_texture_selector. Unfortunately it led to more code churn than I would have liked. Thankfully it is mechanical and simple: Rather than getting the texture reference from Tracker::set_single, we grab it earlier (for validation) and pass it to set_single instead of the texture guard. As a bonus we are looking texture texture up less often than we used to. --- wgpu-core/src/command/clear.rs | 3 +- wgpu-core/src/command/transfer.rs | 105 +++++++++++++++++------------- wgpu-core/src/device/queue.rs | 10 +-- wgpu-core/src/track/texture.rs | 12 ++-- 4 files changed, 70 insertions(+), 60 deletions(-) diff --git a/wgpu-core/src/command/clear.rs b/wgpu-core/src/command/clear.rs index bf0c90ff49..3269de0312 100644 --- a/wgpu-core/src/command/clear.rs +++ b/wgpu-core/src/command/clear.rs @@ -273,9 +273,8 @@ pub(crate) fn clear_texture( // // We could in theory distinguish these two scenarios in the internal clear_texture api in order to remove this check and call the cheaper change_replace_tracked whenever possible. let dst_barrier = texture_tracker - .set_single(storage, dst_texture_id.0, selector, clear_usage) + .set_single(dst_texture, dst_texture_id.0, selector, clear_usage) .unwrap() - .1 .map(|pending| pending.into_hal(dst_texture)); unsafe { encoder.transition_textures(dst_barrier.into_iter()); diff --git a/wgpu-core/src/command/transfer.rs b/wgpu-core/src/command/transfer.rs index b1d0278ad0..04294e00d6 100644 --- a/wgpu-core/src/command/transfer.rs +++ b/wgpu-core/src/command/transfer.rs @@ -159,12 +159,8 @@ pub enum CopyError { pub(crate) fn extract_texture_selector( copy_texture: &ImageCopyTexture, copy_size: &Extent3d, - texture_guard: &Storage, TextureId>, + texture: &Texture, ) -> Result<(TextureSelector, hal::TextureCopyBase, wgt::TextureFormat), TransferError> { - let texture = texture_guard - .get(copy_texture.texture) - .map_err(|_| TransferError::InvalidTexture(copy_texture.texture))?; - let format = texture.desc.format; let copy_aspect = hal::FormatAspects::from(format) & hal::FormatAspects::from(copy_texture.aspect); @@ -664,8 +660,19 @@ impl Global { return Ok(()); } + let dst_texture = texture_guard + .get(destination.texture) + .map_err(|_| TransferError::InvalidTexture(destination.texture))?; + + let (hal_copy_size, array_layer_count) = validate_texture_copy_range( + destination, + &dst_texture.desc, + CopySide::Destination, + copy_size, + )?; + let (dst_range, dst_base, _) = - extract_texture_selector(destination, copy_size, &*texture_guard)?; + extract_texture_selector(destination, copy_size, dst_texture)?; // Handle texture init *before* dealing with barrier transitions so we have an easier time inserting "immediate-inits" that may be required by prior discards in rare cases. handle_dst_texture_init(cmd_buf, device, destination, copy_size, &texture_guard)?; @@ -684,11 +691,11 @@ impl Global { } let src_barrier = src_pending.map(|pending| pending.into_hal(src_buffer)); - let (dst_texture, dst_pending) = cmd_buf + let dst_pending = cmd_buf .trackers .textures .set_single( - &*texture_guard, + dst_texture, destination.texture, dst_range, hal::TextureUses::COPY_DST, @@ -706,12 +713,6 @@ impl Global { let dst_barrier = dst_pending.map(|pending| pending.into_hal(dst_texture)); let format_desc = dst_texture.desc.format.describe(); - let (hal_copy_size, array_layer_count) = validate_texture_copy_range( - destination, - &dst_texture.desc, - CopySide::Destination, - copy_size, - )?; let (required_buffer_bytes_in_copy, bytes_per_array_layer) = validate_linear_texture_data( &source.layout, dst_texture.desc.format, @@ -791,17 +792,23 @@ impl Global { return Ok(()); } - let (src_range, src_base, _) = - extract_texture_selector(source, copy_size, &*texture_guard)?; + let src_texture = texture_guard + .get(source.texture) + .map_err(|_| TransferError::InvalidTexture(source.texture))?; + + let (hal_copy_size, array_layer_count) = + validate_texture_copy_range(source, &src_texture.desc, CopySide::Source, copy_size)?; + + let (src_range, src_base, _) = extract_texture_selector(source, copy_size, src_texture)?; // Handle texture init *before* dealing with barrier transitions so we have an easier time inserting "immediate-inits" that may be required by prior discards in rare cases. handle_src_texture_init(cmd_buf, device, source, copy_size, &texture_guard)?; - let (src_texture, src_pending) = cmd_buf + let src_pending = cmd_buf .trackers .textures .set_single( - &*texture_guard, + src_texture, source.texture, src_range, hal::TextureUses::COPY_SRC, @@ -850,8 +857,6 @@ impl Global { let dst_barrier = dst_pending.map(|pending| pending.into_hal(dst_buffer)); let format_desc = src_texture.desc.format.describe(); - let (hal_copy_size, array_layer_count) = - validate_texture_copy_range(source, &src_texture.desc, CopySide::Source, copy_size)?; let (required_buffer_bytes_in_copy, bytes_per_array_layer) = validate_linear_texture_data( &destination.layout, src_texture.desc.format, @@ -948,10 +953,37 @@ impl Global { return Ok(()); } + let src_texture = texture_guard + .get(source.texture) + .map_err(|_| TransferError::InvalidTexture(source.texture))?; + let dst_texture = texture_guard + .get(destination.texture) + .map_err(|_| TransferError::InvalidTexture(source.texture))?; + + // src and dst texture format must be the same. + let src_format = src_texture.desc.format; + let dst_format = dst_texture.desc.format; + if src_format != dst_format { + return Err(TransferError::MismatchedTextureFormats { + src_format, + dst_format, + } + .into()); + } + + let (src_copy_size, array_layer_count) = + validate_texture_copy_range(source, &src_texture.desc, CopySide::Source, copy_size)?; + let (dst_copy_size, _) = validate_texture_copy_range( + destination, + &dst_texture.desc, + CopySide::Destination, + copy_size, + )?; + let (src_range, src_tex_base, _) = - extract_texture_selector(source, copy_size, &*texture_guard)?; + extract_texture_selector(source, copy_size, src_texture)?; let (dst_range, dst_tex_base, _) = - extract_texture_selector(destination, copy_size, &*texture_guard)?; + extract_texture_selector(destination, copy_size, dst_texture)?; if src_tex_base.aspect != dst_tex_base.aspect { return Err(TransferError::MismatchedAspects.into()); } @@ -960,11 +992,11 @@ impl Global { handle_src_texture_init(cmd_buf, device, source, copy_size, &texture_guard)?; handle_dst_texture_init(cmd_buf, device, destination, copy_size, &texture_guard)?; - let (src_texture, src_pending) = cmd_buf + let src_pending = cmd_buf .trackers .textures .set_single( - &*texture_guard, + src_texture, source.texture, src_range, hal::TextureUses::COPY_SRC, @@ -985,11 +1017,11 @@ impl Global { .into_iter() .collect(); - let (dst_texture, dst_pending) = cmd_buf + let dst_pending = cmd_buf .trackers .textures .set_single( - &*texture_guard, + dst_texture, destination.texture, dst_range, hal::TextureUses::COPY_DST, @@ -1005,29 +1037,8 @@ impl Global { ); } - // src and dst texture format must be the same. - let src_format = src_texture.desc.format; - let dst_format = dst_texture.desc.format; - - if src_format != dst_format { - return Err(TransferError::MismatchedTextureFormats { - src_format, - dst_format, - } - .into()); - } - barriers.extend(dst_pending.map(|pending| pending.into_hal(dst_texture))); - let (src_copy_size, array_layer_count) = - validate_texture_copy_range(source, &src_texture.desc, CopySide::Source, copy_size)?; - let (dst_copy_size, _) = validate_texture_copy_range( - destination, - &dst_texture.desc, - CopySide::Destination, - copy_size, - )?; - let hal_copy_size = hal::CopyExtent { width: src_copy_size.width.min(dst_copy_size.width), height: src_copy_size.height.min(dst_copy_size.height), diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index 748f1057da..c77bd2d6e9 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -588,8 +588,10 @@ impl Global { } let (mut texture_guard, _) = hub.textures.write(&mut token); // For clear we need write access to the texture. TODO: Can we acquire write lock later? + let dst = texture_guard.get_mut(destination.texture).unwrap(); + let (selector, dst_base, texture_format) = - extract_texture_selector(destination, size, &*texture_guard)?; + extract_texture_selector(destination, size, dst)?; let format_desc = texture_format.describe(); //Note: `_source_bytes_per_array_layer` is ignored since we have a staging copy, // and it can have a different value. @@ -631,7 +633,6 @@ impl Global { (size.depth_or_array_layers - 1) * block_rows_per_image + height_blocks; let stage_size = stage_bytes_per_row as u64 * block_rows_in_copy as u64; - let dst = texture_guard.get_mut(destination.texture).unwrap(); if !dst.desc.usage.contains(wgt::TextureUsages::COPY_DST) { return Err( TransferError::MissingCopyDstUsageFlag(None, Some(destination.texture)).into(), @@ -678,10 +679,11 @@ impl Global { } } - let (dst, transition) = trackers + let dst = texture_guard.get(destination.texture).unwrap(); + let transition = trackers .textures .set_single( - &*texture_guard, + dst, destination.texture, selector, hal::TextureUses::COPY_DST, diff --git a/wgpu-core/src/track/texture.rs b/wgpu-core/src/track/texture.rs index 4ad833dc05..17f4e57964 100644 --- a/wgpu-core/src/track/texture.rs +++ b/wgpu-core/src/track/texture.rs @@ -517,15 +517,13 @@ impl TextureTracker { /// /// If the ID is higher than the length of internal vectors, /// the vectors will be extended. A call to set_size is not needed. - pub fn set_single<'a>( + pub fn set_single( &mut self, - storage: &'a hub::Storage, TextureId>, + texture: &Texture, id: TextureId, selector: TextureSelector, new_state: TextureUses, - ) -> Option<(&'a Texture, Drain<'_, PendingTransition>)> { - let texture = storage.get(id).ok()?; - + ) -> Option>> { let (index32, epoch, _) = id.unzip(); let index = index32 as usize; @@ -535,7 +533,7 @@ impl TextureTracker { unsafe { insert_or_barrier_update( - texture_data_from_texture(storage, index32), + (&texture.life_guard, &texture.full_range), Some(&mut self.start_set), &mut self.end_set, &mut self.metadata, @@ -551,7 +549,7 @@ impl TextureTracker { ) } - Some((texture, self.temp.drain(..))) + Some(self.temp.drain(..)) } /// Sets the given state for all texture in the given tracker. From 4084d8a0f616647d27a2731726aa976ddaa7faa7 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Mon, 10 Oct 2022 20:24:13 +0200 Subject: [PATCH 3/5] Add an entry in the changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba1c3cd815..432e5f13c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ Bottom level categories: - Bother to free the `hal::Api::CommandBuffer` when a `wgpu_core::command::CommandEncoder` is dropped. By @jimblandy in [#3069](https://github.com/gfx-rs/wgpu/pull/3069). - Fixed the mipmap example by adding the missing WRITE_TIMESTAMP_INSIDE_PASSES feature. By @Olaroll in [#3081](https://github.com/gfx-rs/wgpu/pull/3081). - Avoid panicking in some interactions with invalid resources by @nical in (#3094)[https://github.com/gfx-rs/wgpu/pull/3094] +- Fixed an integer overflow in `copy_texture_to_texture` by @nical [#3090](https://github.com/gfx-rs/wgpu/pull/3090) #### WebGPU - Use `log` instead of `println` in hello example by @JolifantoBambla in [#2858](https://github.com/gfx-rs/wgpu/pull/2858) From 3c8ed516d5c5251f7add25d3e6e625757207c9e9 Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Wed, 30 Nov 2022 20:02:07 -0500 Subject: [PATCH 4/5] Touchup --- wgpu-core/src/device/queue.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index cf8c721b85..ccb6c8838f 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -601,7 +601,6 @@ impl Global { extract_texture_selector(destination, size, dst)?; let format_desc = texture_format.describe(); - let dst = texture_guard.get_mut(destination.texture).unwrap(); if !dst.desc.usage.contains(wgt::TextureUsages::COPY_DST) { return Err( TransferError::MissingCopyDstUsageFlag(None, Some(destination.texture)).into(), From f51052ccdfb49f0bf5cf1a0f7bf032fabec2427a Mon Sep 17 00:00:00 2001 From: Connor Fitzgerald Date: Wed, 30 Nov 2022 20:10:27 -0500 Subject: [PATCH 5/5] Format --- wgpu/tests/transfer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wgpu/tests/transfer.rs b/wgpu/tests/transfer.rs index c18074bb2b..1e94a31ff1 100644 --- a/wgpu/tests/transfer.rs +++ b/wgpu/tests/transfer.rs @@ -1,4 +1,4 @@ -use crate::common::{initialize_test, TestParameters, fail}; +use crate::common::{fail, initialize_test, TestParameters}; #[test] fn copy_overflow_z() {