core/advertising: Fix crash when unregistering advertisement too fast

When advertisement is unregistered during MGMT_OP_ADD_ADVERTISING it will crash in add_adv_callback because struct btd_adv_client no longer exist.

This is seen also in debug log from bluetoothd:
bluetoothd[29698]: src/advertising.c:register_advertisement() RegisterAdvertisement
bluetoothd[29698]: src/advertising.c:client_create() Adding proxy for /org/bluez/example/advertisement0 bluetoothd[29698]: src/advertising.c:register_advertisement() Registered advertisement at path /org/bluez/example/advertisement0 bluetoothd[29698]: src/advertising.c:parse_service_uuids() Adding ServiceUUID: 180D
bluetoothd[29698]: src/advertising.c:parse_service_uuids() Adding ServiceUUID: 180F
bluetoothd[29698]: src/advertising.c:parse_manufacturer_data() Adding ManufacturerData for ffff bluetoothd[29698]: src/advertising.c:parse_service_data() Adding ServiceData for 9999
bluetoothd[29698]: src/advertising.c:parse_data() Adding Data for type 0x26 len 3
bluetoothd[29698]: src/advertising.c:refresh_adv() Refreshing advertisement: /org/bluez/example/advertisement0 bluetoothd[29698]: src/advertising.c:unregister_advertisement() UnregisterAdvertisement
bluetoothd[29698]: src/advertising.c:add_adv_callback() Advertisement registered: �
Segmentation fault (core dumped)

---
 src/advertising.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

index 3ed1376..f53c14c 100644
@@ -73,6 +73,7 @@ struct btd_adv_client {
 	uint16_t discoverable_to;
 	unsigned int to_id;
 	unsigned int disc_to_id;
+	unsigned int add_adv_id;
 	GDBusClient *client;
 	GDBusProxy *proxy;
 	DBusMessage *reg;

@@ -117,6 +118,15 @@ static void client_free(void *data)
 		g_dbus_client_unref(client->client);
 	}
 
+	if (client->reg) {
+		g_dbus_send_message(btd_get_dbus_connection(),
+				dbus_message_new_method_return(client->reg));
+		dbus_message_unref(client->reg);
+	}
+
+	if (client->add_adv_id)
+		mgmt_cancel(client->manager->mgmt, client->add_adv_id);
+
 	if (client->instance)
 		util_clear_uid(&client->manager->instance_bitmap,
 						client->instance);

@@ -765,7 +775,8 @@ static uint8_t *generate_scan_rsp(struct btd_adv_client *client,
 	return bt_ad_generate(client->scan, len);
 }
 
-static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func)
+static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func,
+						unsigned int *mgmt_id)
 {
 	struct mgmt_cp_add_advertising *cp;
 	uint8_t param_len;

@@ -774,6 +785,7 @@ static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func)
 	uint8_t *scan_rsp;
 	size_t scan_rsp_len = -1;
 	uint32_t flags = 0;
+	unsigned int mgmt_ret;
 
 	DBG("Refreshing advertisement: %s", client->path);
 

@@ -822,13 +834,17 @@ static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func)
 	free(adv_data);
 	free(scan_rsp);
 
-	if (!mgmt_send(client->manager->mgmt, MGMT_OP_ADD_ADVERTISING,
-				client->manager->mgmt_index, param_len, cp,
-				func, client, NULL)) {
+	mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_ADVERTISING,
+			client->manager->mgmt_index, param_len, cp,
+			func, client, NULL);
+
+	if (!mgmt_ret) {
 		error("Failed to add Advertising Data");
 		free(cp);
 		return -EINVAL;
 	}
+	if (mgmt_id)
+		*mgmt_id = mgmt_ret;
 
 	free(cp);
 

@@ -845,7 +861,7 @@ static gboolean client_discoverable_timeout(void *user_data)
 
 	bt_ad_clear_flags(client->data);
 
-	refresh_adv(client, NULL);
+	refresh_adv(client, NULL, NULL);
 
 	return FALSE;
 }

@@ -948,7 +964,7 @@ static void properties_changed(GDBusProxy *proxy, const char *name,
 			continue;
 
 		if (parser->func(iter, client)) {
-			refresh_adv(client, NULL);
+			refresh_adv(client, NULL, NULL);
 			break;
 		}
 	}

@@ -980,6 +996,8 @@ 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;
 
+	client->add_adv_id = 0;
+
 	if (status)
 		goto done;
 

@@ -1059,7 +1077,7 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
 		goto fail;
 	}
 
-	err = refresh_adv(client, add_adv_callback);
+	err = refresh_adv(client, add_adv_callback, &client->add_adv_id);
 	if (!err)
 		return NULL;
 

@@ -1449,7 +1467,7 @@ void btd_adv_manager_destroy(struct btd_adv_manager *manager)
 
 static void manager_refresh(void *data, void *user_data)
 {
-	refresh_adv(data, user_data);
+	refresh_adv(data, user_data, NULL);
 }
 
 void btd_adv_manager_refresh(struct btd_adv_manager *manager)

2.7.4

After "register_advertisement()" callback "client_proxy_added()" was yet called but it was actually called after: "unregister_advertisement()" which caused segmentation fault because client was removed twice.

---
 src/advertising.c | 2 ++
 1 file changed, 2 insertions(+)

index f53c14c..45ff19f 100644
@@ -190,6 +190,8 @@ static void client_remove(void *data)
 	struct btd_adv_client *client = data;
 	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);
 
 	cp.instance = client->instance;

2.7.4

