[RFC PATCH BlueZ 0/1] advertising: Fix double-free in client_free

[RFC PATCH BlueZ 0/1] advertising: Fix double-free in client_free

[Pavel Bozhko]

bluetoothd 5.79 aborts with SIGABRT in client_free() while freeing
client->owner, called from client_free_idle_cb(). The crash shows up
at random intervals. I cannot force it by hand.

The frames that match the 5.79 tree are:

  client_free (advertising.c:150)
  client_free_idle_cb (advertising.c:161)

Line 150 is g_free(client->owner), and glibc reports a double free.
The minidump also contains unrelated frames from a bad unwind
(open_cfm, _init). Those are not this path.

No commit on origin/master since 5.79 stops the second idle free.
0d57c12f5aca ("advertising: Fix leaking clients that exit while
registering") installs the disconnect watch during
RegisterAdvertisement, and the remove and failure paths still each
queue a free. We cannot leave 5.79 yet: buildroot pins that release.

Please comment on both changes. The diff below is what we apply on
5.79. It does not apply to current master: client_remove() takes
struct btd_adv_client *, and the disconnect watch is set from
RegisterAdvertisement. Patch 1/1 is that fix rebased on origin/master.

--- 5.79 example, not for application on master ---

    diff --git a/src/advertising.c b/src/advertising.c
    index bd121e525..45fdb5b43 100644
    --- a/src/advertising.c
    +++ b/src/advertising.c
    @@ -88,6 +88,7 @@ struct btd_adv_client {
     	uint32_t max_interval;
     	int8_t tx_power;
     	mgmt_request_func_t refresh_done_func;
    +	bool pending_destroy;
     };
 
     struct dbus_obj_match {
    @@ -200,6 +201,12 @@ static void client_remove(void *data)
     	struct btd_adv_client *client = data;
     	struct mgmt_cp_remove_advertising cp;
 
    +	/* Prevent double-free if client_remove is called multiple times */
    +	if (client->pending_destroy)
    +		return;
    +
    +	client->pending_destroy = true;
    +
     	g_dbus_client_set_proxy_handlers(client->client, NULL, NULL, NULL,
     									client);
     	g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
    @@ -1265,6 +1272,12 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
     		return;
 
     	if (status) {
    +		/* Prevent double-free if already pending destroy */
    +		if (client->pending_destroy)
    +			return;
    +
    +		client->pending_destroy = true;
    +
     		reply = btd_error_failed(client->reg,
     					"Failed to register advertisement");
     		queue_remove(client->manager->clients, client);
    @@ -1286,6 +1299,10 @@ static void add_adv_callback(uint8_t status, uint16_t length,
 
     	client->add_adv_id = 0;
 
    +	/* Client is already being destroyed, skip processing */
    +	if (client->pending_destroy)
    +		return;
    +
     	if (status)
     		goto done;
 
    @@ -1332,6 +1349,10 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 
     	client->add_adv_id = 0;
 
    +	/* Client is already being destroyed, skip processing */
    +	if (client->pending_destroy)
    +		return;
    +
     	if (status)
     		goto fail;
 
    @@ -1509,6 +1530,12 @@ static void client_proxy_added(GDBusProxy *proxy, void *data)
     	if (!reply)
     		return;
 
    +	/* Prevent double-free if already pending destroy */
    +	if (client->pending_destroy)
    +		return;
    +
    +	client->pending_destroy = true;
    +
     	/* Failed to publish for some reason, remove. */
     	queue_remove(client->manager->clients, client);
 

Pavel Bozhko (1):
  advertising: Fix double-free in client_free

 src/advertising.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 52 insertions(+), 10 deletions(-)

base-commit: 9659adb6819d7e0284d0d2ba226a3894423d8db4
-- 
2.50.1 (Apple Git-155)

[RFC PATCH BlueZ 1/1] advertising: Fix double-free in client_free

[Pavel Bozhko]

Unregister, disconnect and a failed RegisterAdvertisement can all run
before an earlier idle client_free(). Each of those paths queues its
own idle free, and the second g_free(client->owner) aborts bluetoothd.
A late MGMT add completion does the same while the client is still live.

Watching the disconnect during registration (0d57c12f5aca) makes the
overlap reachable before Add Advertising completes. Those paths still
queue a free of their own.

Destroy the client once. Later remove and add completions do nothing.

Fixes: cf99200d1505 ("core/advertising: Implement RegisterAdvertisement")
Assisted-by: Grok:4.7
---
 src/advertising.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 52 insertions(+), 10 deletions(-)

Not runtime tested. The 5.79 crash is client_free() at g_free(client->owner),
reached from client_free_idle_cb().

diff --git a/src/advertising.c b/src/advertising.c
index 5c929d6c2..3e6155db0 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -88,6 +88,7 @@ struct btd_adv_client {
 	uint32_t max_interval;
 	int8_t tx_power;
 	mgmt_request_func_t refresh_done_func;
+	bool pending_destroy;
 };
 
 struct dbus_obj_match {
@@ -195,13 +196,43 @@ static void remove_advertising(struct btd_adv_manager *manager,
 			manager->mgmt_index, sizeof(cp), &cp, NULL, NULL, NULL);
 }
 
+/*
+ * Queue client_free once. Later calls are ignored so remove and a late
+ * MGMT completion cannot free the client twice.
+ */
+static bool client_destroy_start(struct btd_adv_client *client)
+{
+	if (client->pending_destroy)
+		return false;
+
+	client->pending_destroy = true;
+
+	if (client->client) {
+		g_dbus_client_set_proxy_handlers(client->client, NULL, NULL,
+						 NULL, client);
+		g_dbus_client_set_disconnect_watch(client->client, NULL,
+						   NULL);
+	}
+
+	if (client->proxy)
+		g_dbus_proxy_set_property_watch(client->proxy, NULL, NULL);
+
+	if (client->add_adv_id) {
+		mgmt_cancel(client->manager->mgmt, client->add_adv_id);
+		client->add_adv_id = 0;
+	}
+
+	g_idle_add(client_free_idle_cb, client);
+
+	return true;
+}
+
 static void client_remove(struct btd_adv_client *client)
 {
 	struct mgmt_cp_remove_advertising cp;
 
-	g_dbus_client_set_proxy_handlers(client->client, NULL, NULL, NULL,
-									client);
-	g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
+	if (!client_destroy_start(client))
+		return;
 
 	cp.instance = client->instance;
 
@@ -211,8 +242,6 @@ static void client_remove(struct btd_adv_client *client)
 
 	queue_remove(client->manager->clients, client);
 
-	g_idle_add(client_free_idle_cb, client);
-
 	g_dbus_emit_property_changed(btd_get_dbus_connection(),
 				adapter_get_path(client->manager->adapter),
 				LE_ADVERTISING_MGR_IFACE, "SupportedInstances");
@@ -1373,6 +1402,9 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
 {
 	DBusMessage *reply;
 
+	if (client->pending_destroy)
+		return;
+
 	if (status)
 		error("Failed to add advertisement: %s (0x%02x)",
 						mgmt_errstr(status), status);
@@ -1385,11 +1417,12 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
 		return;
 
 	if (status) {
+		if (!client_destroy_start(client))
+			return;
+
 		reply = btd_error_failed(client->reg,
 					"Failed to register advertisement");
 		queue_remove(client->manager->clients, client);
-		g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
-		g_idle_add(client_free_idle_cb, client);
 
 	} else {
 		DBusMessageIter iter;
@@ -1416,6 +1449,9 @@ static void add_adv_callback(uint8_t status, uint16_t length,
 	struct btd_adv_client *client = user_data;
 	const struct mgmt_rp_add_advertising *rp = param;
 
+	if (client->pending_destroy)
+		return;
+
 	client->add_adv_id = 0;
 
 	if (status)
@@ -1460,6 +1496,9 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 	unsigned int mgmt_ret;
 	dbus_int16_t tx_power;
 
+	if (client->pending_destroy)
+		return;
+
 	client->add_adv_id = 0;
 
 	if (status)
@@ -1634,15 +1673,18 @@ static void client_proxy_added(GDBusProxy *proxy, void *data)
 	if (g_str_equal(interface, LE_ADVERTISEMENT_IFACE) == FALSE)
 		return;
 
+	if (client->pending_destroy)
+		return;
+
 	reply = parse_advertisement(client);
 	if (!reply)
 		return;
 
 	/* Failed to publish for some reason, remove. */
-	queue_remove(client->manager->clients, client);
-	g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
+	if (!client_destroy_start(client))
+		return;
 
-	g_idle_add(client_free_idle_cb, client);
+	queue_remove(client->manager->clients, client);
 
 	g_dbus_send_message(btd_get_dbus_connection(), reply);

base-commit: 9659adb6819d7e0284d0d2ba226a3894423d8db4
-- 
2.50.1 (Apple Git-155)

Re: [RFC PATCH BlueZ 1/1] advertising: Fix double-free in client_free

[Luiz Augusto von Dentz]

Hi Pavel,

On Thu, Nov 12, 2026 at 12:08 PM Pavel Bozhko
<pvbozhko@salutedevices.com> wrote:
>
> Unregister, disconnect and a failed RegisterAdvertisement can all run
> before an earlier idle client_free(). Each of those paths queues its
> own idle free, and the second g_free(client->owner) aborts bluetoothd.
> A late MGMT add completion does the same while the client is still live.
>
> Watching the disconnect during registration (0d57c12f5aca) makes the
> overlap reachable before Add Advertising completes. Those paths still
> queue a free of their own.
>
> Destroy the client once. Later remove and add completions do nothing.
>
> Fixes: cf99200d1505 ("core/advertising: Implement RegisterAdvertisement")
> Assisted-by: Grok:4.7
> ---
>  src/advertising.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 52 insertions(+), 10 deletions(-)
>
> Not runtime tested. The 5.79 crash is client_free() at g_free(client->owner),
> reached from client_free_idle_cb().
>
> diff --git a/src/advertising.c b/src/advertising.c
> index 5c929d6c2..3e6155db0 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -88,6 +88,7 @@ struct btd_adv_client {
>         uint32_t max_interval;
>         int8_t tx_power;
>         mgmt_request_func_t refresh_done_func;
> +       bool pending_destroy;
>  };
>
>  struct dbus_obj_match {
> @@ -195,13 +196,43 @@ static void remove_advertising(struct btd_adv_manager *manager,
>                         manager->mgmt_index, sizeof(cp), &cp, NULL, NULL, NULL);
>  }
>
> +/*
> + * Queue client_free once. Later calls are ignored so remove and a late
> + * MGMT completion cannot free the client twice.
> + */
> +static bool client_destroy_start(struct btd_adv_client *client)
> +{
> +       if (client->pending_destroy)
> +               return false;
> +
> +       client->pending_destroy = true;
> +
> +       if (client->client) {
> +               g_dbus_client_set_proxy_handlers(client->client, NULL, NULL,
> +                                                NULL, client);
> +               g_dbus_client_set_disconnect_watch(client->client, NULL,
> +                                                  NULL);
> +       }
> +
> +       if (client->proxy)
> +               g_dbus_proxy_set_property_watch(client->proxy, NULL, NULL);
> +
> +       if (client->add_adv_id) {
> +               mgmt_cancel(client->manager->mgmt, client->add_adv_id);
> +               client->add_adv_id = 0;
> +       }
> +
> +       g_idle_add(client_free_idle_cb, client);

Just store the return of g_idle_add and then check it to prevent it to
be scheduled twice, also if the client is freed directly it can then
just remove its id to abort the callback causing a user after free.

> +       return true;
> +}
> +
>  static void client_remove(struct btd_adv_client *client)
>  {
>         struct mgmt_cp_remove_advertising cp;
>
> -       g_dbus_client_set_proxy_handlers(client->client, NULL, NULL, NULL,
> -                                                                       client);
> -       g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
> +       if (!client_destroy_start(client))
> +               return;
>
>         cp.instance = client->instance;
>
> @@ -211,8 +242,6 @@ static void client_remove(struct btd_adv_client *client)
>
>         queue_remove(client->manager->clients, client);
>
> -       g_idle_add(client_free_idle_cb, client);
> -
>         g_dbus_emit_property_changed(btd_get_dbus_connection(),
>                                 adapter_get_path(client->manager->adapter),
>                                 LE_ADVERTISING_MGR_IFACE, "SupportedInstances");
> @@ -1373,6 +1402,9 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
>  {
>         DBusMessage *reply;
>
> +       if (client->pending_destroy)
> +               return;
> +
>         if (status)
>                 error("Failed to add advertisement: %s (0x%02x)",
>                                                 mgmt_errstr(status), status);
> @@ -1385,11 +1417,12 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
>                 return;
>
>         if (status) {
> +               if (!client_destroy_start(client))
> +                       return;
> +
>                 reply = btd_error_failed(client->reg,
>                                         "Failed to register advertisement");
>                 queue_remove(client->manager->clients, client);
> -               g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
> -               g_idle_add(client_free_idle_cb, client);
>
>         } else {
>                 DBusMessageIter iter;
> @@ -1416,6 +1449,9 @@ static void add_adv_callback(uint8_t status, uint16_t length,
>         struct btd_adv_client *client = user_data;
>         const struct mgmt_rp_add_advertising *rp = param;
>
> +       if (client->pending_destroy)
> +               return;
> +
>         client->add_adv_id = 0;
>
>         if (status)
> @@ -1460,6 +1496,9 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
>         unsigned int mgmt_ret;
>         dbus_int16_t tx_power;
>
> +       if (client->pending_destroy)
> +               return;
> +
>         client->add_adv_id = 0;
>
>         if (status)
> @@ -1634,15 +1673,18 @@ static void client_proxy_added(GDBusProxy *proxy, void *data)
>         if (g_str_equal(interface, LE_ADVERTISEMENT_IFACE) == FALSE)
>                 return;
>
> +       if (client->pending_destroy)
> +               return;
> +
>         reply = parse_advertisement(client);
>         if (!reply)
>                 return;
>
>         /* Failed to publish for some reason, remove. */
> -       queue_remove(client->manager->clients, client);
> -       g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
> +       if (!client_destroy_start(client))
> +               return;
>
> -       g_idle_add(client_free_idle_cb, client);
> +       queue_remove(client->manager->clients, client);
>
>         g_dbus_send_message(btd_get_dbus_connection(), reply);
>
> base-commit: 9659adb6819d7e0284d0d2ba226a3894423d8db4
> --
> 2.50.1 (Apple Git-155)
>
>


-- 
Luiz Augusto von Dentz

RE: advertising: Fix double-free in client_free

[bluez.test.bot]

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

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/series/1209793/

---Test result---

Test Summary:
CheckPatch                    PASS      0.40 seconds
GitLint                       PASS      0.23 seconds
BuildEll                      PASS      18.15 seconds
BluezMake                     PASS      272.18 seconds
CheckSmatch                   WARNING   250.94 seconds
bluezmakeextell               PASS      91.05 seconds
TestFunctional                PASS      1107.06 seconds
IncrementalBuild              PASS      279.60 seconds
ScanBuild                     PASS      960.47 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/advertising.c: note: in included file:./src/shared/mgmt.h:95:25: error: redefinition of unsigned int enum mgmt_io_capabilitysrc/advertising.c: note: in included file:./src/shared/mgmt.h:95:25: error: redefinition of unsigned int enum mgmt_io_capability


https://github.com/bluez/bluez/pull/2816

---
Regards,
Linux Bluetooth