pidgin: 8febed94: upnp: Asynch-ronize the callbacks from U...

[darkrain42 at pidgin.im]

----------------------------------------------------------------------
Revision: 8febed9408d870efdef757d67f9a3631e1d6d494
Parent:   9ad88595bfd0e4d54108c600452594860b294c8f
Author:   darkrain42 at pidgin.im
Date:     12/28/10 00:37:20
Branch:   im.pidgin.pidgin
URL: http://d.pidgin.im/viewmtn/revision/info/8febed9408d870efdef757d67f9a3631e1d6d494

Changelog: 

upnp: Asynch-ronize the callbacks from UPnP to calling code.  Refs #12387

I have no idea if this will resolve the crashes, but with the help of the
packet capture, I /think/ these are correct.

Short summary: it's possible for the callback to fire (and ar be freed) before
the top-level function (purple_upnp_cancel_port_mapping) returns, even though
cancel_port_mapping returns the now-invalid ar (which may lead to a subsequent
use-after-free).

At least one call path through the code that I think leads to this (backed
up by one of the debug logs I looked at):
purple_upnp_cancel_port_mapping(...)
	do_port_mapping_cb (has_control_mapping == TRUE, ar->add == FALSE)
		purple_upnp_generate_action_message_and_send(..., done_port_mapping_cb, ar)
			/* We fail to parse the URL (see some debug logs) */
			done_port_mapping_cb
				ar->cb(FALSE, cbdata)
				return;
			return;
		return;
	return ar;

...and something which calls:
do_port_mapping_cb(has_control_mapping == TRUE, ar->add == TRUE)
	ar->cb(FALSE, cbdata)
	g_free(ar)
	return;

Changes against parent 9ad88595bfd0e4d54108c600452594860b294c8f

  patched  libpurple/upnp.c

-------------- next part --------------
============================================================
--- libpurple/upnp.c	43b1d3b5c5adb4634b582237bff891a36c1b1eb8
+++ libpurple/upnp.c	b38a01b6e579af290118b3791968b63a253edfd5
@@ -142,6 +142,7 @@ struct _UPnPMappingAddRemove
 	gboolean add;
 	PurpleUPnPCallback cb;
 	gpointer cb_data;
+	gboolean success;
 	guint tima; /* purple_timeout_add handle */
 	PurpleUtilFetchUrlData *gfud;
 };
@@ -156,6 +157,19 @@ static void lookup_internal_ip(void);
 static void lookup_public_ip(void);
 static void lookup_internal_ip(void);
 
+static gboolean
+fire_ar_cb_async_and_free(gpointer data)
+{
+	UPnPMappingAddRemove *ar = data;
+	if (ar) {
+		if (ar->cb)
+			ar->cb(ar->success, ar->cb_data);
+		g_free(ar);
+	}
+
+	return FALSE;
+}
+
 static void
 fire_discovery_callbacks(gboolean success)
 {
@@ -863,9 +877,8 @@ done_port_mapping_cb(PurpleUtilFetchUrlD
 	} else
 		purple_debug_info("upnp", "Successfully completed port mapping operation\n");
 
-	if (ar->cb)
-		ar->cb(success, ar->cb_data);
-	g_free(ar);
+	ar->success = success;
+	ar->tima = purple_timeout_add(0, fire_ar_cb_async_and_free, ar);
 }
 
 static void
@@ -882,10 +895,8 @@ do_port_mapping_cb(gboolean has_control_
 			if(!(internal_ip = purple_upnp_get_internal_ip())) {
 				purple_debug_error("upnp",
 					"purple_upnp_set_port_mapping(): couldn't get local ip\n");
-				/* UGLY */
-				if (ar->cb)
-					ar->cb(FALSE, ar->cb_data);
-				g_free(ar);
+				ar->success = FALSE;
+				ar->tima = purple_timeout_add(0, fire_ar_cb_async_and_free, ar);
 				return;
 			}
 			strncpy(action_name, "AddPortMapping",
@@ -908,15 +919,16 @@ do_port_mapping_cb(gboolean has_control_
 		return;
 	}
 
-
-	if (ar->cb)
-		ar->cb(FALSE, ar->cb_data);
-	g_free(ar);
+	ar->success = FALSE;
+	ar->tima = purple_timeout_add(0, fire_ar_cb_async_and_free, ar);
 }
 
 static gboolean
 fire_port_mapping_failure_cb(gpointer data)
 {
+	UPnPMappingAddRemove *ar = data;
+
+	ar->tima = 0;
 	do_port_mapping_cb(FALSE, data);
 	return FALSE;
 }