dispatcher: robustify canceling dispatcher calls

Thomas pointed out that using the address of the DispatcherInfo
structure as the dispatcher call ID could cause a mis-cancelation
if malloc re-used the same block in the future.  While the code
should be correctly clearing call IDs after the callback runs
or is canceled, just use numeric IDs to avoid potential crashses.

2 files changed, 51 insertions, 24 deletions

diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c
index 7eef2cb90e..87a89cab28 100644
--- a/src/nm-dispatcher.c
+++ b/src/nm-dispatcher.c
@@ -32,7 +32,7 @@
 #include "nm-glib-compat.h"
 
 static gboolean do_dispatch = TRUE;
-static GSList *requests = NULL;
+static GHashTable *requests = NULL;
 
 static void
 dump_object_to_props (GObject *object, GHashTable *hash)
@@ -133,6 +133,7 @@ fill_vpn_props (NMIP4Config *ip4_config,
 }
 
 typedef struct {
+	guint request_id;
 	DispatcherFunc callback;
 	gpointer user_data;
 	guint idle_id;
@@ -141,12 +142,28 @@ typedef struct {
 static void
 dispatcher_info_free (DispatchInfo *info)
 {
-	requests = g_slist_remove (requests, info);
 	if (info->idle_id)
 		g_source_remove (info->idle_id);
 	g_free (info);
 }
 
+static void
+_ensure_requests (void)
+{
+	if (G_UNLIKELY (requests == NULL)) {
+		requests = g_hash_table_new_full (g_direct_hash,
+		                                  g_direct_equal,
+		                                  NULL,
+		                                  (GDestroyNotify) dispatcher_info_free);
+	}
+}
+
+static void
+dispatcher_info_cleanup (DispatchInfo *info)
+{
+	g_hash_table_remove (requests, GUINT_TO_POINTER (info->request_id));
+}
+
 static const char *
 dispatch_result_to_string (DispatchResult result)
 {
@@ -247,7 +264,7 @@ dispatcher_done_cb (DBusGProxy *proxy, DBusGProxyCall *call, gpointer user_data)
 	}
 
 	if (info->callback)
-		info->callback (info, info->user_data);
+		info->callback (info->request_id, info->user_data);
 
 	g_clear_error (&error);
 	g_object_unref (proxy);
@@ -288,8 +305,8 @@ dispatcher_idle_cb (gpointer user_data)
 
 	info->idle_id = 0;
 	if (info->callback)
-		info->callback (info, info->user_data);
-	dispatcher_info_free (info);
+		info->callback (info->request_id, info->user_data);
+	dispatcher_info_cleanup (info);
 	return G_SOURCE_REMOVE;
 }
 
@@ -303,7 +320,7 @@ _dispatcher_call (DispatcherAction action,
                   NMIP6Config *vpn_ip6_config,
                   DispatcherFunc callback,
                   gpointer user_data,
-                  gconstpointer *out_call_id)
+                  guint *out_call_id)
 {
 	DBusGProxy *proxy;
 	DBusGConnection *g_connection;
@@ -319,9 +336,12 @@ _dispatcher_call (DispatcherAction action,
 	DispatchInfo *info = NULL;
 	gboolean success = FALSE;
 	GError *error = NULL;
+	static guint request_counter = 1;
 
 	g_assert (!blocking || (!callback && !user_data));
 
+	_ensure_requests ();
+
 	/* All actions except 'hostname' require a device */
 	if (action == DISPATCHER_ACTION_HOSTNAME) {
 		nm_log_dbg (LOGD_DISPATCH, "dispatching action '%s'",
@@ -340,6 +360,7 @@ _dispatcher_call (DispatcherAction action,
 	if (do_dispatch == FALSE) {
 		if (blocking == FALSE && (out_call_id || callback)) {
 			info = g_malloc0 (sizeof (*info));
+			info->request_id = request_counter++;
 			info->callback = callback;
 			info->user_data = user_data;
 			info->idle_id = g_idle_add (dispatcher_idle_cb, info);
@@ -422,12 +443,13 @@ _dispatcher_call (DispatcherAction action,
 		}
 	} else {
 		info = g_malloc0 (sizeof (*info));
+		info->request_id = request_counter++;
 		info->callback = callback;
 		info->user_data = user_data;
 		dbus_g_proxy_begin_call_with_timeout (proxy, "Action",
 		                                      dispatcher_done_cb,
 		                                      info,
-		                                      (GDestroyNotify) dispatcher_info_free,
+		                                      (GDestroyNotify) dispatcher_info_cleanup,
 		                                      30000,
 		                                      G_TYPE_STRING, action_to_string (action),
 		                                      DBUS_TYPE_G_MAP_OF_MAP_OF_VARIANT, connection_hash,
@@ -458,10 +480,11 @@ _dispatcher_call (DispatcherAction action,
 done:
 	if (success && info) {
 		/* Track the request in case of cancelation */
-		requests = g_slist_append (requests, info);
+		g_hash_table_insert (requests, GUINT_TO_POINTER (info->request_id), info);
 		if (out_call_id)
-			*out_call_id = (gconstpointer) info;
-	}
+			*out_call_id = info->request_id;
+	} else if (out_call_id)
+		*out_call_id = 0;
 
 	return success;
 }
@@ -487,7 +510,7 @@ nm_dispatcher_call (DispatcherAction action,
                     NMDevice *device,
                     DispatcherFunc callback,
                     gpointer user_data,
-                    gconstpointer *out_call_id)
+                    guint *out_call_id)
 {
 	return _dispatcher_call (action, FALSE, connection, device, NULL, NULL,
 	                         NULL, callback, user_data, out_call_id);
@@ -540,7 +563,7 @@ nm_dispatcher_call_vpn (DispatcherAction action,
                         NMIP6Config *vpn_ip6_config,
                         DispatcherFunc callback,
                         gpointer user_data,
-                        gconstpointer *out_call_id)
+                        guint *out_call_id)
 {
 	return _dispatcher_call (action, FALSE, connection, parent_device, vpn_iface,
 	                         vpn_ip4_config, vpn_ip6_config, callback, user_data, out_call_id);
@@ -573,16 +596,20 @@ nm_dispatcher_call_vpn_sync (DispatcherAction action,
 }
 
 void
-nm_dispatcher_call_cancel (gconstpointer call_id)
+nm_dispatcher_call_cancel (guint call_id)
 {
-	/* 'call_id' is really a DispatchInfo pointer, just opaque to callers.
-	 * Look it up in our requests list, but don't access it directly before
-	 * we've made sure it's a valid request,since it may have long since been
-	 * freed.  Canceling just means the callback doesn't get called, so set
-	 * the DispatcherInfo's callback to NULL.
+	DispatchInfo *info;
+
+	_ensure_requests ();
+
+	/* Canceling just means the callback doesn't get called, so set the
+	 * DispatcherInfo's callback to NULL.
 	 */
-	if (g_slist_find (requests, call_id))
-		((DispatchInfo *) call_id)->callback = NULL;
+	info = g_hash_table_lookup (requests, GUINT_TO_POINTER (call_id));
+	if (info)
+		info->callback = NULL;
+	else
+		g_return_if_reached ();
 }
 
 static void
diff --git a/src/nm-dispatcher.h b/src/nm-dispatcher.h
index cf3e62613f..7b50954fea 100644
--- a/src/nm-dispatcher.h
+++ b/src/nm-dispatcher.h
@@ -42,14 +42,14 @@ typedef enum {
 	DISPATCHER_ACTION_DHCP6_CHANGE
 } DispatcherAction;
 
-typedef void (*DispatcherFunc) (gconstpointer call_id, gpointer user_data);
+typedef void (*DispatcherFunc) (guint call_id, gpointer user_data);
 
 gboolean nm_dispatcher_call (DispatcherAction action,
                              NMConnection *connection,
                              NMDevice *device,
                              DispatcherFunc callback,
                              gpointer user_data,
-                             gconstpointer *out_call_id);
+                             guint *out_call_id);
 
 gboolean nm_dispatcher_call_sync (DispatcherAction action,
                                   NMConnection *connection,
@@ -63,7 +63,7 @@ gboolean nm_dispatcher_call_vpn (DispatcherAction action,
                                  NMIP6Config *vpn_ip6_config,
                                  DispatcherFunc callback,
                                  gpointer user_data,
-                                 gconstpointer *out_call_id);
+                                 guint *out_call_id);
 
 gboolean nm_dispatcher_call_vpn_sync (DispatcherAction action,
                                       NMConnection *connection,
@@ -72,7 +72,7 @@ gboolean nm_dispatcher_call_vpn_sync (DispatcherAction action,
                                       NMIP4Config *vpn_ip4_config,
                                       NMIP6Config *vpn_ip6_config);
 
-void nm_dispatcher_call_cancel (gconstpointer call_id);
+void nm_dispatcher_call_cancel (guint call_id);
 
 void nm_dispatcher_init (void);