[Bluez,2/2] shared/gatt: Add descriptor insert function

Commit Message

Yun-hao Chung April 3, 2024, 11:05 a.m. UTC

service->attributes has an assumption that it should look like:
|char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|...
where desc_x_y means a descriptor of char_x.

In monitor, an attribute is inserted as soon as it is found. It is not
guranteed that all the descriptors of a characteristic would be found
before another characteristic is found.

This adds a function to insert the descriptor with the given handle to
an appropriate place in its service attribute list and make monitor to
use this function when a descripter is found.

Reviewed-by: Archie Pusaka <[e-mail]>
---

 monitor/att.c        |  2 +-
 src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
 src/shared/gatt-db.h |  9 ++++++
 3 files changed, 76 insertions(+), 1 deletion(-)

Comments

Luiz Augusto von Dentz April 3, 2024, 2:36 p.m. UTC | #1

Hi Howard,

On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <[e-mail]> wrote:
>
> service->attributes has an assumption that it should look like:
> |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|...
> where desc_x_y means a descriptor of char_x.

Will need to check the trace but I believe BlueZ always discover all
characteristics before moving to descriptors, if that is not happening
they there is probably a bug somewhere, that said perhaps you are
doing the discovery procedure with another stack which doesn't employ
the same logic, although inefficient it is possible to discover out of
order but that would require reassigning the descriptors to
characteristics once they are found, which is also inefficient but I
would understand if you after supporting other stacks.

> In monitor, an attribute is inserted as soon as it is found. It is not
> guranteed that all the descriptors of a characteristic would be found
> before another characteristic is found.

You might want to include such a trace, don't recall seeing any stack
that does out-order discover.

> This adds a function to insert the descriptor with the given handle to
> an appropriate place in its service attribute list and make monitor to
> use this function when a descripter is found.
>
> Reviewed-by: Archie Pusaka <[e-mail]>
> ---
>
>  monitor/att.c        |  2 +-
>  src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
>  src/shared/gatt-db.h |  9 ++++++
>  3 files changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/monitor/att.c b/monitor/att.c
> index ddeb54d9e..503099745 100644
> --- a/monitor/att.c
> +++ b/monitor/att.c
> @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame,
>         if (!db)
>                 return NULL;
>
> -       return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> +       return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
>  }
>
>  static void att_find_info_rsp_16(const struct l2cap_frame *frame)
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 39bba9b54..f08c5afaa 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service,
>         return service->attributes[i];
>  }
>
> +struct gatt_db_attribute *
> +gatt_db_insert_descriptor(struct gatt_db *db,
> +                                       uint16_t handle,
> +                                       const bt_uuid_t *uuid,
> +                                       uint32_t permissions,
> +                                       gatt_db_read_t read_func,
> +                                       gatt_db_write_t write_func,
> +                                       void *user_data)
> +{
> +       struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL;
> +       struct gatt_db_service *service;
> +       int i, j, num_index, char_index;
> +
> +       attrib = gatt_db_get_service(db, handle);
> +       if (!attrib)
> +               return NULL;
> +
> +       service = attrib->service;
> +       num_index = get_attribute_index(service, 0);
> +       if (!num_index)
> +               return NULL;
> +
> +       // Find the characteristic the descriptor belongs to.
> +       for (i = 0; i < num_index; i++) {
> +               iter_attr = service->attributes[i];
> +               if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid))
> +                       continue;
> +
> +               if (iter_attr->handle > handle)
> +                       continue;
> +
> +               // Find the characteristic with the largest handle among those
> +               // with handles less than the descriptor's handle.
> +               if (!char_attr || iter_attr->handle > char_attr->handle) {
> +                       char_attr = iter_attr;
> +                       char_index = i;
> +               }
> +       }
> +
> +       // There is no characteristic contain this descriptor. Something went
> +       // wrong
> +       if (!char_attr)
> +               return NULL;
> +
> +       // Find the end of this characteristic
> +       for (i = char_index + 1; i < num_index; i++) {
> +               iter_attr = service->attributes[i];
> +
> +               if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) ||
> +                       !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid))
> +                       break;
> +       }
> +
> +       // Move all of the attributes after the end of this characteristic to
> +       // its next index.
> +       for (j = num_index; j > i; j--)
> +               service->attributes[j] = service->attributes[j - 1];
> +
> +       service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0);
> +
> +       set_attribute_data(service->attributes[i], read_func, write_func,
> +                                                       permissions, user_data);
> +
> +       return service->attributes[i];
> +}
> +
>  struct gatt_db_attribute *
>  gatt_db_append_descriptor(struct gatt_db *db,
>                                         uint16_t handle,
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index ec0eccdfc..4c4e88d87 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db,
>                                         gatt_db_write_t write_func,
>                                         void *user_data);
>
> +struct gatt_db_attribute *
> +gatt_db_insert_descriptor(struct gatt_db *db,
> +                                       uint16_t handle,
> +                                       const bt_uuid_t *uuid,
> +                                       uint32_t permissions,
> +                                       gatt_db_read_t read_func,
> +                                       gatt_db_write_t write_func,
> +                                       void *user_data);
> +
>  struct gatt_db_attribute *
>  gatt_db_append_descriptor(struct gatt_db *db,
>                                         uint16_t handle,
> --
> 2.44.0.478.gd926399ef9-goog
>

Yun-hao Chung April 8, 2024, 12:22 p.m. UTC | #2

Hi Luiz,

Thanks for the response.

This issue was found in ChromeOS edition BlueZ. I'm not sure if any
official BlueZ would do the same discovery procedure or not.

I extracted relevant part of the trace as below
==========================
< ACL Data TX: Handle 2048 flags 0x00 dlen 11

                                                #43961 [hci0]
2024-03-05 07:33:55.936283
      ATT: Read By Group Type Request (0x10) len 6
        Handle range: 0x001f-0xffff
        Attribute group type: Primary Service (0x2800)
...
> ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #43963 [hci0] 2024-03-05 07:33:55.967020
> ACL Data RX: Handle 2048 flags 0x01 dlen 3                                                                                                                                                  #43964 [hci0] 2024-03-05 07:33:55.967022
      ATT: Read By Group Type Response (0x11) len 25
        Attribute data length: 6
        Attribute group list: 4 entries
        Handle range: 0x001f-0x0035
        UUID: Human Interface Device (0x1812)
        Handle range: 0x0036-0x0044
        UUID: Human Interface Device (0x1812)
        Handle range: 0x0045-0x0055
        UUID: Human Interface Device (0x1812)
        Handle range: 0x0056-0x0062
        UUID: Logitech International SA (0xfd72)
…
< ACL Data TX: Handle 2048 flags 0x00 dlen 11

                                                #44018 [hci0]
2024-03-05 07:33:56.281436
      ATT: Read By Type Request (0x08) len 6
        Handle range: 0x0021-0xffff
        Attribute type: Characteristic (0x2803)
...
> ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #44023 [hci0] 2024-03-05 07:33:56.311026
      ATT: Read By Type Response (0x09) len 22
        Attribute data length: 7
        Attribute data list: 3 entries
        Handle: 0x0022
        Value: 122300332a
            Properties: 0x12
              Read (0x02)
              Notify (0x10)
            Value Handle: 0x0023
            Value UUID: Boot Mouse Input Report (0x2a33)
        Handle: 0x0025
        Value: 0226004b2a
            Properties: 0x02
              Read (0x02)
            Value Handle: 0x0026
            Value UUID: Report Map (0x2a4b)
        Handle: 0x0027
        Value: 1228004d2a
            Properties: 0x12
              Read (0x02)
              Notify (0x10)
            Value Handle: 0x0028
            Value UUID: Report (0x2a4d)
...
< ACL Data TX: Handle 2048 flags 0x00 dlen 11

                                                #44024 [hci0]
2024-03-05 07:33:56.311458
      ATT: Read By Type Request (0x08) len 6
        Handle range: 0x0028-0xffff
        Attribute type: Characteristic (0x2803)
…
> ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #44029 [hci0] 2024-03-05 07:33:56.341025
      ATT: Read By Type Response (0x09) len 22
        Attribute data length: 7
        Attribute data list: 3 entries
        Handle: 0x002b
        Value: 122c004d2a
            Properties: 0x12
              Read (0x02)
              Notify (0x10)
            Value Handle: 0x002c
            Value UUID: Report (0x2a4d)
        Handle: 0x002f
        Value: 0e30004d2a
            Properties: 0x0e
              Read (0x02)
              Write Without Response (0x04)
              Write (0x08)
            Value Handle: 0x0030
            Value UUID: Report (0x2a4d)
        Handle: 0x0032
        Value: 0433004c2a
            Properties: 0x04
              Write Without Response (0x04)
            Value Handle: 0x0033
            Value UUID: HID Control Point (0x2a4c)
…
< ACL Data TX: Handle 2048 flags 0x00 dlen 9

                                                #44071 [hci0]
2024-03-05 07:33:56.702334
      ATT: Find Information Request (0x04) len 4
        Handle range: 0x0029-0x002a
…
> ACL Data RX: Handle 2048 flags 0x02 dlen 14                                                                                                                                                 #44073 [hci0] 2024-03-05 07:33:56.731069
      ATT: Find Information Response (0x05) len 9
        Format: UUID-16 (0x01)
        Handle: 0x0029
        UUID: Client Characteristic Configuration (0x2902)
        Handle: 0x002a
        UUID: Report Reference (0x2908)
=============================

When decoding packet #44073, we can't append descriptors 0x0029 and
0x002a at the tail of the |service->attributes| since the
characteristics 0x002b,0x002f and 0x0032 are already in the array.



On Wed, Apr 3, 2024 at 10:37 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Howard,
>
> On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <[e-mail]> wrote:
> >
> > service->attributes has an assumption that it should look like:
> > |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|...
> > where desc_x_y means a descriptor of char_x.
>
> Will need to check the trace but I believe BlueZ always discover all
> characteristics before moving to descriptors, if that is not happening
> they there is probably a bug somewhere, that said perhaps you are
> doing the discovery procedure with another stack which doesn't employ
> the same logic, although inefficient it is possible to discover out of
> order but that would require reassigning the descriptors to
> characteristics once they are found, which is also inefficient but I
> would understand if you after supporting other stacks.
>
> > In monitor, an attribute is inserted as soon as it is found. It is not
> > guranteed that all the descriptors of a characteristic would be found
> > before another characteristic is found.
>
> You might want to include such a trace, don't recall seeing any stack
> that does out-order discover.
>
> > This adds a function to insert the descriptor with the given handle to
> > an appropriate place in its service attribute list and make monitor to
> > use this function when a descripter is found.
> >
> > Reviewed-by: Archie Pusaka <[e-mail]>
> > ---
> >
> >  monitor/att.c        |  2 +-
> >  src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
> >  src/shared/gatt-db.h |  9 ++++++
> >  3 files changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/monitor/att.c b/monitor/att.c
> > index ddeb54d9e..503099745 100644
> > --- a/monitor/att.c
> > +++ b/monitor/att.c
> > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame,
> >         if (!db)
> >                 return NULL;
> >
> > -       return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> > +       return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> >  }
> >
> >  static void att_find_info_rsp_16(const struct l2cap_frame *frame)
> > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> > index 39bba9b54..f08c5afaa 100644
> > --- a/src/shared/gatt-db.c
> > +++ b/src/shared/gatt-db.c
> > @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service,
> >         return service->attributes[i];
> >  }
> >
> > +struct gatt_db_attribute *
> > +gatt_db_insert_descriptor(struct gatt_db *db,
> > +                                       uint16_t handle,
> > +                                       const bt_uuid_t *uuid,
> > +                                       uint32_t permissions,
> > +                                       gatt_db_read_t read_func,
> > +                                       gatt_db_write_t write_func,
> > +                                       void *user_data)
> > +{
> > +       struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL;
> > +       struct gatt_db_service *service;
> > +       int i, j, num_index, char_index;
> > +
> > +       attrib = gatt_db_get_service(db, handle);
> > +       if (!attrib)
> > +               return NULL;
> > +
> > +       service = attrib->service;
> > +       num_index = get_attribute_index(service, 0);
> > +       if (!num_index)
> > +               return NULL;
> > +
> > +       // Find the characteristic the descriptor belongs to.
> > +       for (i = 0; i < num_index; i++) {
> > +               iter_attr = service->attributes[i];
> > +               if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid))
> > +                       continue;
> > +
> > +               if (iter_attr->handle > handle)
> > +                       continue;
> > +
> > +               // Find the characteristic with the largest handle among those
> > +               // with handles less than the descriptor's handle.
> > +               if (!char_attr || iter_attr->handle > char_attr->handle) {
> > +                       char_attr = iter_attr;
> > +                       char_index = i;
> > +               }
> > +       }
> > +
> > +       // There is no characteristic contain this descriptor. Something went
> > +       // wrong
> > +       if (!char_attr)
> > +               return NULL;
> > +
> > +       // Find the end of this characteristic
> > +       for (i = char_index + 1; i < num_index; i++) {
> > +               iter_attr = service->attributes[i];
> > +
> > +               if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) ||
> > +                       !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid))
> > +                       break;
> > +       }
> > +
> > +       // Move all of the attributes after the end of this characteristic to
> > +       // its next index.
> > +       for (j = num_index; j > i; j--)
> > +               service->attributes[j] = service->attributes[j - 1];
> > +
> > +       service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0);
> > +
> > +       set_attribute_data(service->attributes[i], read_func, write_func,
> > +                                                       permissions, user_data);
> > +
> > +       return service->attributes[i];
> > +}
> > +
> >  struct gatt_db_attribute *
> >  gatt_db_append_descriptor(struct gatt_db *db,
> >                                         uint16_t handle,
> > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> > index ec0eccdfc..4c4e88d87 100644
> > --- a/src/shared/gatt-db.h
> > +++ b/src/shared/gatt-db.h
> > @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db,
> >                                         gatt_db_write_t write_func,
> >                                         void *user_data);
> >
> > +struct gatt_db_attribute *
> > +gatt_db_insert_descriptor(struct gatt_db *db,
> > +                                       uint16_t handle,
> > +                                       const bt_uuid_t *uuid,
> > +                                       uint32_t permissions,
> > +                                       gatt_db_read_t read_func,
> > +                                       gatt_db_write_t write_func,
> > +                                       void *user_data);
> > +
> >  struct gatt_db_attribute *
> >  gatt_db_append_descriptor(struct gatt_db *db,
> >                                         uint16_t handle,
> > --
> > 2.44.0.478.gd926399ef9-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Luiz Augusto von Dentz April 8, 2024, 2:03 p.m. UTC | #3

Hi,

On Mon, Apr 8, 2024 at 8:23 AM Yun-hao Chung <[e-mail]> wrote:
>
> Hi Luiz,
>
> Thanks for the response.
>
> This issue was found in ChromeOS edition BlueZ. I'm not sure if any
> official BlueZ would do the same discovery procedure or not.
>
> I extracted relevant part of the trace as below
> ==========================
> < ACL Data TX: Handle 2048 flags 0x00 dlen 11
>
>                                                 #43961 [hci0]
> 2024-03-05 07:33:55.936283
>       ATT: Read By Group Type Request (0x10) len 6
>         Handle range: 0x001f-0xffff
>         Attribute group type: Primary Service (0x2800)
> ...
> > ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #43963 [hci0] 2024-03-05 07:33:55.967020
> > ACL Data RX: Handle 2048 flags 0x01 dlen 3                                                                                                                                                  #43964 [hci0] 2024-03-05 07:33:55.967022
>       ATT: Read By Group Type Response (0x11) len 25
>         Attribute data length: 6
>         Attribute group list: 4 entries
>         Handle range: 0x001f-0x0035
>         UUID: Human Interface Device (0x1812)
>         Handle range: 0x0036-0x0044
>         UUID: Human Interface Device (0x1812)
>         Handle range: 0x0045-0x0055
>         UUID: Human Interface Device (0x1812)
>         Handle range: 0x0056-0x0062
>         UUID: Logitech International SA (0xfd72)
> …
> < ACL Data TX: Handle 2048 flags 0x00 dlen 11
>
>                                                 #44018 [hci0]
> 2024-03-05 07:33:56.281436
>       ATT: Read By Type Request (0x08) len 6
>         Handle range: 0x0021-0xffff
>         Attribute type: Characteristic (0x2803)
> ...
> > ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #44023 [hci0] 2024-03-05 07:33:56.311026
>       ATT: Read By Type Response (0x09) len 22
>         Attribute data length: 7
>         Attribute data list: 3 entries
>         Handle: 0x0022
>         Value: 122300332a
>             Properties: 0x12
>               Read (0x02)
>               Notify (0x10)
>             Value Handle: 0x0023
>             Value UUID: Boot Mouse Input Report (0x2a33)
>         Handle: 0x0025
>         Value: 0226004b2a
>             Properties: 0x02
>               Read (0x02)
>             Value Handle: 0x0026
>             Value UUID: Report Map (0x2a4b)
>         Handle: 0x0027
>         Value: 1228004d2a
>             Properties: 0x12
>               Read (0x02)
>               Notify (0x10)
>             Value Handle: 0x0028
>             Value UUID: Report (0x2a4d)
> ...
> < ACL Data TX: Handle 2048 flags 0x00 dlen 11
>
>                                                 #44024 [hci0]
> 2024-03-05 07:33:56.311458
>       ATT: Read By Type Request (0x08) len 6
>         Handle range: 0x0028-0xffff
>         Attribute type: Characteristic (0x2803)
> …
> > ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #44029 [hci0] 2024-03-05 07:33:56.341025
>       ATT: Read By Type Response (0x09) len 22
>         Attribute data length: 7
>         Attribute data list: 3 entries
>         Handle: 0x002b
>         Value: 122c004d2a
>             Properties: 0x12
>               Read (0x02)
>               Notify (0x10)
>             Value Handle: 0x002c
>             Value UUID: Report (0x2a4d)
>         Handle: 0x002f
>         Value: 0e30004d2a
>             Properties: 0x0e
>               Read (0x02)
>               Write Without Response (0x04)
>               Write (0x08)
>             Value Handle: 0x0030
>             Value UUID: Report (0x2a4d)
>         Handle: 0x0032
>         Value: 0433004c2a
>             Properties: 0x04
>               Write Without Response (0x04)
>             Value Handle: 0x0033
>             Value UUID: HID Control Point (0x2a4c)
> …
> < ACL Data TX: Handle 2048 flags 0x00 dlen 9
>
>                                                 #44071 [hci0]
> 2024-03-05 07:33:56.702334
>       ATT: Find Information Request (0x04) len 4
>         Handle range: 0x0029-0x002a
> …
> > ACL Data RX: Handle 2048 flags 0x02 dlen 14                                                                                                                                                 #44073 [hci0] 2024-03-05 07:33:56.731069
>       ATT: Find Information Response (0x05) len 9
>         Format: UUID-16 (0x01)
>         Handle: 0x0029
>         UUID: Client Characteristic Configuration (0x2902)
>         Handle: 0x002a
>         UUID: Report Reference (0x2908)
> =============================
>
> When decoding packet #44073, we can't append descriptors 0x0029 and
> 0x002a at the tail of the |service->attributes| since the
> characteristics 0x002b,0x002f and 0x0032 are already in the array.

So bluetoothd works but the monitor doesn't? They both are using
gatt_db_insert_descriptor which uses servvice_insert_descriptor, so
where exactly is it failing at? The only difference I see is that in
case of gatt-client.c we do:

            attr = gatt_db_get_attribute(client->db, handle);
            if (attr && !bt_uuid_cmp(&uuid,
                    gatt_db_attribute_get_type(attr)))
                continue;


The daemon does actually insert the CCC automatically in case there is
only one handle left for the it:

        if (desc_start == chrc_data->end_handle &&
            (chrc_data->properties & BT_GATT_CHRC_PROP_NOTIFY ||
             chrc_data->properties & BT_GATT_CHRC_PROP_INDICATE)) {
            bt_uuid_t ccc_uuid;

            /* If there is only one descriptor that must be the CCC
             * in case either notify or indicate are supported.
             */
            bt_uuid16_create(&ccc_uuid,
                    GATT_CLIENT_CHARAC_CFG_UUID);
            attr = gatt_db_insert_descriptor(client->db, desc_start,
                            &ccc_uuid, 0, NULL,
                            NULL, NULL);
            if (attr) {
                free(chrc_data);
                continue;
            }
        }

That said perhaps we need to revise the implementation of
get_attribute_index, it does attempt to take the first free index
which is incorrect when we are inserting characteristics it shall not
allocate the indexes one after another, instead the index shall be
handle - service->attribute[0].handle when the handle is set.

>
>
> On Wed, Apr 3, 2024 at 10:37 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Howard,
> >
> > On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <[e-mail]> wrote:
> > >
> > > service->attributes has an assumption that it should look like:
> > > |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|...
> > > where desc_x_y means a descriptor of char_x.
> >
> > Will need to check the trace but I believe BlueZ always discover all
> > characteristics before moving to descriptors, if that is not happening
> > they there is probably a bug somewhere, that said perhaps you are
> > doing the discovery procedure with another stack which doesn't employ
> > the same logic, although inefficient it is possible to discover out of
> > order but that would require reassigning the descriptors to
> > characteristics once they are found, which is also inefficient but I
> > would understand if you after supporting other stacks.
> >
> > > In monitor, an attribute is inserted as soon as it is found. It is not
> > > guranteed that all the descriptors of a characteristic would be found
> > > before another characteristic is found.
> >
> > You might want to include such a trace, don't recall seeing any stack
> > that does out-order discover.
> >
> > > This adds a function to insert the descriptor with the given handle to
> > > an appropriate place in its service attribute list and make monitor to
> > > use this function when a descripter is found.
> > >
> > > Reviewed-by: Archie Pusaka <[e-mail]>
> > > ---
> > >
> > >  monitor/att.c        |  2 +-
> > >  src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
> > >  src/shared/gatt-db.h |  9 ++++++
> > >  3 files changed, 76 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/monitor/att.c b/monitor/att.c
> > > index ddeb54d9e..503099745 100644
> > > --- a/monitor/att.c
> > > +++ b/monitor/att.c
> > > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame,
> > >         if (!db)
> > >                 return NULL;
> > >
> > > -       return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> > > +       return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> > >  }
> > >
> > >  static void att_find_info_rsp_16(const struct l2cap_frame *frame)
> > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> > > index 39bba9b54..f08c5afaa 100644
> > > --- a/src/shared/gatt-db.c
> > > +++ b/src/shared/gatt-db.c
> > > @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service,
> > >         return service->attributes[i];
> > >  }
> > >
> > > +struct gatt_db_attribute *
> > > +gatt_db_insert_descriptor(struct gatt_db *db,
> > > +                                       uint16_t handle,
> > > +                                       const bt_uuid_t *uuid,
> > > +                                       uint32_t permissions,
> > > +                                       gatt_db_read_t read_func,
> > > +                                       gatt_db_write_t write_func,
> > > +                                       void *user_data)
> > > +{
> > > +       struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL;
> > > +       struct gatt_db_service *service;
> > > +       int i, j, num_index, char_index;
> > > +
> > > +       attrib = gatt_db_get_service(db, handle);
> > > +       if (!attrib)
> > > +               return NULL;
> > > +
> > > +       service = attrib->service;
> > > +       num_index = get_attribute_index(service, 0);
> > > +       if (!num_index)
> > > +               return NULL;
> > > +
> > > +       // Find the characteristic the descriptor belongs to.
> > > +       for (i = 0; i < num_index; i++) {
> > > +               iter_attr = service->attributes[i];
> > > +               if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid))
> > > +                       continue;
> > > +
> > > +               if (iter_attr->handle > handle)
> > > +                       continue;
> > > +
> > > +               // Find the characteristic with the largest handle among those
> > > +               // with handles less than the descriptor's handle.
> > > +               if (!char_attr || iter_attr->handle > char_attr->handle) {
> > > +                       char_attr = iter_attr;
> > > +                       char_index = i;
> > > +               }
> > > +       }
> > > +
> > > +       // There is no characteristic contain this descriptor. Something went
> > > +       // wrong
> > > +       if (!char_attr)
> > > +               return NULL;
> > > +
> > > +       // Find the end of this characteristic
> > > +       for (i = char_index + 1; i < num_index; i++) {
> > > +               iter_attr = service->attributes[i];
> > > +
> > > +               if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) ||
> > > +                       !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid))
> > > +                       break;
> > > +       }
> > > +
> > > +       // Move all of the attributes after the end of this characteristic to
> > > +       // its next index.
> > > +       for (j = num_index; j > i; j--)
> > > +               service->attributes[j] = service->attributes[j - 1];
> > > +
> > > +       service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0);
> > > +
> > > +       set_attribute_data(service->attributes[i], read_func, write_func,
> > > +                                                       permissions, user_data);
> > > +
> > > +       return service->attributes[i];
> > > +}
> > > +
> > >  struct gatt_db_attribute *
> > >  gatt_db_append_descriptor(struct gatt_db *db,
> > >                                         uint16_t handle,
> > > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> > > index ec0eccdfc..4c4e88d87 100644
> > > --- a/src/shared/gatt-db.h
> > > +++ b/src/shared/gatt-db.h
> > > @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db,
> > >                                         gatt_db_write_t write_func,
> > >                                         void *user_data);
> > >
> > > +struct gatt_db_attribute *
> > > +gatt_db_insert_descriptor(struct gatt_db *db,
> > > +                                       uint16_t handle,
> > > +                                       const bt_uuid_t *uuid,
> > > +                                       uint32_t permissions,
> > > +                                       gatt_db_read_t read_func,
> > > +                                       gatt_db_write_t write_func,
> > > +                                       void *user_data);
> > > +
> > >  struct gatt_db_attribute *
> > >  gatt_db_append_descriptor(struct gatt_db *db,
> > >                                         uint16_t handle,
> > > --
> > > 2.44.0.478.gd926399ef9-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz

Luiz Augusto von Dentz April 8, 2024, 4:12 p.m. UTC | #4

Hi Yun-hao,

On Mon, Apr 8, 2024 at 10:03 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi,
>
> On Mon, Apr 8, 2024 at 8:23 AM Yun-hao Chung <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> > Thanks for the response.
> >
> > This issue was found in ChromeOS edition BlueZ. I'm not sure if any
> > official BlueZ would do the same discovery procedure or not.
> >
> > I extracted relevant part of the trace as below
> > ==========================
> > < ACL Data TX: Handle 2048 flags 0x00 dlen 11
> >
> >                                                 #43961 [hci0]
> > 2024-03-05 07:33:55.936283
> >       ATT: Read By Group Type Request (0x10) len 6
> >         Handle range: 0x001f-0xffff
> >         Attribute group type: Primary Service (0x2800)
> > ...
> > > ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #43963 [hci0] 2024-03-05 07:33:55.967020
> > > ACL Data RX: Handle 2048 flags 0x01 dlen 3                                                                                                                                                  #43964 [hci0] 2024-03-05 07:33:55.967022
> >       ATT: Read By Group Type Response (0x11) len 25
> >         Attribute data length: 6
> >         Attribute group list: 4 entries
> >         Handle range: 0x001f-0x0035
> >         UUID: Human Interface Device (0x1812)
> >         Handle range: 0x0036-0x0044
> >         UUID: Human Interface Device (0x1812)
> >         Handle range: 0x0045-0x0055
> >         UUID: Human Interface Device (0x1812)
> >         Handle range: 0x0056-0x0062
> >         UUID: Logitech International SA (0xfd72)
> > …
> > < ACL Data TX: Handle 2048 flags 0x00 dlen 11
> >
> >                                                 #44018 [hci0]
> > 2024-03-05 07:33:56.281436
> >       ATT: Read By Type Request (0x08) len 6
> >         Handle range: 0x0021-0xffff
> >         Attribute type: Characteristic (0x2803)
> > ...
> > > ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #44023 [hci0] 2024-03-05 07:33:56.311026
> >       ATT: Read By Type Response (0x09) len 22
> >         Attribute data length: 7
> >         Attribute data list: 3 entries
> >         Handle: 0x0022
> >         Value: 122300332a
> >             Properties: 0x12
> >               Read (0x02)
> >               Notify (0x10)
> >             Value Handle: 0x0023
> >             Value UUID: Boot Mouse Input Report (0x2a33)
> >         Handle: 0x0025
> >         Value: 0226004b2a
> >             Properties: 0x02
> >               Read (0x02)
> >             Value Handle: 0x0026
> >             Value UUID: Report Map (0x2a4b)
> >         Handle: 0x0027
> >         Value: 1228004d2a
> >             Properties: 0x12
> >               Read (0x02)
> >               Notify (0x10)
> >             Value Handle: 0x0028
> >             Value UUID: Report (0x2a4d)
> > ...
> > < ACL Data TX: Handle 2048 flags 0x00 dlen 11
> >
> >                                                 #44024 [hci0]
> > 2024-03-05 07:33:56.311458
> >       ATT: Read By Type Request (0x08) len 6
> >         Handle range: 0x0028-0xffff
> >         Attribute type: Characteristic (0x2803)
> > …
> > > ACL Data RX: Handle 2048 flags 0x02 dlen 27                                                                                                                                                 #44029 [hci0] 2024-03-05 07:33:56.341025
> >       ATT: Read By Type Response (0x09) len 22
> >         Attribute data length: 7
> >         Attribute data list: 3 entries
> >         Handle: 0x002b
> >         Value: 122c004d2a
> >             Properties: 0x12
> >               Read (0x02)
> >               Notify (0x10)
> >             Value Handle: 0x002c
> >             Value UUID: Report (0x2a4d)
> >         Handle: 0x002f
> >         Value: 0e30004d2a
> >             Properties: 0x0e
> >               Read (0x02)
> >               Write Without Response (0x04)
> >               Write (0x08)
> >             Value Handle: 0x0030
> >             Value UUID: Report (0x2a4d)
> >         Handle: 0x0032
> >         Value: 0433004c2a
> >             Properties: 0x04
> >               Write Without Response (0x04)
> >             Value Handle: 0x0033
> >             Value UUID: HID Control Point (0x2a4c)
> > …
> > < ACL Data TX: Handle 2048 flags 0x00 dlen 9
> >
> >                                                 #44071 [hci0]
> > 2024-03-05 07:33:56.702334
> >       ATT: Find Information Request (0x04) len 4
> >         Handle range: 0x0029-0x002a
> > …
> > > ACL Data RX: Handle 2048 flags 0x02 dlen 14                                                                                                                                                 #44073 [hci0] 2024-03-05 07:33:56.731069
> >       ATT: Find Information Response (0x05) len 9
> >         Format: UUID-16 (0x01)
> >         Handle: 0x0029
> >         UUID: Client Characteristic Configuration (0x2902)
> >         Handle: 0x002a
> >         UUID: Report Reference (0x2908)
> > =============================
> >
> > When decoding packet #44073, we can't append descriptors 0x0029 and
> > 0x002a at the tail of the |service->attributes| since the
> > characteristics 0x002b,0x002f and 0x0032 are already in the array.
>
> So bluetoothd works but the monitor doesn't? They both are using
> gatt_db_insert_descriptor which uses servvice_insert_descriptor, so
> where exactly is it failing at? The only difference I see is that in
> case of gatt-client.c we do:
>
>             attr = gatt_db_get_attribute(client->db, handle);
>             if (attr && !bt_uuid_cmp(&uuid,
>                     gatt_db_attribute_get_type(attr)))
>                 continue;
>
>
> The daemon does actually insert the CCC automatically in case there is
> only one handle left for the it:
>
>         if (desc_start == chrc_data->end_handle &&
>             (chrc_data->properties & BT_GATT_CHRC_PROP_NOTIFY ||
>              chrc_data->properties & BT_GATT_CHRC_PROP_INDICATE)) {
>             bt_uuid_t ccc_uuid;
>
>             /* If there is only one descriptor that must be the CCC
>              * in case either notify or indicate are supported.
>              */
>             bt_uuid16_create(&ccc_uuid,
>                     GATT_CLIENT_CHARAC_CFG_UUID);
>             attr = gatt_db_insert_descriptor(client->db, desc_start,
>                             &ccc_uuid, 0, NULL,
>                             NULL, NULL);
>             if (attr) {
>                 free(chrc_data);
>                 continue;
>             }
>         }
>
> That said perhaps we need to revise the implementation of
> get_attribute_index, it does attempt to take the first free index
> which is incorrect when we are inserting characteristics it shall not
> allocate the indexes one after another, instead the index shall be
> handle - service->attribute[0].handle when the handle is set.

Here is a tentative of fixing the index logic:

https://patchwork.kernel.org/project/bluetooth/patch/[e-mail]/

> >
> >
> > On Wed, Apr 3, 2024 at 10:37 PM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Howard,
> > >
> > > On Wed, Apr 3, 2024 at 7:06 AM Howard Chung <[e-mail]> wrote:
> > > >
> > > > service->attributes has an assumption that it should look like:
> > > > |char_uuid|char_1|desc_1_1|desc_1_2|char_uuid|char_2|desc_2_1|char_uuid|...
> > > > where desc_x_y means a descriptor of char_x.
> > >
> > > Will need to check the trace but I believe BlueZ always discover all
> > > characteristics before moving to descriptors, if that is not happening
> > > they there is probably a bug somewhere, that said perhaps you are
> > > doing the discovery procedure with another stack which doesn't employ
> > > the same logic, although inefficient it is possible to discover out of
> > > order but that would require reassigning the descriptors to
> > > characteristics once they are found, which is also inefficient but I
> > > would understand if you after supporting other stacks.
> > >
> > > > In monitor, an attribute is inserted as soon as it is found. It is not
> > > > guranteed that all the descriptors of a characteristic would be found
> > > > before another characteristic is found.
> > >
> > > You might want to include such a trace, don't recall seeing any stack
> > > that does out-order discover.
> > >
> > > > This adds a function to insert the descriptor with the given handle to
> > > > an appropriate place in its service attribute list and make monitor to
> > > > use this function when a descripter is found.
> > > >
> > > > Reviewed-by: Archie Pusaka <[e-mail]>
> > > > ---
> > > >
> > > >  monitor/att.c        |  2 +-
> > > >  src/shared/gatt-db.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  src/shared/gatt-db.h |  9 ++++++
> > > >  3 files changed, 76 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/monitor/att.c b/monitor/att.c
> > > > index ddeb54d9e..503099745 100644
> > > > --- a/monitor/att.c
> > > > +++ b/monitor/att.c
> > > > @@ -4153,7 +4153,7 @@ static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame,
> > > >         if (!db)
> > > >                 return NULL;
> > > >
> > > > -       return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> > > > +       return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
> > > >  }
> > > >
> > > >  static void att_find_info_rsp_16(const struct l2cap_frame *frame)
> > > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> > > > index 39bba9b54..f08c5afaa 100644
> > > > --- a/src/shared/gatt-db.c
> > > > +++ b/src/shared/gatt-db.c
> > > > @@ -1002,6 +1002,72 @@ service_append_descriptor(struct gatt_db_service *service,
> > > >         return service->attributes[i];
> > > >  }
> > > >
> > > > +struct gatt_db_attribute *
> > > > +gatt_db_insert_descriptor(struct gatt_db *db,
> > > > +                                       uint16_t handle,
> > > > +                                       const bt_uuid_t *uuid,
> > > > +                                       uint32_t permissions,
> > > > +                                       gatt_db_read_t read_func,
> > > > +                                       gatt_db_write_t write_func,
> > > > +                                       void *user_data)
> > > > +{
> > > > +       struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL;
> > > > +       struct gatt_db_service *service;
> > > > +       int i, j, num_index, char_index;
> > > > +
> > > > +       attrib = gatt_db_get_service(db, handle);
> > > > +       if (!attrib)
> > > > +               return NULL;
> > > > +
> > > > +       service = attrib->service;
> > > > +       num_index = get_attribute_index(service, 0);
> > > > +       if (!num_index)
> > > > +               return NULL;
> > > > +
> > > > +       // Find the characteristic the descriptor belongs to.
> > > > +       for (i = 0; i < num_index; i++) {
> > > > +               iter_attr = service->attributes[i];
> > > > +               if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid))
> > > > +                       continue;
> > > > +
> > > > +               if (iter_attr->handle > handle)
> > > > +                       continue;
> > > > +
> > > > +               // Find the characteristic with the largest handle among those
> > > > +               // with handles less than the descriptor's handle.
> > > > +               if (!char_attr || iter_attr->handle > char_attr->handle) {
> > > > +                       char_attr = iter_attr;
> > > > +                       char_index = i;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       // There is no characteristic contain this descriptor. Something went
> > > > +       // wrong
> > > > +       if (!char_attr)
> > > > +               return NULL;
> > > > +
> > > > +       // Find the end of this characteristic
> > > > +       for (i = char_index + 1; i < num_index; i++) {
> > > > +               iter_attr = service->attributes[i];
> > > > +
> > > > +               if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) ||
> > > > +                       !bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid))
> > > > +                       break;
> > > > +       }
> > > > +
> > > > +       // Move all of the attributes after the end of this characteristic to
> > > > +       // its next index.
> > > > +       for (j = num_index; j > i; j--)
> > > > +               service->attributes[j] = service->attributes[j - 1];
> > > > +
> > > > +       service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0);
> > > > +
> > > > +       set_attribute_data(service->attributes[i], read_func, write_func,
> > > > +                                                       permissions, user_data);
> > > > +
> > > > +       return service->attributes[i];
> > > > +}
> > > > +
> > > >  struct gatt_db_attribute *
> > > >  gatt_db_append_descriptor(struct gatt_db *db,
> > > >                                         uint16_t handle,
> > > > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> > > > index ec0eccdfc..4c4e88d87 100644
> > > > --- a/src/shared/gatt-db.h
> > > > +++ b/src/shared/gatt-db.h
> > > > @@ -80,6 +80,15 @@ gatt_db_append_characteristic(struct gatt_db *db,
> > > >                                         gatt_db_write_t write_func,
> > > >                                         void *user_data);
> > > >
> > > > +struct gatt_db_attribute *
> > > > +gatt_db_insert_descriptor(struct gatt_db *db,
> > > > +                                       uint16_t handle,
> > > > +                                       const bt_uuid_t *uuid,
> > > > +                                       uint32_t permissions,
> > > > +                                       gatt_db_read_t read_func,
> > > > +                                       gatt_db_write_t write_func,
> > > > +                                       void *user_data);
> > > > +
> > > >  struct gatt_db_attribute *
> > > >  gatt_db_append_descriptor(struct gatt_db *db,
> > > >                                         uint16_t handle,
> > > > --
> > > > 2.44.0.478.gd926399ef9-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Patch

diff --git a/monitor/att.c b/monitor/att.c
index ddeb54d9e..503099745 100644
--- a/monitor/att.c
+++ b/monitor/att.c
@@ -4153,7 +4153,7 @@  static struct gatt_db_attribute *insert_desc(const struct l2cap_frame *frame,
 	if (!db)
 		return NULL;
 
-	return gatt_db_append_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
+	return gatt_db_insert_descriptor(db, handle, uuid, 0, NULL, NULL, NULL);
 }
 
 static void att_find_info_rsp_16(const struct l2cap_frame *frame)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 39bba9b54..f08c5afaa 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -1002,6 +1002,72 @@  service_append_descriptor(struct gatt_db_service *service,
 	return service->attributes[i];
 }
 
+struct gatt_db_attribute *
+gatt_db_insert_descriptor(struct gatt_db *db,
+					uint16_t handle,
+					const bt_uuid_t *uuid,
+					uint32_t permissions,
+					gatt_db_read_t read_func,
+					gatt_db_write_t write_func,
+					void *user_data)
+{
+	struct gatt_db_attribute *attrib, *iter_attr, *char_attr = NULL;
+	struct gatt_db_service *service;
+	int i, j, num_index, char_index;
+
+	attrib = gatt_db_get_service(db, handle);
+	if (!attrib)
+		return NULL;
+
+	service = attrib->service;
+	num_index = get_attribute_index(service, 0);
+	if (!num_index)
+		return NULL;
+
+	// Find the characteristic the descriptor belongs to.
+	for (i = 0; i < num_index; i++) {
+		iter_attr = service->attributes[i];
+		if (bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid))
+			continue;
+
+		if (iter_attr->handle > handle)
+			continue;
+
+		// Find the characteristic with the largest handle among those
+		// with handles less than the descriptor's handle.
+		if (!char_attr || iter_attr->handle > char_attr->handle) {
+			char_attr = iter_attr;
+			char_index = i;
+		}
+	}
+
+	// There is no characteristic contain this descriptor. Something went
+	// wrong
+	if (!char_attr)
+		return NULL;
+
+	// Find the end of this characteristic
+	for (i = char_index + 1; i < num_index; i++) {
+		iter_attr = service->attributes[i];
+
+		if (!bt_uuid_cmp(&characteristic_uuid, &iter_attr->uuid) ||
+			!bt_uuid_cmp(&included_service_uuid, &iter_attr->uuid))
+			break;
+	}
+
+	// Move all of the attributes after the end of this characteristic to
+	// its next index.
+	for (j = num_index; j > i; j--)
+		service->attributes[j] = service->attributes[j - 1];
+
+	service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0);
+
+	set_attribute_data(service->attributes[i], read_func, write_func,
+							permissions, user_data);
+
+	return service->attributes[i];
+}
+
 struct gatt_db_attribute *
 gatt_db_append_descriptor(struct gatt_db *db,
 					uint16_t handle,
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index ec0eccdfc..4c4e88d87 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -80,6 +80,15 @@  gatt_db_append_characteristic(struct gatt_db *db,
 					gatt_db_write_t write_func,
 					void *user_data);
 
+struct gatt_db_attribute *
+gatt_db_insert_descriptor(struct gatt_db *db,
+					uint16_t handle,
+					const bt_uuid_t *uuid,
+					uint32_t permissions,
+					gatt_db_read_t read_func,
+					gatt_db_write_t write_func,
+					void *user_data);
+
 struct gatt_db_attribute *
 gatt_db_append_descriptor(struct gatt_db *db,
 					uint16_t handle,