[BlueZ,v1] adapter: Fix the unref and reset of watch_client's members

Commit Message

Miao-chen Chou June 9, 2020, 1:02 a.m. UTC

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 <[e-mail]>
Reviewed-by: Sonny Sasaka <[e-mail]>
---

 src/adapter.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Von Dentz, Luiz June 9, 2020, 9:25 p.m. UTC | #1

Hi,


On Mon, Jun 8, 2020 at 6:11 PM Von Dentz, Luiz <[e-mail]> wrote:
>
> Hi Miao,
>
> On Mon, Jun 8, 2020 at 6:03 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
>> Reviewed-by: Sonny Sasaka <[e-mail]>
>> ---
>>
>>  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.

Miao-chen Chou June 10, 2020, 10:17 p.m. UTC | #2

Hi Luiz,

For 0001-adapter-Do-not-remove-client-watch-directly-if-disco.patch,
it looks good to me.
For 0002-adapter-Consolitate-code-for-discovery-reply.patch, please
see me following comments.

+static void discovery_reply(struct watch_client *client, uint8_t status)
+{
+       DBusMessage *reply;
+
+       if (!client->msg)
+               return;
+
+       if (!status) {
It'd better to change this to "if (status == MGMT_STATUS_SUCCESS) {".
+               g_dbus_send_reply(dbus_conn, client->msg, DBUS_TYPE_INVALID);
+       } else  {
+               reply = btd_error_busy(client->msg);
+               g_dbus_send_message(dbus_conn, reply);
+       }
+
+       dbus_message_unref(client->msg);
+       client->msg = NULL;
+}
I also notice that we treated the status other than
MGMT_STATUS_SUCCESS to be busy, but this can be addressed as a
separate patch.

For 0003-adapter-Fix-possible-crash-when-stopping-discovery.patch, I
had few comments here.
(1) I didn't see the corresponding changes to pass the pointer of the
adapter as the user data when sending MGMT_OP_STOP_DISCOVERY command.
Should it be part of the patch?
(2) This does resolve the crashing due to use-after-free of a
watch_client. However, the following logic doesn't seem to be correct
to me. If you recall the call path that we discussed, which is
"client1 start_discovery() -> client1 start_discovery_complete() ->
client1 stop_discovery() -> client2 start_discovery() -> client1
detach from D-Bus which triggers discovery_disconnect() -> client1
stop_discovery_complete() -> crash)",
when client2 starts the discovery, client2 is added to
adapter->discovery_list, so once stop_discovery_complete() is called
with client1, client2 is the only client in adapter->discovery_list.
And this statement remains true even with this patch. That being said,
the following "client = adapter->discovery_list->data" would return
client2, which is not supposed to be replied by
stop_discovery_complete() issued by client1. Agree?

+       if (!adapter->discovery_list)
+               return;
+
+       client = adapter->discovery_list->data;

Thanks,
Miao

On Tue, Jun 9, 2020 at 2:25 PM Von Dentz, Luiz <[e-mail]> wrote:
>
> Hi,
>
>
> On Mon, Jun 8, 2020 at 6:11 PM Von Dentz, Luiz <[e-mail]> wrote:
> >
> > Hi Miao,
> >
> > On Mon, Jun 8, 2020 at 6:03 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
> >> Reviewed-by: Sonny Sasaka <[e-mail]>
> >> ---
> >>
> >>  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.

Von Dentz, Luiz June 11, 2020, 5:31 p.m. UTC | #3

Hi Miao,

On Wed, Jun 10, 2020 at 3:18 PM Miao-chen Chou <[e-mail]> wrote:
>
> Hi Luiz,
>
> For 0001-adapter-Do-not-remove-client-watch-directly-if-disco.patch,
> it looks good to me.
> For 0002-adapter-Consolitate-code-for-discovery-reply.patch, please
> see me following comments.
>
> +static void discovery_reply(struct watch_client *client, uint8_t status)
> +{
> +       DBusMessage *reply;
> +
> +       if (!client->msg)
> +               return;
> +
> +       if (!status) {
> It'd better to change this to "if (status == MGMT_STATUS_SUCCESS) {".
> +               g_dbus_send_reply(dbus_conn, client->msg, DBUS_TYPE_INVALID);
> +       } else  {
> +               reply = btd_error_busy(client->msg);
> +               g_dbus_send_message(dbus_conn, reply);
> +       }
> +
> +       dbus_message_unref(client->msg);
> +       client->msg = NULL;
> +}
> I also notice that we treated the status other than
> MGMT_STATUS_SUCCESS to be busy, but this can be addressed as a
> separate patch.

Wasn't that the error we were generating before? Afaik both start and
stop discovery were doing the same in this regard.

> For 0003-adapter-Fix-possible-crash-when-stopping-discovery.patch, I
> had few comments here.
> (1) I didn't see the corresponding changes to pass the pointer of the
> adapter as the user data when sending MGMT_OP_STOP_DISCOVERY command.
> Should it be part of the patch?

