[BlueZ PATCH v1] shared/gatt-client:Ignore orphaned characteristics

[BlueZ PATCH v1] shared/gatt-client:Ignore orphaned characteristics

[Alain Michaud]

The gatt discovery proceedure simplification to discover all
characteristics at once has exposed a device side issue where some
device implementation reports orphaned characteristics.  While this
technically shouldn't be allowed, some instances of this were observed
(namely on some Android phones).

The fix is to simply skip over orphaned characteristics and continue
with everything else that is valid.

This has been tested as part of the Android CTS tests against an
affected platform and was confirmed to have worked around the issue.
---

 src/shared/gatt-client.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 963ad619f..d604c77a3 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
 			util_debug(client->debug_callback, client->debug_data,
 				"Failed to insert characteristic at 0x%04x",
 				chrc_data->value_handle);
-			goto failed;
+
+			/* Skip over invalid characteristic */
+			free(chrc_data);
+			continue;
 		}
 
 		if (gatt_db_attribute_get_handle(attr) !=
-- 
2.26.2.526.g744177e7f7-goog

Re: [BlueZ PATCH v1] shared/gatt-client:Ignore orphaned characteristics

[Marcel Holtmann]

Hi Alain,

> The gatt discovery proceedure simplification to discover all
> characteristics at once has exposed a device side issue where some
> device implementation reports orphaned characteristics.  While this
> technically shouldn't be allowed, some instances of this were observed
> (namely on some Android phones).
> 
> The fix is to simply skip over orphaned characteristics and continue
> with everything else that is valid.
> 
> This has been tested as part of the Android CTS tests against an
> affected platform and was confirmed to have worked around the issue.
> ---
> 
> src/shared/gatt-client.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 963ad619f..d604c77a3 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
> 			util_debug(client->debug_callback, client->debug_data,
> 				"Failed to insert characteristic at 0x%04x",
> 				chrc_data->value_handle);
> -			goto failed;
> +
> +			/* Skip over invalid characteristic */
> +			free(chrc_data);
> +			continue;
> 		}

should we add a warning here?

And I think it would be good to have a bit more verbose comment why this is done this way instead of aborting.

Regards

Marcel

Re: [BlueZ PATCH v1] shared/gatt-client:Ignore orphaned characteristics

[Alain Michaud]

Hi Marcel,


On Fri, May 1, 2020 at 12:26 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > The gatt discovery proceedure simplification to discover all
> > characteristics at once has exposed a device side issue where some
> > device implementation reports orphaned characteristics.  While this
> > technically shouldn't be allowed, some instances of this were observed
> > (namely on some Android phones).
> >
> > The fix is to simply skip over orphaned characteristics and continue
> > with everything else that is valid.
> >
> > This has been tested as part of the Android CTS tests against an
> > affected platform and was confirmed to have worked around the issue.
> > ---
> >
> > src/shared/gatt-client.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> > index 963ad619f..d604c77a3 100644
> > --- a/src/shared/gatt-client.c
> > +++ b/src/shared/gatt-client.c
> > @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
> >                       util_debug(client->debug_callback, client->debug_data,
> >                               "Failed to insert characteristic at 0x%04x",
> >                               chrc_data->value_handle);
> > -                     goto failed;
> > +
> > +                     /* Skip over invalid characteristic */
> > +                     free(chrc_data);
> > +                     continue;
> >               }
>
> should we add a warning here?
Is there a good way other than the util_debug mechanism to write  a warning?

>
> And I think it would be good to have a bit more verbose comment why this is done this way instead of aborting.
Ack, will be added in V2.

>
> Regards
>
> Marcel
>

Re: [BlueZ PATCH v1] shared/gatt-client:Ignore orphaned characteristics

[Marcel Holtmann]

Hi Alain,

>>> The gatt discovery proceedure simplification to discover all
>>> characteristics at once has exposed a device side issue where some
>>> device implementation reports orphaned characteristics.  While this
>>> technically shouldn't be allowed, some instances of this were observed
>>> (namely on some Android phones).
>>> 
>>> The fix is to simply skip over orphaned characteristics and continue
>>> with everything else that is valid.
>>> 
>>> This has been tested as part of the Android CTS tests against an
>>> affected platform and was confirmed to have worked around the issue.
>>> ---
>>> 
>>> src/shared/gatt-client.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>>> index 963ad619f..d604c77a3 100644
>>> --- a/src/shared/gatt-client.c
>>> +++ b/src/shared/gatt-client.c
>>> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
>>>                      util_debug(client->debug_callback, client->debug_data,
>>>                              "Failed to insert characteristic at 0x%04x",
>>>                              chrc_data->value_handle);
>>> -                     goto failed;
>>> +
>>> +                     /* Skip over invalid characteristic */
>>> +                     free(chrc_data);
>>> +                     continue;
>>>              }
>> 
>> should we add a warning here?
> Is there a good way other than the util_debug mechanism to write  a warning?

you could just use btd_warn() here. If this gets too noisy, we either remove it later or we introduce a btd_warn_ratelimited() or btd_warn_once().

Regards

Marcel

Re: [BlueZ PATCH v1] shared/gatt-client:Ignore orphaned characteristics

[Alain Michaud]

On Fri, May 1, 2020 at 1:32 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> >>> The gatt discovery proceedure simplification to discover all
> >>> characteristics at once has exposed a device side issue where some
> >>> device implementation reports orphaned characteristics.  While this
> >>> technically shouldn't be allowed, some instances of this were observed
> >>> (namely on some Android phones).
> >>>
> >>> The fix is to simply skip over orphaned characteristics and continue
> >>> with everything else that is valid.
> >>>
> >>> This has been tested as part of the Android CTS tests against an
> >>> affected platform and was confirmed to have worked around the issue.
> >>> ---
> >>>
> >>> src/shared/gatt-client.c | 5 ++++-
> >>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> >>> index 963ad619f..d604c77a3 100644
> >>> --- a/src/shared/gatt-client.c
> >>> +++ b/src/shared/gatt-client.c
> >>> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
> >>>                      util_debug(client->debug_callback, client->debug_data,
> >>>                              "Failed to insert characteristic at 0x%04x",
> >>>                              chrc_data->value_handle);
> >>> -                     goto failed;
> >>> +
> >>> +                     /* Skip over invalid characteristic */
> >>> +                     free(chrc_data);
> >>> +                     continue;
> >>>              }
> >>
> >> should we add a warning here?
> > Is there a good way other than the util_debug mechanism to write  a warning?
>
> you could just use btd_warn() here. If this gets too noisy, we either remove it later or we introduce a btd_warn_ratelimited() or btd_warn_once().
This being under src/shared, I wasn't sure if that was acceptable to
add a btd dependency here, especially with the work to avoid it via
the util_debug.  Happy to do either way, just want to make sure the
guidance is clear.

>
> Regards
>
> Marcel
>

Re: [BlueZ PATCH v1] shared/gatt-client:Ignore orphaned characteristics

[Marcel Holtmann]

Hi Alain,

>>>>> The gatt discovery proceedure simplification to discover all
>>>>> characteristics at once has exposed a device side issue where some
>>>>> device implementation reports orphaned characteristics.  While this
>>>>> technically shouldn't be allowed, some instances of this were observed
>>>>> (namely on some Android phones).
>>>>> 
>>>>> The fix is to simply skip over orphaned characteristics and continue
>>>>> with everything else that is valid.
>>>>> 
>>>>> This has been tested as part of the Android CTS tests against an
>>>>> affected platform and was confirmed to have worked around the issue.
>>>>> ---
>>>>> 
>>>>> src/shared/gatt-client.c | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>>>>> index 963ad619f..d604c77a3 100644
>>>>> --- a/src/shared/gatt-client.c
>>>>> +++ b/src/shared/gatt-client.c
>>>>> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
>>>>>                     util_debug(client->debug_callback, client->debug_data,
>>>>>                             "Failed to insert characteristic at 0x%04x",
>>>>>                             chrc_data->value_handle);
>>>>> -                     goto failed;
>>>>> +
>>>>> +                     /* Skip over invalid characteristic */
>>>>> +                     free(chrc_data);
>>>>> +                     continue;
>>>>>             }
>>>> 
>>>> should we add a warning here?
>>> Is there a good way other than the util_debug mechanism to write  a warning?
>> 
>> you could just use btd_warn() here. If this gets too noisy, we either remove it later or we introduce a btd_warn_ratelimited() or btd_warn_once().
> This being under src/shared, I wasn't sure if that was acceptable to
> add a btd dependency here, especially with the work to avoid it via
> the util_debug.  Happy to do either way, just want to make sure the
> guidance is clear.

good point. My bad. You better leave the logging out then.

Regards

Marcel

Re: [BlueZ PATCH v1] shared/gatt-client:Ignore orphaned characteristics

[Luiz Augusto von Dentz]

Hi Marcel, Alain,

On Fri, May 1, 2020 at 11:04 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> >>>>> The gatt discovery proceedure simplification to discover all
> >>>>> characteristics at once has exposed a device side issue where some
> >>>>> device implementation reports orphaned characteristics.  While this
> >>>>> technically shouldn't be allowed, some instances of this were observed
> >>>>> (namely on some Android phones).
> >>>>>
> >>>>> The fix is to simply skip over orphaned characteristics and continue
> >>>>> with everything else that is valid.
> >>>>>
> >>>>> This has been tested as part of the Android CTS tests against an
> >>>>> affected platform and was confirmed to have worked around the issue.
> >>>>> ---
> >>>>>
> >>>>> src/shared/gatt-client.c | 5 ++++-
> >>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> >>>>> index 963ad619f..d604c77a3 100644
> >>>>> --- a/src/shared/gatt-client.c
> >>>>> +++ b/src/shared/gatt-client.c
> >>>>> @@ -632,7 +632,10 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
> >>>>>                     util_debug(client->debug_callback, client->debug_data,
> >>>>>                             "Failed to insert characteristic at 0x%04x",
> >>>>>                             chrc_data->value_handle);
> >>>>> -                     goto failed;
> >>>>> +
> >>>>> +                     /* Skip over invalid characteristic */
> >>>>> +                     free(chrc_data);
> >>>>> +                     continue;
> >>>>>             }
> >>>>
> >>>> should we add a warning here?
> >>> Is there a good way other than the util_debug mechanism to write  a warning?
> >>
> >> you could just use btd_warn() here. If this gets too noisy, we either remove it later or we introduce a btd_warn_ratelimited() or btd_warn_once().
> > This being under src/shared, I wasn't sure if that was acceptable to
> > add a btd dependency here, especially with the work to avoid it via
> > the util_debug.  Happy to do either way, just want to make sure the
> > guidance is clear.
>
> good point. My bad. You better leave the logging out then.

There is already some logging a little bit before so I guess we are
covered here.

-- 
Luiz Augusto von Dentz