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 <[email protected]>
---
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
Hi Daniel,
On Fri, Aug 14, 2020 at 4:09 PM Daniel Winkler <[email protected]> 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 <[email protected]>
>
> ---
>
> 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.
--
Luiz Augusto von Dentz