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)
Signed-off-by: Simon Mikuda <[email protected]>
---
src/advertising.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/src/advertising.c b/src/advertising.c
index 3ed1376..f53c14c 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -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,
On Tue, Nov 12, 2019 at 3:13 PM Simon Mikuda
<[email protected]> wrote:
>
> 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)
>
> Signed-off-by: Simon Mikuda <[email protected]>
> ---
> src/advertising.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/src/advertising.c b/src/advertising.c
> index 3ed1376..f53c14c 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -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
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