mirror of
https://github.com/gfx-rs/wgpu.git
synced 2024-11-25 16:24:24 +00:00
Fix missing validation for Device::clear_buffer
where offset + size > buffer.size
was not checked when size
was omitted. (#5282)
style: fix fmt. of `assert!(…)` in `clear_texture_via_buffer_copies` refactor: `command_encoder_clear_buffer`: s/end/end_offset fix: always check buffer clear `offset` for OOB 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> 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!
This commit is contained in:
parent
c27f743558
commit
7214da6820
@ -85,6 +85,7 @@ Bottom level categories:
|
||||
- Fix docs.rs wasm32 builds. By @cwfitzgerald in [#5310](https://github.com/gfx-rs/wgpu/pull/5310)
|
||||
- Improve error message when binding count limit hit. By @hackaugusto in [#5298](https://github.com/gfx-rs/wgpu/pull/5298)
|
||||
- Remove an unnecessary `clone` during GLSL shader injestion. By @a1phyr in [#5118](https://github.com/gfx-rs/wgpu/pull/5118).
|
||||
- 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).
|
||||
|
||||
#### DX12
|
||||
- Fix `panic!` when dropping `Instance` without `InstanceFlags::VALIDATION`. By @hakolao in [#5134](https://github.com/gfx-rs/wgpu/pull/5134)
|
||||
|
@ -164,3 +164,62 @@ static MAP_OFFSET: GpuTestConfiguration = GpuTestConfiguration::new().run_async(
|
||||
assert_eq!(*byte, 0);
|
||||
}
|
||||
});
|
||||
|
||||
#[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"
|
||||
));
|
||||
});
|
||||
|
||||
#[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`"
|
||||
)));
|
||||
});
|
||||
|
@ -40,6 +40,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,
|
||||
@ -118,25 +123,27 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
|
||||
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 {
|
||||
|
||||
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
|
||||
.checked_add(size)
|
||||
.ok_or(ClearError::OffsetPlusSizeExceeds64BitBounds {
|
||||
start_offset: offset,
|
||||
end_offset: destination_end_offset,
|
||||
buffer_size: dst_buffer.size,
|
||||
});
|
||||
}
|
||||
requested_size: size,
|
||||
})?;
|
||||
if end_offset > dst_buffer.size {
|
||||
return Err(ClearError::BufferOverrun {
|
||||
start_offset: offset,
|
||||
end_offset,
|
||||
buffer_size: dst_buffer.size,
|
||||
});
|
||||
}
|
||||
|
||||
let end = 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(());
|
||||
}
|
||||
@ -145,7 +152,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
|
||||
cmd_buf_data.buffer_memory_init_actions.extend(
|
||||
dst_buffer.initialization_status.read().create_action(
|
||||
&dst_buffer,
|
||||
offset..end,
|
||||
offset..end_offset,
|
||||
MemoryInitKind::ImplicitlyInitialized,
|
||||
),
|
||||
);
|
||||
@ -155,7 +162,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
|
||||
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(())
|
||||
}
|
||||
@ -367,7 +374,7 @@ fn clear_texture_via_buffer_copies<A: HalApi>(
|
||||
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
|
||||
);
|
||||
|
Loading…
Reference in New Issue
Block a user