Validate texture copy ranges earlier to prevent integer overflow (#3090)

Co-authored-by: Connor Fitzgerald <[email protected]>
gfx-rs · Dec 1, 2022 · e90aace · e90aace
1 parent 46b1216
commit e90aace
Show file tree

Hide file tree

Showing 7 changed files with 146 additions and 63 deletions.
diff --git 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
@@ -280,9 +280,8 @@ pub(crate) fn clear_texture<A: HalApi>(
     // 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
@@ -159,12 +159,8 @@ pub enum CopyError {
 pub(crate) fn extract_texture_selector<A: hal::Api>(
     copy_texture: &ImageCopyTexture,
     copy_size: &Extent3d,
-    texture_guard: &Storage<Texture<A>, TextureId>,
+    texture: &Texture<A>,
 ) -> 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<G: GlobalIdentityHandlerFactory> Global<G> {
             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<G: GlobalIdentityHandlerFactory> Global<G> {
         }
         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<G: GlobalIdentityHandlerFactory> Global<G> {
         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<G: GlobalIdentityHandlerFactory> Global<G> {
             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<G: GlobalIdentityHandlerFactory> Global<G> {
         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<G: GlobalIdentityHandlerFactory> Global<G> {
             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<G: GlobalIdentityHandlerFactory> Global<G> {
         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<G: GlobalIdentityHandlerFactory> Global<G> {
             .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<G: GlobalIdentityHandlerFactory> Global<G> {
             );
         }
 
-        // 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
@@ -594,14 +594,13 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
             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<G: GlobalIdentityHandlerFactory> Global<G> {
             (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<G: GlobalIdentityHandlerFactory> Global<G> {
             }
         }
 
-        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
@@ -517,15 +517,13 @@ impl<A: hub::HalApi> TextureTracker<A> {
     ///
     /// 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<Texture<A>, TextureId>,
+        texture: &Texture<A>,
         id: TextureId,
         selector: TextureSelector,
         new_state: TextureUses,
-    ) -> Option<(&'a Texture<A>, Drain<'_, PendingTransition<TextureUses>>)> {
-        let texture = storage.get(id).ok()?;
-
+    ) -> Option<Drain<'_, PendingTransition<TextureUses>>> {
         let (index32, epoch, _) = id.unzip();
         let index = index32 as usize;
 
@@ -535,7 +533,7 @@ impl<A: hub::HalApi> TextureTracker<A> {
 
         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<A: hub::HalApi> TextureTracker<A> {
             )
         }
 
-        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
@@ -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
@@ -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()));
+        });
+    })
+}