Yep, it should be fixed now.

> (2) This does resolve the crashing due to use-after-free of a
> watch_client. However, the following logic doesn't seem to be correct
> to me. If you recall the call path that we discussed, which is
> "client1 start_discovery() -> client1 start_discovery_complete() ->
> client1 stop_discovery() -> client2 start_discovery() -> client1
> detach from D-Bus which triggers discovery_disconnect() -> client1
> stop_discovery_complete() -> crash)",
> when client2 starts the discovery, client2 is added to
> adapter->discovery_list, so once stop_discovery_complete() is called
> with client1, client2 is the only client in adapter->discovery_list.
> And this statement remains true even with this patch. That being said,
> the following "client = adapter->discovery_list->data" would return
> client2, which is not supposed to be replied by
> stop_discovery_complete() issued by client1. Agree?

Yep, that will need tracking of the client who initiated the
operation, I will send a patch for that later today.

> +       if (!adapter->discovery_list)
> +               return;
> +
> +       client = adapter->discovery_list->data;
>
> Thanks,
> Miao
>
> On Tue, Jun 9, 2020 at 2:25 PM Von Dentz, Luiz <[e-mail]> wrote:
> >
> > Hi,
> >
> >
> > On Mon, Jun 8, 2020 at 6:11 PM Von Dentz, Luiz <[e-mail]> wrote:
> > >
> > > Hi Miao,
> > >
> > > On Mon, Jun 8, 2020 at 6:03 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
> > >> Reviewed-by: Sonny Sasaka <[e-mail]>
> > >> ---
> > >>
> > >>  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.

Luiz Augusto von Dentz June 11, 2020, 9:01 p.m. UTC | #4

Hi Miao,

On Thu, Jun 11, 2020 at 12:18 PM Von Dentz, Luiz
<[e-mail]> wrote:
>
> Hi Miao,
>
> On Wed, Jun 10, 2020 at 3:18 PM Miao-chen Chou <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> > For 0001-adapter-Do-not-remove-client-watch-directly-if-disco.patch,
> > it looks good to me.
> > For 0002-adapter-Consolitate-code-for-discovery-reply.patch, please
> > see me following comments.
> >
> > +static void discovery_reply(struct watch_client *client, uint8_t status)
> > +{
> > +       DBusMessage *reply;
> > +
> > +       if (!client->msg)
> > +               return;
> > +
> > +       if (!status) {
> > It'd better to change this to "if (status == MGMT_STATUS_SUCCESS) {".
> > +               g_dbus_send_reply(dbus_conn, client->msg, DBUS_TYPE_INVALID);
> > +       } else  {
> > +               reply = btd_error_busy(client->msg);
> > +               g_dbus_send_message(dbus_conn, reply);
> > +       }
> > +
> > +       dbus_message_unref(client->msg);
> > +       client->msg = NULL;
> > +}
> > I also notice that we treated the status other than
> > MGMT_STATUS_SUCCESS to be busy, but this can be addressed as a
> > separate patch.
>
> Wasn't that the error we were generating before? Afaik both start and
> stop discovery were doing the same in this regard.
>
> > For 0003-adapter-Fix-possible-crash-when-stopping-discovery.patch, I
> > had few comments here.
> > (1) I didn't see the corresponding changes to pass the pointer of the
> > adapter as the user data when sending MGMT_OP_STOP_DISCOVERY command.
> > Should it be part of the patch?
>
> Yep, it should be fixed now.
>
> > (2) This does resolve the crashing due to use-after-free of a
> > watch_client. However, the following logic doesn't seem to be correct
> > to me. If you recall the call path that we discussed, which is
> > "client1 start_discovery() -> client1 start_discovery_complete() ->
> > client1 stop_discovery() -> client2 start_discovery() -> client1
> > detach from D-Bus which triggers discovery_disconnect() -> client1
> > stop_discovery_complete() -> crash)",
> > when client2 starts the discovery, client2 is added to
> > adapter->discovery_list, so once stop_discovery_complete() is called
> > with client1, client2 is the only client in adapter->discovery_list.
> > And this statement remains true even with this patch. That being said,
> > the following "client = adapter->discovery_list->data" would return
> > client2, which is not supposed to be replied by
> > stop_discovery_complete() issued by client1. Agree?
>
> Yep, that will need tracking of the client who initiated the
> operation, I will send a patch for that later today.

Ive pushed a couple more fixes to track better the clients:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=227cfdf8e01f639f74b36b179f8ccf9a2061e932

I was not able to reproduce this race condition anymore, although I
was only able to reproduce when doing start discovery and quitting
bluetoothctl before the response.

