From 36ab067905fffdd405aa12c6c5bce00e7a7b9b09 Mon Sep 17 00:00:00 2001 From: Thomas Bluemel Date: Thu, 8 Sep 2016 08:49:54 -0600 Subject: [PATCH 1/4] clock: Keep weak reference to underlying clock Fixes potential segmentation fault when using a GstClockID that is referencing an already freed GstClock Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/187 --- gst/gstclock.c | 102 ++++++++++++++++++++++++++++++++++-- gst/gstclock.h | 18 ++++++- libs/gst/base/gstbasesink.c | 5 +- 3 files changed, 116 insertions(+), 9 deletions(-) diff --git a/gst/gstclock.c b/gst/gstclock.c index aa1ed5ca4e4..007ceed3755 100644 --- a/gst/gstclock.c +++ b/gst/gstclock.c @@ -249,7 +249,10 @@ gst_clock_entry_new (GstClock * clock, GstClockTime time, "created entry %p, time %" GST_TIME_FORMAT, entry, GST_TIME_ARGS (time)); entry->refcount = 1; +#ifndef GST_DISABLE_DEPRECATED entry->clock = clock; +#endif + g_weak_ref_init (&entry->ABI.clock, clock); entry->type = type; entry->time = time; entry->interval = interval; @@ -270,7 +273,8 @@ gst_clock_entry_reinit (GstClock * clock, GstClockEntry * entry, GstClockTime time, GstClockTime interval, GstClockEntryType type) { g_return_val_if_fail (entry->status != GST_CLOCK_BUSY, FALSE); - g_return_val_if_fail (entry->clock == clock, FALSE); + g_return_val_if_fail (gst_clock_id_uses_clock ((GstClockID) entry, clock), + FALSE); entry->type = type; entry->time = time; @@ -354,6 +358,8 @@ _gst_clock_id_free (GstClockID id) if (entry->destroy_data) entry->destroy_data (entry->user_data); + g_weak_ref_clear (&entry->ABI.clock); + /* FIXME: add tracer hook for struct allocations such as clock entries */ g_slice_free (GstClockEntry, id); @@ -526,7 +532,9 @@ gst_clock_id_wait (GstClockID id, GstClockTimeDiff * jitter) entry = (GstClockEntry *) id; requested = GST_CLOCK_ENTRY_TIME (entry); - clock = GST_CLOCK_ENTRY_CLOCK (entry); + clock = g_weak_ref_get (&entry->ABI.clock); + if (G_UNLIKELY (clock == NULL)) + goto invalid_entry; /* can't sync on invalid times */ if (G_UNLIKELY (!GST_CLOCK_TIME_IS_VALID (requested))) @@ -549,6 +557,7 @@ gst_clock_id_wait (GstClockID id, GstClockTimeDiff * jitter) if (entry->type == GST_CLOCK_ENTRY_PERIODIC) entry->time = requested + entry->interval; + gst_object_unref (clock); return res; /* ERRORS */ @@ -556,13 +565,20 @@ invalid_time: { GST_CAT_DEBUG_OBJECT (GST_CAT_CLOCK, clock, "invalid time requested, returning _BADTIME"); + gst_object_unref (clock); return GST_CLOCK_BADTIME; } not_supported: { GST_CAT_DEBUG_OBJECT (GST_CAT_CLOCK, clock, "clock wait is not supported"); + gst_object_unref (clock); return GST_CLOCK_UNSUPPORTED; } +invalid_entry: + { + GST_CAT_DEBUG (GST_CAT_CLOCK, "clock entry %p lost its clock", id); + return GST_CLOCK_ERROR; + } } /** @@ -600,7 +616,9 @@ gst_clock_id_wait_async (GstClockID id, entry = (GstClockEntry *) id; requested = GST_CLOCK_ENTRY_TIME (entry); - clock = GST_CLOCK_ENTRY_CLOCK (entry); + clock = g_weak_ref_get (&entry->ABI.clock); + if (G_UNLIKELY (clock == NULL)) + goto invalid_entry; /* can't sync on invalid times */ if (G_UNLIKELY (!GST_CLOCK_TIME_IS_VALID (requested))) @@ -617,6 +635,7 @@ gst_clock_id_wait_async (GstClockID id, res = cclass->wait_async (clock, entry); + gst_object_unref (clock); return res; /* ERRORS */ @@ -625,13 +644,20 @@ invalid_time: (func) (clock, GST_CLOCK_TIME_NONE, id, user_data); GST_CAT_DEBUG_OBJECT (GST_CAT_CLOCK, clock, "invalid time requested, returning _BADTIME"); + gst_object_unref (clock); return GST_CLOCK_BADTIME; } not_supported: { GST_CAT_DEBUG_OBJECT (GST_CAT_CLOCK, clock, "clock wait is not supported"); + gst_object_unref (clock); return GST_CLOCK_UNSUPPORTED; } +invalid_entry: + { + GST_CAT_DEBUG (GST_CAT_CLOCK, "clock entry %p lost its clock", id); + return GST_CLOCK_ERROR; + } } /** @@ -655,12 +681,23 @@ gst_clock_id_unschedule (GstClockID id) g_return_if_fail (id != NULL); entry = (GstClockEntry *) id; - clock = entry->clock; + clock = g_weak_ref_get (&entry->ABI.clock); + if (G_UNLIKELY (clock == NULL)) + goto invalid_entry; cclass = GST_CLOCK_GET_CLASS (clock); if (G_LIKELY (cclass->unschedule)) cclass->unschedule (clock, entry); + + gst_object_unref (clock); + return; + +invalid_entry: + { + GST_CAT_DEBUG (GST_CAT_CLOCK, "clock entry %p lost its clock", id); + return; + } } @@ -1338,6 +1375,63 @@ gst_clock_get_master (GstClock * clock) return result; } +/** + * gst_clock_id_get_clock: + * @id: a #GstClockID + * + * This function returns the underlying clock. + * + * Returns: (transfer full) (nullable): a #GstClock or %NULL when the + * underlying clock has been freed. Unref after usage. + * + * MT safe. + */ +GstClock * +gst_clock_id_get_clock (GstClockID id) +{ + GstClockEntry *entry; + + g_return_val_if_fail (id != NULL, NULL); + + entry = (GstClockEntry *) id; + return g_weak_ref_get (&entry->ABI.clock); +} + +/** + * gst_clock_id_uses_clock: + * @id: a #GstClockID to check + * @clock: a #GstClock to compare against + * + * This function returns whether @id uses @clock as the underlying clock. + * @clock can be NULL, in which case the return value indicates whether + * the underlying clock has been freed. If this is the case, the @id is + * no longer usable and should be freed. + * + * Returns: whether the clock @id uses the same underlying #GstClock @clock. + * + * MT safe. + */ +gboolean +gst_clock_id_uses_clock (GstClockID id, GstClock * clock) +{ + GstClockEntry *entry; + GstClock *entry_clock; + gboolean ret = FALSE; + + g_return_val_if_fail (id != NULL, FALSE); + + entry = (GstClockEntry *) id; + entry_clock = g_weak_ref_get (&entry->ABI.clock); + if (entry_clock == clock) + ret = TRUE; + + if (G_LIKELY (entry_clock != NULL)) + gst_object_unref (entry_clock); + + return ret; +} + + /** * gst_clock_add_observation: * @clock: a #GstClock diff --git a/gst/gstclock.h b/gst/gstclock.h index c80eeba037c..d21aedd6e09 100644 --- a/gst/gstclock.h +++ b/gst/gstclock.h @@ -339,13 +339,18 @@ typedef enum { * Cast to a clock entry */ #define GST_CLOCK_ENTRY(entry) ((GstClockEntry *)(entry)) + +#ifndef GST_DISABLE_DEPRECATED /** * GST_CLOCK_ENTRY_CLOCK: * @entry: the entry to query * * Get the owner clock of the entry + * + * Deprecated: Use gst_clock_id_get_clock() instead. */ #define GST_CLOCK_ENTRY_CLOCK(entry) ((entry)->clock) +#endif /** * GST_CLOCK_ENTRY_TYPE: * @entry: the entry to query @@ -387,7 +392,9 @@ typedef enum { struct _GstClockEntry { gint refcount; /*< protected >*/ +#ifndef GST_DISABLE_DEPRECATED GstClock *clock; +#endif GstClockEntryType type; GstClockTime time; GstClockTime interval; @@ -399,7 +406,10 @@ struct _GstClockEntry { gboolean woken_up; /*< private >*/ - gpointer _gst_reserved[GST_PADDING]; + union { + gpointer _gst_reserved[GST_PADDING]; + GWeakRef clock; + } ABI; }; #include @@ -600,6 +610,12 @@ void gst_clock_id_unref (GstClockID id); GST_API gint gst_clock_id_compare_func (gconstpointer id1, gconstpointer id2); +GST_API +GstClock * gst_clock_id_get_clock (GstClockID id); + +GST_API +gboolean gst_clock_id_uses_clock (GstClockID id, GstClock * clock); + GST_API GstClockTime gst_clock_id_get_time (GstClockID id); diff --git a/libs/gst/base/gstbasesink.c b/libs/gst/base/gstbasesink.c index 2ef99c93f3b..4368c1a34c4 100644 --- a/libs/gst/base/gstbasesink.c +++ b/libs/gst/base/gstbasesink.c @@ -2286,11 +2286,8 @@ gst_base_sink_wait_clock (GstBaseSink * sink, GstClockTime time, time += base_time; /* Re-use existing clockid if available */ - /* FIXME: Casting to GstClockEntry only works because the types - * are the same */ if (G_LIKELY (sink->priv->cached_clock_id != NULL - && GST_CLOCK_ENTRY_CLOCK ((GstClockEntry *) sink-> - priv->cached_clock_id) == clock)) { + && gst_clock_id_uses_clock (sink->priv->cached_clock_id, clock))) { if (!gst_clock_single_shot_id_reinit (clock, sink->priv->cached_clock_id, time)) { gst_clock_id_unref (sink->priv->cached_clock_id); -- GitLab From f34472822c257359d69ebf671b81d85646a40618 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Sat, 3 Nov 2018 18:29:03 +0200 Subject: [PATCH 2/4] clock: Fix deprecation handling of the GstClock clock field --- gst/gstclock.c | 4 ++++ gst/gstclock.h | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/gst/gstclock.c b/gst/gstclock.c index 007ceed3755..baa6286088e 100644 --- a/gst/gstclock.c +++ b/gst/gstclock.c @@ -249,8 +249,12 @@ gst_clock_entry_new (GstClock * clock, GstClockTime time, "created entry %p, time %" GST_TIME_FORMAT, entry, GST_TIME_ARGS (time)); entry->refcount = 1; +#ifndef GST_REMOVE_DEPRECATED #ifndef GST_DISABLE_DEPRECATED entry->clock = clock; +#else + entry->_clock = clock; +#endif #endif g_weak_ref_init (&entry->ABI.clock, clock); entry->type = type; diff --git a/gst/gstclock.h b/gst/gstclock.h index d21aedd6e09..317f44a544d 100644 --- a/gst/gstclock.h +++ b/gst/gstclock.h @@ -392,8 +392,12 @@ typedef enum { struct _GstClockEntry { gint refcount; /*< protected >*/ +#ifndef GST_REMOVE_DEPRECATED #ifndef GST_DISABLE_DEPRECATED GstClock *clock; +#else + gpointer _clock; +#endif #endif GstClockEntryType type; GstClockTime time; -- GitLab From 4de89865d479032c2e97859446bff520a0a0f212 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Sat, 3 Nov 2018 18:29:17 +0200 Subject: [PATCH 3/4] clock: Add new functions to the documentation --- docs/gst/gstreamer-sections.txt | 2 ++ gst/gstclock.c | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/gst/gstreamer-sections.txt b/docs/gst/gstreamer-sections.txt index cfcd7832eab..03f3272f561 100644 --- a/docs/gst/gstreamer-sections.txt +++ b/docs/gst/gstreamer-sections.txt @@ -667,6 +667,8 @@ gst_clock_wait_for_sync gst_clock_is_synced gst_clock_set_synced gst_clock_id_get_time +gst_clock_id_get_clock +gst_clock_id_uses_clock gst_clock_id_wait gst_clock_id_wait_async gst_clock_id_unschedule diff --git a/gst/gstclock.c b/gst/gstclock.c index baa6286088e..d82748c8338 100644 --- a/gst/gstclock.c +++ b/gst/gstclock.c @@ -1389,6 +1389,8 @@ gst_clock_get_master (GstClock * clock) * underlying clock has been freed. Unref after usage. * * MT safe. + * + * Since: 1.16 */ GstClock * gst_clock_id_get_clock (GstClockID id) @@ -1405,7 +1407,7 @@ gst_clock_id_get_clock (GstClockID id) * gst_clock_id_uses_clock: * @id: a #GstClockID to check * @clock: a #GstClock to compare against - * + * * This function returns whether @id uses @clock as the underlying clock. * @clock can be NULL, in which case the return value indicates whether * the underlying clock has been freed. If this is the case, the @id is @@ -1414,6 +1416,8 @@ gst_clock_id_get_clock (GstClockID id) * Returns: whether the clock @id uses the same underlying #GstClock @clock. * * MT safe. + * + * Since: 1.16 */ gboolean gst_clock_id_uses_clock (GstClockID id, GstClock * clock) @@ -1423,6 +1427,7 @@ gst_clock_id_uses_clock (GstClockID id, GstClock * clock) gboolean ret = FALSE; g_return_val_if_fail (id != NULL, FALSE); + g_return_val_if_fail (clock != NULL, FALSE); entry = (GstClockEntry *) id; entry_clock = g_weak_ref_get (&entry->ABI.clock); -- GitLab From 8250b8d81a2c877c87167a4421fcf75d591c5b57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Sat, 3 Nov 2018 18:44:48 +0200 Subject: [PATCH 4/4] clock: Move clock weak ref into its own ABI struct Otherwise it will be hard to add other things into the padding later without breaking API. --- gst/gstclock.c | 14 +++++++------- gst/gstclock.h | 4 +++- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/gst/gstclock.c b/gst/gstclock.c index d82748c8338..dc4af8cad79 100644 --- a/gst/gstclock.c +++ b/gst/gstclock.c @@ -256,7 +256,7 @@ gst_clock_entry_new (GstClock * clock, GstClockTime time, entry->_clock = clock; #endif #endif - g_weak_ref_init (&entry->ABI.clock, clock); + g_weak_ref_init (&entry->ABI.abi.clock, clock); entry->type = type; entry->time = time; entry->interval = interval; @@ -362,7 +362,7 @@ _gst_clock_id_free (GstClockID id) if (entry->destroy_data) entry->destroy_data (entry->user_data); - g_weak_ref_clear (&entry->ABI.clock); + g_weak_ref_clear (&entry->ABI.abi.clock); /* FIXME: add tracer hook for struct allocations such as clock entries */ @@ -536,7 +536,7 @@ gst_clock_id_wait (GstClockID id, GstClockTimeDiff * jitter) entry = (GstClockEntry *) id; requested = GST_CLOCK_ENTRY_TIME (entry); - clock = g_weak_ref_get (&entry->ABI.clock); + clock = g_weak_ref_get (&entry->ABI.abi.clock); if (G_UNLIKELY (clock == NULL)) goto invalid_entry; @@ -620,7 +620,7 @@ gst_clock_id_wait_async (GstClockID id, entry = (GstClockEntry *) id; requested = GST_CLOCK_ENTRY_TIME (entry); - clock = g_weak_ref_get (&entry->ABI.clock); + clock = g_weak_ref_get (&entry->ABI.abi.clock); if (G_UNLIKELY (clock == NULL)) goto invalid_entry; @@ -685,7 +685,7 @@ gst_clock_id_unschedule (GstClockID id) g_return_if_fail (id != NULL); entry = (GstClockEntry *) id; - clock = g_weak_ref_get (&entry->ABI.clock); + clock = g_weak_ref_get (&entry->ABI.abi.clock); if (G_UNLIKELY (clock == NULL)) goto invalid_entry; @@ -1400,7 +1400,7 @@ gst_clock_id_get_clock (GstClockID id) g_return_val_if_fail (id != NULL, NULL); entry = (GstClockEntry *) id; - return g_weak_ref_get (&entry->ABI.clock); + return g_weak_ref_get (&entry->ABI.abi.clock); } /** @@ -1430,7 +1430,7 @@ gst_clock_id_uses_clock (GstClockID id, GstClock * clock) g_return_val_if_fail (clock != NULL, FALSE); entry = (GstClockEntry *) id; - entry_clock = g_weak_ref_get (&entry->ABI.clock); + entry_clock = g_weak_ref_get (&entry->ABI.abi.clock); if (entry_clock == clock) ret = TRUE; diff --git a/gst/gstclock.h b/gst/gstclock.h index 317f44a544d..3d683fdefce 100644 --- a/gst/gstclock.h +++ b/gst/gstclock.h @@ -412,7 +412,9 @@ struct _GstClockEntry { /*< private >*/ union { gpointer _gst_reserved[GST_PADDING]; - GWeakRef clock; + struct { + GWeakRef clock; + } abi; } ABI; }; -- GitLab