Re: problem with gupnp_service_proxy_remove_notify()

[Jens Georg <mail jensge org>]

On Fr, 2012-12-14 at 11:33 +0100, Sven Neumann wrote:

Thank you for your great analysis. I've also tried to come up with a
sensible solution for this, but it likely ends up being very complex and
with side-effects like receiving events after un-subscribing from within
the handler.

I would say the easiest and probably still most sane thing is to
document this behaviour clearly for now and have an enhancement bug so
we don't forget about it.

> Hi,
> 
> we've identified a problem with the GUPnPServiceProxy API that I would
> like to bring to your attention. The current implementation of
> notifications in GUPnPServiceProxy does not deal nicely with code that
> calls gupnp_service_proxy_remove_notify() from a notification callback.
> Actually it will most certainly crash if you attempt to do this.
> 
> What happens is that we are inside this emit_notification() looping over
> registered callbacks that want to get notified about a change of a state
> variable:
> 
>         NotifyData *data;
>         GList *l;
> 
>         data = g_hash_table_lookup (proxy->priv->notify_hash,
>                                     var_node->name);
>         if (data == NULL)
>                 return;
> 
> 	/* some not relevant code removed here... */
> 
>         /* Call callbacks */
>         for (l = data->callbacks; l; l = l->next) {
>                 CallbackData *callback_data;
> 
>                 callback_data = l->data;
> 
>                 callback_data->callback (proxy,
>                                          (const char *) var_node->name,
>                                          &value,
>                                          callback_data->user_data);
>         }
> 
> Now if the callback uses gupnp_service_proxy_add_notify() or
> gupnp_service_proxy_remove_notify() it will change the list of callbacks
> in NotifyData while we are iterating it. Probably the most common case
> (and the one that we hit in our code) is that the callback removes
> itself from the service-proxy. In that case the GList struct that we are
> holding a pointer to will be released. This is likely going to cause a
> crash when the for-loop accesses l->next.
> 
> We've tried to come up with a patch to solve this. One could for example
> unroll the list of callbacks on the stack using a recursive function.
> That would make the code safe against manipulation of the list by the
> callbacks. But it would still not be totally safe as a callback might
> remove other callbacks and these would then still be called, which is
> probably not what the user of GUPnPServiceProxy expects.
> 
> So our conclusion is that a proper solution for this problem would be
> rather complex and difficult to test and maintain. Perhaps a better
> solution is to document the fact that gupnp_service_proxy_add_notify()
> and gupnp_service_proxy_remove_notify() must not be called from a notify
> callback.
> 
> What do you think?
> 
> 
> Regards,
> Sven
> 
> 
> 
> 
> 
> _______________________________________________
> gupnp-list mailing list
> gupnp-list gnome org
> https://mail.gnome.org/mailman/listinfo/gupnp-list
>