> > +       if (!adapter->discovery_list)
> > +               return;
> > +
> > +       client = adapter->discovery_list->data;
> >
> > Thanks,
> > Miao
> >
> > On Tue, Jun 9, 2020 at 2:25 PM Von Dentz, Luiz <[e-mail]> wrote:
> > >
> > > Hi,
> > >
> > >
> > > On Mon, Jun 8, 2020 at 6:11 PM Von Dentz, Luiz <[e-mail]> wrote:
> > > >
> > > > Hi Miao,
> > > >
> > > > On Mon, Jun 8, 2020 at 6:03 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
> > > >> Reviewed-by: Sonny Sasaka <[e-mail]>
> > > >> ---
> > > >>
> > > >>  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.

Miao-chen Chou June 12, 2020, 5:29 a.m. UTC | #5

Hi Luiz,

Thanks for the update! Feel free to drop this patch if you've verified
it working. Here is how I verified my original changes for unref and
reset.
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.

For the use-after_free crash, I will take a look at the new patches.

Regards,
Miao

On Thu, Jun 11, 2020 at 2:02 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Miao,
>
> On Thu, Jun 11, 2020 at 12:18 PM Von Dentz, Luiz
> <[e-mail]> wrote:
> >
> > Hi Miao,
> >
> > On Wed, Jun 10, 2020 at 3:18 PM Miao-chen Chou <[e-mail]> wrote:
> > >
> > > Hi Luiz,
> > >
> > > For 0001-adapter-Do-not-remove-client-watch-directly-if-disco.patch,
> > > it looks good to me.
> > > For 0002-adapter-Consolitate-code-for-discovery-reply.patch, please
> > > see me following comments.
> > >
> > > +static void discovery_reply(struct watch_client *client, uint8_t status)
> > > +{
> > > +       DBusMessage *reply;
> > > +
> > > +       if (!client->msg)
> > > +               return;
> > > +
> > > +       if (!status) {
> > > It'd better to change this to "if (status == MGMT_STATUS_SUCCESS) {".
> > > +               g_dbus_send_reply(dbus_conn, client->msg, DBUS_TYPE_INVALID);
> > > +       } else  {
> > > +               reply = btd_error_busy(client->msg);
> > > +               g_dbus_send_message(dbus_conn, reply);
> > > +       }
> > > +
> > > +       dbus_message_unref(client->msg);
> > > +       client->msg = NULL;
> > > +}
> > > I also notice that we treated the status other than
> > > MGMT_STATUS_SUCCESS to be busy, but this can be addressed as a
> > > separate patch.
> >
> > Wasn't that the error we were generating before? Afaik both start and
> > stop discovery were doing the same in this regard.
> >
> > > For 0003-adapter-Fix-possible-crash-when-stopping-discovery.patch, I
> > > had few comments here.
> > > (1) I didn't see the corresponding changes to pass the pointer of the
> > > adapter as the user data when sending MGMT_OP_STOP_DISCOVERY command.
> > > Should it be part of the patch?
> >
> > Yep, it should be fixed now.
> >
> > > (2) This does resolve the crashing due to use-after-free of a
> > > watch_client. However, the following logic doesn't seem to be correct
> > > to me. If you recall the call path that we discussed, which is
> > > "client1 start_discovery() -> client1 start_discovery_complete() ->
> > > client1 stop_discovery() -> client2 start_discovery() -> client1
> > > detach from D-Bus which triggers discovery_disconnect() -> client1
> > > stop_discovery_complete() -> crash)",
> > > when client2 starts the discovery, client2 is added to
> > > adapter->discovery_list, so once stop_discovery_complete() is called
> > > with client1, client2 is the only client in adapter->discovery_list.
> > > And this statement remains true even with this patch. That being said,
> > > the following "client = adapter->discovery_list->data" would return
> > > client2, which is not supposed to be replied by
> > > stop_discovery_complete() issued by client1. Agree?
> >
> > Yep, that will need tracking of the client who initiated the
> > operation, I will send a patch for that later today.
>
> Ive pushed a couple more fixes to track better the clients:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=227cfdf8e01f639f74b36b179f8ccf9a2061e932
>
> I was not able to reproduce this race condition anymore, although I
> was only able to reproduce when doing start discovery and quitting
> bluetoothctl before the response.
>
> > > +       if (!adapter->discovery_list)
> > > +               return;
> > > +
> > > +       client = adapter->discovery_list->data;
> > >
> > > Thanks,
> > > Miao
> > >
> > > On Tue, Jun 9, 2020 at 2:25 PM Von Dentz, Luiz <[e-mail]> wrote:
> > > >
> > > > Hi,
> > > >
> > > >
> > > > On Mon, Jun 8, 2020 at 6:11 PM Von Dentz, Luiz <[e-mail]> wrote:
> > > > >
> > > > > Hi Miao,
> > > > >
> > > > > On Mon, Jun 8, 2020 at 6:03 PM Miao-chen Chou <[e-mail]> 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 <[e-mail]>
> > > > >> Reviewed-by: Sonny Sasaka <[e-mail]>
> > > > >> ---
> > > > >>
> > > > >>  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.
>
>
>
> --
> Luiz Augusto von Dentz

Patch

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);
+		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;
 	}