Stricter limit on POS/MPOS/TLEN in sam_parse1() #1812
Merged
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
Help avoid overflow on arithmetic involving POS, MPOS and TLEN by limiting values in the SAM parser to fit in 62 bits (or 63 for TLEN as it's signed). The new limit is still massively bigger than any known reference so it should not cause any problems in practice. Credit to OSS-Fuzz Fixes oss-fuzz 68750
|
Does |
Limiting POS to 62 bits helps avoid the risk of signed overflow when it's set to a very extreme value. The maximum is still much higher than the length of the longest currently known reference.
|
There's probably less urgency on that one, as I think there's fewer troublesome calculations done on positions in VCF files. But the change doesn't appear to cause any problems in HTSlib or BCFtools, so I guess it's worth doing. |
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.
Help avoid overflow on arithmetic involving POS, MPOS and TLEN by limiting values in the SAM parser to fit in 62 bits (or 63 for TLEN as it's signed). The new limit is still massively bigger than any known reference so it should not cause any problems in practice.
The actual problem reported was a signed value overflow in
process_one_read(). Getting this calculation to work correctly for the entire range ofint64_twould be complicated. It's easier just to ensure the inputs are within a range where overflow cannot happen.Credit to OSS-Fuzz
Fixes oss-fuzz 68750