[PATCH] core: Double free on adapter_stop

[PATCH] core: Double free on adapter_stop

[Alex Deymo]

The discovery_list list has the list of current discovery clients and is
removed on adapter_stop (for example due a "power off" command). The
g_slist_free_full will call discovery_free on every element of the list
and remove the nodes of the list, but discovery_destroy (called by
discovery_free) will not only free the element, but also remove it from
the list. This causes the list node to be freed twice, once by
g_slist_free_full and once by g_slist_remove.

This fix calls successively discovery_free and lets it remove the list one
by one.
---
 src/adapter.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index e553626..ac322de 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4272,8 +4272,11 @@ static void adapter_stop(struct btd_adapter *adapter)
 	cancel_passive_scanning(adapter);
 
 	if (adapter->discovery_list) {
-		g_slist_free_full(adapter->discovery_list, discovery_free);
-		adapter->discovery_list = NULL;
+		while (adapter->discovery_list) {
+			struct discovery_client *client =
+						adapter->discovery_list->data;
+			discovery_free(client);
+		}
 
 		adapter->discovering = false;
 	}
-- 
1.8.1.3

Re: [PATCH] core: Double free on adapter_stop

[Johan Hedberg]

Hi Alex,

On Fri, Mar 29, 2013, Alex Deymo wrote:
> The discovery_list list has the list of current discovery clients and is
> removed on adapter_stop (for example due a "power off" command). The
> g_slist_free_full will call discovery_free on every element of the list
> and remove the nodes of the list, but discovery_destroy (called by
> discovery_free) will not only free the element, but also remove it from
> the list. This causes the list node to be freed twice, once by
> g_slist_free_full and once by g_slist_remove.
> 
> This fix calls successively discovery_free and lets it remove the list one
> by one.
> ---
>  src/adapter.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index e553626..ac322de 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -4272,8 +4272,11 @@ static void adapter_stop(struct btd_adapter *adapter)
>  	cancel_passive_scanning(adapter);
>  
>  	if (adapter->discovery_list) {
> -		g_slist_free_full(adapter->discovery_list, discovery_free);
> -		adapter->discovery_list = NULL;
> +		while (adapter->discovery_list) {
> +			struct discovery_client *client =
> +						adapter->discovery_list->data;
> +			discovery_free(client);
> +		}
>  
>  		adapter->discovering = false;
>  	}

Good catch, but you could go even further and remove the discovery_free
function too since its only purpose was to match the expected type for
g_slist_free_full (which you no-longer use). Please add a code comment
though clarifying that g_dbus_remove_watch takes care of the freeing and
list element removal.

Also, I'd go ahead and remove one level of nesting here since the
if-statement before the while loop is a bit redundant (the setting of
discovering to false can be unconditional afterwards).

Johan

[PATCH v2] core: Fix a double free on adapter_stop

[Alex Deymo]

The discovery_list list has the list of current discovery clients and is
removed on adapter_stop (for example due a "power off" command). The
g_slist_free_full will call discovery_free on every element of the list
and remove the nodes of the list, but discovery_destroy (called by
discovery_free) will not only free the element, but also remove it from
the list. This causes the list node to be freed twice, once by
g_slist_free_full and once by g_slist_remove.

This fix calls successively discovery_destroy and lets it remove the list's
elements one by one.
---
 src/adapter.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index e553626..9a3bc54 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1540,15 +1540,6 @@ static gboolean remove_temp_devices(gpointer user_data)
 	return FALSE;
 }
 
-static void discovery_free(void *data)
-{
-	struct discovery_client *client = data;
-
-	DBG("owner %s", client->owner);
-
-	g_dbus_remove_watch(dbus_conn, client->watch);
-}
-
 static void discovery_destroy(void *user_data)
 {
 	struct discovery_client *client = user_data;
@@ -4271,12 +4262,13 @@ static void adapter_stop(struct btd_adapter *adapter)
 
 	cancel_passive_scanning(adapter);
 
-	if (adapter->discovery_list) {
-		g_slist_free_full(adapter->discovery_list, discovery_free);
-		adapter->discovery_list = NULL;
-
-		adapter->discovering = false;
+	while (adapter->discovery_list) {
+		struct discovery_client *client = adapter->discovery_list->data;
+		/* g_dbus_remove_watch will remove the client from the adapter's
+		 * list and free it using the discovery_destroy function. */
+		g_dbus_remove_watch(dbus_conn, client->watch);
 	}
+	adapter->discovering = false;
 
 	while (adapter->connections) {
 		struct btd_device *device = adapter->connections->data;
-- 
1.8.1.3

Re: [PATCH v2] core: Fix a double free on adapter_stop

[Johan Hedberg]

Hi Alex,

On Mon, Apr 01, 2013, Alex Deymo wrote:
> The discovery_list list has the list of current discovery clients and is
> removed on adapter_stop (for example due a "power off" command). The
> g_slist_free_full will call discovery_free on every element of the list
> and remove the nodes of the list, but discovery_destroy (called by
> discovery_free) will not only free the element, but also remove it from
> the list. This causes the list node to be freed twice, once by
> g_slist_free_full and once by g_slist_remove.
> 
> This fix calls successively discovery_destroy and lets it remove the list's
> elements one by one.
> ---
>  src/adapter.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)

Applied (after a couple minor coding style changes). Thanks.

Johan