* [PATCH] advertising: Fix crash when advertisement is unregistered while MGMT ADD_ADVERTISEMENT call
@ 2019-10-31 6:52 Simon Mikuda
2019-11-01 14:49 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 3+ messages in thread
From: Simon Mikuda @ 2019-10-31 6:52 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Simon Mikuda
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 | 53 +++++++++++++++++++++++++++++------------------------
1 file changed, 29 insertions(+), 24 deletions(-)
diff --git a/src/advertising.c b/src/advertising.c
index 3ed1376..7d8cadf 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -63,6 +63,8 @@ struct btd_adv_manager {
#define AD_TYPE_PERIPHERAL 1
struct btd_adv_client {
+ int ref_count;
+
struct btd_adv_manager *manager;
char *owner;
char *path;
@@ -136,13 +138,6 @@ static void client_free(void *data)
free(client);
}
-static gboolean client_free_idle_cb(void *data)
-{
- client_free(data);
-
- return FALSE;
-}
-
static void client_release(void *data)
{
struct btd_adv_client *client = data;
@@ -175,12 +170,19 @@ static void remove_advertising(struct btd_adv_manager *manager,
manager->mgmt_index, sizeof(cp), &cp, NULL, NULL, NULL);
}
-static void client_remove(void *data)
+static struct btd_adv_client *client_ref(struct btd_adv_client *client)
+{
+ __sync_fetch_and_add(&client->ref_count, 1);
+
+ return client;
+}
+
+static void client_unref(struct btd_adv_client *client)
{
- struct btd_adv_client *client = data;
struct mgmt_cp_remove_advertising cp;
- g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
+ if (__sync_sub_and_fetch(&client->ref_count, 1))
+ return;
cp.instance = client->instance;
@@ -188,10 +190,6 @@ static void client_remove(void *data)
client->manager->mgmt_index, sizeof(cp), &cp,
NULL, NULL, NULL);
- 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");
@@ -199,13 +197,18 @@ static void client_remove(void *data)
g_dbus_emit_property_changed(btd_get_dbus_connection(),
adapter_get_path(client->manager->adapter),
LE_ADVERTISING_MGR_IFACE, "ActiveInstances");
+
+ queue_remove(client->manager->clients, client);
+
+ client_free(client);
+
}
static void client_disconnect_cb(DBusConnection *conn, void *user_data)
{
DBG("Client disconnected");
- client_remove(user_data);
+ client_unref(user_data);
}
static bool parse_type(DBusMessageIter *iter, struct btd_adv_client *client)
@@ -564,7 +567,7 @@ static gboolean client_timeout(void *user_data)
client->to_id = 0;
client_release(client);
- client_remove(client);
+ client_unref(client);
return FALSE;
}
@@ -963,8 +966,6 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
mgmt_errstr(status), status);
reply = btd_error_failed(client->reg,
"Failed to register advertisement");
- queue_remove(client->manager->clients, client);
- g_idle_add(client_free_idle_cb, client);
} else
reply = dbus_message_new_method_return(client->reg);
@@ -972,6 +973,8 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
g_dbus_send_message(btd_get_dbus_connection(), reply);
dbus_message_unref(client->reg);
client->reg = NULL;
+
+ client_unref(client);
}
static void add_adv_callback(uint8_t status, uint16_t length,
@@ -1060,8 +1063,11 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
}
err = refresh_adv(client, add_adv_callback);
- if (!err)
+ if (!err) {
+ /* Hold reference to client until add_adv_callback is finished */
+ client_ref(client);
return NULL;
+ }
fail:
return btd_error_failed(client->reg, "Failed to parse advertisement.");
@@ -1082,14 +1088,12 @@ static void client_proxy_added(GDBusProxy *proxy, void *data)
return;
/* Failed to publish for some reason, remove. */
- queue_remove(client->manager->clients, client);
-
- g_idle_add(client_free_idle_cb, client);
-
g_dbus_send_message(btd_get_dbus_connection(), reply);
dbus_message_unref(client->reg);
client->reg = NULL;
+
+ client_unref(client);
}
static struct btd_adv_client *client_create(struct btd_adv_manager *manager,
@@ -1189,6 +1193,7 @@ static DBusMessage *register_advertisement(DBusConnection *conn,
DBG("Registered advertisement at path %s", match.path);
queue_push_tail(manager->clients, client);
+ client_ref(client);
return NULL;
}
@@ -1218,7 +1223,7 @@ static DBusMessage *unregister_advertisement(DBusConnection *conn,
if (!client)
return btd_error_does_not_exist(msg);
- client_remove(client);
+ client_unref(client);
return dbus_message_new_method_return(msg);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] advertising: Fix crash when advertisement is unregistered while MGMT ADD_ADVERTISEMENT call
2019-10-31 6:52 [PATCH] advertising: Fix crash when advertisement is unregistered while MGMT ADD_ADVERTISEMENT call Simon Mikuda
@ 2019-11-01 14:49 ` Luiz Augusto von Dentz
2019-11-04 9:50 ` Simon Mikuda
0 siblings, 1 reply; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2019-11-01 14:49 UTC (permalink / raw)
To: Simon Mikuda; +Cc: linux-bluetooth
Hi Simon,
On Thu, Oct 31, 2019 at 9:45 AM Simon Mikuda
<simon.mikuda@streamunlimited.com> wrote:
Subject should be less than 72 characters so it first nicely on git
shortlog, you can add a short description before the backtrace.
> 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 | 53 +++++++++++++++++++++++++++++------------------------
> 1 file changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/src/advertising.c b/src/advertising.c
> index 3ed1376..7d8cadf 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -63,6 +63,8 @@ struct btd_adv_manager {
> #define AD_TYPE_PERIPHERAL 1
>
> struct btd_adv_client {
> + int ref_count;
> +
> struct btd_adv_manager *manager;
> char *owner;
> char *path;
> @@ -136,13 +138,6 @@ static void client_free(void *data)
> free(client);
> }
>
> -static gboolean client_free_idle_cb(void *data)
> -{
> - client_free(data);
> -
> - return FALSE;
> -}
> -
> static void client_release(void *data)
> {
> struct btd_adv_client *client = data;
> @@ -175,12 +170,19 @@ static void remove_advertising(struct btd_adv_manager *manager,
> manager->mgmt_index, sizeof(cp), &cp, NULL, NULL, NULL);
> }
>
> -static void client_remove(void *data)
> +static struct btd_adv_client *client_ref(struct btd_adv_client *client)
> +{
> + __sync_fetch_and_add(&client->ref_count, 1);
> +
> + return client;
> +}
> +
> +static void client_unref(struct btd_adv_client *client)
> {
> - struct btd_adv_client *client = data;
> struct mgmt_cp_remove_advertising cp;
>
> - g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
> + if (__sync_sub_and_fetch(&client->ref_count, 1))
> + return;
While the idea of using refcount to protect might seem good I think we
would be better off using mgmt_cancel for this case so the callback is
not called, or you are not using it on purpose?
> cp.instance = client->instance;
>
> @@ -188,10 +190,6 @@ static void client_remove(void *data)
> client->manager->mgmt_index, sizeof(cp), &cp,
> NULL, NULL, NULL);
>
> - 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");
> @@ -199,13 +197,18 @@ static void client_remove(void *data)
> g_dbus_emit_property_changed(btd_get_dbus_connection(),
> adapter_get_path(client->manager->adapter),
> LE_ADVERTISING_MGR_IFACE, "ActiveInstances");
> +
> + queue_remove(client->manager->clients, client);
> +
> + client_free(client);
> +
> }
>
> static void client_disconnect_cb(DBusConnection *conn, void *user_data)
> {
> DBG("Client disconnected");
>
> - client_remove(user_data);
> + client_unref(user_data);
> }
>
> static bool parse_type(DBusMessageIter *iter, struct btd_adv_client *client)
> @@ -564,7 +567,7 @@ static gboolean client_timeout(void *user_data)
> client->to_id = 0;
>
> client_release(client);
> - client_remove(client);
> + client_unref(client);
>
> return FALSE;
> }
> @@ -963,8 +966,6 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
> mgmt_errstr(status), status);
> reply = btd_error_failed(client->reg,
> "Failed to register advertisement");
> - queue_remove(client->manager->clients, client);
> - g_idle_add(client_free_idle_cb, client);
>
> } else
> reply = dbus_message_new_method_return(client->reg);
> @@ -972,6 +973,8 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
> g_dbus_send_message(btd_get_dbus_connection(), reply);
> dbus_message_unref(client->reg);
> client->reg = NULL;
> +
> + client_unref(client);
> }
>
> static void add_adv_callback(uint8_t status, uint16_t length,
> @@ -1060,8 +1063,11 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
> }
>
> err = refresh_adv(client, add_adv_callback);
> - if (!err)
> + if (!err) {
> + /* Hold reference to client until add_adv_callback is finished */
> + client_ref(client);
> return NULL;
> + }
>
> fail:
> return btd_error_failed(client->reg, "Failed to parse advertisement.");
> @@ -1082,14 +1088,12 @@ static void client_proxy_added(GDBusProxy *proxy, void *data)
> return;
>
> /* Failed to publish for some reason, remove. */
> - queue_remove(client->manager->clients, client);
> -
> - g_idle_add(client_free_idle_cb, client);
> -
> g_dbus_send_message(btd_get_dbus_connection(), reply);
>
> dbus_message_unref(client->reg);
> client->reg = NULL;
> +
> + client_unref(client);
> }
>
> static struct btd_adv_client *client_create(struct btd_adv_manager *manager,
> @@ -1189,6 +1193,7 @@ static DBusMessage *register_advertisement(DBusConnection *conn,
> DBG("Registered advertisement at path %s", match.path);
>
> queue_push_tail(manager->clients, client);
> + client_ref(client);
>
> return NULL;
> }
> @@ -1218,7 +1223,7 @@ static DBusMessage *unregister_advertisement(DBusConnection *conn,
> if (!client)
> return btd_error_does_not_exist(msg);
>
> - client_remove(client);
> + client_unref(client);
>
> return dbus_message_new_method_return(msg);
> }
> --
> 2.7.4
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] advertising: Fix crash when advertisement is unregistered while MGMT ADD_ADVERTISEMENT call
2019-11-01 14:49 ` Luiz Augusto von Dentz
@ 2019-11-04 9:50 ` Simon Mikuda
0 siblings, 0 replies; 3+ messages in thread
From: Simon Mikuda @ 2019-11-04 9:50 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz!
Using mgmt_cancel didn't came to my mind, but it makes more sense.
Advantage about using refcount is that you will always have
advertisement instance_id from kernel callback so you can use
advertising MGMT directly from e.g. btmgmt and it will still work ok.
Also I could remove g_idle_add(client_free_idle_cb, client); which is
little bit tricky to use and it is easy to make double free when editing
- there is also a bug about this but it is harder to reproduce.
I will send new patches with updated description.
Thank you!
BR,
Simon
On 01/11/2019 15:49, Luiz Augusto von Dentz wrote:
> Hi Simon,
>
> On Thu, Oct 31, 2019 at 9:45 AM Simon Mikuda
> <simon.mikuda@streamunlimited.com> wrote:
>
> Subject should be less than 72 characters so it first nicely on git
> shortlog, you can add a short description before the backtrace.
>
>> 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 | 53 +++++++++++++++++++++++++++++------------------------
>> 1 file changed, 29 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/advertising.c b/src/advertising.c
>> index 3ed1376..7d8cadf 100644
>> --- a/src/advertising.c
>> +++ b/src/advertising.c
>> @@ -63,6 +63,8 @@ struct btd_adv_manager {
>> #define AD_TYPE_PERIPHERAL 1
>>
>> struct btd_adv_client {
>> + int ref_count;
>> +
>> struct btd_adv_manager *manager;
>> char *owner;
>> char *path;
>> @@ -136,13 +138,6 @@ static void client_free(void *data)
>> free(client);
>> }
>>
>> -static gboolean client_free_idle_cb(void *data)
>> -{
>> - client_free(data);
>> -
>> - return FALSE;
>> -}
>> -
>> static void client_release(void *data)
>> {
>> struct btd_adv_client *client = data;
>> @@ -175,12 +170,19 @@ static void remove_advertising(struct btd_adv_manager *manager,
>> manager->mgmt_index, sizeof(cp), &cp, NULL, NULL, NULL);
>> }
>>
>> -static void client_remove(void *data)
>> +static struct btd_adv_client *client_ref(struct btd_adv_client *client)
>> +{
>> + __sync_fetch_and_add(&client->ref_count, 1);
>> +
>> + return client;
>> +}
>> +
>> +static void client_unref(struct btd_adv_client *client)
>> {
>> - struct btd_adv_client *client = data;
>> struct mgmt_cp_remove_advertising cp;
>>
>> - g_dbus_client_set_disconnect_watch(client->client, NULL, NULL);
>> + if (__sync_sub_and_fetch(&client->ref_count, 1))
>> + return;
> While the idea of using refcount to protect might seem good I think we
> would be better off using mgmt_cancel for this case so the callback is
> not called, or you are not using it on purpose?
>
>> cp.instance = client->instance;
>>
>> @@ -188,10 +190,6 @@ static void client_remove(void *data)
>> client->manager->mgmt_index, sizeof(cp), &cp,
>> NULL, NULL, NULL);
>>
>> - 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");
>> @@ -199,13 +197,18 @@ static void client_remove(void *data)
>> g_dbus_emit_property_changed(btd_get_dbus_connection(),
>> adapter_get_path(client->manager->adapter),
>> LE_ADVERTISING_MGR_IFACE, "ActiveInstances");
>> +
>> + queue_remove(client->manager->clients, client);
>> +
>> + client_free(client);
>> +
>> }
>>
>> static void client_disconnect_cb(DBusConnection *conn, void *user_data)
>> {
>> DBG("Client disconnected");
>>
>> - client_remove(user_data);
>> + client_unref(user_data);
>> }
>>
>> static bool parse_type(DBusMessageIter *iter, struct btd_adv_client *client)
>> @@ -564,7 +567,7 @@ static gboolean client_timeout(void *user_data)
>> client->to_id = 0;
>>
>> client_release(client);
>> - client_remove(client);
>> + client_unref(client);
>>
>> return FALSE;
>> }
>> @@ -963,8 +966,6 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
>> mgmt_errstr(status), status);
>> reply = btd_error_failed(client->reg,
>> "Failed to register advertisement");
>> - queue_remove(client->manager->clients, client);
>> - g_idle_add(client_free_idle_cb, client);
>>
>> } else
>> reply = dbus_message_new_method_return(client->reg);
>> @@ -972,6 +973,8 @@ static void add_client_complete(struct btd_adv_client *client, uint8_t status)
>> g_dbus_send_message(btd_get_dbus_connection(), reply);
>> dbus_message_unref(client->reg);
>> client->reg = NULL;
>> +
>> + client_unref(client);
>> }
>>
>> static void add_adv_callback(uint8_t status, uint16_t length,
>> @@ -1060,8 +1063,11 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
>> }
>>
>> err = refresh_adv(client, add_adv_callback);
>> - if (!err)
>> + if (!err) {
>> + /* Hold reference to client until add_adv_callback is finished */
>> + client_ref(client);
>> return NULL;
>> + }
>>
>> fail:
>> return btd_error_failed(client->reg, "Failed to parse advertisement.");
>> @@ -1082,14 +1088,12 @@ static void client_proxy_added(GDBusProxy *proxy, void *data)
>> return;
>>
>> /* Failed to publish for some reason, remove. */
>> - queue_remove(client->manager->clients, client);
>> -
>> - g_idle_add(client_free_idle_cb, client);
>> -
>> g_dbus_send_message(btd_get_dbus_connection(), reply);
>>
>> dbus_message_unref(client->reg);
>> client->reg = NULL;
>> +
>> + client_unref(client);
>> }
>>
>> static struct btd_adv_client *client_create(struct btd_adv_manager *manager,
>> @@ -1189,6 +1193,7 @@ static DBusMessage *register_advertisement(DBusConnection *conn,
>> DBG("Registered advertisement at path %s", match.path);
>>
>> queue_push_tail(manager->clients, client);
>> + client_ref(client);
>>
>> return NULL;
>> }
>> @@ -1218,7 +1223,7 @@ static DBusMessage *unregister_advertisement(DBusConnection *conn,
>> if (!client)
>> return btd_error_does_not_exist(msg);
>>
>> - client_remove(client);
>> + client_unref(client);
>>
>> return dbus_message_new_method_return(msg);
>> }
>> --
>> 2.7.4
>>
>
--
StreamUnlimited Engineering GmbH
Levocska 9, 851 01 Bratislava, Slovakia
Office: +42 1 2 6720 0088
simon.mikuda@streamunlimited.com,www.streamunlimited.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-04 9:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 6:52 [PATCH] advertising: Fix crash when advertisement is unregistered while MGMT ADD_ADVERTISEMENT call Simon Mikuda
2019-11-01 14:49 ` Luiz Augusto von Dentz
2019-11-04 9:50 ` Simon Mikuda
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.