Re: [PATCH] Fix removal of variable change notifications

[Jens Georg <mail jensge org>]

On Fr, 2012-12-07 at 14:43 +0100, Sven Neumann wrote:
> On Fri, 2012-12-07 at 12:38 +0100, Jens Georg wrote:
> > On Fr, 2012-12-07 at 11:59 +0100, Sven Neumann wrote:
> > > Need to pass the key to g_hash_table_remove(), not the
> > > value. This fixes crashes in emit_notification().
> > 
> > Is there any way to trigger this so we can add a test?
> 
> Well, the code is obviously wrong.

Agree. Also I wanted a test for "Fix a bug, write a test for it".

> But if you absolutely want to test
> this, you should be able to reproduce the crash by instantiating a
> service-proxy object, using gupnp_service_proxy_add_notify() on it, then
> use gupnp_service_proxy_remove_notify() to unregister yourself from
> change notifications. After that change the state variable on the
> service. You will most likely see a crash in emit_notification() because
> it is accessing released memory. If it doesn't crash, then you should
> use valgrind. It will definitely show you an invalid read.

No, it doesn't, just tried it. And if you look at the life-time of the
values in the hash-table, nothing is freeing the data apart from
destroying the hashtable, a second call to _insert or properly removing
the entry. The latter doesn't happen, I can't see how the second one
will happen since the code is checking for existence before calling it
and the destroy happens in finalize.