Re: [gupnp] [PATCH] Fix potential crash in resource_expire()

[Sven Neumann <s neumann raumfeld com>]

Hi,

On Thu, 2010-12-16 at 14:56 +0200, Zeeshan Ali (Khattak) wrote:

> > On Mon, Nov 15, 2010 at 10:58 AM, Sven Neumann <s neumann raumfeld com> wrote:
> >> GSSDPResourceBrowser emits "resource-unavailable" before it has removed
> >> the resource from its cache. Now if the application changes the cache
> >> in response to this signal emission the code will crash.
> >>
> >> Fix this potential crash by emitting the signal after the resource
> >> has been removed from the cache.
> 
>   gupnp-universal-cp had started to randomly crash recently. The crash
> was happening just after a 'signal xxx invalid for instance xxx'
> warning from gobject. Running it as
> `G_DEBUG='fatal-warnings,fatal-criticals' gdb gupnp-universal-cp` I
> narrowed down the issue to the part which you changed with this patch
> recently:
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> g_logv (log_domain=<value optimized out>,
> log_level=G_LOG_LEVEL_WARNING, format=0xaf3500 "%s: signal id `%u' is
> invalid for instance `%p'",
>     args1=0xbfffeaec "\226\063\257") at gmessages.c:563
> 563		  g_private_set (g_log_depth, GUINT_TO_POINTER (depth));
> (gdb) bt
> #0  g_logv (log_domain=<value optimized out>,
> log_level=G_LOG_LEVEL_WARNING, format=0xaf3500 "%s: signal id `%u' is
> invalid for instance `%p'",
>     args1=0xbfffeaec "\226\063\257") at gmessages.c:563
> #1  0x00b55b42 in g_log (log_domain=0xaefaa4 "GLib-GObject",
> log_level=G_LOG_LEVEL_WARNING,
>     format=0xaf3500 "%s: signal id `%u' is invalid for instance `%p'")
> at gmessages.c:577
> #2  0x00add90d in g_signal_emit_valist (instance=0x828eed0,
> signal_id=208, detail=0, var_args=0xbfffec4c "`\006*\b\364\337\277")
> at gsignal.c:2931
> #3  0x00ade552 in g_signal_emit (instance=0x828eed0, signal_id=208,
> detail=0) at gsignal.c:3040
> #4  0x0083dd8b in resource_expire (user_data=0x824fd00) at
> gssdp-resource-browser.c:596
> #5  0x00b4b19c in g_timeout_dispatch (source=0x82bb1b8, callback=0x1,
> user_data=0x824fd00) at gmain.c:3688
> #6  0x00b4a9b5 in g_main_dispatch (context=0x8083e08) at gmain.c:2267
> #7  g_main_context_dispatch (context=0x8083e08) at gmain.c:2824
> #8  0x00b4ebc8 in g_main_context_iterate (context=0x8083e08,
> block=<value optimized out>, dispatch=1, self=0x8057048) at
> gmain.c:2902
> #9  0x00b4f107 in g_main_loop_run (loop=0xb4800c60) at gmain.c:3110
> #10 0x00277169 in IA__gtk_main () at gtkmain.c:1238
> #11 0x0804d29f in main (argc=1, argv=0xbfffef24) at main.c:119
> 
>   I reset the HEAD to the parent commit of this and so far I have
> failed to reproduce the issue. Are you sure your patch fixes a crash
> rather than the opposite? :)

Well, yes, it definitely fixed a crash for us. But I have now looked at
the code again and it looks like while it fixed a possible crash it also
introduced another. Sorry for that. I am attaching a patch that should
fix the problem. Could you please give it a test?


Sven


>From 7691277f3eed7b08dd332fa2853d88bfce054e14 Mon Sep 17 00:00:00 2001
From: Sven Neumann <s neumann raumfeld com>
Date: Thu, 16 Dec 2010 14:41:21 +0100
Subject: [PATCH] Fix issue introduced with commit 20dce239.

The Reource struct is not any longer valid at the point we emit the
signal, so better not access it there.
---
 libgssdp/gssdp-resource-browser.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/libgssdp/gssdp-resource-browser.c b/libgssdp/gssdp-resource-browser.c
index 7d98c1e..10f7301 100644
--- a/libgssdp/gssdp-resource-browser.c
+++ b/libgssdp/gssdp-resource-browser.c
@@ -580,10 +580,12 @@ gssdp_resource_browser_get_active (GSSDPResourceBrowser *resource_browser)
 static gboolean
 resource_expire (gpointer user_data)
 {
+        GSSDPResourceBrowser *resource_browser;
         Resource *resource;
         char *usn;
 
         resource = user_data;
+        resource_browser = resource->resource_browser;
 
         /* Steal the USN pointer from the resource as we need it for the signal
          * emission.
@@ -593,7 +595,7 @@ resource_expire (gpointer user_data)
 
         g_hash_table_remove (resource->resource_browser->priv->resources, usn);
 
-        g_signal_emit (resource->resource_browser,
+        g_signal_emit (resource_browser,
                        signals[RESOURCE_UNAVAILABLE],
                        0,
                        usn);
-- 
1.7.1