[PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 2911 bytes --]

Even for outgoing pairing requests we may receive the UUIDs property
changed after the device is paired and try to register it twice.

The easiest way to reproduce this is when Extended Inquiry Response is
supported.

When the device is paired, we receive the "Paired" PropertyChanged,
inside modem_register_from_proxy(), g_dbus_proxy_get_property() gets
the UUIDs extracted from the EIR data.  Later, when the service
discovery is finished, the UUIDs property is re-sent and both may
contain the HFP AG UUID.

Valgrind log:

ofonod[8157]: src/modem.c:ofono_modem_create() name: hfp/org/bluez/hci0/dev_40_98_4E_32_D7_39, type: hfp
ofonod[8157]: src/modem.c:set_modem_property() modem 0x66a2db0 property Remote
ofonod[8157]: src/modem.c:set_modem_property() modem 0x66a2db0 property DevicePath
ofonod[8157]: src/modem.c:ofono_modem_register() 0x66a2db0
ofonod[8157]: plugins/hfp_hf_bluez5.c:hfp_probe() modem: 0x66a2db0
ofonod[8157]: Modem register failed on path /hfp/org/bluez/hci0/dev_40_98_4E_32_D7_39
ofonod[8157]: plugins/hfp_hf_bluez5.c:hfp_remove() modem: 0x66a2db0
ofonod[8157]: plugins/hfp_hf_bluez5.c:profile_new_connection() Profile handler NewConnection
ofonod[8157]: src/modem.c:get_modem_property() modem 0x66a2db0 property DevicePath
==8157== Invalid write of size 4
==8157==    at 0x4712A5: hfp_slc_info_init (slc.c:59)
==8157==    by 0x486B00: profile_new_connection (hfp_hf_bluez5.c:168)
==8157==    by 0x412C70: process_message.isra.4 (object.c:258)
==8157==    by 0x5381984: _dbus_object_tree_dispatch_and_unlock (in /usr/lib64/libdbus-1.so.3.7.2)
==8157==    by 0x5373C4F: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.7.2)
==8157==    by 0x4107B7: message_dispatch (mainloop.c:76)
==8157==    by 0x5089BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==8157==    by 0x5089044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==8157==    by 0x5089377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
==8157==    by 0x5089771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
==8157==    by 0x41042B: main (main.c:249)
==8157==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
==8157==
---
 plugins/hfp_hf_bluez5.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 2b9275b..86d8c72 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -713,6 +713,12 @@ static void modem_register_from_proxy(GDBusProxy *proxy, const char *path)
 	if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE)
 		return;
 
+	modem = ofono_modem_find(device_path_compare, (void *) path);
+	if (modem) {
+		DBG("Modem for device %s already registered", path);
+		return;
+	}
+
 	dbus_message_iter_get_basic(&iter, &remote);
 
 	modem = modem_register(path, remote, alias);
-- 
1.8.2.1

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

Hi Vinicius,

On 04/22/2013 05:53 PM, Vinicius Costa Gomes wrote:
> Even for outgoing pairing requests we may receive the UUIDs property
> changed after the device is paired and try to register it twice.
>
> The easiest way to reproduce this is when Extended Inquiry Response is
> supported.
>
> When the device is paired, we receive the "Paired" PropertyChanged,
> inside modem_register_from_proxy(), g_dbus_proxy_get_property() gets
> the UUIDs extracted from the EIR data.  Later, when the service
> discovery is finished, the UUIDs property is re-sent and both may
> contain the HFP AG UUID.

My sources indicated to me that BlueZ should perform the SDP query 
first, and then signal Paired.  Is this something we can not count on or 
is this an implementation issue inside BlueZ itself?

Regards,
-Denis

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

Hi Denis,

On Tue, Apr 23, 2013, Denis Kenzior wrote:
> On 04/22/2013 05:53 PM, Vinicius Costa Gomes wrote:
> >Even for outgoing pairing requests we may receive the UUIDs property
> >changed after the device is paired and try to register it twice.
> >
> >The easiest way to reproduce this is when Extended Inquiry Response is
> >supported.
> >
> >When the device is paired, we receive the "Paired" PropertyChanged,
> >inside modem_register_from_proxy(), g_dbus_proxy_get_property() gets
> >the UUIDs extracted from the EIR data.  Later, when the service
> >discovery is finished, the UUIDs property is re-sent and both may
> >contain the HFP AG UUID.
> 
> My sources indicated to me that BlueZ should perform the SDP query
> first, and then signal Paired.  Is this something we can not count
> on or is this an implementation issue inside BlueZ itself?

BlueZ has always done pairing first and only then SDP. This is because
there are security mode 3 devices out there that do not permit any kind
of connection before pairing has been completed (i.e. even if we wanted
to do SDP first we can't with them). That said, we do at least delay the
NewConnection() callback until SDP has been completed.

Johan

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

Hi Johan,

>>> Even for outgoing pairing requests we may receive the UUIDs property
>>> changed after the device is paired and try to register it twice.
>>> 
>>> The easiest way to reproduce this is when Extended Inquiry Response is
>>> supported.
>>> 
>>> When the device is paired, we receive the "Paired" PropertyChanged,
>>> inside modem_register_from_proxy(), g_dbus_proxy_get_property() gets
>>> the UUIDs extracted from the EIR data.  Later, when the service
>>> discovery is finished, the UUIDs property is re-sent and both may
>>> contain the HFP AG UUID.
>> 
>> My sources indicated to me that BlueZ should perform the SDP query
>> first, and then signal Paired.  Is this something we can not count
>> on or is this an implementation issue inside BlueZ itself?
> 
> BlueZ has always done pairing first and only then SDP. This is because
> there are security mode 3 devices out there that do not permit any kind
> of connection before pairing has been completed (i.e. even if we wanted
> to do SDP first we can't with them). That said, we do at least delay the
> NewConnection() callback until SDP has been completed.

so when are we sending "Paired" property? We might have to delay that property update until SDP finished.

We have to make sure that we finished SDP after the pairing before updating any clients with property changes.

Regards

Marcel

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

Hi Marcel,

On Tue, Apr 23, 2013, Marcel Holtmann wrote:
> >>> Even for outgoing pairing requests we may receive the UUIDs property
> >>> changed after the device is paired and try to register it twice.
> >>> 
> >>> The easiest way to reproduce this is when Extended Inquiry Response is
> >>> supported.
> >>> 
> >>> When the device is paired, we receive the "Paired" PropertyChanged,
> >>> inside modem_register_from_proxy(), g_dbus_proxy_get_property() gets
> >>> the UUIDs extracted from the EIR data.  Later, when the service
> >>> discovery is finished, the UUIDs property is re-sent and both may
> >>> contain the HFP AG UUID.
> >> 
> >> My sources indicated to me that BlueZ should perform the SDP query
> >> first, and then signal Paired.  Is this something we can not count
> >> on or is this an implementation issue inside BlueZ itself?
> > 
> > BlueZ has always done pairing first and only then SDP. This is because
> > there are security mode 3 devices out there that do not permit any kind
> > of connection before pairing has been completed (i.e. even if we wanted
> > to do SDP first we can't with them). That said, we do at least delay the
> > NewConnection() callback until SDP has been completed.
> 
> so when are we sending "Paired" property? We might have to delay that
> property update until SDP finished.

Right now it's directly bound to when the device is actually paired,
i.e. ignoring any other ongoing operations such as SDP.

> We have to make sure that we finished SDP after the pairing before
> updating any clients with property changes.

I don't completely understand the rationale of wanting to make BlueZ's
clients so fragile. You could in theory get new services way after
pairing if this is configurable on the remote side. You could also get
the result of calling Device1.Connect() instead of Device1.Pair() in
BlueZ.

On the other hand, we do delay the response to Device1.Pair() until SDP
is complete so delaying the Paired property would in a way be consistent
with that. I'll look into how simple this would be. We already track the
completion of SDP for each device internally with a svc_resolved boolean
so probably a new flag to indicate that "Paired" still needs to be
emitted should be all the extra context tracking we need.

Johan

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

Hi Johan,

>>>>> Even for outgoing pairing requests we may receive the UUIDs property
>>>>> changed after the device is paired and try to register it twice.
>>>>> 
>>>>> The easiest way to reproduce this is when Extended Inquiry Response is
>>>>> supported.
>>>>> 
>>>>> When the device is paired, we receive the "Paired" PropertyChanged,
>>>>> inside modem_register_from_proxy(), g_dbus_proxy_get_property() gets
>>>>> the UUIDs extracted from the EIR data.  Later, when the service
>>>>> discovery is finished, the UUIDs property is re-sent and both may
>>>>> contain the HFP AG UUID.
>>>> 
>>>> My sources indicated to me that BlueZ should perform the SDP query
>>>> first, and then signal Paired.  Is this something we can not count
>>>> on or is this an implementation issue inside BlueZ itself?
>>> 
>>> BlueZ has always done pairing first and only then SDP. This is because
>>> there are security mode 3 devices out there that do not permit any kind
>>> of connection before pairing has been completed (i.e. even if we wanted
>>> to do SDP first we can't with them). That said, we do at least delay the
>>> NewConnection() callback until SDP has been completed.
>> 
>> so when are we sending "Paired" property? We might have to delay that
>> property update until SDP finished.
> 
> Right now it's directly bound to when the device is actually paired,
> i.e. ignoring any other ongoing operations such as SDP.
> 
>> We have to make sure that we finished SDP after the pairing before
>> updating any clients with property changes.
> 
> I don't completely understand the rationale of wanting to make BlueZ's
> clients so fragile. You could in theory get new services way after
> pairing if this is configurable on the remote side. You could also get
> the result of calling Device1.Connect() instead of Device1.Pair() in
> BlueZ.
> 
> On the other hand, we do delay the response to Device1.Pair() until SDP
> is complete so delaying the Paired property would in a way be consistent
> with that. I'll look into how simple this would be. We already track the
> completion of SDP for each device internally with a svc_resolved boolean
> so probably a new flag to indicate that "Paired" still needs to be
> emitted should be all the extra context tracking we need.

the pairing procedure is special and we should try to avoid any pointless round trips or wake ups for the clients here. We do know that we are running SDP after pairing anyway. So lets just wait for it to finish. Especially if we do that already anyway for Device1.Pair().

Regards

Marcel

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

Hi Marcel,

On Tue, Apr 23, 2013, Marcel Holtmann wrote:
> > I don't completely understand the rationale of wanting to make BlueZ's
> > clients so fragile. You could in theory get new services way after
> > pairing if this is configurable on the remote side. You could also get
> > the result of calling Device1.Connect() instead of Device1.Pair() in
> > BlueZ.
> > 
> > On the other hand, we do delay the response to Device1.Pair() until SDP
> > is complete so delaying the Paired property would in a way be consistent
> > with that. I'll look into how simple this would be. We already track the
> > completion of SDP for each device internally with a svc_resolved boolean
> > so probably a new flag to indicate that "Paired" still needs to be
> > emitted should be all the extra context tracking we need.
> 
> the pairing procedure is special and we should try to avoid any
> pointless round trips or wake ups for the clients here. We do know
> that we are running SDP after pairing anyway. So lets just wait for it
> to finish. Especially if we do that already anyway for Device1.Pair().

This ended up being quite simple to do, so I just pushed a mostly
untested patch for this to bluez.git. It'd be good if someone could
verify that it actually ends up helping the situation with oFono.

Johan

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 1936 bytes --]

Hi,

On 20:28 Tue 23 Apr, Johan Hedberg wrote:
> Hi Marcel,
> 
> On Tue, Apr 23, 2013, Marcel Holtmann wrote:
> > > I don't completely understand the rationale of wanting to make BlueZ's
> > > clients so fragile. You could in theory get new services way after
> > > pairing if this is configurable on the remote side. You could also get
> > > the result of calling Device1.Connect() instead of Device1.Pair() in
> > > BlueZ.
> > > 
> > > On the other hand, we do delay the response to Device1.Pair() until SDP
> > > is complete so delaying the Paired property would in a way be consistent
> > > with that. I'll look into how simple this would be. We already track the
> > > completion of SDP for each device internally with a svc_resolved boolean
> > > so probably a new flag to indicate that "Paired" still needs to be
> > > emitted should be all the extra context tracking we need.
> > 
> > the pairing procedure is special and we should try to avoid any
> > pointless round trips or wake ups for the clients here. We do know
> > that we are running SDP after pairing anyway. So lets just wait for it
> > to finish. Especially if we do that already anyway for Device1.Pair().
> 
> This ended up being quite simple to do, so I just pushed a mostly
> untested patch for this to bluez.git. It'd be good if someone could
> verify that it actually ends up helping the situation with oFono.

This indeed helps with the issue of receiving UUIDs property after the
Paired. Thanks.

It should be noted that we have BlueZ releases without this fix, that can
crash oFono, it may be worth considering this patch for this reason.

The issue with NewConnection() arriving before the "Paired" property changed
signal remains.

> 
> Johan
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono


Cheers,
-- 
Vinicius

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

Hi Johan, Vinicius,

On 04/25/2013 04:37 AM, Johan Hedberg wrote:
> Hi Mikel,
>
> On Thu, Apr 25, 2013, Mikel Astiz wrote:
>> On Tue, Apr 23, 2013 at 7:28 PM, Johan Hedberg<johan.hedberg@gmail.com>  wrote:
>>> Hi Marcel,
>>>
>>> On Tue, Apr 23, 2013, Marcel Holtmann wrote:
>>>>> I don't completely understand the rationale of wanting to make BlueZ's
>>>>> clients so fragile. You could in theory get new services way after
>>>>> pairing if this is configurable on the remote side. You could also get
>>>>> the result of calling Device1.Connect() instead of Device1.Pair() in
>>>>> BlueZ.
>>>>>
>>>>> On the other hand, we do delay the response to Device1.Pair() until SDP
>>>>> is complete so delaying the Paired property would in a way be consistent
>>>>> with that. I'll look into how simple this would be. We already track the
>>>>> completion of SDP for each device internally with a svc_resolved boolean
>>>>> so probably a new flag to indicate that "Paired" still needs to be
>>>>> emitted should be all the extra context tracking we need.
>>>>
>>>> the pairing procedure is special and we should try to avoid any
>>>> pointless round trips or wake ups for the clients here. We do know
>>>> that we are running SDP after pairing anyway. So lets just wait for it
>>>> to finish. Especially if we do that already anyway for Device1.Pair().
>>>
>>> This ended up being quite simple to do, so I just pushed a mostly
>>> untested patch for this to bluez.git. It'd be good if someone could
>>> verify that it actually ends up helping the situation with oFono.
>>>
>>
>> How do remotely-initiated connections fit into this?
>>
>> We've seen devices that try to connect profiles immediately after
>> pairing, I believe before the discovery is complete.
>
> Both the NewConnection() (including the authorization of the incoming
> L2CAP/RFCOMM connect request) and the Paired property would even then be
> delayed until our side has completed its full service discovery.
>

So should we be reverting commit 456b8c9723b9b73c3ea4cadc8c6f84ca90675f9a?

Regards,
-Denis

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Mikel Astiz]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

Hi Johan, Marcel,

On Tue, Apr 23, 2013 at 7:28 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Marcel,
>
> On Tue, Apr 23, 2013, Marcel Holtmann wrote:
>> > I don't completely understand the rationale of wanting to make BlueZ's
>> > clients so fragile. You could in theory get new services way after
>> > pairing if this is configurable on the remote side. You could also get
>> > the result of calling Device1.Connect() instead of Device1.Pair() in
>> > BlueZ.
>> >
>> > On the other hand, we do delay the response to Device1.Pair() until SDP
>> > is complete so delaying the Paired property would in a way be consistent
>> > with that. I'll look into how simple this would be. We already track the
>> > completion of SDP for each device internally with a svc_resolved boolean
>> > so probably a new flag to indicate that "Paired" still needs to be
>> > emitted should be all the extra context tracking we need.
>>
>> the pairing procedure is special and we should try to avoid any
>> pointless round trips or wake ups for the clients here. We do know
>> that we are running SDP after pairing anyway. So lets just wait for it
>> to finish. Especially if we do that already anyway for Device1.Pair().
>
> This ended up being quite simple to do, so I just pushed a mostly
> untested patch for this to bluez.git. It'd be good if someone could
> verify that it actually ends up helping the situation with oFono.
>

How do remotely-initiated connections fit into this?

We've seen devices that try to connect profiles immediately after
pairing, I believe before the discovery is complete.

Cheers,
Mikel

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]

Hi Mikel,

On Thu, Apr 25, 2013, Mikel Astiz wrote:
> On Tue, Apr 23, 2013 at 7:28 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > Hi Marcel,
> >
> > On Tue, Apr 23, 2013, Marcel Holtmann wrote:
> >> > I don't completely understand the rationale of wanting to make BlueZ's
> >> > clients so fragile. You could in theory get new services way after
> >> > pairing if this is configurable on the remote side. You could also get
> >> > the result of calling Device1.Connect() instead of Device1.Pair() in
> >> > BlueZ.
> >> >
> >> > On the other hand, we do delay the response to Device1.Pair() until SDP
> >> > is complete so delaying the Paired property would in a way be consistent
> >> > with that. I'll look into how simple this would be. We already track the
> >> > completion of SDP for each device internally with a svc_resolved boolean
> >> > so probably a new flag to indicate that "Paired" still needs to be
> >> > emitted should be all the extra context tracking we need.
> >>
> >> the pairing procedure is special and we should try to avoid any
> >> pointless round trips or wake ups for the clients here. We do know
> >> that we are running SDP after pairing anyway. So lets just wait for it
> >> to finish. Especially if we do that already anyway for Device1.Pair().
> >
> > This ended up being quite simple to do, so I just pushed a mostly
> > untested patch for this to bluez.git. It'd be good if someone could
> > verify that it actually ends up helping the situation with oFono.
> >
> 
> How do remotely-initiated connections fit into this?
> 
> We've seen devices that try to connect profiles immediately after
> pairing, I believe before the discovery is complete.

Both the NewConnection() (including the authorization of the incoming
L2CAP/RFCOMM connect request) and the Paired property would even then be
delayed until our side has completed its full service discovery.

Johan

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

Hi Denis,

On 03:27 Thu 25 Apr, Denis Kenzior wrote:
> Hi Johan, Vinicius,
> 
> On 04/25/2013 04:37 AM, Johan Hedberg wrote:
> >Hi Mikel,
> >
> >On Thu, Apr 25, 2013, Mikel Astiz wrote:
> >>On Tue, Apr 23, 2013 at 7:28 PM, Johan Hedberg<johan.hedberg@gmail.com>  wrote:
> >>>Hi Marcel,
> >>>
> >>>On Tue, Apr 23, 2013, Marcel Holtmann wrote:
> >>>>>I don't completely understand the rationale of wanting to make BlueZ's
> >>>>>clients so fragile. You could in theory get new services way after
> >>>>>pairing if this is configurable on the remote side. You could also get
> >>>>>the result of calling Device1.Connect() instead of Device1.Pair() in
> >>>>>BlueZ.
> >>>>>
> >>>>>On the other hand, we do delay the response to Device1.Pair() until SDP
> >>>>>is complete so delaying the Paired property would in a way be consistent
> >>>>>with that. I'll look into how simple this would be. We already track the
> >>>>>completion of SDP for each device internally with a svc_resolved boolean
> >>>>>so probably a new flag to indicate that "Paired" still needs to be
> >>>>>emitted should be all the extra context tracking we need.
> >>>>
> >>>>the pairing procedure is special and we should try to avoid any
> >>>>pointless round trips or wake ups for the clients here. We do know
> >>>>that we are running SDP after pairing anyway. So lets just wait for it
> >>>>to finish. Especially if we do that already anyway for Device1.Pair().
> >>>
> >>>This ended up being quite simple to do, so I just pushed a mostly
> >>>untested patch for this to bluez.git. It'd be good if someone could
> >>>verify that it actually ends up helping the situation with oFono.
> >>>
> >>
> >>How do remotely-initiated connections fit into this?
> >>
> >>We've seen devices that try to connect profiles immediately after
> >>pairing, I believe before the discovery is complete.
> >
> >Both the NewConnection() (including the authorization of the incoming
> >L2CAP/RFCOMM connect request) and the Paired property would even then be
> >delayed until our side has completed its full service discovery.
> >
> 
> So should we be reverting commit 456b8c9723b9b73c3ea4cadc8c6f84ca90675f9a?

I guess so. Without that commit this problem doesn't exist.

> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius

Re: [PATCH] hfp_hf_bluez5: Fix re-registering a modem for a device

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

Hi Vinicius,

>>> Both the NewConnection() (including the authorization of the incoming
>>> L2CAP/RFCOMM connect request) and the Paired property would even then be
>>> delayed until our side has completed its full service discovery.
>>>
>>
>> So should we be reverting commit 456b8c9723b9b73c3ea4cadc8c6f84ca90675f9a?
>
> I guess so. Without that commit this problem doesn't exist.
>

I went ahead and reverted this.

Regards,
-Denis