gst/gstpad.c: Really unlink the peer pad instead of setting the peer pointer to NULL...

Original commit message from CVS:
* gst/gstpad.c: (gst_pad_dispose):
Really unlink the peer pad instead of setting the peer pointer to NULL
when we dispose the pad.
This correctly calls the unlink functions and makes sure that the peer
does not have a handle to invalid memory. See #504671.
* tests/check/gst/gstpad.c: (GST_START_TEST), (gst_pad_suite):
Add testsuite for above case.

diff --git a/ChangeLog b/ChangeLog
index 9d19294..e684da4 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2007-12-21  Wim Taymans  <wim.taymans@collabora.co.uk>
+
+       * gst/gstpad.c: (gst_pad_dispose):
+       Really unlink the peer pad instead of setting the peer pointer to NULL
+       when we dispose the pad.
+       This correctly calls the unlink functions and makes sure that the peer
+       does not have a handle to invalid memory. See #504671.
+
+       * tests/check/gst/gstpad.c: (GST_START_TEST), (gst_pad_suite):
+       Add testsuite for above case.
+
 2007-12-20  Tim-Philipp Müller  <tim at centricular dot net>
 
        Patch by: Peter Kjellerstedt <pkj axis com>

diff --git a/gst/gstpad.c b/gst/gstpad.c
index 5067cb9..1dca9ec 100644 (file)
--- a/gst/gstpad.c
+++ b/gst/gstpad.c
@@ -367,12 +367,22 @@ static void
 gst_pad_dispose (GObject * object)
 {
   GstPad *pad = GST_PAD (object);
+  GstPad *peer;
 
   GST_CAT_DEBUG_OBJECT (GST_CAT_REFCOUNTING, pad, "dispose");
 
-  /* we don't hold a ref to the peer so we can just set the
-   * peer to NULL. */
-  GST_PAD_PEER (pad) = NULL;
+  /* unlink the peer pad */
+  if ((peer = gst_pad_get_peer (pad))) {
+    /* window for MT unsafeness, someone else could unlink here
+     * and then we call unlink with wrong pads. The unlink
+     * function would catch this and safely return failed. */
+    if (GST_PAD_IS_SRC (pad))
+      gst_pad_unlink (pad, peer);
+    else
+      gst_pad_unlink (peer, pad);
+
+    gst_object_unref (peer);
+  }
 
   /* clear the caps */
   gst_caps_replace (&GST_PAD_CAPS (pad), NULL);

diff --git a/tests/check/gst/gstpad.c b/tests/check/gst/gstpad.c
index 9f6e827..df02e11 100644 (file)
--- a/tests/check/gst/gstpad.c
+++ b/tests/check/gst/gstpad.c
@@ -510,6 +510,73 @@ GST_START_TEST (test_push_negotiation)
 
 GST_END_TEST;
 
+/* see that an unref also unlinks the pads */
+GST_START_TEST (test_src_unref_unlink)
+{
+  GstPad *src, *sink;
+  GstCaps *caps;
+  GstPadLinkReturn plr;
+
+  sink = gst_pad_new ("sink", GST_PAD_SINK);
+  fail_if (sink == NULL);
+
+  src = gst_pad_new ("src", GST_PAD_SRC);
+  fail_if (src == NULL);
+
+  caps = gst_caps_from_string ("foo/bar");
+
+  gst_pad_set_caps (src, caps);
+  gst_pad_set_caps (sink, caps);
+
+  plr = gst_pad_link (src, sink);
+  fail_unless (GST_PAD_LINK_SUCCESSFUL (plr));
+
+  /* unref the srcpad */
+  gst_object_unref (src);
+
+  /* sink should be unlinked now */
+  fail_if (gst_pad_is_linked (sink));
+
+  /* cleanup */
+  gst_object_unref (sink);
+  gst_caps_unref (caps);
+}
+
+GST_END_TEST;
+
+/* see that an unref also unlinks the pads */
+GST_START_TEST (test_sink_unref_unlink)
+{
+  GstPad *src, *sink;
+  GstCaps *caps;
+  GstPadLinkReturn plr;
+
+  sink = gst_pad_new ("sink", GST_PAD_SINK);
+  fail_if (sink == NULL);
+
+  src = gst_pad_new ("src", GST_PAD_SRC);
+  fail_if (src == NULL);
+
+  caps = gst_caps_from_string ("foo/bar");
+
+  gst_pad_set_caps (src, caps);
+  gst_pad_set_caps (sink, caps);
+
+  plr = gst_pad_link (src, sink);
+  fail_unless (GST_PAD_LINK_SUCCESSFUL (plr));
+
+  /* unref the sinkpad */
+  gst_object_unref (sink);
+
+  /* src should be unlinked now */
+  fail_if (gst_pad_is_linked (src));
+
+  /* cleanup */
+  gst_object_unref (src);
+  gst_caps_unref (caps);
+}
+
+GST_END_TEST;
 
 Suite *
 gst_pad_suite (void)
@@ -530,6 +597,8 @@ gst_pad_suite (void)
   tcase_add_test (tc_chain, test_push_linked);
   tcase_add_test (tc_chain, test_flowreturn);
   tcase_add_test (tc_chain, test_push_negotiation);
+  tcase_add_test (tc_chain, test_src_unref_unlink);
+  tcase_add_test (tc_chain, test_sink_unref_unlink);
 
   return s;
 }