Fix bug where bin number could overflow when looking for max_off #1595
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When searching for `max_off`, hts_itr_query() and hts_itr_multi_bam() look for a bin to the right of the end of the region. For whole chromosomes, this would be HTS_POS_MAX, which is far beyond the maximum bin position supported. The `bin` calculation overflowed leading to either a negative bin number or an incorrect positive one, depending on the number of levels in the index. Negative bin numbers simply caused time to be wasted as the search loop eventually counted up to zero, but incorrect positive ones could cause the iterator to finish too early. Fix by catching the out-of-bounds case and setting max_off to UINT64_MAX, whch should be used for bins beyond the end of the indexable range.
jkbonfield
reviewed
Apr 5, 2023
| bin++; | ||
| // First check if end lies within the range of the index (it won't | ||
| // if it's HTS_POS_MAX) | ||
| if (end < 1LL << (idx->min_shift + 3 * idx->n_lvls)) { |
There was a problem hiding this comment.
It's a useful shortcut I think, but it looks like using uint64_t for bin would have made the old code work too, as hts_bin_first is then huge and bin gets reset to 0, falling out of the while loop with max_off = UINT64_MAX.
Otherwise we're still using a negative bin below, eg:
for (i = n_off = 0; i < iter->bins.n; ++i)
if ((k = kh_get(bin, bidx, iter->bins.a[i])) != kh_end(bidx))
n_off += kh_value(bidx, k).n;
Edit: argh! That blasted obscure templating code... ignore me. "bin" isn't a variable here, but a namespace shadowing a variable name. Sigh.
There was a problem hiding this comment.
Ah I now see how to get a positive legal bin and still cause mayhem, so agreed your solution of checking for the maximum value is definitely the best way of handling it.
$ tabix -m10 -f /tmp/_.bed.gz;./tabix /tmp/_.bed.gz a
That takes 10 sec to query my 5 entry bed file. (Admittedly with debugging on)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When searching for
max_off,hts_itr_query()andhts_itr_multi_bam()look for a bin to the right of the end of the region. For whole chromosomes, this would be HTS_POS_MAX, which is far beyond the maximum bin position supported. Thebincalculation overflowed leading to either a negative bin number or an incorrect positive one, depending on the number of levels in the index. Negative bin numbers simply caused time to be wasted as the search loop eventually counted up to zero, but incorrect positive ones could cause the iterator to finish too early.Fix by catching the out-of-bounds case and setting max_off to UINT64_MAX, which should be used for bins beyond the end of the indexable range.
Luckily in practice this didn't cause too much harm, at least for the default
min_shiftvalue. Indexes with up to six levels overflowed to negative bin numbers. For seven you got one referencing a region starting at about 17Gbases, and 257Gbases for eight, so it's unlikely searches on real data were affected. The fix is trivial though and avoids some negative value shifts so worth doing.