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

Avoid integer overflow on multiplication in write_texture. #3146

Merged
merged 4 commits into from
Nov 7, 2022

Conversation

nical
Copy link
Contributor

@nical nical commented Oct 28, 2022

Checklist

  • Run cargo clippy.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections

Found by

https://bugzilla.mozilla.org/show_bug.cgi?id=1791809

Description

With large depth values in copy_size, validate_linear_texture_data can run into integer overflows.
This is avoided by validating the copy depth before calling validate_linear_texture_data in queue_write_texture.
The other two validate_linear_texture_data call sites already have the copy size validated beforehand.

Testing

I can add a test when I come back from vacation a week from now.

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022

Codecov Report

Merging #3146 (26a5505) into master (c453397) will decrease coverage by 0.00%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master    #3146      +/-   ##
==========================================
- Coverage   65.41%   65.41%   -0.01%     
==========================================
  Files          81       81              
  Lines       38763    38766       +3     
==========================================
+ Hits        25358    25359       +1     
- Misses      13405    13407       +2     
Impacted Files Coverage Δ
wgpu-core/src/device/queue.rs 70.69% <66.66%> (-0.12%) ⬇️
wgpu-core/src/command/transfer.rs 70.87% <100.00%> (+0.10%) ⬆️
wgpu-core/src/track/range.rs 92.85% <0.00%> (-2.15%) ⬇️
wgpu-hal/src/gles/egl.rs 37.95% <0.00%> (+0.12%) ⬆️
wgpu-core/src/hub.rs 60.98% <0.00%> (+0.15%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nical
Copy link
Contributor Author

nical commented Oct 28, 2022

I started writing the test and that was a good thing as it showed that fixing the cause of the panic lets us panic in backend_direct.rs instead which treats validation errors as fatal in queue_write_texture so that has to be fixed as well, I'll do it in a week or so.

@cwfitzgerald
Copy link
Member

I also think we may have the same problem in copy_texture_*? Or do those use the right order?

@nical
Copy link
Contributor Author

nical commented Nov 7, 2022

copy_texture_to_buffer already validates the size by calling validate_texture_copy_range before validate_linear_texture_data. The other place is copy_texture_to_texture however it doesn't call validate_linear_texture_data. I don't see risky arithmetic before the validate_copy_range calls but these are easy to miss. The fuzzers will find issues if any.

with large depth values in copy_size validate_linear_texture_data can run into integer overflows
This is avoided by validating the copy depth before calling validate_linear_texture_data in
queue_write_texture.
The other two validate_linear_texture_data call sites already have the copy size validated beforehand.
@nical
Copy link
Contributor Author

nical commented Nov 7, 2022

I added a commit that adds a ref to the error sink in the queue id, the same way it is done for device and similar handles, in order for relevant queue methods to be able handle errors gracefully and get the new test to pass.

This is allows us to make (some of) the queue methods forward errors instead of panicking.
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, as always!

@cwfitzgerald cwfitzgerald merged commit db30e39 into gfx-rs:master Nov 7, 2022
@nical nical deleted the write-texture-overflow branch November 8, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants