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

Fix some major LZMA2 decompression issues #61

Merged
merged 8 commits into from Dec 9, 2020
Merged

Conversation

qnga
Copy link
Contributor

@qnga qnga commented Dec 2, 2020

Pull Request Overview

This pull request fixes several issues with the LZMA2 decompression.

  • Lzbuffer length reset. Although I could not find a file that this change allows to successfully decompress, I'm convinced it is needed. If I correctly understood, the Java implementation does the reset there.
  • Unpacked_size mismatches (for instance, it's likely to solve LZMAError("Expected unpacked size of 149198 but decompressed to 483334")' #11). The unpacked_size provided in the LZMA2 header is the size of the unpacked chunk, whereas lzbuffer.len() that was used before, is the size of the dict since the last reset. Not every chunk leads to a dict reset.
  • LZMA2 header parsing. In some cases, properties were not reset while they should be.

Testing Strategy

This pull request was tested by myself. It allows to decompress several files whose decompression failed before. This is the case of the good-1-lzma2-4.xz file from the XZ test suite, so I added it in the test directory. The last two fixes are required to decompress it.

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

Note that this PR seems to improve the decompression of some other tests from the XZ test suite: decompression fails because of CRC errors instead of invalid status errors.

TODO or Help Wanted

This pull request is based on the v0.1.3 version and still needs to be rebased on top of master. Lots of changes occurred since the v0.1.3 version and for some reason I'm not able to reproduce my results with master so far. I'm afraid a regression might have been introduced at some point. I'll try again but any help would be welcome.

@gendx gendx mentioned this pull request Dec 3, 2020
2 tasks
@gendx
Copy link
Owner

gendx commented Dec 3, 2020

Thanks for taking a look at this!

TODO or Help Wanted

This pull request is based on the v0.1.3 version and still needs to be rebased on top of master. Lots of changes occurred since the v0.1.3 version and for some reason I'm not able to reproduce my results with master so far. I'm afraid a regression might have been introduced at some point. I'll try again but any help would be welcome.

I merged #59 this week, which refactored quite a bit of code. But even if your bug ended up being fixed in the meantime, adding more test cases is always welcome!

@qnga
Copy link
Contributor Author

qnga commented Dec 4, 2020

That was the contrary, my bug hadn't been fixed in the meantime and I was not able to fix it on master. The good news is that I succeeded today, and three more test cases from the XZ test suite pass now with master + my own fixes.

@qnga qnga marked this pull request as ready for review December 4, 2020 16:57
@gendx
Copy link
Owner

gendx commented Dec 8, 2020

This overall looks good.

Just a couple comments regarding the failing Travis-CI:

@qnga
Copy link
Contributor Author

qnga commented Dec 9, 2020

Okay, I've reformatted the code according to cargo's rules.

@gendx gendx merged commit 9595050 into gendx:master Dec 9, 2020
@qnga qnga deleted the fix/lzma2 branch December 9, 2020 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants