-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(core): Use more checked arithmetic #9357
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -885,11 +885,11 @@ fn dispatch_indirect( | |
| return Err(ComputePassErrorInner::UnalignedIndirectBufferOffset(offset)); | ||
| } | ||
|
|
||
| let end_offset = offset + size_of::<wgt::DispatchIndirectArgs>() as u64; | ||
| if end_offset > buffer.size { | ||
| let args_size = size_of::<wgt::DispatchIndirectArgs>() as u64; | ||
| if buffer.size < args_size || buffer.size - args_size < offset { | ||
| return Err(ComputePassErrorInner::IndirectBufferOverrun { | ||
| offset, | ||
| end_offset, | ||
| end_offset: offset + args_size, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This nitpick also applies to other diagnostics that have an end offset that may not be in bounds, e.g.,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will include this with the next round of changes. |
||
| buffer_size: buffer.size, | ||
| }); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Doesn't the user control
offsethere? Shouldn't this be a validation error instead (maybe as follow-up)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).