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;

1 files changed, 23 insertions, 11 deletions

diff --git a/libpurple/upnp.c b/libpurple/upnp.c
index de678cdba6..25dbd828b2 100644
--- a/libpurple/upnp.c
+++ b/libpurple/upnp.c
@@ -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 purple_upnp_discover_send_broadcast(UPnPDiscoveryData *dd);
 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(PurpleUtilFetchUrlData *url_data, gpointer user_data,
 	} 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_mapping, gpointer data)
 			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_mapping, gpointer data)
 		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;
 }