libFLAC has several places like this: if(0 == (ptr = realloc(ptr, size))) return false; which results in memory leaks if realloc fails (the old value of ptr is lost). The patch should fix this. -------------- next part -------------- A non-text attachment was scrubbed... Name: realloc_memleak_fix.patch Type: application/octet-stream Size: 6272 bytes Desc: not available Url : http://lists.xiph.org/pipermail/flac-dev/attachments/20150706/2d957d95/attachment.obj
lvqcl wrote:> libFLAC has several places like this: > > if(0 == (ptr = realloc(ptr, size))) > return false; > > which results in memory leaks if realloc fails (the old value of ptr is lost). > The patch should fix this.Applied. Thanks. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Erik de Castro Lopo wrote:>> libFLAC has several places like this: >> >> if(0 == (ptr = realloc(ptr, size))) >> return false; >> >> which results in memory leaks if realloc fails (the old value of ptr is lost). >> The patch should fix this. > > Applied. Thanks.Are you sure? Because I can't see it in git.
lvqcl wrote:> libFLAC has several places like this: > > if(0 == (ptr = realloc(ptr, size))) > return false; > > which results in memory leaks if realloc fails (the old value of ptr is lost). > The patch should fix this.I found a problem with this patch. Specifcally, where ever the patch tries to free() the old pointer where safe_realloc_mul_2op_() fails, can result in a double free(). This is because, when safe_realloc_mul_2op_() has either of its size arguments equal to zero, will call realloc(ptr, 0) which according to the realloc manpage is an implicit free() anyway. Working on a fix for this and re-visiting some of this realloc() stuff. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Erik de Castro Lopo wrote:> I found a problem with this patch. > > Specifcally, where ever the patch tries to free() the old pointer > where safe_realloc_mul_2op_() fails, can result in a double free(). > > This is because, when safe_realloc_mul_2op_() has either of its size > arguments equal to zero, will call realloc(ptr, 0) which according to > the realloc manpage is an implicit free() anyway. > > Working on a fix for this and re-visiting some of this realloc() > stuff. >According to the following links -- http://www.cplusplus.com/reference/cstdlib/realloc/ http://stackoverflow.com/questions/6502077/malloc-and-realloc-functions http://stackoverflow.com/questions/16759849/using-realloc-x-0-instead-of-free-and-using-malloc-with-length-of-a-string -- the realloc behavior is different in C90 and C99: realloc(ptr,0) will free ptr in C90 but the behaviour is implementation defined in C99. So one shouldn't use realloc(ptr,0) instead of free(ptr). For example: in metadata_object.c functions that call realloc(ptr, size) check the size argument and never call realloc() if size==0. What a caller function expects from safe_realloc_mul_2op_(ptr, sz1, sz2) if it calls it with the 2nd or 3rd argunent equal to 0? And i have no idea what /* preserve POSIX realloc(ptr, 0) semantics */ comment means.
Erik de Castro Lopo wrote:> Working on a fix for this and re-visiting some of this realloc() > stuff.I noticed that your patch removes the call to vorbiscomment_entry_array_delete_() in FLAC__metadata_object_vorbiscomment_resize_comments() function, and I think that it reintroduces one place of a potential memory leak: 'object->data.vorbis_comment.comments' is a pointer to an array of FLAC__StreamMetadata_VorbisComment_Entry structs, and these structs contain pointers. So it's necessary to free all 'object->data.vorbis_comment.comments[i].entry' pointers before freeing 'object->data.vorbis_comment.comments' pointer itself.