Skip to content

Commit

Permalink
fix: always check buffer clear offset for OOB
Browse files Browse the repository at this point in the history
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:
<https://gpuweb.github.io/gpuweb/#dom-gpucommandencoder-clearbuffer>
  • Loading branch information
ErichDonGubler committed Feb 26, 2024
1 parent 751cddc commit 9747a0e
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 16 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
27 changes: 27 additions & 0 deletions tests/tests/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
));
});
28 changes: 12 additions & 16 deletions wgpu-core/src/command/clear.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(());
Expand Down

0 comments on commit 9747a0e

Please sign in to comment.