htslib
htslib copied to clipboard
Memory issues in bcf_get_format_values (and other places that use realloc)
1. Memory leaks caused by failed realloc
function.
bcf_get_format_values
uses realloc
internally to resize as needed the memory block pointed to by *dst
. E.g.
if ( *ndst < n )
{
*dst = realloc(*dst, n);
if ( !*dst ) return -4; // could not alloc
*ndst = n;
}
https://github.com/samtools/htslib/blob/develop/vcf.c#L3765-L3767
The problem with this block is that if realloc fails, it returns a null pointer and leaves the original block untouched. See http://en.cppreference.com/w/c/memory/realloc. However, by assigning the return value of realloc
to *dst
the library and the caller of the function loses the original block, making it impossible to free unless a copy of the pointer was made before hand. The above block (and ones like it) should be replaced by
if ( *ndst < n )
{
void *newdst = realloc(*dst, n);
if ( !newdst ) return -4; // could not alloc
*dst = newdst;
*ndst = n;
}
This way, the original value of *dst
is preserved if realloc
fails allowing it to be freed by the caller.
2. Typo
I also spotted a typo at https://github.com/samtools/htslib/blob/develop/vcf.c#L3780. This line should be if ( !*dst ) return -4;
.
Good spot on that typo.
The realloc issue sadly turns up in a lot of places. Eg a grep of realloc in htslib/cram/*.c I see 39 reallocs of which only 12 appear to assign to a new variable (that code along with the issues are down to me). In htslib/*.c it's 51 reallocs with 11 assigning to new variables. So it's definitely a common idiom with many authors following the same ease-of-typing route. Fortunately it's not actually a serious problem for most applications - if you've run out of memory, then frankly you're probably going to be reporting an error and shutting down within a second anyway!
If I recall cppcheck
spots these and that's one of the things we want to work our way through these and other issues spotted.
Yeah, I figured that the typo would be the easiest to fix and the rest involves a massive update.