Skip to content

Fixed memory allocation issue when decoding CRAM.#357

Merged
jmarshall merged 2 commits intodevelopfrom
cram_mem_fix
Mar 21, 2016
Merged

Fixed memory allocation issue when decoding CRAM.#357
jmarshall merged 2 commits intodevelopfrom
cram_mem_fix

Conversation

@jkbonfield
Copy link
Copy Markdown
Contributor

This bug has existed a long time, but is now more likely to triggered by 6d2810c due to making it more likely to overrun the buffer.

The issue is caused by zero length sequences ("*") with a CIGAR alignment. A full bullet-proof fix will require more work to use the ...decode_block() codec variants, but this fixes the common case of length 0.

This bug has existed a long time, but is now more likely to trigger by
6d2810c due to making it more likely
to overrun the buffer.

The issue is caused by zero length sequences ("*") with a CIGAR
alignment.  A full bullet-proof fix will require more work to use the
...decode_block() codec variants, but this fixes the common case of
length 0.
@jmarshall jmarshall added this to the 1.3.1 milestone Mar 21, 2016
These are from Staden io_lib, which exposed the buffer overrun fixed
in 1a050b4. The CRAMs have been made
by a version of Cramtools-3.0.jar and are designed for
interoperability testing.
@jmarshall jmarshall merged commit 5ff6cf0 into develop Mar 21, 2016
@jmarshall
Copy link
Copy Markdown
Member

Thanks — merged with some additions. Did you notice this warning in the new tests?

./test_view -i reference=xx.fa xx#large_aux_java.cram > xx#large_aux_java.tmp.sam
WARNING: Header @SQ length mismatch for ref xx, 30 vs 20
./compare_sam.pl -nomd xx#large_aux.sam xx#large_aux_java.tmp.sam
.. ok

@jmarshall jmarshall deleted the cram_mem_fix branch March 21, 2016 16:47
@jkbonfield
Copy link
Copy Markdown
Contributor Author

Actually no I didn't spot that - it was lost in the mass of output. It's caused by comparing to an older variant of the test ref seqs, although fortunately harmless.

Thanks.

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.

2 participants