Skip to content
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 hts_idx_get_n_no_coor() out-of-bounds memory access #1335

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

jmarshall
Copy link
Member

For CRAI-indexed CRAM files, idx actually points to an hts_cram_idx_t — which has no n_no_coor field. Return an appropriate "not available" value instead.

This OOB bug can be observed with valgrind samtools idxstats test/range.cram if you disable the slow method format check:

--- samtools-a/bam_index.c
+++ samtools-b/bam_index.c
@@ -209,7 +209,7 @@ int bam_idxstats(int argc, char *argv[])
-    if (hts_get_format(fp)->format != bam) {
+    if (0 && hts_get_format(fp)->format != bam) {
     slow_method:

It is also observable via pysam: see pysam-developers/pysam#1048.

@jmarshall
Copy link
Member Author

jmarshall commented Sep 28, 2021

AppVeyor failure heisenbug is due to an unrelated bug in PR #1325's fastq_parse1() changes: in SRA names mode, s[i] is the wrong offset into the string — it reflects the addition of the initial value of i to x->name.s twice. It's amazing it doesn't fail on other platforms too.

IMHO the FASTA/Q name extraction code needs an overhaul, as it doesn't handle the case of spaces before the name, e.g.,

>   name   comment blah blah
ATGC

which is borderline crap, but something that faidx has had to deal with for years (cf samtools/samtools#449). Thus demonstrating that it does exist out in the world.

@daviesrob
Copy link
Member

Thanks. A fix for the fastq_name2 problem is now in #1337.

For CRAI-indexed CRAM files idx actually points to an hts_cram_idx_t,
which has no n_no_coor field. Return an appropriate "not available"
value instead.
@daviesrob daviesrob merged commit 2dbbcf6 into samtools:develop Sep 28, 2021
@jmarshall jmarshall deleted the crai-no-no_coor branch September 28, 2021 12:08
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