fix: always check buffer clear offset for OOB
#5282
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ErichDonGubler
added
area: validation
ErichDonGubler
force-pushed
the
clear_buffer-fix-offset-ck
branch
from
46b4398 to
5b12fc6
Compare
7 tasks
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>
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!
ErichDonGubler
force-pushed
the
clear_buffer-fix-offset-ck
branch
from
2e6138e to
2cbb42d
Compare
ErichDonGubler
dismissed
cwfitzgerald’s stale review
Concerns handled, AFAICT.
nical
approved these changes
Feb 26, 2024
cwfitzgerald
added
the
PR: needs back-porting
cwfitzgerald
pushed a commit
to cwfitzgerald/wgpu
that referenced
this pull request
Feb 29, 2024
…e > buffer.size` was not checked when `size` was omitted. (gfx-rs#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!
cwfitzgerald
pushed a commit
to cwfitzgerald/wgpu
that referenced
this pull request
Feb 29, 2024
…e > buffer.size` was not checked when `size` was omitted. (gfx-rs#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!
cwfitzgerald
removed
the
PR: needs back-porting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recommended review experience is commit-by-commit.
Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories
This fix was prompted by Firefox's bug 1881065.
Description
Describe what problem this is solving, and how it's solved.
Fuzz testing in Firefox encountered crashes for calls of
Global::command_encoder_clear_bufferwhere:offsetis greater thanbuffer.size, but…sizeisNone.Oops! We should always check this (i.e., even when
sizeisNone), because we have no guarantee thatoffsetand the fallback value ofsizeis in bounds. 😅 So, we change validation here to unconditionally computesizeand run checks we previously gated behindif let Some(size) = size { … }.For convenience, the spec. link for this method: https://gpuweb.github.io/gpuweb/#dom-gpucommandencoder-clearbuffer
Testing
Explain how this change is tested.
The following new tests have been added, and can be tested as arguments to
cargo xtask test:wgpu_test::buffer::clear_offset_outside_resource_boundswgpu_test::buffer::clear_offset_plus_size_outside_u64_boundsChecklist
cargo fmt.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.