fpgaminer / rust-lzma

A Rust crate that provides a simple interface for LZMA compression and decompression.
MIT License
79 stars 14 forks source link

LzmaWriter.write should not return zero unless there's an error #13

Closed jpap closed 6 years ago

jpap commented 7 years ago

I've noticed that the LzmaWriter write can return zero bytes consumed, but then if called again and again, it will eventually accept more bytes.

This means we cannot use write_all, since it will stop with error ErrorKind::WriteZero as soon as the write above returns zero.

The workaround is ugly,

let todo = buffer.len();
let mut index = 0;
while {
    let bytes = decompressor.write(&buffer[index..]).unwrap();
    index += bytes;

    index < todo
} {}

when it could have been just:

decompressor.write_all(&buffer).unwrap();
fpgaminer commented 7 years ago

Thank you for reporting this bug!

Do you happen to have a sample of compressed data that causes this behavior? I'd like to throw it into a unit test.

clamydo commented 7 years ago

I can also reproduce this for LzmaWriter::new_compressor. This is a really annoying, critical bug. I tried to isolate the failing data written into, but this does not produces the same error anymore.

Since I'm not calling write_all by myself, I cannot use the workaround.

clamydo commented 7 years ago

I think the issue is with returning bytes_read from .write() in https://github.com/fpgaminer/rust-lzma/blob/d2aac5d4643440476eaef1f9f80fbf95dc8cb6b6/src/writer.rs#L112

Since it seems to be possible that liblzma has read none of the input buffer (i.e. bytes_read == 0), but it still needs to write data (stream.avail_out != 0). So it is note a good measure in the sense that it does not fulfil the requirements of the std. Returning 0 from write is considered an IO error.

edit: corrections

clamydo commented 7 years ago

To me, encoding an error in the number of bytes written as write_all in std does is wrong. It fails, if a writer still needs to write data but does not want to consume more at a step, as in this case. But it is the way it is.

clamydo commented 7 years ago

This should be fixed in https://github.com/fpgaminer/rust-lzma/pull/17