[PATCH 1/4] gdbus: Fix crash when getting disconnected from the bus

[PATCH 1/4] gdbus: Fix crash when getting disconnected from the bus

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

From: Johan Hedberg <johan.hedberg@intel.com>

When getting disconnected from the bus sometimes (maybe always?)
dbus_watch_handle() can cause the "info" context to be free'd meaning
that we should not try to access it after the call. The only member we
need access to is the connection pointer and as the code already has a
ref() call for it it's only natural to solve the issue by adding a local
variable not dependent on "info".

The backtrace of the crash fixed looks as follows:

 Invalid read of size 8
   at 0x121085: watch_func (mainloop.c:105)
   by 0x4C72694: g_main_context_dispatch (gmain.c:2539)
   by 0x4C729C7: g_main_context_iterate.isra.23 (gmain.c:3146)
   by 0x4C72DC1: g_main_loop_run (gmain.c:3340)
   by 0x120541: main (main.c:551)
 Address 0x5bbcd90 is 16 bytes inside a block of size 24 free'd
   at 0x4A079AE: free (vg_replace_malloc.c:427)
   by 0x4C7837E: g_free (gmem.c:252)
   by 0x4F708BF: dbus_watch_set_data (dbus-watch.c:614)
   by 0x4F70938: _dbus_watch_unref (dbus-watch.c:132)
   by 0x4F6E9A7: _dbus_transport_handle_watch (dbus-transport.c:884)
   by 0x4F59AFB: _dbus_connection_handle_watch (dbus-connection.c:1497)
   by 0x4F70AF9: dbus_watch_handle (dbus-watch.c:683)
   by 0x121084: watch_func (mainloop.c:103)
   by 0x4C72694: g_main_context_dispatch (gmain.c:2539)
   by 0x4C729C7: g_main_context_iterate.isra.23 (gmain.c:3146)
   by 0x4C72DC1: g_main_loop_run (gmain.c:3340)
   by 0x120541: main (main.c:551)
---
 gdbus/mainloop.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdbus/mainloop.c b/gdbus/mainloop.c
index cff326f..099b67f 100644
--- a/gdbus/mainloop.c
+++ b/gdbus/mainloop.c
@@ -92,8 +92,9 @@ static gboolean watch_func(GIOChannel *chan, GIOCondition cond, gpointer data)
 	struct watch_info *info = data;
 	unsigned int flags = 0;
 	DBusDispatchStatus status;
+	DBusConnection *conn;
 
-	dbus_connection_ref(info->conn);
+	conn = dbus_connection_ref(info->conn);
 
 	if (cond & G_IO_IN)  flags |= DBUS_WATCH_READABLE;
 	if (cond & G_IO_OUT) flags |= DBUS_WATCH_WRITABLE;
@@ -102,10 +103,10 @@ static gboolean watch_func(GIOChannel *chan, GIOCondition cond, gpointer data)
 
 	dbus_watch_handle(info->watch, flags);
 
-	status = dbus_connection_get_dispatch_status(info->conn);
-	queue_dispatch(info->conn, status);
+	status = dbus_connection_get_dispatch_status(conn);
+	queue_dispatch(conn, status);
 
-	dbus_connection_unref(info->conn);
+	dbus_connection_unref(conn);
 
 	return TRUE;
 }
-- 
1.7.11.4

[PATCH 2/4] gdbus: Fix wrong signal handler match

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 6177 bytes --]

From: Lucas De Marchi <lucas.demarchi@profusion.mobi>

When we add a signal handler with g_dbus_add_signal_watch(), this
function tries to multiplex the matches added in libdbus by checking
if there's a previous filter_data with the same fields. However, if the
field is NULL it accepts as being the same. The result is that the
following watches will use the same filter data:

watch1 = g_dbus_add_signal_watch(conn, BUS_NAME, NULL, iface, member,
						cb1, data1, NULL);
watch2 = g_dbus_add_signal_watch(conn, BUS_NAME, "/path2", iface, member,
						cb2, data2, NULL);
watch3 = g_dbus_add_signal_watch(conn, BUS_NAME, "/path3", iface, member,
						cb3, data3, NULL);

The result is that when a signal arrives with path == "/path2", all 3
callbacks above will be called, with the same signal delivered to all of
them.

Another problem is that, if we invert the calls like below, only signals
to cb1 will never be trigerred, nonetheless it used path == NULL.

