Re: [PATCH] bluez:update handle for large database

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Amit,

On Fri, Jun 5, 2020 at 7:30 AM Amitsi5x <amitx.k.singh@intel.com> wrote:
>
> From: amit <amitx.k.singh@intel.com>
>
> Update handle for large database and
> added condition before free to avoid double free
>
> Signed-off-by: amit <amitx.k.singh@intel.com>
> ---
>  src/shared/gatt-client.c | 12 +++++++-----
>  src/shared/gatt-db.c     | 15 +++++++++------
>  2 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 19ff6ab65..3cb6ae443 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1131,8 +1131,6 @@ static void discover_secondary_cb(bool success, uint8_t att_ecode,
>                                 success = false;
>                                 goto done;
>                         }
> -                       /* Database has changed adjust last handle */
> -                       op->last = end;
>                 }
>
>                 /* Update pending list */
> @@ -1392,9 +1390,13 @@ static void db_hash_read_cb(bool success, uint8_t att_ecode,
>         util_hexdump(' ', value, len, client->debug_callback,
>                                                 client->debug_data);
>
> -       /* Store ithe new hash in the db */
> -       gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL,
> -                                       db_hash_write_value_cb, client);
> +       /* Store the new hash in the db */
> +       if(gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL,
> +                                               db_hash_write_value_cb, client)) {
> +               util_debug(client->debug_callback, client->debug_data,"DB Hash match write: skipping discovery");
> +               queue_remove_all(op->pending_svcs, NULL, NULL, NULL);

Not following this change, if we got to write the db hash that means
the old value did not match.

> +       }
> +
>
>  discover:
>         if (!op->success) {
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index b44f7b5e9..15af4c20a 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -344,10 +344,15 @@ static bool db_hash_update(void *user_data)
>         gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash);
>         bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->hash);
>
> -       for (i = 0; i < hash.i; i++)
> -               free(hash.iov[i].iov_base);
> +       for (i = 0; i < hash.i; i++) {
> +               if(hash.iov[i].iov_base)
> +                       free(hash.iov[i].iov_base);
> +       }
> +
> +       if(hash.iov)
> +               free(hash.iov);
>
> -       free(hash.iov);
> +       hash.iov = NULL;

I believe this error was actually introduced by your changes actually,
see below.

>         return false;
>  }
> @@ -689,7 +694,7 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
>         service->num_handles = num_handles;
>
>         /* Fast-forward next_handle if the new service was added to the end */
> -       db->next_handle = MAX(handle + num_handles, db->next_handle);
> +       db->next_handle += num_handles;

Note that if the service was not added to the end this starts adding
gaps in between, so I'm afraid I will have to nack this change.

>         return service->attributes[0];
>
> @@ -811,8 +816,6 @@ service_insert_characteristic(struct gatt_db_service *service,
>          * declaration. All characteristic definitions shall have a
>          * Characteristic Value declaration.
>          */
> -       if (handle == UINT16_MAX)
> -               return NULL;

This perhaps is the real reason, it seems to me that you have more
than UINT16_MAX handles so the handles loop around and start over from
0 which is invalid and will most likely cause double frees etc and
they can be multiple attributes assigned to the same handle. How big
is the database you are trying to test? If that is going past
UINT16_MAX it is probably broken and nothing can be done to fix it on
the client side which is why we stop adding attributes after it.

>         i = get_attribute_index(service, 1);
>         if (!i)
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz