From 5805bde955ca28c3ef7a4b71515e375d03bd4c37 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Wed, 21 Feb 2024 15:20:50 -0500 Subject: [PATCH 1/4] =?UTF-8?q?style:=20fix=20fmt.=20of=20`assert!(?= =?UTF-8?q?=E2=80=A6)`=20in=20`clear=5Ftexture=5Fvia=5Fbuffer=5Fcopies`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- wgpu-core/src/command/clear.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wgpu-core/src/command/clear.rs b/wgpu-core/src/command/clear.rs index 2569fea1a4..8d18b30389 100644 --- a/wgpu-core/src/command/clear.rs +++ b/wgpu-core/src/command/clear.rs @@ -366,7 +366,7 @@ fn clear_texture_via_buffer_copies( assert!( max_rows_per_copy > 0, "Zero buffer size is too small to fill a single row \ - of a texture with format {:?} and desc {:?}", + of a texture with format {:?} and desc {:?}", texture_desc.format, texture_desc.size ); From 426aa0cbedaee7766c54b3673b3679323997c114 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Wed, 21 Feb 2024 15:03:16 -0500 Subject: [PATCH 2/4] refactor: `command_encoder_clear_buffer`: s/end/end_offset --- wgpu-core/src/command/clear.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wgpu-core/src/command/clear.rs b/wgpu-core/src/command/clear.rs index 8d18b30389..7d09754205 100644 --- a/wgpu-core/src/command/clear.rs +++ b/wgpu-core/src/command/clear.rs @@ -131,11 +131,11 @@ impl Global { } } - let end = match size { + let end_offset = match size { Some(size) => offset + size, None => dst_buffer.size, }; - if offset == end { + if offset == end_offset { log::trace!("Ignoring fill_buffer of size 0"); return Ok(()); } @@ -144,7 +144,7 @@ impl Global { cmd_buf_data.buffer_memory_init_actions.extend( dst_buffer.initialization_status.read().create_action( &dst_buffer, - offset..end, + offset..end_offset, MemoryInitKind::ImplicitlyInitialized, ), ); @@ -154,7 +154,7 @@ impl Global { let cmd_buf_raw = cmd_buf_data.encoder.open()?; unsafe { cmd_buf_raw.transition_buffers(dst_barrier.into_iter()); - cmd_buf_raw.clear_buffer(dst_raw, offset..end); + cmd_buf_raw.clear_buffer(dst_raw, offset..end_offset); } Ok(()) } From c1dae9c278ebf74c4d3e167d3dfbc1dfc3dd1878 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Wed, 21 Feb 2024 15:07:36 -0500 Subject: [PATCH 3/4] fix: always check buffer clear `offset` for OOB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fuzz testing in Firefox encountered crashes for calls of `Global::command_encoder_clear_buffer` where: * `offset` is greater than `buffer.size`, but… * `size` is `None`. Oops! We should _always_ check this (i.e., even when `size` is `None`), because we have no guarantee that `offset` and the fallback value of `size` is in bounds. 😅 So, we change validation here to unconditionally compute `size` and run checks we previously gated behind `if let Some(size) = size { … }`. For convenience, the spec. link for this method: --- CHANGELOG.md | 1 + tests/tests/buffer.rs | 27 +++++++++++++++++++++++++++ wgpu-core/src/command/clear.rs | 28 ++++++++++++---------------- 3 files changed, 40 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8d63cadb6..f5262a1368 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -119,6 +119,7 @@ Bottom level categories: - Fix timeout when presenting a surface where no work has been done. By @waywardmonkeys in [#5200](https://github.com/gfx-rs/wgpu/pull/5200) - Simplify and speed up the allocation of internal IDs. By @nical in [#5229](https://github.com/gfx-rs/wgpu/pull/5229) - Fix an issue where command encoders weren't properly freed if an error occurred during command encoding. By @ErichDonGubler in [#5251](https://github.com/gfx-rs/wgpu/pull/5251). +- Fix missing validation for `Device::clear_buffer` where `offset + size buffer.size` was not checked when `size` was omitted. By @ErichDonGubler in [#5282](https://github.com/gfx-rs/wgpu/pull/5282). #### WGL diff --git a/tests/tests/buffer.rs b/tests/tests/buffer.rs index 23f244c249..f9d2e03c69 100644 --- a/tests/tests/buffer.rs +++ b/tests/tests/buffer.rs @@ -326,3 +326,30 @@ static MINIMUM_BUFFER_BINDING_SIZE_DISPATCH: GpuTestConfiguration = GpuTestConfi let _ = encoder.finish(); }); }); + +#[gpu_test] +static CLEAR_OFFSET_OUTSIDE_RESOURCE_BOUNDS: GpuTestConfiguration = GpuTestConfiguration::new() + .parameters(TestParameters::default()) + .run_sync(|ctx| { + let size = 16; + + let buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor { + label: None, + size, + usage: wgpu::BufferUsages::COPY_DST, + mapped_at_creation: false, + }); + + let out_of_bounds = size.checked_add(wgpu::COPY_BUFFER_ALIGNMENT).unwrap(); + + ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); + ctx.device + .create_command_encoder(&Default::default()) + .clear_buffer(&buffer, out_of_bounds, None); + let err_msg = pollster::block_on(ctx.device.pop_error_scope()) + .unwrap() + .to_string(); + assert!(err_msg.contains( + "Clear of 20..20 would end up overrunning the bounds of the buffer of size 16" + )); + }); diff --git a/wgpu-core/src/command/clear.rs b/wgpu-core/src/command/clear.rs index 7d09754205..25dc417b80 100644 --- a/wgpu-core/src/command/clear.rs +++ b/wgpu-core/src/command/clear.rs @@ -117,24 +117,20 @@ impl Global { if offset % wgt::COPY_BUFFER_ALIGNMENT != 0 { return Err(ClearError::UnalignedBufferOffset(offset)); } - if let Some(size) = size { - if size % wgt::COPY_BUFFER_ALIGNMENT != 0 { - return Err(ClearError::UnalignedFillSize(size)); - } - let destination_end_offset = offset + size; - if destination_end_offset > dst_buffer.size { - return Err(ClearError::BufferOverrun { - start_offset: offset, - end_offset: destination_end_offset, - buffer_size: dst_buffer.size, - }); - } + + let size = size.unwrap_or(dst_buffer.size.saturating_sub(offset)); + if size % wgt::COPY_BUFFER_ALIGNMENT != 0 { + return Err(ClearError::UnalignedFillSize(size)); + } + let end_offset = offset + size; + if end_offset > dst_buffer.size { + return Err(ClearError::BufferOverrun { + start_offset: offset, + end_offset, + buffer_size: dst_buffer.size, + }); } - let end_offset = match size { - Some(size) => offset + size, - None => dst_buffer.size, - }; if offset == end_offset { log::trace!("Ignoring fill_buffer of size 0"); return Ok(()); From 2cbb42d2a7b4e03ec9a0379f13f33a64c8f545d7 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Wed, 21 Feb 2024 15:10:29 -0500 Subject: [PATCH 4/4] fix: `command_encoder_clear_buffer`: err. on `offset + size > u64::MAX` Rust would have made this operation either an overflow in release mode, or a panic in debug mode. Neither seem appropriate for this context, where I suspect an error should be returned instead. Web browsers, for instance, shouldn't crash simply because of an issue of this nature. Users may, quite reasonably, have bad arguments to this in early stages of development! --- tests/tests/buffer.rs | 32 ++++++++++++++++++++++++++++++++ wgpu-core/src/command/clear.rs | 13 ++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/tests/buffer.rs b/tests/tests/buffer.rs index f9d2e03c69..a5fcf3e595 100644 --- a/tests/tests/buffer.rs +++ b/tests/tests/buffer.rs @@ -353,3 +353,35 @@ static CLEAR_OFFSET_OUTSIDE_RESOURCE_BOUNDS: GpuTestConfiguration = GpuTestConfi "Clear of 20..20 would end up overrunning the bounds of the buffer of size 16" )); }); + +#[gpu_test] +static CLEAR_OFFSET_PLUS_SIZE_OUTSIDE_U64_BOUNDS: GpuTestConfiguration = + GpuTestConfiguration::new() + .parameters(TestParameters::default()) + .run_sync(|ctx| { + let buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor { + label: None, + size: 16, // unimportant for this test + usage: wgpu::BufferUsages::COPY_DST, + mapped_at_creation: false, + }); + + let max_valid_offset = u64::MAX - (u64::MAX % wgpu::COPY_BUFFER_ALIGNMENT); + let smallest_aligned_invalid_size = wgpu::COPY_BUFFER_ALIGNMENT; + + ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); + ctx.device + .create_command_encoder(&Default::default()) + .clear_buffer( + &buffer, + max_valid_offset, + Some(smallest_aligned_invalid_size), + ); + let err_msg = pollster::block_on(ctx.device.pop_error_scope()) + .unwrap() + .to_string(); + assert!(err_msg.contains(concat!( + "Clear starts at offset 18446744073709551612 with size of 4, ", + "but these added together exceed `u64::MAX`" + ))); + }); diff --git a/wgpu-core/src/command/clear.rs b/wgpu-core/src/command/clear.rs index 25dc417b80..e404fabb14 100644 --- a/wgpu-core/src/command/clear.rs +++ b/wgpu-core/src/command/clear.rs @@ -39,6 +39,11 @@ pub enum ClearError { UnalignedFillSize(BufferAddress), #[error("Buffer offset {0:?} is not a multiple of `COPY_BUFFER_ALIGNMENT`")] UnalignedBufferOffset(BufferAddress), + #[error("Clear starts at offset {start_offset} with size of {requested_size}, but these added together exceed `u64::MAX`")] + OffsetPlusSizeExceeds64BitBounds { + start_offset: BufferAddress, + requested_size: BufferAddress, + }, #[error("Clear of {start_offset}..{end_offset} would end up overrunning the bounds of the buffer of size {buffer_size}")] BufferOverrun { start_offset: BufferAddress, @@ -122,7 +127,13 @@ impl Global { if size % wgt::COPY_BUFFER_ALIGNMENT != 0 { return Err(ClearError::UnalignedFillSize(size)); } - let end_offset = offset + size; + let end_offset = + offset + .checked_add(size) + .ok_or(ClearError::OffsetPlusSizeExceeds64BitBounds { + start_offset: offset, + requested_size: size, + })?; if end_offset > dst_buffer.size { return Err(ClearError::BufferOverrun { start_offset: offset,