changeset 31518:b39b6d0008c5

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;

--- a/libpurple/upnp.c	Tue Dec 28 01:21:35 2010 +0000
+++ b/libpurple/upnp.c	Tue Dec 28 05:37:20 2010 +0000
@@ -142,6 +142,7 @@
 	gboolean add;
 	PurpleUPnPCallback cb;
 	gpointer cb_data;
+	gboolean success;
 	guint tima; /* purple_timeout_add handle */
 	PurpleUtilFetchUrlData *gfud;
 };
@@ -156,6 +157,19 @@
 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 @@
 	} 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 @@
 			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 @@
 		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;
 }