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 733119 - utils: Unref/release pads in error cases when linking pads
utils: Unref/release pads in error cases when linking pads
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal minor
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-13 13:36 UTC by Sebastian Rasmussen
Modified: 2014-08-06 08:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch 1/2 that plugs the leaks. (7.17 KB, patch)
2014-07-13 13:38 UTC, Sebastian Rasmussen
none Details | Review
Proposed patch 2/2 that adds a unit test to verify the changes in 1/2. (9.74 KB, patch)
2014-07-13 13:38 UTC, Sebastian Rasmussen
none Details | Review
Proposed patch 1/2 that plugs the leaks. (9.38 KB, patch)
2014-08-06 00:27 UTC, Sebastian Rasmussen
committed Details | Review
Proposed patch 2/2 that adds a unit test to verify the changes in 1/2. (9.93 KB, patch)
2014-08-06 00:28 UTC, Sebastian Rasmussen
committed Details | Review

Description Sebastian Rasmussen 2014-07-13 13:36:45 UTC
I accidentally discovered that gst_element_link_pads_full() had several instances where if it failed to link it was leaking reference to pads or forgot to release request pads.

The attached patch attempts to fix these leaks as well as add a unit test to verify that no leaks persist. I ran this successfully in valgrind.

Now, this slightly changes the behaviour of gst_element_link_pads_full() as it previously would in some cases not release any request pads on the elements you tried to link together. I don't know if there is any code out there that used to take this into account.

Also please review the changes carefully. While I think this patch improves the situation, the code in this helper(!)-function is sufficiently hairy to have made me suspicious of my own changes.
Comment 1 Sebastian Rasmussen 2014-07-13 13:38:15 UTC
Created attachment 280579 [details] [review]
Proposed patch 1/2 that plugs the leaks.
Comment 2 Sebastian Rasmussen 2014-07-13 13:38:45 UTC
Created attachment 280580 [details] [review]
Proposed patch 2/2 that adds a unit test to verify the changes in 1/2.
Comment 3 Sebastian Rasmussen 2014-07-24 19:45:04 UTC
I'm not sure if aids in reviewing this, but I ran this in lcov and it seems like almost all of gst_element_link_pads_full() is covered by the testcases. The parts that are not handle the scenario where a specific srcpad is not named so gst_element_link_pads_full() chooses a random srcpad which fails to link with a compatible destpad so gst_element_link_pads_full() advances to the next srcpad. And the reverse case where the destpad is not named and gst_element_link_pads_full() is looking for a compatible srcpad. I don't think it is worthwhile to add tests for these cases, at least not right now.
Comment 4 Thiago Sousa Santos 2014-07-28 13:55:01 UTC
This fix broke some tests in core: collectpads, ghostpad and bin.
Comment 5 Sebastian Rasmussen 2014-08-06 00:27:55 UTC
Created attachment 282614 [details] [review]
Proposed patch 1/2 that plugs the leaks.
Comment 6 Sebastian Rasmussen 2014-08-06 00:28:16 UTC
Created attachment 282615 [details] [review]
Proposed patch 2/2 that adds a unit test to verify the changes in 1/2.
Comment 7 Sebastian Rasmussen 2014-08-06 00:28:48 UTC
Ok, I have now updated my patches to not cause any issues for collectpads, ghostpad and bin tests.
Comment 8 Sebastian Dröge (slomo) 2014-08-06 08:02:16 UTC
commit 2e4ce5caf6aeab383d79006cb4fb7714eda13174
Author: Sebastian Rasmussen <sebras@hotmail.com>
Date:   Sun Jul 13 15:31:08 2014 +0200

    tests: Add test verifying gst_element_link_pads_full()
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=733119

commit a041e65503dec792e904a803e82f4f263d92ec22
Author: Sebastian Rasmussen <sebras@hotmail.com>
Date:   Sun Jul 13 15:28:32 2014 +0200

    utils: Unref/release pads in error cases when linking pads
    
    Previously gst_element_link_pads_full() forgot to unreference or release
    request pads in several error cases. Also comments were added mentioning
    why releasing is not necessary in some places.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=733119