watch2 = g_dbus_add_signal_watch(conn, BUS_NAME, "/path2", iface, member,
						cb2, data2, NULL);
watch1 = g_dbus_add_signal_watch(conn, BUS_NAME, NULL, iface, member,
						cb1, data1, NULL);
watch3 = g_dbus_add_signal_watch(conn, BUS_NAME, "/path3", iface, member,
						cb3, data3, NULL);

This is fixed by not multiplexing the matchs with filter data if any of
the fields are different, including being NULL. When a signal arrives,
if a field is NULL we accept it as a match, but not when adding the
signal handler.
---
 gdbus/watch.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 94 insertions(+), 21 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index d749176..2661928 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -78,6 +78,47 @@ struct filter_data {
 	gboolean registered;
 };
 
+static struct filter_data *filter_data_find_match(DBusConnection *connection,
+							const char *name,
+							const char *owner,
+							const char *path,
+							const char *interface,
+							const char *member,
+							const char *argument)
+{
+	GSList *current;
+
+	for (current = listeners;
+			current != NULL; current = current->next) {
+		struct filter_data *data = current->data;
+
+		if (connection != data->connection)
+			continue;
+
+		if (g_strcmp0(name, data->name) != 0)
+			continue;
+
+		if (g_strcmp0(owner, data->owner) != 0)
+			continue;
+
+		if (g_strcmp0(path, data->path) != 0)
+			continue;
+
+		if (g_strcmp0(interface, data->interface) != 0)
+			continue;
+
+		if (g_strcmp0(member, data->member) != 0)
+			continue;
+
+		if (g_strcmp0(argument, data->argument) != 0)
+			continue;
+
+		return data;
+	}
+
+	return NULL;
+}
+
 static struct filter_data *filter_data_find(DBusConnection *connection,
 							const char *name,
 							const char *owner,
@@ -221,8 +262,8 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 		name = sender;
 
 proceed:
-	data = filter_data_find(connection, name, owner, path, interface,
-					member, argument);
+	data = filter_data_find_match(connection, name, owner, path,
+						interface, member, argument);
 	if (data)
 		return data;
 
@@ -501,6 +542,7 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 {
 	struct filter_data *data;
 	const char *sender, *path, *iface, *member, *arg = NULL;
+	GSList *current, *delete_listener = NULL;
 
 	/* Only filter signals */
 	if (dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_SIGNAL)
@@ -512,30 +554,63 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	member = dbus_message_get_member(message);
 	dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &arg, DBUS_TYPE_INVALID);
 
-	/* Sender is always bus name */
-	data = filter_data_find(connection, NULL, sender, path, iface, member,
-					arg);
-	if (data == NULL) {
-		error("Got %s.%s signal which has no listeners", iface, member);
-		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
-	}
+	/* Sender is always the owner */
+
+	for (current = listeners; current != NULL; current = current->next) {
+		data = current->data;
+
+		if (connection != data->connection)
+			continue;
+
+		if (data->owner && g_str_equal(sender, data->owner) == FALSE)
+			continue;
 
-	if (data->handle_func) {
-		data->lock = TRUE;
+		if (data->path && g_str_equal(path, data->path) == FALSE)
+			continue;
+
+		if (data->interface && g_str_equal(iface,
+						data->interface) == FALSE)
+			continue;
 
-		data->handle_func(connection, message, data);
+		if (data->member && g_str_equal(member, data->member) == FALSE)
+			continue;
 
-		data->callbacks = data->processed;
-		data->processed = NULL;
-		data->lock = FALSE;
+		if (data->argument && g_str_equal(arg,
+						data->argument) == FALSE)
+			continue;
+
+		if (data->handle_func) {
+			data->lock = TRUE;
+
+			data->handle_func(connection, message, data);
+
+			data->callbacks = data->processed;
+			data->processed = NULL;
+			data->lock = FALSE;
+		}
+
+		if (!data->callbacks)
+			delete_listener = g_slist_prepend(delete_listener,
+								current);
 	}
 
-	if (data->callbacks)
-		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+	for (current = delete_listener; current != NULL;
+					current = delete_listener->next) {
+		GSList *l = current->data;
 
-	remove_match(data);
+		data = l->data;
 
-	listeners = g_slist_remove(listeners, data);
+		/* Has any other callback added callbacks back to this data? */
+		if (data->callbacks != NULL)
+			continue;
+
+		remove_match(data);
+		listeners = g_slist_remove_link(listeners, l);
+
+		filter_data_free(data);
+	}
+
+	g_slist_free(delete_listener);
 
 	/* Remove filter if there are no listeners left for the connection */
 	if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
@@ -543,8 +618,6 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
 
-	filter_data_free(data);
-
 	return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
-- 
1.7.11.4

[PATCH 3/4] gdbus: Refactor filter_data_find()

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

From: Lucas De Marchi <lucas.demarchi@profusion.mobi>

Now this function is only used for searching the listeners of a
connection and the other parameters are not needed anymore.
---
 gdbus/watch.c | 43 +++++--------------------------------------
 1 file changed, 5 insertions(+), 38 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 2661928..a402ca9 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -119,13 +119,7 @@ static struct filter_data *filter_data_find_match(DBusConnection *connection,
 	return NULL;
 }
 
-static struct filter_data *filter_data_find(DBusConnection *connection,
-							const char *name,
-							const char *owner,
-							const char *path,
-							const char *interface,
-							const char *member,
-							const char *argument)
+static struct filter_data *filter_data_find(DBusConnection *connection)
 {
 	GSList *current;
 
@@ -136,30 +130,6 @@ static struct filter_data *filter_data_find(DBusConnection *connection,
 		if (connection != data->connection)
 			continue;
 
-		if (name && data->name &&
-				g_str_equal(name, data->name) == FALSE)
-			continue;
-
-		if (owner && data->owner &&
-				g_str_equal(owner, data->owner) == FALSE)
-			continue;
-
-		if (path && data->path &&
-				g_str_equal(path, data->path) == FALSE)
-			continue;
-
-		if (interface && data->interface &&
-				g_str_equal(interface, data->interface) == FALSE)
-			continue;
-
-		if (member && data->member &&
-				g_str_equal(member, data->member) == FALSE)
-			continue;
-
-		if (argument && data->argument &&
-				g_str_equal(argument, data->argument) == FALSE)
-			continue;
-
 		return data;
 	}
 
@@ -245,7 +215,7 @@ static struct filter_data *filter_data_get(DBusConnection *connection,
 	struct filter_data *data;
 	const char *name = NULL, *owner = NULL;
 
-	if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL, NULL) == NULL) {
+	if (filter_data_find(connection) == NULL) {
 		if (!dbus_connection_add_filter(connection,
 					message_filter, NULL, NULL)) {
 			error("dbus_connection_add_filter() failed");
@@ -419,8 +389,7 @@ static gboolean filter_data_remove_callback(struct filter_data *data,
 	listeners = g_slist_remove(listeners, data);
 
 	/* Remove filter if there are no listeners left for the connection */
-	if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
-								NULL) == NULL)
+	if (filter_data_find(connection) == NULL)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
 
@@ -613,8 +582,7 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 	g_slist_free(delete_listener);
 
 	/* Remove filter if there are no listeners left for the connection */
-	if (filter_data_find(connection, NULL, NULL, NULL, NULL, NULL,
-								NULL) == NULL)
+	if (filter_data_find(connection) == NULL)
 		dbus_connection_remove_filter(connection, message_filter,
 						NULL);
 
@@ -810,8 +778,7 @@ void g_dbus_remove_all_watches(DBusConnection *connection)
 {
 	struct filter_data *data;
 
-	while ((data = filter_data_find(connection, NULL, NULL, NULL, NULL,
-					NULL, NULL))) {
+	while ((data = filter_data_find(connection))) {
 		listeners = g_slist_remove(listeners, data);
 		filter_data_call_and_free(data);
 	}
-- 
1.7.11.4

[PATCH 4/4] gdbus: Fix not freeing list node by using g_slist_delete_link

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

g_slist_remove_link does not free the node which can cause leaks so
replace that with g_slist_delete_link which does free memory properly.
---
 gdbus/watch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index a402ca9..07feb61 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -574,7 +574,7 @@ static DBusHandlerResult message_filter(DBusConnection *connection,
 			continue;
 
 		remove_match(data);
-		listeners = g_slist_remove_link(listeners, l);
+		listeners = g_slist_delete_link(listeners, l);
 
 		filter_data_free(data);
 	}
-- 
1.7.11.4

Re: [PATCH 1/4] gdbus: Fix crash when getting disconnected from the bus

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 76 bytes --]

Hi Luiz,

All four patches have been pushed, thanks.

Regards,
-Denis