Skip to content

Assertion failure in cram_index_build() on a malformed CRAM file #2060

Closed
#2061
Closed
Assertion failure in cram_index_build() on a malformed CRAM file#2060
#2061

Description

@CrJyyy
CrJyyy
opened on Aug 4, 2026

Summary

A 42-byte file makes sam_index_build2() abort on an assertion instead of returning an error.
cram_index_build() is documented to return -1 on read failure, but a container whose first
block is not a compression header trips assert() and kills the process.

$ base64 -d <<< 'Q1JBTQEAAAAAAAAAAAAAZm9vb29vAQAAAAAAAAAAAABmYUAAAABvAAAA' > poc.cram
$ ./repro poc.cram
repro: cram/cram_index.c:823: cram_index_build: Assertion
       `c->comp_hdr_block->content_type == COMPRESSION_HEADER' failed.
Aborted (core dumped)

repro.c is eleven lines and does only what samtools index <file> does:

#include <stdio.h>
#include "htslib/sam.h"

int main(int argc, char **argv)
{
    if (argc < 2) { fprintf(stderr, "usage: %s <file.cram>\n", argv[0]); return 2; }
    int r = sam_index_build2(argv[1], NULL, 0);
    printf("sam_index_build2 returned %d\n", r);
    return 0;
}

Version

Reproduced on the 1.24 release tarball (latest, 2026-07-09) and on current develop; the
assertion is at cram/cram_index.c:823 in both.

$ ./configure --disable-bz2 --disable-lzma --disable-libcurl --disable-gcs --disable-s3
$ make lib-static
$ gcc -O2 -g -I. -o repro repro.c libhts.a -lz -lpthread -lm

No sanitizer, no fuzzing engine, no special flags. HTSlib's own configure does not define
NDEBUG, so the assertion is live in a default build.

Host: Linux x86-64, GCC.

Cause

cram_index_build() (cram/cram_index.c:787) is documented as

/*
 * Builds an index file.
 * ...
 * Returns 0 on success,
 *         negative on failure (-1 for read failure, -4 for write failure)
 */

but the container loop asserts on the content type of a block that comes straight from the file:

    while ((c = cram_read_container(fd))) {
        ...
        if (!(c->comp_hdr_block = cram_read_block(fd)))
            goto err;
        assert(c->comp_hdr_block->content_type == COMPRESSION_HEADER);   /* line 823 */

        c->comp_hdr = cram_decode_compression_header(fd, c->comp_hdr_block);
        if (!c->comp_hdr)
            goto err;

The block immediately before and the one immediately after are both handled with goto err.
Only the content-type check is an assertion, and nothing upstream of it guarantees the invariant
— cram_read_block() returns whatever type the file declares.

Impact

Assertion abort only. Rebuilding all of HTSlib with -DNDEBUG and running the same input gives

$ ./repro_ndebug poc.cram
sam_index_build2 returned -1

and an ASan+UBSan build of the same configuration is clean on it, so cram_decode_compression_header()
rejects the block through the normal error path and there is no memory-safety consequence behind the
assertion. The practical effect is that any program indexing an untrusted CRAM with a default-built
HTSlib aborts rather than reporting a bad file.

Possible fix

Treating it like the surrounding failures fixes it:

--- a/cram/cram_index.c
+++ b/cram/cram_index.c
@@ -820,7 +820,10 @@
         if (!(c->comp_hdr_block = cram_read_block(fd)))
             goto err;
-        assert(c->comp_hdr_block->content_type == COMPRESSION_HEADER);
+        if (c->comp_hdr_block->content_type != COMPRESSION_HEADER) {
+            hts_log_error("Expected a compression header block");
+            goto err;
+        }

With that applied:

$ ./repro_fixed poc.cram
[E::cram_index_build] Expected a compression header block
sam_index_build2 returned -1

test/test.pl gives 349 passed / 7 failed out of 356 — byte-identical to the unpatched 1.24 tree
in this environment (the 7 failures are pre-existing here and come from configuring without bz2 /
lzma / libcurl). I have not checked whether this is the fix you would prefer; you may want the
container reader to reject the block earlier instead.

Note on coverage

test/fuzz/hts_open_fuzzer.c never calls sam_index_build* or any other index-building entry
point, so this path is not reachable from the OSS-Fuzz target — which is presumably why it has
survived.

How it was found

Automated fuzzing that called sam_index_build2() on a temporary file filled with fuzzer bytes.
The 162-byte original was minimised to the 42 bytes above and re-verified against a stock 1.24
release build, so the reproducer involves no fuzzing harness.

Activity

  1. jkbonfield commented on Aug 5, 2026

    @jkbonfield
    jkbonfield
    on Aug 5, 2026
    Contributor

    Thanks for this. I put your suggested change into PR #2061.

    You've also reminded me that I had some unfinished work to add index fuzzing, amongst other things. It's not finished yet as I was struggling to avoid spamming lots of temporary filenames (and for now just had fixed paths which forbid multi-threading or multiple tests running). However it did find a variety of issues, just not this one.

  2. jkbonfield
    added a commit that references this issue on Aug 6, 2026

    Remove an assert in cram_index_build.

    9aed115
  3. daviesrob
    closed this as completedin 4809e29on Aug 6, 2026
  4. pull
    added a commit that references this issue on Aug 6, 2026

    Remove an assert in cram_index_build.

    4809e29

  5. CrJyyy commented on Aug 9, 2026

    @CrJyyy
    CrJyyy
    on Aug 9, 2026
    Author

    Thanks for the quick fix.

    One follow-up, since it may be worth more than the single assert. That assert came from a generated fuzz harness calling sam_index_build2() on a temporary file of fuzzer bytes. test/fuzz/hts_open_fuzzer.c never calls sam_index_build*, sam_idx_init, or any other index-building entry point, so cram_index_build() and the BAI/CSI builders are not currently fuzzed at all — presumably why this one survived.

    Would a target covering that be welcome in test/fuzz/? I am happy to write and test it. One caveat you should weigh first: it needs a real file per iteration. cram_index_build() is reachable only through the filename-based API — the call chain is sam_index_build2 → sam_index_build3 → hts_open(fn) — so the in-memory hopen("mem:", ...) approach hts_open_fuzzer.c uses cannot work here, and the per-iteration file costs throughput.

    If you would rather leave index building unfuzzed, that is a perfectly good answer — I would just rather ask than send an unsolicited PR.

  6. daviesrob commented on Aug 10, 2026

    @daviesrob
    daviesrob
    on Aug 10, 2026
    Member

    Fuzz testing the indexes is a bit of a pain due for the need for two files. We have already got a pull request (#2039) that adds a fuzzer to exercise the index code. Unfortunately it occasionally doesn't clean up after itself, so when I tried it the VM it was on eventually ran out of space. It also didn't find much, probably because it reads index files that it has made itself. It may be possible to fix it by getting the interfaces to read directly from file descriptors, but that will need some additions to the rest of the library.

    It may be more useful to fuzz mutated indexes on a pre-existing file. That way there's only one file being mutated again, and it may manage to get to some interesting paths through the code. I'm not sure how efficient it would be though as many changes are likely to completely break the index, which would hopefully be rejected very quickly. That might make it difficult for the fuzzer to make progress as most of its changes would just lead to the same error handling code. It could be worth a try though to see how far it does get.

  7. jkbonfield commented on Aug 10, 2026

    @jkbonfield
    jkbonfield
    on Aug 10, 2026
    Contributor

    Thanks for offering to do the work and for asking first.

    I do have a branch doing this, in https://github.com/jkbonfield/htslib/tree/fuzz-index, which found a few bugs although oddly not this one. I need to tidy that up and remove some of the commits which made their way into a separate memory-leak fixing PR (already merged).

    The reason I didn't do a PR for this was it needs more work. Either it was creating lots of temporary files and leaving them in place as there's no tidy up procedure when a fuzzer fails, slowly running me out of disk space, or it was using a fixed filename (eg in /tmp) and forbidding multiple threads from running. I think we can overcome this maybe with data or mem URIs, but it needs more experimentation.

    The point about harming throughput is an interesting one though, and it makes me wonder if we need a separate index fuzzer that can run in parallel with the file parsing fuzzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions