Skip to content

Using bgzf_read_small() breaks subsequent bgzf_useek() calls #1798

Description

@jmarshall

PR #1772 added a simplified inline version of bgzf_read() that uses inline code when the request can be satisfied directly from the buffer, otherwise punts to the real bgzf_read(). Very laudable. However I encountered seek problems when using it.

I rederived the inline function by starting with the code of the full bgzf_read(), assuming the invariant that length < fp‑>block_length - fp‑>block_offset, and simplifying the code accordingly. I ended up with a function that is fairly similar to bgzf_read_small() as added to htslib/bgzf.h but with some additions:

static inline ssize_t bgzf_read_small(BGZF *fp, void *data, size_t length) {
     // A block length of 0 implies current block isn't loaded (see
     // bgzf_seek_common).  That gives negative available so careful on types
     if ((ssize_t)length < fp->block_length - fp->block_offset) {
         // Short cut the common and easy mode
         memcpy((uint8_t *)data,
                (uint8_t *)fp->uncompressed_block + fp->block_offset,
                length);
         fp->block_offset += length;
+
+        // For raw gzip streams this avoids short reads.
+        if (fp->block_offset == fp->block_length) {
+            fp->block_address = bgzf_htell(fp);
+            fp->block_offset = fp->block_length = 0;
+        }
+
+        fp->uncompressed_address += length;
+
         return length;
     } else {
         return bgzf_read(fp, data, length);
     }
 }

Because bgzf_read_small() does not currently update fp‑>uncompressed_address, subsequent bgzf_useek() calls may jump to the wrong location. And probably other functions use fp‑>uncompressed_address too and are affected.

The if block looks harder to deal with, because it calls bgzf_htell() which is private within bgzf.c. However it turns out that the invariant implies that this if will never be true, so in fact I should have simplified it away to nothing too. Phew.

So bgzf_read_small() just needs fp->uncompressed_address += length; added to it to make it equivalent to bgzf_read().

(I have not analysed bgzf_write_small() to see if it has any similar infelicities. I checked and believe there are no similar problems in bgzf_write_small().)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions