diff --git a/CHANGELOG.md b/CHANGELOG.md index 153058845f..030960c49c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,6 +109,7 @@ Additionally `Surface::get_default_config` now returns an Option and returns Non - 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) - Remove `wgpu_types::Features::DEPTH24PLUS_STENCIL8`, making `wgpu::TextureFormat::Depth24PlusStencil8` available on all backends. By @Healthire in (#3151)[https://github.com/gfx-rs/wgpu/pull/3151] - Fix an integer overflow in `queue_write_texture` by @nical in (#3146)[https://github.com/gfx-rs/wgpu/pull/3146] - Make `RenderPassCompatibilityError` and `CreateShaderModuleError` not so huge. By @jimblandy in (#3226)[https://github.com/gfx-rs/wgpu/pull/3226] diff --git a/wgpu-core/src/command/clear.rs b/wgpu-core/src/command/clear.rs index 52d02a889e..aa9d60638e 100644 --- a/wgpu-core/src/command/clear.rs +++ b/wgpu-core/src/command/clear.rs @@ -280,9 +280,8 @@ pub(crate) fn clear_texture( // 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 44112221c4..063e33a0a0 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); @@ -710,8 +706,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 @@ -732,11 +739,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, @@ -754,12 +761,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, @@ -839,19 +840,25 @@ 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, @@ -900,8 +907,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, @@ -998,10 +1003,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()); } @@ -1012,11 +1044,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, @@ -1037,11 +1069,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, @@ -1057,29 +1089,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 091128cba2..ccb6c8838f 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -594,14 +594,13 @@ impl Global { return Ok(()); } - // For clear we need write access to the texture. - // TODO: Can we acquire write lock later? - let (mut texture_guard, _) = hub.textures.write(&mut token); + 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(); - 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(), @@ -655,6 +654,12 @@ 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; + if !dst.desc.usage.contains(wgt::TextureUsages::COPY_DST) { + return Err( + TransferError::MissingCopyDstUsageFlag(None, Some(destination.texture)).into(), + ); + } + let mut trackers = device.trackers.lock(); let encoder = device.pending_writes.activate(); @@ -698,10 +703,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 5b22fb527f..bc0eb2875e 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. diff --git a/wgpu/tests/root.rs b/wgpu/tests/root.rs index eae0332fcc..e721e3a7a7 100644 --- a/wgpu/tests/root.rs +++ b/wgpu/tests/root.rs @@ -16,6 +16,7 @@ mod resource_error; mod shader; 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..1e94a31ff1 --- /dev/null +++ b/wgpu/tests/transfer.rs @@ -0,0 +1,67 @@ +use crate::common::{fail, initialize_test, TestParameters}; + +#[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())); + }); + }) +}