[Bug 725809] New: [PATCH] GstGhostPad rare crash because of missing reference count on its target pad

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=725809
  GStreamer | gstreamer (core) | git

           Summary: [PATCH] GstGhostPad rare crash because of missing
                    reference count on its target pad
    Classification: Platform
           Product: GStreamer
           Version: git
        OS/Version: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: Normal
         Component: gstreamer (core)
        AssignedTo: gstreamer-bugs at lists.freedesktop.org
        ReportedBy: kiagiadakis.george at gmail.com
         QAContact: gstreamer-bugs at lists.freedesktop.org
     GNOME version: ---


Created an attachment (id=271082)
 View: https://bugzilla.gnome.org/attachment.cgi?id=271082
 Review: https://bugzilla.gnome.org/review?bug=725809&attachment=271082

proposed patch

Hello,

GstGhostPad has a bug in gst_ghost_pad_set_target() where it unlinks its target
pad from its internal proxy pad without holding a reference to the target pad.
This can lead to a crash in the rare case that another thread destroys the
target pad at the same time.

This has been observed to happen with rtspsrc in the 0.10 series and I believe
it can also be observed in 1.0 before this patch was applied:
http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/gst/rtsp/gstrtspsrc.c?id=d9bc48edc977f86b39aabe9e1f125603c82bec6a

Before this patch was applied, rtspsrc would internally create and add elements
to itself (being a GstBin) without locking their state, which would lead to the
following happening in its set_state(NULL) sequence:

- change_state() gets called with GST_STATE_CHANGE_PAUSED_TO_READY
- rtspsrc sends a command to its thread to close the connections and stop
- change_state() returns immediately and later gets called again with
GST_STATE_CHANGE_READY_TO_NULL
- the rtspsrc implementation of change_state() calls the parent's (GstBin's)
implementation, which sets all children to NULL state, which causes rtpbin (one
of the children) to destroy its dynamic srcpad that is proxied in rtspsrc with
a ghost pad
- meanwhile, rtspsrc's other thread is closing the connections and destroying
its resources, including the ghost pad that proxies rtpbin's srcpad
- if rtpbin manages to destroy the srcpad while gst_pad_unlink(), called from
gst_ghost_pad_set_target(), is running on the other thread -> crash!

Of course, the problem with rtspsrc is no longer relevant. However, the fact
that GstGhostPad does not hold a reference to the pad while it is calling a
method on it is still relevant.

The attached patch should solve the issue. Note that I am using
gst_pad_get_peer(internal) because I believe it is essential to hold the
internal pad's lock, NOT the ghost pad's lock. The target pad can still be
destroyed with the ghost pad locked, but it cannot be destroyed with the
internal pad locked, because the target's dispose() function would have to
unlink it and therefore take the lock of both pads (internal & target). For
this reason, I believe that gst_proxy_pad_get_target() also suffers from the
same bug; it locks the ghost pad, which is irrelevant, and tries to take the
peer of the internal pad.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.