don't check for EOF when unpacked size is specified

[ibaryshnikov]

Pull Request Overview

This pull request fixes the case where the last byte is 0 with known unpacked size.
Related issue is #11, in particular #11 (comment)

Testing Strategy

This pull request was tested by...

• Added relevant unit tests.
• Added relevant end-to-end tests (such as .lzma, .lzma2, .xz files).

Supporting Documentation and References

The best reference I was able to find is
https://svn.python.org/projects/external/xz-5.0.3/doc/lzma-file-format.txt

Uncompressed Size is stored as unsigned 64-bit little endian
integer. A special value of 0xFFFF_FFFF_FFFF_FFFF indicates
that Uncompressed Size is unknown. End of Payload Marker (*)
is used if and only if Uncompressed Size is unknown.

TODO, help wanted

I'm unsure what self.rep[0] is checking here

if self.rep[0] == 0xFFFF_FFFF {
    if rangecoder.is_finished_ok()? {
        break;
    }
    return Err(error::Error::LZMAError(String::from(
        "Found end-of-stream marker but more bytes are available",
    )));
}

[dragly]

This fix works for us as well. I agree with what @gendx said in #11 (comment) and would prefer to find a test-case that reproduces the issue with a proper unpacked size. However, it would be nice if this PR could be merged and an issue created for the test case, if the code is otherwise sound.

[ibaryshnikov]

@dragly

would prefer to find a test-case that reproduces the issue with a proper unpacked size

I think the test case I submitted has a proper unpacked size. If it doesn't, I'd be curious to know why

[dragly]

Sorry, I misread your example. I thought you provided the unpacked data and that the length you gave was different from the actual amount of data.

One option is to just change the round_trip_file test to also do a round-trip with unpacked size written to the header (the default is not to write it to the header):

fn round_trip_file(filename: &str) {
    use std::io::Read;

    let mut x = Vec::new();
    std::fs::File::open(filename)
        .unwrap()
        .read_to_end(&mut x)
        .unwrap();

    round_trip(x.as_slice());

    // Do another round trip, but this time also write it to the header
    let encode_options = lzma_rs::compress::Options {
        unpacked_size: lzma_rs::compress::UnpackedSize::WriteToHeader(Some(x.len() as u64)),
    };
    let decode_options = lzma_rs::decompress::Options {
        unpacked_size: lzma_rs::decompress::UnpackedSize::ReadFromHeader,
    };
    round_trip_with_options(x.as_slice(), &encode_options, &decode_options);
}

This actually trips up the range-coder-edge-case file with the following error:

called `Result::unwrap()` on an `Err` value: LZMAError("Expected unpacked size of 3040092 but decompressed to 3040057")

So I think that it is sufficient to modify the test to include the additional round-trip.

However, if you prefer to use your own data (where does it come from, by the way?), you could add the result of uncompressing your bytes as a new unpacked-size-edge-case file and change the round_trip_files test to

#[test]
fn round_trip_files() {
    let _ = env_logger::try_init();
    round_trip_file("tests/files/foo.txt");
    round_trip_file("tests/files/range-coder-edge-case");
    round_trip_file("tests/files/unpacked-size-edge-case");
}

Either way, it becomes clear that the issue is inherent to the lzma-rs library, because it shows that it is not able to properly decompress a file it has itself compressed.

[gendx]

However, it would be nice if this PR could be merged and an issue created for the test case, if the code is otherwise sound.

Sorry for the delay in reviewing this, things have been quite busy on my side last month.

---

I also agree with @dragly's comment regarding the tests. Updating round_trip tests to test the header and catch this regression would be more robust in the long run.

[ibaryshnikov]

@dragly the data is a part of openctm file. It appears that I'm working with wasm, too.

@dragly @gendx I like the idea of using roundtrip test instead, I will send an update shortly.

[gendx]

bors r+

[bors]

Timed out