After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 785429 - wavparse: Fixed memory leak in gstwavparse.c
wavparse: Fixed memory leak in gstwavparse.c
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal minor
: 1.12.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-07-26 07:07 UTC by DEEPAK SRIVASTAVA
Modified: 2017-08-11 08:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch file (771 bytes, patch)
2017-07-26 07:07 UTC, DEEPAK SRIVASTAVA
needs-work Details | Review
Fixed memory leak patch file (759 bytes, patch)
2017-07-26 09:45 UTC, DEEPAK SRIVASTAVA
needs-work Details | Review
implemented review comments. (897 bytes, patch)
2017-07-27 04:27 UTC, DEEPAK SRIVASTAVA
needs-work Details | Review
Review comments implemented. (2.73 KB, patch)
2017-08-01 11:55 UTC, DEEPAK SRIVASTAVA
needs-work Details | Review
UpdatedPatch (1.52 KB, patch)
2017-08-10 09:48 UTC, DEEPAK SRIVASTAVA
committed Details | Review

Description DEEPAK SRIVASTAVA 2017-07-26 07:07:42 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 1 Sebastian Dröge (slomo) 2017-07-26 09:35:20 UTC
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)
Comment 2 DEEPAK SRIVASTAVA 2017-07-26 09:45:52 UTC
Created attachment 356411 [details] [review]
Fixed memory leak patch file
Comment 3 Sebastian Dröge (slomo) 2017-07-26 11:59:30 UTC
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
Comment 4 DEEPAK SRIVASTAVA 2017-07-27 04:27:51 UTC
Created attachment 356454 [details] [review]
implemented review comments.
Comment 5 Sebastian Dröge (slomo) 2017-07-27 11:19:32 UTC
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
Comment 6 DEEPAK SRIVASTAVA 2017-08-01 11:55:08 UTC
Created attachment 356717 [details] [review]
Review comments implemented.
Comment 7 Sebastian Dröge (slomo) 2017-08-09 11:32:20 UTC
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.
Comment 8 DEEPAK SRIVASTAVA 2017-08-10 09:48:55 UTC
Created attachment 357333 [details] [review]
UpdatedPatch

Please review the updated patch.
Comment 9 Sebastian Dröge (slomo) 2017-08-10 10:24:12 UTC
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