Decoding limits #28
Decoding limits #28
Conversation
flif/src/decoder.rs
Outdated
| Ok(Decoder { info, rac }) | ||
| } | ||
|
|
||
| pub fn new_with_limits(reader: R, limits: Limits) -> Result<Self> { |
There was a problem hiding this comment.
Change to with_limits, this matches the convention on structs like Vec
flif/src/decoder.rs
Outdated
|
|
||
| // read the metadata chunks | ||
| let (metadata, non_optional_byte) = Metadata::all_from_reader(&mut reader)?; | ||
| let (metadata, non_opt_byte) = Metadata::all_from_reader(&mut reader, &limits)?; |
There was a problem hiding this comment.
no need to shorten non_optional_byte here
There was a problem hiding this comment.
Line becomes too long in my opinion and breaking line on :: or arguments does not look good here. But ok, will revert it.
| @@ -72,6 +76,27 @@ impl Flif { | |||
| } | |||
| } | |||
|
|
|||
| /// Limits on input images to prevent OOM based DoS | |||
| #[derive(Copy, Clone ,Debug)] | |||
| pub struct Limits { | |||
There was a problem hiding this comment.
It may be useful to add a limit for the maniac tree in this PR. If we add it later it will be a breaking change since it will add a new field to the struct.
The limit doesn't have to do anything yet, but to prevent API breakage it should at least exist on the Limits type
There was a problem hiding this comment.
Will it be enough to have one maniac_depth: usize field?
There was a problem hiding this comment.
It might be better to have a maniac_nodes: usize field, since there could be very lopsided maniac trees that wouldn't take up as much space as a balanced tree of smaller depth.
There was a problem hiding this comment.
Done. For now default limit is 1024.
There was a problem hiding this comment.
It might be a good idea to set that to 4096. That seems like a lot but if remember correctly the sea snail image has 1100ish nodes. There may be more in larger images.
Limits can be configured by providing
Limitsvalue to new methods. Default values allow maximum size ofDecodingImage's data equal to 2^26, which is equal to 512 MB.It's better to merge this PR first, after it I'll add limits to fuzzing in #26 .