Out-of-range panic in gif::Decoder::into_frames()

[Shnatsel]

The attached test samples trigger a panic in gif::Decoder::into_frames()

gif-oor.zip

Code for reproducing the issue:

fn gif_decode(data: &[u8]) {
    if let Ok(decoder) = image::gif::Decoder::new(data) {
        let (width, height) = decoder.dimensions();
        if width.saturating_mul(height) > 2_000_000_000 {
            return;
        }

        let frames = decoder.into_frames();
        let _ = frames.collect_frames();
    }
}

This bug is similar to, but distinct from #816. The attached testcases still cause crashes on latest git master (as of commit 865d2fc) while the testcase from #816 does not.

Found with AFL.rs

[Shnatsel]

I've filed this issue here because the backtrace points to a function in image. Should I file it against the gif crate instead?

[HeroicKatora]

I believe this may be fixed now, can't reproduce this. I'm running cargo fuzz instead of afl since the latter still requires local workarounds to compile ( 😞 ) but that shouldn't change much? I think everything is fixed at the moment.

[Shnatsel]

Nice, I'll re-run AFL with my fuzzing seeds and see if it uncovers anything else.

AFL is much (10x) faster if you're only looking for panics because it doesn't require address sanitizer or any other expensive sanitizers to be enabled. codegen-units=1 in Cargo.toml fixes the AFL issue, that workaround doesn't have to be local.

[HeroicKatora]

Still seems to require changing the linker to gold. I also found cargo-fuzz to have a better pipeline and UX? Some crash reports from afl I can't reproduce at all and running the binary directly (after having painstakingly finding the crashing file in any of out, queue, crashes seemingly at random) gives no indication whatsoever to help me debug them or even figure out why they are reported in the first place. Also cargo-fuzz doesn't require renaming the files for NTFS compatibility.

[Shnatsel]

Still seems to require changing the linker to gold

Nope, codegen-units=1 alone does the trick.

If you're getting crashes from AFL you can't reproduce, you're probably hitting a memory limit - fuzzer treats this as a crash. Try specifying larger value for -m parameter for AFL.

cargo-fuzz definitely has better UX. Its backend, libfuzzer, can function without sanitizers too (sancov alone is enough), passing this argument set is simply not implemented in the cargo-fuzz wrapper.

[HeroicKatora]

Nope, codegen-units=1 alone does the trick.

Well I did just test that, and I even tested actively updating to 0.4.4 without success.

[Shnatsel]

Oh! Make sure you're setting RUSTFLAGS='--cfg=fuzzing' env variable when trying to reproduce these bugs. I've just spent 15 minutes wondering why my test crashing under fuzzer doesn't crash standalone, and that was the trick. This flag disables checksum verification so that the randomly mutated input from fuzzer can actually reach interesting code instead of being rejected immediately due to checksum mismatch.

[Shnatsel]

I can no longer reproduce this issue as of ac93e75, closing. I'll leave afl overnight to look for other panics.

[Shnatsel]

No further issues other than #877 were found in 50 million executions as of commit ac93e75

Fuzzer was seeded with both of AFL GIF corpora and seeds from earlier runs on image-gif crate, minimized with afl-cmin prior to fuzzing.