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

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

Closed
Shnatsel opened this issue Mar 2, 2019 · 9 comments
Closed

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

Shnatsel opened this issue Mar 2, 2019 · 9 comments

Comments

@Shnatsel
Copy link
Contributor

Shnatsel commented Mar 2, 2019

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
Copy link
Contributor Author

Shnatsel commented Jun 9, 2019

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
Copy link
Member

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
Copy link
Contributor Author

Shnatsel commented Oct 1, 2019

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
Copy link
Member

HeroicKatora commented Oct 1, 2019

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
Copy link
Contributor Author

Shnatsel commented Oct 1, 2019

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
Copy link
Member

HeroicKatora commented Oct 1, 2019

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
Copy link
Contributor Author

Shnatsel commented Oct 1, 2019

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
Copy link
Contributor Author

Shnatsel commented Oct 1, 2019

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

@Shnatsel Shnatsel closed this as completed Oct 1, 2019
@Shnatsel
Copy link
Contributor Author

Shnatsel commented Oct 2, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants