Bzip2 level = 0 panic - Githubissues

LumaRay commented 1 year ago

When using bzip2 with compression level 0, it panics with this error:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `-2`,
 right: `0`', /home/test/.cargo/registry/src/github.com-1ecc6299db9ec823/bzip2-0.4.3/src/mem.rs:123:13

According to docs, 0 is allowed for bzip2: https://docs.rs/zip/0.6.3/zip/write/struct.FileOptions.html#method.compression_method

zamazan4ik commented 1 year ago

According to the bzip2 documentation, it also allows 0 compression level: https://docs.rs/bzip2/latest/bzip2/struct.Compression.html#method.new

So since bzip2 panicks I guess will be better if you create the issue there. Then we will update zip-rs library (at least I hope so). Right now it is seems like a bzip2 bug.

Plecra commented 1 year ago

Yeah, this is an incorrect addition to the bzip2 API. Wyrd. Their Compression level is used directly (here) as this blockSize100k parameter documented to be 1..=9. Let's poke upstream.

Plecra commented 1 year ago

To patch the bug for now, we should convert compression level 0 to level 1 before passing it onto bzip2 :)

Pr0methean commented 1 year ago

https://crates.io/crates/zip_next fixes this by clamping the Bzip compression level to a minimum of 1.

Plecra commented 1 year ago

Yeah, you've bumped up the minimum compression level here https://github.com/Pr0methean/zip-next/commit/43a9db888680d96f976452c3b6f68b7004b4e7f9#diff-8e31462ea4b11fdadc15fd98e895c1902595d9d34947c58eecca1125a5e6224dL1138-R1128

Since this isn't looking like an easy resolution in the upstream crate, I think you're right that zip should try to fix it. We might be able to roll our own implementation of uncompressed bzip if it has a simple enough header

Plecra commented 1 year ago

Thanks for the bump :)

Pr0methean commented 1 year ago

Some if not all of the bzip steps can probably be made no-ops (e.g. instead of the run-length coding step, insert a \0 after every 4 bytes), but I don't see what the point of this would be compared to using Stored.

Pr0methean commented 5 months ago

Succeeded at reproducing this.

Running: fuzz/artifacts/fuzz_write/minimized-from-556ccff6e325fa01d012975627629beff0273502
thread '<unnamed>' panicked at /Users/hennickc/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bzip2-0.4.4/src/mem.rs:123:13:
assertion `left == right` failed
  left: -2
 right: 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
==49704== ERROR: libFuzzer: deadly signal
    #0 0x106c1b6b5 in __sanitizer_print_stack_trace+0x35 (librustc-nightly_rt.asan.dylib:x86_64h+0xed6b5)
    #1 0x105b8a73a in fuzzer::PrintStackTrace()+0x2a (fuzz_write:x86_64+0x10046273a)
    #2 0x105b7be13 in fuzzer::Fuzzer::CrashCallback()+0x43 (fuzz_write:x86_64+0x100453e13)
    #3 0x7ff8185a4fdc in _sigtramp+0x1c (libsystem_platform.dylib:x86_64+0x3fdc)

Pr0methean commented 5 months ago

Return value of -2 is BZ_PARAM_ERROR: https://github.com/alexcrichton/bzip2-rs/blob/master/bzip2-sys/bzip2-1.0.8/bzlib.h

Pr0methean commented 5 months ago

Looks like the error is being returned from here: https://github.com/alexcrichton/bzip2-rs/blob/3032f3790742bffda521e54d14429f459e078eba/bzip2-sys/bzip2-1.0.8/bzlib.c#L1261

The C library doesn't accept a blockSize100k value of 0, and the Rust wrapper sets that parameter to equal the compression quality.

Pr0methean commented 5 months ago

This is a known issue in the bzip2 crate: https://github.com/alexcrichton/bzip2-rs/issues/84 We already work around it in zip by setting the minimum compression level to 1.

If anyone would like to update this or suggest a better-maintained bzip2 implementation, please open a new issue at https://github.com/zip-rs/zip2, since this repo is no longer maintained.

zip-rs / zip-old

Bzip2 level = 0 panic #326