Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing validation for timestamp writes in render and compute passes #6578

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148]
- Lower `QUERY_SET_MAX_QUERIES` (and enforced limits) from 8192 to 4096 to match WebGPU spec. By @ErichDonGubler in [#6525](https://github.com/gfx-rs/wgpu/pull/6525).
- Allow non-filterable float on texture bindings never used with samplers when using a derived bind group layout. By @ErichDonGubler in [#6531](https://github.com/gfx-rs/wgpu/pull/6531/).
- Replace potentially unsound usage of `PreHashedMap` with `FastHashMap`. By @jamienicol in [#6541](https://github.com/gfx-rs/wgpu/pull/6541).
- Add missing validation for timestamp writes in compute and render passes. By @ErichDonGubler in [#6578](https://github.com/gfx-rs/wgpu/pull/6578).
- Check the status of the `TIMESTAMP_QUERY` feature before other validation.
- Check that indices are in-bounds for the query set.
- Check that begin and end indices are not equal.

#### Naga

Expand Down
47 changes: 38 additions & 9 deletions wgpu-core/src/command/compute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ use crate::{
use thiserror::Error;
use wgt::{BufferAddress, DynamicOffset};

use super::{bind::BinderError, memory_init::CommandBufferTextureMemoryActions};
use super::{
bind::BinderError, memory_init::CommandBufferTextureMemoryActions, SimplifiedQueryType,
};
use crate::ray_tracing::TlasAction;
use std::sync::Arc;
use std::{fmt, mem::size_of, str};
Expand Down Expand Up @@ -309,26 +311,53 @@ impl Global {
};

arc_desc.timestamp_writes = if let Some(tw) = desc.timestamp_writes {
let query_set = match hub.query_sets.get(tw.query_set).get() {
let &PassTimestampWrites {
query_set,
beginning_of_pass_write_index,
end_of_pass_write_index,
} = tw;

match cmd_buf
.device
.require_features(wgt::Features::TIMESTAMP_QUERY)
{
Ok(()) => (),
Err(e) => return make_err(e.into(), arc_desc),
}

let query_set = match hub.query_sets.get(query_set).get() {
Ok(query_set) => query_set,
Err(e) => return make_err(e.into(), arc_desc),
};

match query_set.same_device(&cmd_buf.device) {
Ok(()) => (),
Err(e) => return make_err(e.into(), arc_desc),
}
match cmd_buf
.device
.require_features(wgt::Features::TIMESTAMP_QUERY)

for idx in [beginning_of_pass_write_index, end_of_pass_write_index]
.into_iter()
.flatten()
{
Ok(()) => (),
Err(e) => return make_err(e.into(), arc_desc),
match query_set.validate_query(SimplifiedQueryType::Timestamp, idx, None) {
Ok(()) => (),
Err(e) => return make_err(e.into(), arc_desc),
}
}

if let Some((begin, end)) = beginning_of_pass_write_index.zip(end_of_pass_write_index) {
if begin == end {
return make_err(
CommandEncoderError::TimestampWriteIndicesEqual { idx: begin },
arc_desc,
);
}
}

Some(ArcPassTimestampWrites {
query_set,
beginning_of_pass_write_index: tw.beginning_of_pass_write_index,
end_of_pass_write_index: tw.end_of_pass_write_index,
beginning_of_pass_write_index,
end_of_pass_write_index,
})
} else {
None
Expand Down
6 changes: 6 additions & 0 deletions wgpu-core/src/command/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,12 @@ pub enum CommandEncoderError {
InvalidResource(#[from] InvalidResourceError),
#[error(transparent)]
MissingFeatures(#[from] MissingFeatures),
#[error(
"begin and end indices of pass timestamp writes are both set to {idx}, which is not allowed"
)]
TimestampWriteIndicesEqual { idx: u32 },
#[error(transparent)]
TimestampWritesInvalid(#[from] QueryUseError),
}

impl Global {
Expand Down
2 changes: 1 addition & 1 deletion wgpu-core/src/command/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ pub enum ResolveError {
}

impl QuerySet {
fn validate_query(
pub(crate) fn validate_query(
self: &Arc<Self>,
query_type: SimplifiedQueryType,
query_index: u32,
Expand Down
31 changes: 27 additions & 4 deletions wgpu-core/src/command/render.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::binding_model::BindGroup;
use crate::command::{
validate_and_begin_occlusion_query, validate_and_begin_pipeline_statistics_query,
SimplifiedQueryType,
};
use crate::init_tracker::BufferInitTrackerAction;
use crate::pipeline::RenderPipeline;
Expand Down Expand Up @@ -1392,15 +1393,37 @@ impl Global {
};

arc_desc.timestamp_writes = if let Some(tw) = desc.timestamp_writes {
let query_set = query_sets.get(tw.query_set).get()?;
query_set.same_device(device)?;
let &PassTimestampWrites {
query_set,
beginning_of_pass_write_index,
end_of_pass_write_index,
} = tw;

let query_set = query_sets.get(query_set).get()?;

device.require_features(wgt::Features::TIMESTAMP_QUERY)?;

query_set.same_device(device)?;

for idx in [beginning_of_pass_write_index, end_of_pass_write_index]
.into_iter()
.flatten()
{
query_set.validate_query(SimplifiedQueryType::Timestamp, idx, None)?;
}

if let Some((begin, end)) =
beginning_of_pass_write_index.zip(end_of_pass_write_index)
{
if begin == end {
return Err(CommandEncoderError::TimestampWriteIndicesEqual { idx: begin });
}
}

Some(ArcPassTimestampWrites {
query_set,
beginning_of_pass_write_index: tw.beginning_of_pass_write_index,
end_of_pass_write_index: tw.end_of_pass_write_index,
beginning_of_pass_write_index,
end_of_pass_write_index,
})
} else {
None
Expand Down