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 678017 - gstfunnel: access of freed pad object
gstfunnel: access of freed pad object
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.10.37
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-06-13 13:16 UTC by David Svensson Fors
Modified: 2012-06-14 13:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstfunnel: avoid access of freed pad (1.31 KB, patch)
2012-06-13 13:16 UTC, David Svensson Fors
committed Details | Review

Description David Svensson Fors 2012-06-13 13:16:48 UTC
Created attachment 216277 [details] [review]
gstfunnel: avoid access of freed pad

When running gst-rtsp-server in valgrind, I've found an issue in gstfunnel.c. In gst_funnel_release_pad, a pad object is accessed after a call to gst_element_remove_pad. This can be problematic, because gst_element_remove_pad may free the pad. The attached suggested patch avoids this problem.
Comment 1 Vincent Penquerc'h 2012-06-14 11:14:27 UTC
While this seems right at first glance, I saw while looking further that gst_element_remove_pad is marked with "@pad: (transfer none):", which seems to conflict with the idea that the pad may be unreffed/destroyed. That annotation may be wrong, or I'm mistaken on the exact semantics of it.
Comment 2 Tim-Philipp Müller 2012-06-14 12:59:28 UTC
The transfer none just means that if the caller owned a ref to the pad before calling _remove_pad(), it will still own that ref afterwards. It doesn't have anything to do with whether the pad may be unreffed by some other piece of code that previously held a ref (such as the element). It can't be destroyed though, because the caller needs to hold a valid ref while calling _remove_pad().

I *think* the element implementation of request_new_pad is wrong here, I think it should return a ref, but element_add_pad() took ownership of the pad after creating it.

Perhaps compare with other elements such as tee.
Comment 3 Vincent Penquerc'h 2012-06-14 13:12:55 UTC
After IRC discussion, I've changed the annotation to full, then pushed the original patch (0.10 and master):

commit 066b515985897495cae32fca5b7eeeec260c40c9
Author: David Svensson Fors <davidsf@axis.com>
Date:   Tue Jun 12 13:26:35 2012 +0200

    gstfunnel: avoid access of freed pad
    
    Save the value of the pad's got_eos in gst_funnel_release_pad,
    before calling gst_element_remove_pad. This is because
    gst_element_remove_pad may free the pad.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=678017


Thanks