problem with gupnp_service_proxy_remove_notify()

[Sven Neumann <s neumann raumfeld com>]

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