Hi,
On Tue, Nov 12, 2019 at 3:59 PM Simon Mikuda
<simon.mikuda@xxxxxxxxxxxxxxxxxxx> 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 <simon.mikuda@xxxxxxxxxxxxxxxxxxx>
> ---
> 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
Applied, thanks.
--
Luiz Augusto von Dentz
