Simplify crai reading and zlib_mem_inflate() - #2085
Conversation
Use BGZF via bgzf_open() etc. to simplify reading compressed .crai index files. As bgzf_open() calls hopen() itself, all plugins etc. will still be supported, along with both plain-text and gzip-compressed files. As a side effect, BGZF-compressed index files will now work (previously only plain-gzip was supported). Also adds bgzf_close() to the fail: block to fix a leak that happened if an error was detected while reading the index file. Signed-off-by: Rob Davies <rmd+git@sanger.ac.uk>
As the only caller passes in the uncompressed size, and checks the actual amount of data returned against that value, there is no need to support growing the size of the output buffer. This means decompression can be done in one shot, both when using libdeflate and (as a cram block can't hold more than INT_MAX bytes) zlib. zlib_mem_inflate() can also be made static as the only caller left is in cram/cram_io.c Signed-off-by: Rob Davies <rmd+git@sanger.ac.uk>
The maximum compression ratio of gzip is 1032 (LZ match of 258 bytes encoded in a 2bit huffman code) so it's not worth trying to uncompress one that claims to do better than this. Signed-off-by: Rob Davies <rmd+git@sanger.ac.uk>
| goto fail; | ||
| } | ||
| data = new_data; | ||
| assert (*size > 0); |
There was a problem hiding this comment.
size comes from b->comp_size which is read from disk. We shouldn't be using assert of user-data derived fields.
We could just return NULL instead and the standard error handling will pick it up.
There was a problem hiding this comment.
Currently *size can never be zero due to this check. This assert is in place as a guard against future changes that may break that invariant.
There was a problem hiding this comment.
Ok I see that now, although if there's a check elsewhere then the assert seems pointless. As you say it's to guard against something later breaking, but a check and return NULL is an equally valid guard. I just feel uneasy about using assert for data directly decoded from a file, even if it's currently not possible to trigger.
|
|
||
| /* Starting point at uncompressed size, and scale after that */ | ||
| data = malloc(data_alloc = csize*1.2+100); | ||
| assert(*size > 0); |
There was a problem hiding this comment.
As above, make this a return NULL instead.
There was a problem hiding this comment.
Similarly this can't happen at the moment. It exists to prevent future accidents.
| // These should always be true due to type of cram_block::comp_size | ||
| // and cram_block::uncomp_size | ||
| assert(*size < UINT_MAX); | ||
| assert(csize < UINT_MAX); |
There was a problem hiding this comment.
Although it's broken data, size and csize could be negative. (I've no idea why the original CRAM format permitted this, but I'm assuming because it was developed in Java initially which has no unsigned types.)
Size is already checked above, but csize < 0 should be checked somewhere and return NULL again.
There was a problem hiding this comment.
cram_read_block() currently ensures csize >= 0, so this should again never trigger. Or is it possible to get a block in a way where this might not be true?
Note that this very old assert in cram_uncompress_block() does the same check for uncomp_size. Also csize and *size here are size_t so cannot be negative.
There was a problem hiding this comment.
No, that's probably sufficient. However if we're adding more checks to this function then it's probably sensible to add both upper and lower bounds ones so it's trivial to read and understand without having to do a deep dive into all code path that led us here.
That said, I failed to spot they're size_t and not ssize_t, so ignore me. :-)
| s.avail_in = (uInt) csize; | ||
| s.total_in = 0; | ||
| s.next_out = data; | ||
| s.avail_out = data_alloc; | ||
| s.avail_out = (uInt) *size; |
There was a problem hiding this comment.
Unnecessary casts. We should have already checked signed vs unsigned sizes.
There was a problem hiding this comment.
The casts are to suppress compiler warnings about narrowing conversions, should they be in use.
We should really be better at adding casts like this, as they act as a handy warning about places where overflow bugs might be lurking.
2 participants
Use BGZF interface in place of
hFILEincram_index_load()so it no longer has to include its own decompression code. This allowszlib_mem_inflate()to be simplified as the only remaining caller both passes in the uncompressed size and rejects data that does not decompress to the size given, so logic for growing the output buffer can be removed.Fixes a bug reported in #2081 where
zlib_mem_inflate()could get stuck if the uncompressed size passed to it was 1, but the decompressed data needed more space.Fixes a bug in
cram_index_load()where thehFILEstructure could be leaked if an error was detected when reading the index file.Adds a quick test to
cram_uncompress_block()so that GZIP blocks claiming an impossible compression ratio can be rejected without trying to decompress them first.Closes #2081