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 787711 - Gstomxh264enc : Fix Memory leak in error case
Gstomxh264enc : Fix Memory leak in error case
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-omx
1.12.1
Other All
: Normal normal
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-09-15 09:04 UTC by Ponnam Srinivas
Modified: 2017-09-22 08:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch file attached (918 bytes, patch)
2017-09-19 04:44 UTC, Ponnam Srinivas
needs-work Details | Review
patch file attached (1.70 KB, patch)
2017-09-20 04:50 UTC, Ponnam Srinivas
none Details | Review
Attached the patch (1.71 KB, patch)
2017-09-21 10:00 UTC, Ponnam Srinivas
committed Details | Review

Description Ponnam Srinivas 2017-09-15 09:04:20 UTC
Hi,

 It is observed that there is memory leak in omxmodule.

File Name : omx/Gstomx264enc.c

Function Name : gst_omx_h264_enc_get_caps

In Gstomx264enc.c at gst_omx_h264_enc_get_caps line : 441

memory is allocated for caps ,But in case of fail it has not been unref.

Sol :  gst_caps_unref (caps);

Please provide your feedback.
Comment 1 Nicolas Dufresne (ndufresne) 2017-09-15 13:55:51 UTC
You should maybe provide a patch, checkout the gst-omx (master), correct the issue, commmit, git format-patch -1, and then attach the patch file to this ticket.
Comment 2 Ponnam Srinivas 2017-09-15 16:57:57 UTC
Okay I will attach patch file soon
Comment 3 Sebastian Dröge (slomo) 2017-09-18 10:38:51 UTC
See https://gstreamer.freedesktop.org/documentation/contribute/#how-to-submit-patches for more details
Comment 4 Ponnam Srinivas 2017-09-18 10:42:40 UTC
I am creating the Patch. I will a attach soon
Comment 5 Ponnam Srinivas 2017-09-19 03:46:43 UTC
sorry for delay. definitely will attach the patch today
Comment 6 Ponnam Srinivas 2017-09-19 04:44:00 UTC
Created attachment 360020 [details] [review]
patch file attached
Comment 7 Tim-Philipp Müller 2017-09-19 12:37:54 UTC
Comment on attachment 360020 [details] [review]
patch file attached

Thanks for the patch. Same comments as in bug #787714. Feel free to squash both patches into one commit as well :)
Comment 8 Ponnam Srinivas 2017-09-20 04:50:13 UTC
Created attachment 360101 [details] [review]
patch file attached

attached patch. Please review it.
Please accept this patch. Next time onwards I will add in single commit if multiple patchs in same module
Comment 9 Sebastian Dröge (slomo) 2017-09-20 08:35:13 UTC
Comment on attachment 360101 [details] [review]
patch file attached

https://bugzilla.gnome.org/show_bug.cgi?id=787714#c8
Comment 10 Ponnam Srinivas 2017-09-21 10:00:13 UTC
Created attachment 360174 [details] [review]
Attached the patch

correct the indentation. Please review it .
attach patch file.
Comment 11 Ponnam Srinivas 2017-09-22 04:06:29 UTC
Please review attach patch file and share feedback
Comment 12 Sebastian Dröge (slomo) 2017-09-22 08:19:16 UTC
Please have some patience, if you want to get your patches reviewed faster help out getting other patches reviewed. There are many.
Comment 13 Tim-Philipp Müller 2017-09-22 08:35:28 UTC
The unrefs after g_assert_not_reached () look 

The static analyzer should be thought about g_assert_not_reached instead.
Comment 14 Tim-Philipp Müller 2017-09-22 08:35:52 UTC
"look bogus".
Comment 15 Sebastian Dröge (slomo) 2017-09-22 08:53:47 UTC
commit 1f3663743422e7dbcbc22ec938437bd057151863 (HEAD -> master)
Author: Ponnam Srinivas <p.srinivas@samsung.com>
Date:   Thu Sep 21 15:21:36 2017 +0530

    omxh264enc: fix caps leak
    
    https://bugzilla.gnome.org/show_bug.cgi?id=787711
Comment 16 Sebastian Dröge (slomo) 2017-09-22 08:55:25 UTC
(In reply to Tim-Philipp Müller from comment #13)
> The unrefs after g_assert_not_reached () look 
> 
> The static analyzer should be thought about g_assert_not_reached instead.

Yes, for clean-ness I'd keep the unrefs there though. Someone might want to remove the assertions later because they are actually valid cases, and then forget to put the unref there.