Hi Luiz,
On 03/01/2024 16:50, Luiz Augusto von Dentz wrote:
Hi Frédéric,
On Wed, Jan 3, 2024 at 4:28 AM Frédéric Danis
<frederic.danis@xxxxxxxxxxxxx> wrote:
PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd
and ending at 0xffff.
Don't we have a test for it under unit/test-gatt.c? Perhaps it would
be a good idea to add one while doing this change.
Yes
My idea should be to add a new unordered database and run
gatt_db_get_hash() on it.
This resets the next_handle to 0 in gatt_db_insert_service() instead of
setting it to 0x10000. Other services are added later.
This could end-up by a crash in db_hash_update() if not enough space has
been allocated for hash.iov and some entries are overwritten.
I understand we don't want to loop around but handle 0x10000 is not
valid either.
Afaiu the next_handle is used as:
- the next available handle, with special value 0 to define an empty db
- and the maximum size to allocate during db_hash_update()
So, 0x10000 is not a valid handle but is a valid size.
gatt_db_insert_service() is already protected to not use handle > 0xFFFF.
---
src/shared/gatt-db.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 676f963ec..d32c9a70f 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -58,7 +58,7 @@ struct gatt_db {
struct bt_crypto *crypto;
uint8_t hash[16];
unsigned int hash_id;
- uint16_t next_handle;
+ uint32_t next_handle;
I wonder if we can just set the next_handle to 0 and then check it
when using it, that way it indicates that it had looped around and
handle 0 is invalid already so we shouldn't allocate anything on it.
Not sure this can work as 0 can mean it's an empty db or a db requesting
UINT16_MAX+1 elements.
During this test case, it loops to 0, but as other services are added
setting next_handle to another value, ending up to allocate less memory
than expected (i.e. UINT16_MAX+1 elements).
We may replace the next_handle by last_handle, use gatt_db_isempty()
instead of the handle 0 to check for empty db, and allocate
last_handle+1 in db_hash_update().
Does it seems better?
Regards,
Fred
--
Frédéric Danis
Senior Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom
Registered in England & Wales, no. 5513718
