htslib icon indicating copy to clipboard operation
htslib copied to clipboard

Memory issues in bcf_get_format_values (and other places that use realloc)

Open reedacartwright opened this issue 7 years ago • 2 comments

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;.

reedacartwright avatar May 05 '17 05:05 reedacartwright

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.

jkbonfield avatar May 05 '17 08:05 jkbonfield

Yeah, I figured that the typo would be the easiest to fix and the rest involves a massive update.

reedacartwright avatar May 05 '17 20:05 reedacartwright