After "register_advertisement()" callback "client_proxy_added()" was yet called but it was actually called after: "unregister_advertisement()" which caused segmentation fault because client was removed twice.

---
src/advertising.c | 2 ++

1 file changed, 2 insertions(+)

index f53c14c..45ff19f 100644
@@ -190,6 +190,8 @@ static void client_remove(void *data)

struct btd_adv_client *client = data;
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); cp.instance = client->instance;

2.7.4

When advertisement is unregistered during MGMT_OP_ADD_ADVERTISING it will crash in add_adv_callback because struct btd_adv_client no longer exist.

This is seen also in debug log from bluetoothd:
bluetoothd[29698]: src/advertising.c:register_advertisement() RegisterAdvertisement
bluetoothd[29698]: src/advertising.c:client_create() Adding proxy for /org/bluez/example/advertisement0 bluetoothd[29698]: src/advertising.c:register_advertisement() Registered advertisement at path /org/bluez/example/advertisement0 bluetoothd[29698]: src/advertising.c:parse_service_uuids() Adding ServiceUUID: 180D
bluetoothd[29698]: src/advertising.c:parse_service_uuids() Adding ServiceUUID: 180F
bluetoothd[29698]: src/advertising.c:parse_manufacturer_data() Adding ManufacturerData for ffff bluetoothd[29698]: src/advertising.c:parse_service_data() Adding ServiceData for 9999
bluetoothd[29698]: src/advertising.c:parse_data() Adding Data for type 0x26 len 3
bluetoothd[29698]: src/advertising.c:refresh_adv() Refreshing advertisement: /org/bluez/example/advertisement0 bluetoothd[29698]: src/advertising.c:unregister_advertisement() UnregisterAdvertisement
bluetoothd[29698]: src/advertising.c:add_adv_callback() Advertisement registered: �
Segmentation fault (core dumped)

---
src/advertising.c | 34 ++++++++++++++++++++++++++--------

1 file changed, 26 insertions(+), 8 deletions(-)

index 3ed1376..f53c14c 100644
@@ -73,6 +73,7 @@ struct btd_adv_client {

uint16_t discoverable_to;
unsigned int to_id;
unsigned int disc_to_id;
+ unsigned int add_adv_id;
GDBusClient *client;
GDBusProxy *proxy;
DBusMessage *reg;
@@ -117,6 +118,15 @@ static void client_free(void *data) g_dbus_client_unref(client->client);
}
+ if (client->reg) {
+ g_dbus_send_message(btd_get_dbus_connection(),
+ dbus_message_new_method_return(client->reg));
+ dbus_message_unref(client->reg);
+ }
+
+ if (client->add_adv_id)
+ mgmt_cancel(client->manager->mgmt, client->add_adv_id);
+
if (client->instance)
util_clear_uid(&client->manager->instance_bitmap,
client->instance);
@@ -765,7 +775,8 @@ static uint8_t *generate_scan_rsp(struct btd_adv_client *client, return bt_ad_generate(client->scan, len);
}
-static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func) +static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func,
+ unsigned int *mgmt_id)
{
struct mgmt_cp_add_advertising *cp;
uint8_t param_len;
@@ -774,6 +785,7 @@ static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func) uint8_t *scan_rsp;
size_t scan_rsp_len = -1;
uint32_t flags = 0;
+ unsigned int mgmt_ret;
DBG("Refreshing advertisement: %s", client->path);
@@ -822,13 +834,17 @@ static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func) free(adv_data);
free(scan_rsp);
- if (!mgmt_send(client->manager->mgmt, MGMT_OP_ADD_ADVERTISING,
- client->manager->mgmt_index, param_len, cp,
- func, client, NULL)) {
+ mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_ADVERTISING,
+ client->manager->mgmt_index, param_len, cp,
+ func, client, NULL);
+
+ if (!mgmt_ret) {
error("Failed to add Advertising Data");
free(cp);
return -EINVAL;
}
+ if (mgmt_id)
+ *mgmt_id = mgmt_ret;
free(cp);
@@ -845,7 +861,7 @@ static gboolean client_discoverable_timeout(void *user_data) bt_ad_clear_flags(client->data);
- refresh_adv(client, NULL);
+ refresh_adv(client, NULL, NULL);
return FALSE;
}
@@ -948,7 +964,7 @@ static void properties_changed(GDBusProxy *proxy, const char *name, continue;
if (parser->func(iter, client)) {
- refresh_adv(client, NULL);
+ refresh_adv(client, NULL, NULL);
break;
}
}
@@ -980,6 +996,8 @@ 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;
+ client->add_adv_id = 0;
+
if (status)
goto done;
@@ -1059,7 +1077,7 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client) goto fail;
}
- err = refresh_adv(client, add_adv_callback);
+ err = refresh_adv(client, add_adv_callback, &client->add_adv_id);
if (!err)
return NULL;
@@ -1449,7 +1467,7 @@ void btd_adv_manager_destroy(struct btd_adv_manager *manager) static void manager_refresh(void *data, void *user_data)
{
- refresh_adv(data, user_data);
+ refresh_adv(data, user_data, NULL);
}
void btd_adv_manager_refresh(struct btd_adv_manager *manager)

2.7.4

Hi Simon,

Looks like this patch is mangled, there is no leading tabs for indentation, etc, check out our HACKING document it contain instruction on how to produce valid patches:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/HACKING#n95

Sometimes this happens with certain mail server, but you can always submit the patches with the likes of gmail since for git it doesn't matter how you submit it since it only cares the author entry we should be preserved regardless of the mail server you have used.

Luiz Augusto von Dentz