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: <20200608180241.BlueZ.v1.1.Ibf8331f6c835d53fe7ca978de962f93981573d9a@changeid>
 <CACumGOKqrBQJzYt_ayW6KRmu9QKCCPYiojdozvGwO3yTR-2Jyg@mail.gmail.com>
In-Reply-To: <CACumGOKqrBQJzYt_ayW6KRmu9QKCCPYiojdozvGwO3yTR-2Jyg@mail.gmail.com>
From:   "Von Dentz, Luiz" <luiz.von.dentz@intel.com>
Date:   Tue, 9 Jun 2020 14:25:19 -0700
Message-ID: <CACumGOKyAfdgP6t4PnNBzmVmFayV4b3gPOjux3aGg_de2T104g@mail.gmail.com>
Subject: Re: [BlueZ PATCH v1] adapter: Fix the unref and reset of
 watch_client's members
To:     Miao-chen Chou <mcchou@chromium.org>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Michael Sun <michaelfsun@google.com>,
        Alain Michaud <alainm@chromium.org>,
        Yoni Shavit <yshavit@chromium.org>,
        Sonny Sasaka <sonnysasaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi,


On Mon, Jun 8, 2020 at 6:11 PM Von Dentz, Luiz <luiz.von.dentz@intel.com> wrote:
>
> Hi Miao,
>
> On Mon, Jun 8, 2020 at 6:03 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>>
>> This properly handles the unref of client->msg in
>> stop_discovery_complete() and the reset of it. This also handles the unref
>> of client->msg, the reset of client->watch and the reset of client->msg in
>> start_discovery_complete().
>>
>> The following test was performed:
>> (1) Intentionally changed the MGMT status other than MGMT_STATUS_SUCCESS
>> in stop_discovery_complete() and start_discovery_complete() and built
>> bluetoothd.
>> (2) In bluetoothctl console, issued scan on/scan off to invoke
>> StartDiscovery and verified that new discovery requests can be processed.
>>
>> Reviewed-by: Alain Michaud <alainm@chromium.org>
>> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
>> ---
>>
>>  src/adapter.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 76acfea70..0857a3115 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -1652,6 +1652,9 @@ fail:
>>                 reply = btd_error_busy(client->msg);
>>                 g_dbus_send_message(dbus_conn, reply);
>>                 g_dbus_remove_watch(dbus_conn, client->watch);
>
>
> We shouldn't be removing the watch directly since the client may have registered filters so we let discovery_remove do it by calling discovery_free if necessary.
>
>>
>> +               client->watch = 0;
>> +               dbus_message_unref(client->msg);
>> +               client->msg = NULL;
>>                 discovery_remove(client, false);
>>                 return;
>>         }
>> @@ -1926,6 +1929,8 @@ static void stop_discovery_complete(uint8_t status, uint16_t length,
>>                 if (client->msg) {
>>                         reply = btd_error_busy(client->msg);
>>                         g_dbus_send_message(dbus_conn, reply);
>> +                       dbus_message_unref(client->msg);
>> +                       client->msg = NULL;
>>                 }
>>                 goto done;
>>         }
>> --
>> 2.26.2
>
>
> Ive sent similar fixes upstream, let me attach them here just in case.

Any comments on these changes, I would like to push them as soon as possible.