Re: [PATCH] advertising: Fix crash when advertisement is unregistered while MGMT ADD_ADVERTISEMENT call

[Simon Mikuda <simon.mikuda@streamunlimited.com>]

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