| Message ID | 20200814155807.Bluez.1.Ia90c97ad9ec0b51b7aaae1216864e8379c1470d5@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [Bluez] advertising: Fix dbus response for over-advertising | expand |
Hi Daniel, On Fri, Aug 14, 2020 at 4:09 PM Daniel Winkler <danielwinkler@google.com> wrote: > > client_free would always send a dbus method_return to fix the case where > a request to Unregister occurred before the MGMT call returned. However, > in the code path where too many advertisements are registered, this > method_return prevents the failure from being sent properly. This patch > makes sure the reference to the register_advertisement DBusMessage is > not stored in the client structure until the end of > register_advertisement. This ensures that we only respond once, either > in register_advertisement or in client_free, not both. > > It also changes the dbus response in the fast unregister_advertisement > case from a method_return to a btd_error_failed, since the registration > was never allowed to complete, and thus was not successful. > > The patch was tested in the following ways: > > To verify it did not break the segfault fix in > caff2b48ca54bbc57b5da3f63317767489aa5b48, I repro'd the failure by > quickly unregistering after registering, and verified that the segfault > is still fixed with this change. > > Ran through our automated tests that register too many advertisements > and verify that the registration fails with the intended "Maximum > Advertisements Reached" error response. > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > --- > > src/advertising.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/advertising.c b/src/advertising.c > index 076d591b6..e5f25948d 100644 > --- a/src/advertising.c > +++ b/src/advertising.c > @@ -119,9 +119,13 @@ static void client_free(void *data) > } > > if (client->reg) { > - g_dbus_send_message(btd_get_dbus_connection(), > - dbus_message_new_method_return(client->reg)); > + DBusMessage *reply; > + > + reply = btd_error_failed(client->reg, > + "Failed to complete registration"); > + g_dbus_send_message(btd_get_dbus_connection(), reply); > dbus_message_unref(client->reg); > + client->reg = NULL; > } > > if (client->add_adv_id) > @@ -1152,8 +1156,6 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, > g_dbus_client_set_proxy_handlers(client->client, client_proxy_added, > NULL, NULL, client); > > - client->reg = dbus_message_ref(msg); > - > client->data = bt_ad_new(); > if (!client->data) > goto fail; > @@ -1216,6 +1218,8 @@ static DBusMessage *register_advertisement(DBusConnection *conn, > > DBG("Registered advertisement at path %s", match.path); > > + client->reg = dbus_message_ref(msg); > + > queue_push_tail(manager->clients, client); > > return NULL; > -- > 2.28.0.220.ged08abb693-goog Applied, thanks.
diff --git a/src/advertising.c b/src/advertising.c index 076d591b6..e5f25948d 100644 --- a/src/advertising.c +++ b/src/advertising.c @@ -119,9 +119,13 @@ static void client_free(void *data) } if (client->reg) { - g_dbus_send_message(btd_get_dbus_connection(), - dbus_message_new_method_return(client->reg)); + DBusMessage *reply; + + reply = btd_error_failed(client->reg, + "Failed to complete registration"); + g_dbus_send_message(btd_get_dbus_connection(), reply); dbus_message_unref(client->reg); + client->reg = NULL; } if (client->add_adv_id) @@ -1152,8 +1156,6 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager, g_dbus_client_set_proxy_handlers(client->client, client_proxy_added, NULL, NULL, client); - client->reg = dbus_message_ref(msg); - client->data = bt_ad_new(); if (!client->data) goto fail; @@ -1216,6 +1218,8 @@ static DBusMessage *register_advertisement(DBusConnection *conn, DBG("Registered advertisement at path %s", match.path); + client->reg = dbus_message_ref(msg); + queue_push_tail(manager->clients, client); return NULL;
client_free would always send a dbus method_return to fix the case where a request to Unregister occurred before the MGMT call returned. However, in the code path where too many advertisements are registered, this method_return prevents the failure from being sent properly. This patch makes sure the reference to the register_advertisement DBusMessage is not stored in the client structure until the end of register_advertisement. This ensures that we only respond once, either in register_advertisement or in client_free, not both. It also changes the dbus response in the fast unregister_advertisement case from a method_return to a btd_error_failed, since the registration was never allowed to complete, and thus was not successful. The patch was tested in the following ways: To verify it did not break the segfault fix in caff2b48ca54bbc57b5da3f63317767489aa5b48, I repro'd the failure by quickly unregistering after registering, and verified that the segfault is still fixed with this change. Ran through our automated tests that register too many advertisements and verify that the registration fails with the intended "Maximum Advertisements Reached" error response. Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> --- src/advertising.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)