Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200814155807.Bluez.1.Ia90c97ad9ec0b51b7aaae1216864e8379c1470d5@changeid>
In-Reply-To: <20200814155807.Bluez.1.Ia90c97ad9ec0b51b7aaae1216864e8379c1470d5@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 17 Aug 2020 10:34:58 -0700
Message-ID: <CABBYNZKrRGb7FkCVOix8jmNwr94BQNXrS8U+a8VRBNxHoQ2ArA@mail.gmail.com>
Subject: Re: [Bluez PATCH] advertising: Fix dbus response for over-advertising
To:     Daniel Winkler <danielwinkler@google.com>
Cc:     Simon Mikuda <simon.mikuda@streamunlimited.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Sonny Sasaka <sonnysasaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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.

-- 
Luiz Augusto von Dentz