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: <20200821073714.19626-1-sonnysasaka@chromium.org>
In-Reply-To: <20200821073714.19626-1-sonnysasaka@chromium.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 21 Aug 2020 10:38:22 -0700
Message-ID: <CABBYNZJN0V5iPkJgbh3Xc8cF4Qdp0azcoPM7bjsto4VeVjrzFA@mail.gmail.com>
Subject: Re: [PATCH BlueZ] adapter: Fix crash in discovery_disconnect
To:     Sonny Sasaka <sonnysasaka@chromium.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Sonny,

On Fri, Aug 21, 2020 at 12:42 AM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> discovery_disconnect crashed because the adapter pointer has been freed
> before. This patch makes sure that discovery list is cleaned up before
> adapter pointer is freed.
>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
>
> ---
>  src/adapter.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 5e896a9f0..c0b02bf3f 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -5316,12 +5316,25 @@ static void free_service_auth(gpointer data, gpointer user_data)
>         g_free(auth);
>  }
>
> +static void remove_discovery_list(struct btd_adapter *adapter)
> +{
> +       g_slist_free_full(adapter->set_filter_list, discovery_free);
> +       adapter->set_filter_list = NULL;
> +
> +       g_slist_free_full(adapter->discovery_list, discovery_free);
> +       adapter->discovery_list = NULL;
> +}
> +
>  static void adapter_free(gpointer user_data)
>  {
>         struct btd_adapter *adapter = user_data;
>
>         DBG("%p", adapter);
>
> +       // Make sure the adapter's discovery list is cleaned up before freeing
> +       // the adapter.

Please use C style comments.

> +       remove_discovery_list(adapter);
> +
>         if (adapter->pairable_timeout_id > 0) {
>                 g_source_remove(adapter->pairable_timeout_id);
>                 adapter->pairable_timeout_id = 0;
> @@ -6846,11 +6859,7 @@ static void adapter_stop(struct btd_adapter *adapter)
>
>         cancel_passive_scanning(adapter);
>
> -       g_slist_free_full(adapter->set_filter_list, discovery_free);
> -       adapter->set_filter_list = NULL;
> -
> -       g_slist_free_full(adapter->discovery_list, discovery_free);
> -       adapter->discovery_list = NULL;
> +       remove_discovery_list(adapter);
>
>         discovery_cleanup(adapter, 0);
>
> --
> 2.26.2

Otherwise it looks good.

-- 
Luiz Augusto von Dentz