GNOME Bugzilla – Bug 785429
wavparse: Fixed memory leak in gstwavparse.c
Last modified: 2017-08-11 08:06:30 UTC
Created attachment 356398 [details] [review] Patch file Below declaration has been done in gstwavparse.h GList *notes; and for this memory is allocated in gst_wavparse_note_chunk() in gstwavparse.c, which is not released in gst_wavparse_reset (). Patch file is attached. Please provide your feedback.
Comment on attachment 356398 [details] [review] Patch file Patch seems ok, but please update the commit message: https://gstreamer.freedesktop.org/documentation/contribute/index.html#writing-good-commit-messages (It's also not a *possible* leak, it is a leak)
Created attachment 356411 [details] [review] Fixed memory leak patch file
Review of attachment 356411 [details] [review]: The format of the commit message is still wrong, it should be: elementname-or-component: short summary what is fixed [empty line] Long description if needed [empty line] https://link.to/this/bug ::: gst/wavparse/gstwavparse.c @@ +237,3 @@ wav->labls = NULL; + if (wav->notes) + g_list_free_full (wav->notes, g_free); This is leaking the text field of the GstWavParseNote structs
Created attachment 356454 [details] [review] implemented review comments.
Review of attachment 356454 [details] [review]: ::: gst/wavparse/gstwavparse.c @@ +237,3 @@ wav->labls = NULL; + if (wav->notes) + g_list_free_full (wav->notes, g_free); This still leaks the text field of those structs. Note that the text field of the labls is also leaked above already... if you want to send a patch for that one too
Created attachment 356717 [details] [review] Review comments implemented.
Review of attachment 356717 [details] [review]: Please run your code through "gst-indent" ::: gst/wavparse/gstwavparse.c @@ +95,3 @@ +static void gst_wavparse_labl_chunk_remove_data(GList* object); +static void gst_wavparse_note_chunk_remove_data(GList* object); How about gst_wavparse_notes_free() instead? Shorter and more consistent @@ +806,3 @@ +{ + if(!labl_list) + return; You already check for NULL in the callers @@ +807,3 @@ + if(!labl_list) + return; + for(GList* l = labl_list; l; l = l->next) This is C99, please declare the list at the top of the block @@ +811,3 @@ + GstWavParseLabl *labl = (GstWavParseLabl*)l->data; + if(labl->text) + g_free(labl->text); g_free(NULL) is valid, no need to check for NULL first @@ +813,3 @@ + g_free(labl->text); + } + g_list_free_full (labl_list, g_free); More useful would probably be "g_list_free_full (labl_list, labl_free)" instaed of having the loop above, and just freeing everything in the newly defined labl_free function.
Created attachment 357333 [details] [review] UpdatedPatch Please review the updated patch.
commit 059420b678a00d503a772c35bebb41877c1c50a1 (HEAD -> master) Author: Deepak Srivastava <srivastava.d@samsung.com> Date: Thu Aug 10 15:14:31 2017 +0530 wavparse: Fix memory leak in wavparse element Fixing of leaking the text field of the GstWavParseNote and GstWavParseLabl structure. https://bugzilla.gnome.org/show_bug.cgi?id=785429