fix(core): Use more checked arithmetic - #9357
Conversation
ErichDonGubler
left a comment
ErichDonGubler
left a comment
There was a problem hiding this comment.
Definitely an improvement, woot!
| return Err(ComputePassErrorInner::IndirectBufferOverrun { | ||
| offset, | ||
| end_offset, | ||
| end_offset: offset + args_size, |
There was a problem hiding this comment.
nitpick: We should not be trying to compute an end offset if it's possibly bad. We can fix this as follow-up, though.
suggestion: Let's store the size instead of the end offset, like with other bounds checking errors that we've been changing recently.
There was a problem hiding this comment.
This nitpick also applies to other diagnostics that have an end offset that may not be in bounds, e.g., BuildAccelerationStructureError::InsufficientBufferSize.
There was a problem hiding this comment.
I will include this with the next round of changes.
| #[derive(Clone, Debug, Error)] | ||
| pub enum InvalidWorkgroupSizeError { | ||
| #[error( | ||
| "Workgroup size {dimensions:?} ({total} total invocations) must be less or equal to \ | ||
| the per-dimension limit `Limits::{per_dimension_limits_desc}` of {per_dimension_limits:?} \ | ||
| and the total invocation limit `Limits::{total_limit_desc}` of {total_limit}" | ||
| )] | ||
| LimitExceeded { | ||
| dimensions: [u32; 3], | ||
| per_dimension_limits: [u32; 3], | ||
| per_dimension_limits_desc: &'static str, | ||
| total: u32, | ||
| total_limit: u32, | ||
| total_limit_desc: &'static str, | ||
| }, | ||
| #[error("Workgroup sizes {dimensions:?} must be positive")] | ||
| Zero { dimensions: [u32; 3] }, | ||
| } | ||
|
|
There was a problem hiding this comment.
issue(non-blocking): We should note this new API surface in a CHANGELOG entry.
| #[error( | ||
| "Shader entry point's workgroup size {dimensions:?} ({total} total invocations) must be \ | ||
| less or equal to the per-dimension limit `Limits::{per_dimension_limits_desc}` of \ | ||
| {per_dimension_limits:?} and the total invocation limit `Limits::{total_limit_desc}` of \ | ||
| {total_limit}" | ||
| )] | ||
| InvalidWorkgroupSize { | ||
| dimensions: [u32; 3], | ||
| per_dimension_limits: [u32; 3], | ||
| per_dimension_limits_desc: &'static str, | ||
| total: u32, | ||
| total_limit: u32, | ||
| total_limit_desc: &'static str, | ||
| }, | ||
| #[error(transparent)] | ||
| InvalidWorkgroupSize(#[from] InvalidWorkgroupSizeError), |
There was a problem hiding this comment.
issue: This is technically a breaking change, and we should note that in a CHANGELOG entry. This could be follow-up, I suppose.
There was a problem hiding this comment.
This feedback also applies to DrawError::InvalidGroupSize.
| #[error( | ||
| "Shader entry point's workgroup size {dimensions:?} ({total} total invocations) must be \ | ||
| less or equal to the per-dimension limit `Limits::{per_dimension_limits_desc}` of \ | ||
| {per_dimension_limits:?} and the total invocation limit `Limits::{total_limit_desc}` of \ | ||
| {total_limit}" | ||
| )] | ||
| InvalidWorkgroupSize { | ||
| dimensions: [u32; 3], | ||
| per_dimension_limits: [u32; 3], | ||
| per_dimension_limits_desc: &'static str, | ||
| total: u32, | ||
| total_limit: u32, | ||
| total_limit_desc: &'static str, | ||
| }, | ||
| #[error(transparent)] | ||
| InvalidWorkgroupSize(#[from] InvalidWorkgroupSizeError), |
There was a problem hiding this comment.
This feedback also applies to DrawError::InvalidGroupSize.
| let vertex_limits = super::VertexLimits::new(state.vertex_buffer_sizes(), &pipeline.steps); | ||
|
|
||
| let stride = super::get_src_stride_of_indirect_args(family); | ||
| assert!(offset <= wgt::BufferAddress::MAX - stride); |
There was a problem hiding this comment.
question: Doesn't the user control offset here? Shouldn't this be a validation error instead (maybe as follow-up)?
There was a problem hiding this comment.
It seemed unlikely enough to me that this would actually occur that it didn't seem worth introducing an error for it. This is multi_draw_indirect, so not standardized functionality, and I don't think we will even accept numbers from JavaScript beyond the range that consecutive integers can be represented exactly (2^52 or so).
ErichDonGubler
added
type: bug
2 participants
Use checked arithmetic in more places.
Testing
In an ideal world there would be tests for this, but time is finite.
Squash or Rebase? Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.