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 segfault due to ";HRUN=3" INFO field #95

Merged
merged 1 commit into from
Jun 15, 2020

Conversation

jmarshall
Copy link
Contributor

Fix vcf_var_has_info_key()'s key comparison, which causes the crash in #89 due to ;HRUN=3:

NC_045512	1044	.	CA	C	94	PASS	;HRUN=3

Previously any key would be returned as being matched by the entirely empty "key=value" at the start of ";HRUN=3". Note that INFO is semicolon-separated, so this is in fact invalid VCF.

This resulted in a crash when e.g. lofreq_filter.c looking for "SB" expected a value but in this case the returned sb_char was NULL. Crash fixed by no longer returning a hit for "SB" in this case.

(Also convert a couple of previously missed legacy samtools API usages. Oops.)

@andreas-wilm
Copy link
Contributor

andreas-wilm commented Jun 12, 2020

Thanks for reporting John! I think I found the culprit for the truncated info field (I used the same char* twice in sprintf, which leads to undefined results according to the POSIX standard; sorry this was shoddily written). Just committed a fix with 8206db2 (more exactly 1e44677). Your changes make certainly sense in addition. Will merge as soon as it's confirmed that this was indeed the culprit. Many thanks again

@jmarshall
Copy link
Contributor Author

Yep, your fix looks okay and stops the malformed ;HRUN=3 from occurring. Applying both fixes hits the problem from both sides…

I don't think there was anything undefined about the sprintfs; it was just a bug in that it overwrote the text that you had already put in buf.

Previously any key would be returned as being matched by the empty
"key=value" at the start of ";HRUN=3" (which is invalid VCF).

This resulted in a crash when e.g. lofreq_filter.c looking for "SB"
expected a value but in this case the returned sb_char was NULL.
Crash fixed by no longer returning a hit for "SB" in this case.

(Also convert a couple of previously missed legacy samtools API usages.)
@jmarshall
Copy link
Contributor Author

…pushed a more complete fix for the don't recognise AD=1;SBNOT=2;HRUN=3 as SB=2 part.

Copy link

@bgruening bgruening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks @jmarshall!

@andreas-wilm andreas-wilm merged commit 9ffbd58 into CSB5:master Jun 15, 2020
@andreas-wilm
Copy link
Contributor

Thanks again @jmarshall and @bgruening !

@jmarshall jmarshall deleted the info_key_compare branch June 15, 2020 07:43
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.

3 participants