Hi Daniel,
On Fri, Aug 14, 2020 at 4:09 PM Daniel Winkler <danielwinkler@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>
> ---
>
> 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
