/pidgin/main: 79fe6b95f105: Fix a race condition in appsrc read/...

[Michael McConville]

How did you notice this race condition? I was looking for what was
likely the same bug. Every so often, Pidgin 3.0 would lock, printing
this in a tight infinite loop until I SIGKILL'd it:

> (Pidgin:1238): GStreamer-CRITICAL **: gst_poll_write_control: assertion 'set != NULL' failed
> (18:31:28) GStreamer: gstsystemclock: write control failed in wakeup_async, trying again: 35:Resource temporarily unavailable

I was on OpenBSD with GStreamer 1.4.5. The GStreamer people suggested
that it was a GStreamer bug, maybe this one:

	https://bugzilla.gnome.org/show_bug.cgi?id=750397

Jakub Adam wrote:
> Changeset: 79fe6b95f1057b65505559ffdc66136e2145cf25
> Author:	 Jakub Adam <jakub.adam at ktknet.cz>
> Date:	 2015-08-21 10:35 +0200
> Branch:	 release-2.x.y
> URL: https://hg.pidgin.im/pidgin/main/rev/79fe6b95f105
> 
> Description:
> 
> Fix a race condition in appsrc read/write callbacks
> 
> We can't use writable_timer_id as a token because the timeout is added
> into libpurple's main event loop, which runs in a different thread than
> from where call_appsrc_writable_locked() was called. Consequently, the
> callback may run even before purple_timeout_add() returns the timer ID
> to us.
> 
> The same applies to readable_timer_id in call_appsink_readable_locked().
> 
> diffstat:
> 
>  libpurple/mediamanager.c |  55 ++++++++++++++++++++++++++++++++---------------
>  1 files changed, 37 insertions(+), 18 deletions(-)
> 
> diffs (155 lines):
> 
> diff --git a/libpurple/mediamanager.c b/libpurple/mediamanager.c
> --- a/libpurple/mediamanager.c
> +++ b/libpurple/mediamanager.c
> @@ -105,6 +105,7 @@ struct _PurpleMediaManagerPrivate
>  	/* Application data streams */
>  	GList *appdata_info; /* holds PurpleMediaAppDataInfo */
>  	GMutex appdata_mutex;
> +	guint appdata_cb_token; /* last used read/write callback token */
>  #endif
>  };
>  
> @@ -124,6 +125,8 @@ typedef struct {
>  	guint sample_offset;
>  	gboolean writable;
>  	gboolean connected;
> +	guint writable_cb_token;
> +	guint readable_cb_token;
>  	guint writable_timer_id;
>  	guint readable_timer_id;
>  	GCond readable_cond;
> @@ -562,6 +565,11 @@ free_appdata_info_locked (PurpleMediaApp
>  	g_free (info->session_id);
>  	g_free (info->participant);
>  
> +	/* This lets the potential read or write callbacks waiting for appdata_mutex
> +	 * know the info structure has been destroyed. */
> +	info->readable_cb_token = 0;
> +	info->writable_cb_token = 0;
> +
>  	if (info->readable_timer_id) {
>  		purple_timeout_remove (info->readable_timer_id);
>  		info->readable_timer_id = 0;
> @@ -750,19 +758,19 @@ appsrc_writable (gpointer user_data)
>  	gchar *participant;
>  	gboolean writable;
>  	gpointer cb_data;
> -	guint *timer_id_ptr = &info->writable_timer_id;
> -	guint timer_id = *timer_id_ptr;
> +	guint *cb_token_ptr = &info->writable_cb_token;
> +	guint cb_token = *cb_token_ptr;
>  
>  
>  	g_mutex_lock (&manager->priv->appdata_mutex);
> -	if (timer_id == 0 || timer_id != *timer_id_ptr) {
> +	if (cb_token == 0 || cb_token != *cb_token_ptr) {
>  		/* In case info was freed while we were waiting for the mutex to unlock
> -		 * we still have a pointer to the timer_id which should still be
> +		 * we still have a pointer to the cb_token which should still be
>  		 * accessible since it's in the Glib slice allocator. It gets set to 0
>  		 * just after the timeout is canceled which happens also before the
>  		 * AppDataInfo is freed, so even if that memory slice gets reused, the
> -		 * timer_id would be different from its previous value (unless
> -		 * extremely unlucky). So checking if the value for the timer_id changed
> +		 * cb_token would be different from its previous value (unless
> +		 * extremely unlucky). So checking if the value for the cb_token changed
>  		 * should be enough to prevent any kind of race condition in which the
>  		 * media/AppDataInfo gets destroyed in one thread while the timeout was
>  		 * triggered and is waiting on the mutex to get unlocked in this thread
> @@ -777,7 +785,7 @@ appsrc_writable (gpointer user_data)
>  	writable = info->writable && info->connected;
>  	cb_data = info->user_data;
>  
> -    info->writable_timer_id = 0;
> +	info->writable_cb_token = 0;
>  	g_mutex_unlock (&manager->priv->appdata_mutex);
>  
>  
> @@ -805,10 +813,18 @@ appsrc_writable (gpointer user_data)
>  static void
>  call_appsrc_writable_locked (PurpleMediaAppDataInfo *info)
>  {
> +	PurpleMediaManager *manager = purple_media_manager_get ();
> +
>  	/* We already have a writable callback scheduled, don't create another one */
> -	if (info->writable_timer_id || info->callbacks.writable == NULL)
> +	if (info->writable_cb_token || info->callbacks.writable == NULL)
>  		return;
>  
> +	/* We can't use writable_timer_id as a token, because the timeout is added
> +	 * into libpurple's main event loop, which runs in a different thread than
> +	 * from where call_appsrc_writable_locked() was called. Consequently, the
> +	 * callback may run even before purple_timeout_add() returns the timer ID
> +	 * to us. */
> +	info->writable_cb_token = ++manager->priv->appdata_cb_token;
>  	info->writable_timer_id = purple_timeout_add (0, appsrc_writable, info);
>  }
>  
> @@ -930,11 +946,11 @@ appsink_readable (gpointer user_data)
>  	gchar *session_id;
>  	gchar *participant;
>  	gpointer cb_data;
> -	guint *timer_id_ptr = &info->readable_timer_id;
> -	guint timer_id = *timer_id_ptr;
> +	guint *cb_token_ptr = &info->readable_cb_token;
> +	guint cb_token = *cb_token_ptr;
>  
>  	g_mutex_lock (&manager->priv->appdata_mutex);
> -	if (timer_id == 0 || timer_id != *timer_id_ptr) {
> +	if (cb_token == 0 || cb_token != *cb_token_ptr) {
>  		/* Avoided a race condition (see writable callback) */
>  		g_mutex_unlock (&manager->priv->appdata_mutex);
>  		return FALSE;
> @@ -956,13 +972,13 @@ appsink_readable (gpointer user_data)
>  		g_object_unref (media);
>  		g_free (session_id);
>  		g_free (participant);
> -		if (timer_id == 0 || timer_id != *timer_id_ptr) {
> +		if (cb_token == 0 || cb_token != *cb_token_ptr) {
>  			/* We got cancelled */
>  			g_mutex_unlock (&manager->priv->appdata_mutex);
>  			return FALSE;
>  		}
>  	}
> -    info->readable_timer_id = 0;
> +	info->readable_cb_token = 0;
>  	g_mutex_unlock (&manager->priv->appdata_mutex);
>  	return FALSE;
>  }
> @@ -970,13 +986,16 @@ appsink_readable (gpointer user_data)
>  static void
>  call_appsink_readable_locked (PurpleMediaAppDataInfo *info)
>  {
> +	PurpleMediaManager *manager = purple_media_manager_get ();
> +
>  	/* We must signal that a new sample has arrived to release blocking reads */
>  	g_cond_broadcast (&info->readable_cond);
>  
>  	/* We already have a writable callback scheduled, don't create another one */
> -	if (info->readable_timer_id || info->callbacks.readable == NULL)
> +	if (info->readable_cb_token || info->callbacks.readable == NULL)
>  		return;
>  
> +	info->readable_cb_token = ++manager->priv->appdata_cb_token;
>  	info->readable_timer_id = purple_timeout_add (0, appsink_readable, info);
>  }
>  
> @@ -1663,14 +1682,14 @@ purple_media_manager_set_application_dat
>  	if (info->notify)
>  		info->notify (info->user_data);
>  
> -	if (info->readable_timer_id) {
> +	if (info->readable_cb_token) {
>  		purple_timeout_remove (info->readable_timer_id);
> -		info->readable_timer_id = 0;
> +		info->readable_cb_token = 0;
>  	}
>  
> -	if (info->writable_timer_id) {
> +	if (info->writable_cb_token) {
>  		purple_timeout_remove (info->writable_timer_id);
> -		info->writable_timer_id = 0;
> +		info->writable_cb_token = 0;
>  	}
>  
>  	if (callbacks) {
> 
> _______________________________________________
> Commits mailing list
> Commits at pidgin.im
> https://pidgin.im/cgi-bin/mailman/listinfo/commits