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

["Singh, AmitX K" <amitx.k.singh@intel.com>]

Hi Luiz

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Friday, June 5, 2020 10:46 PM
> To: Singh, AmitX K <amitx.k.singh@intel.com>
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH] bluez:update handle for large database
> 
> 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.
> 

When we verify the PTS test case GATT/CL/GAD/BV-02-C test case, the Test case demands to perform multiple connections to the PTS device with different database upon each connection, where the current code does not update the database hash on each connect iteration that yields to seg fault.
Added support for remove pending service if any after updating new hash in database.

> > +       }
> > +
> >
> >  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.
> 

When we verify the PTS test case GATT/CL/GAD/BV-02-C test case, the TC demands to perform multiple connections to the PTS device with different database upon each connection. 
In one connection setup database having handle value is UINT16_MAX,  where the current code does not able when handle is max  .

> >         i = get_attribute_index(service, 1);
> >         if (!i)
> > --
> > 2.17.1
> >
> 
> 
> --
> Luiz Augusto von Dentz