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

Add decoder fuzzing #36

Merged
merged 7 commits into from
May 28, 2020
Merged

Add decoder fuzzing #36

merged 7 commits into from
May 28, 2020

Conversation

Shnatsel
Copy link
Contributor

Version of #34 as a single commit and without the fuzzing corpus

…s; fuzz xz decoder for equivalent output to xz2 crate (liblzma wrapper)
@Shnatsel Shnatsel mentioned this pull request Apr 16, 2020
fuzz/README.md Show resolved Hide resolved
fuzz/fuzz_targets/decompress_lzma.rs Show resolved Hide resolved
fuzz/fuzz_targets/decompress_lzma.rs Show resolved Hide resolved
fuzz/fuzz_targets/compare_xz.rs Outdated Show resolved Hide resolved
fuzz/fuzz_targets/compare_xz.rs Outdated Show resolved Hide resolved
@Shnatsel
Copy link
Contributor Author

It has occurred to me that completely identical error behaviour on arbitrary input is a pretty high bar, so it would be nice to have a less stringent test. I've added fuzzing targets checking that data compressed by the reference implementation can be decompressed by lzma-rs and vice versa. They seem to pass.

let bf = std::io::Cursor::new(compressed);
let mut decomp: Vec<u8> = Vec::new();
// create new XZ decompression stream with 8Gb memory limit and checksum verification disabled
let xz_stream = stream::Stream::new_stream_decoder(8*1024*1024*1024, stream::IGNORE_CHECK).expect("Failed to create stream");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that the cargo fmt -- --check didn't trigger.
I'm even more surprised that Travis-CI didn't report any test results on the pull request.

@gendx
Copy link
Owner

gendx commented Apr 20, 2020

It has occurred to me that completely identical error behaviour on arbitrary input is a pretty high bar, so it would be nice to have a less stringent test. I've added fuzzing targets checking that data compressed by the reference implementation can be decompressed by lzma-rs and vice versa. They seem to pass.

These checks are definitely useful in addition!

  • Decoding random inputs exercises failure checks.
  • Decoding well-encoded inputs exercises deeper decoding paths.

My last comments are mostly nit picking at this point. In addition, if you (or anyone reading this) happen to have an idea about why my Travis-CI/Bors integration is broken, that would be very useful.

@gendx gendx added the waiting on author Waiting for pull request author to address review comments label May 5, 2020
@Shnatsel
Copy link
Contributor Author

I see the PR has "waiting on author" tag, but it's not clear to me what still needs to be changed. Could you provide me with a list of what needs to be done before this can be merged?

@gendx
Copy link
Owner

gendx commented May 28, 2020

I see the PR has "waiting on author" tag, but it's not clear to me what still needs to be changed. Could you provide me with a list of what needs to be done before this can be merged?

The following comment is still pending (in particular, running cargo fmt): #36 (comment).


Additionally, the code for imports has migrated to the 2018 edition in the meantime (no need for extern crate foo statements anymore) #38.

@Shnatsel
Copy link
Contributor Author

I've run cargo fmt and migrated the fuzzing harness to 2018 edition. If there are any other blockers, please let me know.

@gendx gendx merged commit 2bd2aeb into gendx:master May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting on author Waiting for pull request author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants