-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 Compress::Gzip
extra field
#14550
Conversation
This pull request has been mentioned on Crystal Forum. There might be relevant details there: |
Spec would be nice :) |
Hi @Sija The current test is the following link. It already contains tests on Extra field. Here we use Gzip::Writer.open(io) do |gzip|
header = gzip.header
header.modification_time = SAMPLE_TIME
header.os = SAMPLE_OS
header.extra = SAMPLE_EXTRA
header.name = SAMPLE_NAME
header.comment = SAMPLE_COMMENT
io.bytesize.should eq(0)
gzip.flush
io.bytesize.should_not eq(0)
gzip.print SAMPLE_CONTENTS
end The created gzip stream is then read using Gzip::Reader. Gzip::Reader.open(io) do |gzip|
header = gzip.header.not_nil!
header.modification_time.should eq(SAMPLE_TIME)
header.os.should eq(SAMPLE_OS)
header.extra.should eq(SAMPLE_EXTRA)
header.name.should eq(SAMPLE_NAME)
header.comment.should eq(SAMPLE_COMMENT)
# Reading zero bytes is OK
gzip.read(Bytes.empty).should eq(0)
gzip.gets_to_end.should eq(SAMPLE_CONTENTS)
end The original code passed the test. Because it created an incorrect stream with XLEN size of one byte instead of two bytes and read it the wrong way. This pull request failed the test the first time because I was only correcting the reading XLEN. So I also corrected the writing XLEN as well and now it passes the test. Even then, should we really add another test? If so, what kind of test should we add? One idea might be to have a hard-coded slice and check that it can be read by @oprypin Thanks for linking the issue. I think this also fixes the issue (#8933) |
There should be at least one test that ensures correctly reading an extra field, and one for correctly writing an extra field. So it's probably a good idea to include a gzipped file with extra field as a test fixture and make sure we can read it and produce the same file when writing. |
@straight-shoota (1) Add a small file https://github.com/crystal-lang/crystal/blob/master/spec/std/data (2) Write some test code like the following crystal/spec/std/compress/zip/zip_file_spec.cr Lines 108 to 114 in 89f2e43
Is my understanding correct? |
I created a draft Gzip file for testing. C code to generate the Gzip file Generated gzip file |
A minimal test was written by adding the simple gzip file presented above. |
Confirmed that the test fails when I Revert the fix commit.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kojix2
Compress::Gzip
extra field
Hi.
If a Gzip file had a extra field, it could not be opened with
Compress::Gzip::Reader
.This is because the length of an extra field, XLEN, is 2 bytes (uint16), but was treated as 1 byte.
This pull request aims to fix this issue.
Thank you.
Reference: BGZF file specifications: https://samtools.github.io/hts-specs/SAMv1.pdf
Reference: GZIP file format specification version 4.3
https://www.rfc-editor.org/rfc/rfc1952