-
Notifications
You must be signed in to change notification settings - Fork 140
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
Limit decoded images to 2^26 pixels by default #82
Conversation
Looks good to me. |
The limit appears somewhat arbitrary to me. Is it referring to a limit already established in another library? |
Considering that it triggers on a quadratic image of width and length each |
In the BMP decoder in image there is a max of There is also a limit on how much memory the decoder allocates at a first try, and it will only allocate more if there is actually image data for it. Maybe something similar could be done here. Ideally though it would be nice if a limit could be specified, so the caller could give some reasonable expected max size based on say the file size. |
The max in BMP is enforced for width and height individually, so in total limit the size to something slightly smaller than 2^32. The limit proposed here is for the actual pixel count, The suggestion on additional memory optimization sounds sensible, so like an adjusted |
Yeah I meant to write |
The limit was copied from the flif crate. |
Ah, I didn't realize that. In the corresponding pull request, the choice was explained as follows:
Now I wonder if we should differentiate between a pixel and a memory limit. However, that is more of a bikeshedding issue for now, with prior art demonstrated it might be sensible to just merge it for now. Increasing the default limit in the future or adding alternative and optional additional limits should not be a breaking change anyways. |
src/decoder/mod.rs
Outdated
pub struct Limits { | ||
/// max number of pixels: `width * height` (default: 67M = 2<sup>26</sup>) | ||
pixels: u64, | ||
} |
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.
I think there shouldn't be any problems with making pixels
field public. This will also allow us to remove set_pixels
setter.
src/decoder/mod.rs
Outdated
/// assert!(decoder.read_info().is_ok()); | ||
/// ``` | ||
pub fn set_limits(&mut self, limits: Limits) { | ||
self.limits = limits; |
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.
How about using new_with_limits(r: R, limits: Limits)
method instead? It will allow to create decoder in one line and new
can be defined as Self::new_with_limits(r, Default::default())
.
8382e9a
to
a7d1af1
Compare
For reference, libpng enforces 1,000,000 by 1,000,000 pixels limit by default, which is roughly 2^40 pixels or 2^44 bytes per image in the extreme case. Source and further information: https://libpng.sourceforge.io/decompression_bombs.html |
|
Merging. |
Fixes #80