Fix unbound allocation in fuzzer_script_exr #1781
Conversation
fuzz/fuzzers/fuzzer_script_exr.rs
Outdated
| let decoded_size = usize::try_from(decoder.total_bytes()).unwrap(); | ||
| if total_size - current_pos < decoded_size as u64 { |
There was a problem hiding this comment.
I don't think this is the logic we want. The EXR format supports compression, so the decoded size is usually going to be larger than the file size. Instead, for fuzzing purposes we could just impose a maximum size of say 256 MB.
(As a smaller point, we should probably check decoded_size before calling usize::try_from(...).unwrap() so that we don't fail there on 32-bit systems)
There was a problem hiding this comment.
If we upper bound the decoded size against anything, then comparing against the limits configured for the reader would also make sense (i.e. `Limits::max_alloc). We are doing another immediate allocation afterall.
There was a problem hiding this comment.
If it's unclear, I'm suggesting that properly supporting <OpenExrDecoder as ImageDecoder>::set_limits and then using that function to configure the limit would be a much more thorough fix. That way, we get better feedback if all allocations are accounted for or if the fuzzer manages to find additional ones inside the decoder.
There was a problem hiding this comment.
I don't think this is the logic we want. The EXR format supports compression, so the decoded size is usually going to be larger than the file size. Instead, for fuzzing purposes we could just impose a maximum size of say 256 MB.
(As a smaller point, we should probably check decoded_size before calling usize::try_from(...).unwrap() so that we don't fail there on 32-bit systems)
Thanks, I didn't know that there is a compression:). I changed fix on what you offer.
There was a problem hiding this comment.
If it's unclear, I'm suggesting that properly supporting
<OpenExrDecoder as ImageDecoder>::set_limitsand then using that function to configure the limit would be a much more thorough fix. That way, we get better feedback if all allocations are accounted for or if the fuzzer manages to find additional ones inside the decoder.
Sounds pretty good, I need a little bit more time to look at code, maybe find some examples for this
There was a problem hiding this comment.
I've provided an implementation for OpenExrDecoder::set_limits function. In this function I check some common cases and a case with total_bytes(). Maybe we need to save Limits in Decoder, and check them sometimes. But I don't familiar with exr decoding for now:). What do you think?
There was a problem hiding this comment.
@HeroicKatora the unbounded allocation that's being fixed here is actually in the fuzz harness itself, which set_limits isn't designed to address
Fix for #1778