search

samtools / htslib

C library for high-throughput sequencing data formats
Other
810 stars 447 forks source link

Add HTS_RESULT_USED to all/some vcf.h functions #740

Open pd3 opened 6 years ago

pd3 commented 6 years ago

This is a reminder for me to discuss whether we should enforce checks of return codes. Just encountered a nasty case of big truncated data, probably preventable if the chief bcftools programmer was not lazy. If yes, the next API/ABI breaking release would be a good time to do it.

jkbonfield commented 6 years ago

Absolutely yes IMO.

See the HTS_RESULT_USED macro in htslib/*.h declarations. On compilers supporting it, these enforce checking of return values. It's not perfect, but goes a long way forward to making us check.

The second part, as you've hinted at, is that the prototypes themselves need changing in a number of places so they're no longer void. See https://github.com/samtools/htslib/issues/246.

Some of it is more problematic than others. Eg functions that realloc (hts_expand for one) will lead to immediate crashes if memory runs out. This is far from elegant, but the likely correct choice here is to bubble an error code up the call chain and exit from main with an error gracefully instead of crash and burn, giving the same net effect (detectable failure). Far more insidious are functions that write to disk (or stdout with a redirect) and don't detect running out of disk space. These lead to silent data truncation and are a level above simple malloc checking issues.

Ideally we'd do all of it, but we have to start somewhere so I propose we start on the I/O side as it's more serious.

Edit: that said, it appears I started on malloc, strdup, etc before! https://github.com/jkbonfield/htslib/tree/vcf_checks

daviesrob commented 6 years ago

Yes, it's very important to check all I/O functions.

Other ones we need to pay more attention to are khash and kstring. With both of those, if malloc fails the data doesn't get added and they return an error code. If you don't check for the error then you end up with data going missing (assuming you somehow survive the low memory problem).

As checking kstring functions can get a bit tedious, I often try to solve this by expanding the kstring first and checking that works. Although in discussion here, James floated the idea of having an error bit in the kstring to indicate the failure. That would allow the test to be done after all the data is written, which might be easier. We would have to modify kstring a bit to get this to work though.