* [PATCH BlueZ v3] shared/gatt-db: Fix munmap_chunk invalid pointer
@ 2024-01-05 12:34 Frédéric Danis
2024-01-05 13:42 ` [BlueZ,v3] " bluez.test.bot
2024-01-05 15:08 ` [PATCH BlueZ v3] " Luiz Augusto von Dentz
0 siblings, 2 replies; 4+ messages in thread
From: Frédéric Danis @ 2024-01-05 12:34 UTC (permalink / raw)
To: linux-bluetooth
PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd
and ending at 0xffff.
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.
---
v1 -> v2: Replace next_handle by last_handle
Check empty db using gatt_db_isempty(db) instead of
next_handle == 0
Add robustness unit test to check that gatt_db_get_hash()
doesn't crash
v2 -> v3: Fix line length checkpatch errors
---
src/shared/gatt-db.c | 19 ++++++-----
unit/test-gatt.c | 80 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 89 insertions(+), 10 deletions(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 676f963ec..9559583d1 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;
+ uint16_t last_handle;
struct queue *services;
struct queue *notify_list;
@@ -255,7 +255,7 @@ struct gatt_db *gatt_db_new(void)
db->crypto = bt_crypto_new();
db->services = queue_new();
db->notify_list = queue_new();
- db->next_handle = 0x0001;
+ db->last_handle = 0x0000;
return gatt_db_ref(db);
}
@@ -356,14 +356,15 @@ static bool db_hash_update(void *user_data)
db->hash_id = 0;
- if (!db->next_handle)
+ if (gatt_db_isempty(db))
return false;
- hash.iov = new0(struct iovec, db->next_handle);
+ hash.iov = new0(struct iovec, db->last_handle + 1);
hash.i = 0;
gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash);
- bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->hash);
+ bt_crypto_gatt_hash(db->crypto, hash.iov, db->last_handle + 1,
+ db->hash);
for (i = 0; i < hash.i; i++)
free(hash.iov[i].iov_base);
@@ -624,7 +625,7 @@ bool gatt_db_clear_range(struct gatt_db *db, uint16_t start_handle,
done:
if (gatt_db_isempty(db))
- db->next_handle = 0;
+ db->last_handle = 0;
return true;
}
@@ -700,7 +701,7 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
return NULL;
if (!handle)
- handle = db->next_handle;
+ handle = db->last_handle + 1;
if (num_handles < 1 || (handle + num_handles - 1) > UINT16_MAX)
return NULL;
@@ -747,8 +748,8 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
service->attributes[0]->handle = handle;
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);
+ /* Fast-forward last_handle if the new service was added to the end */
+ db->last_handle = MAX(handle + num_handles - 1, db->last_handle);
return service->attributes[0];
diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index f92d860c4..9457d9079 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -1908,6 +1908,67 @@ static struct gatt_db *make_test_spec_large_db_1(void)
return make_db(specs);
}
+/*
+ * Defined Test database 3:
+ * Tiny database fits into a single minimum sized-pdu with services unordered.
+ * Satisfies requirements:
+ * 5. At least one characteristic at the MAX handle
+ * 7. at least one service uuid with multiple instances
+ * 8. Some simple services, some with included services
+ * 9. an instance where handle of included service comes before the including
+ * service
+ * 11. Simple characteristics (no desc) and complex characteristics
+ * (multiple descriptors)
+ * 12. Instances of complex chars with 16-bit and 128-bit uuids
+ * (although not in scrambled order)
+ */
+
+static struct gatt_db *make_test_spec_unordered_db(void)
+{
+ const struct att_handle_spec specs[] = {
+ SECONDARY_SERVICE(0x0003, DEVICE_INFORMATION_UUID, 16),
+ CHARACTERISTIC_STR(GATT_CHARAC_MANUFACTURER_NAME_STRING,
+ BT_ATT_PERM_READ |
+ BT_ATT_PERM_WRITE,
+ BT_GATT_CHRC_PROP_READ |
+ BT_GATT_CHRC_PROP_NOTIFY |
+ BT_GATT_CHRC_PROP_INDICATE |
+ BT_GATT_CHRC_PROP_EXT_PROP,
+ "BlueZ"),
+ DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ |
+ BT_ATT_PERM_WRITE, 0x00, 0x00),
+ DESCRIPTOR_STR(GATT_CHARAC_USER_DESC_UUID, BT_ATT_PERM_READ,
+ "Manufacturer Name"),
+ DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
+ 0x00),
+ CHARACTERISTIC_STR(GATT_CHARAC_SOFTWARE_REVISION_STRING,
+ BT_ATT_PERM_READ,
+ BT_GATT_CHRC_PROP_READ |
+ BT_GATT_CHRC_PROP_INDICATE,
+ "5.59"),
+ DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ
+ | BT_ATT_PERM_WRITE, 0x00, 0x00),
+
+ PRIMARY_SERVICE(0xFFFF - 9 + 1, GAP_UUID, 9),
+ INCLUDE(0x0003),
+ CHARACTERISTIC_STR(GATT_CHARAC_DEVICE_NAME, BT_ATT_PERM_READ,
+ BT_GATT_CHRC_PROP_READ,
+ "BlueZ Unit Tester"),
+ CHARACTERISTIC(0000B009-0000-0000-0123-456789abcdef,
+ BT_ATT_PERM_READ | BT_ATT_PERM_WRITE,
+ BT_GATT_CHRC_PROP_READ |
+ BT_GATT_CHRC_PROP_EXT_PROP, 0x09),
+ DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
+ 0x00),
+ CHARACTERISTIC(GATT_CHARAC_APPEARANCE, BT_ATT_PERM_READ,
+ BT_GATT_CHRC_PROP_READ, 0x00, 0x00),
+ PRIMARY_SERVICE(0x0001, DEVICE_INFORMATION_UUID, 1),
+ { }
+ };
+
+ return make_db(specs);
+}
+
static void test_client(gconstpointer data)
{
create_context(512, data);
@@ -2345,10 +2406,22 @@ static const struct test_step test_indication_server_1 = {
.length = 0x03,
};
+static void test_hash_db(gconstpointer data)
+{
+ struct context *context = create_context(512, data);
+
+ /* test that gatt_db_get_hash is able to manage unordered db and
+ * doesn't crash
+ */
+ gatt_db_get_hash(context->server_db);
+
+ context_quit(context);
+}
+
int main(int argc, char *argv[])
{
struct gatt_db *service_db_1, *service_db_2, *service_db_3;
- struct gatt_db *ts_small_db, *ts_large_db_1;
+ struct gatt_db *ts_small_db, *ts_large_db_1, *ts_unordered_db;
tester_init(&argc, &argv);
@@ -2357,6 +2430,7 @@ int main(int argc, char *argv[])
service_db_3 = make_service_data_3_db();
ts_small_db = make_test_spec_small_db();
ts_large_db_1 = make_test_spec_large_db_1();
+ ts_unordered_db = make_test_spec_unordered_db();
/*
* Server Configuration
@@ -4487,5 +4561,9 @@ int main(int argc, char *argv[])
raw_pdu(0xff, 0x00),
raw_pdu());
+ define_test_server("/robustness/hash-db",
+ test_hash_db, ts_unordered_db, NULL,
+ {});
+
return tester_run();
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [BlueZ,v3] shared/gatt-db: Fix munmap_chunk invalid pointer
2024-01-05 12:34 [PATCH BlueZ v3] shared/gatt-db: Fix munmap_chunk invalid pointer Frédéric Danis
@ 2024-01-05 13:42 ` bluez.test.bot
2024-01-05 15:08 ` [PATCH BlueZ v3] " Luiz Augusto von Dentz
1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2024-01-05 13:42 UTC (permalink / raw)
To: linux-bluetooth, frederic.danis
[-- Attachment #1: Type: text/plain, Size: 946 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=814647
---Test result---
Test Summary:
CheckPatch PASS 0.57 seconds
GitLint PASS 0.31 seconds
BuildEll PASS 23.72 seconds
BluezMake PASS 696.49 seconds
MakeCheck PASS 11.75 seconds
MakeDistcheck PASS 157.81 seconds
CheckValgrind PASS 220.34 seconds
CheckSmatch PASS 324.24 seconds
bluezmakeextell PASS 105.42 seconds
IncrementalBuild PASS 663.94 seconds
ScanBuild PASS 912.32 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH BlueZ v3] shared/gatt-db: Fix munmap_chunk invalid pointer
2024-01-05 12:34 [PATCH BlueZ v3] shared/gatt-db: Fix munmap_chunk invalid pointer Frédéric Danis
2024-01-05 13:42 ` [BlueZ,v3] " bluez.test.bot
@ 2024-01-05 15:08 ` Luiz Augusto von Dentz
2024-01-11 11:30 ` Frédéric Danis
1 sibling, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2024-01-05 15:08 UTC (permalink / raw)
To: Frédéric Danis; +Cc: linux-bluetooth
Hi Frédéric,
On Fri, Jan 5, 2024 at 7:34 AM Frédéric Danis
<frederic.danis@collabora.com> wrote:
>
> PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd
> and ending at 0xffff.
> 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.
> ---
> v1 -> v2: Replace next_handle by last_handle
> Check empty db using gatt_db_isempty(db) instead of
> next_handle == 0
> Add robustness unit test to check that gatt_db_get_hash()
> doesn't crash
> v2 -> v3: Fix line length checkpatch errors
> ---
> src/shared/gatt-db.c | 19 ++++++-----
> unit/test-gatt.c | 80 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 89 insertions(+), 10 deletions(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 676f963ec..9559583d1 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;
> + uint16_t last_handle;
Don't we have the last_handle already by doing queue_peek_tail, etc
and look into the database? Perhaps we want to avoid this sort of
lookup though, but I'd leave a comment in that case.
> struct queue *services;
>
> struct queue *notify_list;
> @@ -255,7 +255,7 @@ struct gatt_db *gatt_db_new(void)
> db->crypto = bt_crypto_new();
> db->services = queue_new();
> db->notify_list = queue_new();
> - db->next_handle = 0x0001;
> + db->last_handle = 0x0000;
>
> return gatt_db_ref(db);
> }
> @@ -356,14 +356,15 @@ static bool db_hash_update(void *user_data)
>
> db->hash_id = 0;
>
> - if (!db->next_handle)
> + if (gatt_db_isempty(db))
> return false;
>
> - hash.iov = new0(struct iovec, db->next_handle);
> + hash.iov = new0(struct iovec, db->last_handle + 1);
> hash.i = 0;
>
> gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash);
> - bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->hash);
> + bt_crypto_gatt_hash(db->crypto, hash.iov, db->last_handle + 1,
> + db->hash);
>
> for (i = 0; i < hash.i; i++)
> free(hash.iov[i].iov_base);
> @@ -624,7 +625,7 @@ bool gatt_db_clear_range(struct gatt_db *db, uint16_t start_handle,
>
> done:
> if (gatt_db_isempty(db))
> - db->next_handle = 0;
> + db->last_handle = 0;
>
> return true;
> }
> @@ -700,7 +701,7 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
> return NULL;
>
> if (!handle)
> - handle = db->next_handle;
> + handle = db->last_handle + 1;
>
> if (num_handles < 1 || (handle + num_handles - 1) > UINT16_MAX)
> return NULL;
> @@ -747,8 +748,8 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
> service->attributes[0]->handle = handle;
> 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);
> + /* Fast-forward last_handle if the new service was added to the end */
> + db->last_handle = MAX(handle + num_handles - 1, db->last_handle);
>
> return service->attributes[0];
Let's split the new test case into its own patch.
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index f92d860c4..9457d9079 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -1908,6 +1908,67 @@ static struct gatt_db *make_test_spec_large_db_1(void)
> return make_db(specs);
> }
>
> +/*
> + * Defined Test database 3:
> + * Tiny database fits into a single minimum sized-pdu with services unordered.
> + * Satisfies requirements:
> + * 5. At least one characteristic at the MAX handle
> + * 7. at least one service uuid with multiple instances
> + * 8. Some simple services, some with included services
> + * 9. an instance where handle of included service comes before the including
> + * service
> + * 11. Simple characteristics (no desc) and complex characteristics
> + * (multiple descriptors)
> + * 12. Instances of complex chars with 16-bit and 128-bit uuids
> + * (although not in scrambled order)
> + */
> +
> +static struct gatt_db *make_test_spec_unordered_db(void)
> +{
> + const struct att_handle_spec specs[] = {
> + SECONDARY_SERVICE(0x0003, DEVICE_INFORMATION_UUID, 16),
> + CHARACTERISTIC_STR(GATT_CHARAC_MANUFACTURER_NAME_STRING,
> + BT_ATT_PERM_READ |
> + BT_ATT_PERM_WRITE,
> + BT_GATT_CHRC_PROP_READ |
> + BT_GATT_CHRC_PROP_NOTIFY |
> + BT_GATT_CHRC_PROP_INDICATE |
> + BT_GATT_CHRC_PROP_EXT_PROP,
> + "BlueZ"),
> + DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ |
> + BT_ATT_PERM_WRITE, 0x00, 0x00),
> + DESCRIPTOR_STR(GATT_CHARAC_USER_DESC_UUID, BT_ATT_PERM_READ,
> + "Manufacturer Name"),
> + DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
> + 0x00),
> + CHARACTERISTIC_STR(GATT_CHARAC_SOFTWARE_REVISION_STRING,
> + BT_ATT_PERM_READ,
> + BT_GATT_CHRC_PROP_READ |
> + BT_GATT_CHRC_PROP_INDICATE,
> + "5.59"),
> + DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ
> + | BT_ATT_PERM_WRITE, 0x00, 0x00),
> +
> + PRIMARY_SERVICE(0xFFFF - 9 + 1, GAP_UUID, 9),
> + INCLUDE(0x0003),
> + CHARACTERISTIC_STR(GATT_CHARAC_DEVICE_NAME, BT_ATT_PERM_READ,
> + BT_GATT_CHRC_PROP_READ,
> + "BlueZ Unit Tester"),
> + CHARACTERISTIC(0000B009-0000-0000-0123-456789abcdef,
> + BT_ATT_PERM_READ | BT_ATT_PERM_WRITE,
> + BT_GATT_CHRC_PROP_READ |
> + BT_GATT_CHRC_PROP_EXT_PROP, 0x09),
> + DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
> + 0x00),
> + CHARACTERISTIC(GATT_CHARAC_APPEARANCE, BT_ATT_PERM_READ,
> + BT_GATT_CHRC_PROP_READ, 0x00, 0x00),
> + PRIMARY_SERVICE(0x0001, DEVICE_INFORMATION_UUID, 1),
> + { }
> + };
> +
> + return make_db(specs);
> +}
> +
> static void test_client(gconstpointer data)
> {
> create_context(512, data);
> @@ -2345,10 +2406,22 @@ static const struct test_step test_indication_server_1 = {
> .length = 0x03,
> };
>
> +static void test_hash_db(gconstpointer data)
> +{
> + struct context *context = create_context(512, data);
> +
> + /* test that gatt_db_get_hash is able to manage unordered db and
> + * doesn't crash
> + */
> + gatt_db_get_hash(context->server_db);
> +
> + context_quit(context);
> +}
> +
> int main(int argc, char *argv[])
> {
> struct gatt_db *service_db_1, *service_db_2, *service_db_3;
> - struct gatt_db *ts_small_db, *ts_large_db_1;
> + struct gatt_db *ts_small_db, *ts_large_db_1, *ts_unordered_db;
>
> tester_init(&argc, &argv);
>
> @@ -2357,6 +2430,7 @@ int main(int argc, char *argv[])
> service_db_3 = make_service_data_3_db();
> ts_small_db = make_test_spec_small_db();
> ts_large_db_1 = make_test_spec_large_db_1();
> + ts_unordered_db = make_test_spec_unordered_db();
Afaik the db cannot be unordered, handles always must be in ascending
order, what can happen is to have gaps in between, so I'd rename this
to be something like ts_tail_db or something like that. Is there a
specific test case in the test spec that does require this db though,
looks like you are just introducing a custom one, which is valid, but
I also want to make use of this db on the standard tests as well if
that is how they are intended.
>
> /*
> * Server Configuration
> @@ -4487,5 +4561,9 @@ int main(int argc, char *argv[])
> raw_pdu(0xff, 0x00),
> raw_pdu());
>
> + define_test_server("/robustness/hash-db",
> + test_hash_db, ts_unordered_db, NULL,
> + {});
> +
> return tester_run();
> }
> --
> 2.34.1
>
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH BlueZ v3] shared/gatt-db: Fix munmap_chunk invalid pointer
2024-01-05 15:08 ` [PATCH BlueZ v3] " Luiz Augusto von Dentz
@ 2024-01-11 11:30 ` Frédéric Danis
0 siblings, 0 replies; 4+ messages in thread
From: Frédéric Danis @ 2024-01-11 11:30 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
On 05/01/2024 16:08, Luiz Augusto von Dentz wrote:
> Hi Frédéric,
>
> On Fri, Jan 5, 2024 at 7:34 AM Frédéric Danis
> <frederic.danis@collabora.com> wrote:
>> PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd
>> and ending at 0xffff.
>> 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.
>> ---
>> v1 -> v2: Replace next_handle by last_handle
>> Check empty db using gatt_db_isempty(db) instead of
>> next_handle == 0
>> Add robustness unit test to check that gatt_db_get_hash()
>> doesn't crash
>> v2 -> v3: Fix line length checkpatch errors
>> ---
>> src/shared/gatt-db.c | 19 ++++++-----
>> unit/test-gatt.c | 80 +++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 89 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index 676f963ec..9559583d1 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;
>> + uint16_t last_handle;
> Don't we have the last_handle already by doing queue_peek_tail, etc
> and look into the database? Perhaps we want to avoid this sort of
> lookup though, but I'd leave a comment in that case.
Ok, I will add a comment about it in the commit message.
>> struct queue *services;
>>
>> struct queue *notify_list;
>> @@ -255,7 +255,7 @@ struct gatt_db *gatt_db_new(void)
>> db->crypto = bt_crypto_new();
>> db->services = queue_new();
>> db->notify_list = queue_new();
>> - db->next_handle = 0x0001;
>> + db->last_handle = 0x0000;
>>
>> return gatt_db_ref(db);
>> }
>> @@ -356,14 +356,15 @@ static bool db_hash_update(void *user_data)
>>
>> db->hash_id = 0;
>>
>> - if (!db->next_handle)
>> + if (gatt_db_isempty(db))
>> return false;
>>
>> - hash.iov = new0(struct iovec, db->next_handle);
>> + hash.iov = new0(struct iovec, db->last_handle + 1);
>> hash.i = 0;
>>
>> gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash);
>> - bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->hash);
>> + bt_crypto_gatt_hash(db->crypto, hash.iov, db->last_handle + 1,
>> + db->hash);
>>
>> for (i = 0; i < hash.i; i++)
>> free(hash.iov[i].iov_base);
>> @@ -624,7 +625,7 @@ bool gatt_db_clear_range(struct gatt_db *db, uint16_t start_handle,
>>
>> done:
>> if (gatt_db_isempty(db))
>> - db->next_handle = 0;
>> + db->last_handle = 0;
>>
>> return true;
>> }
>> @@ -700,7 +701,7 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
>> return NULL;
>>
>> if (!handle)
>> - handle = db->next_handle;
>> + handle = db->last_handle + 1;
>>
>> if (num_handles < 1 || (handle + num_handles - 1) > UINT16_MAX)
>> return NULL;
>> @@ -747,8 +748,8 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db,
>> service->attributes[0]->handle = handle;
>> 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);
>> + /* Fast-forward last_handle if the new service was added to the end */
>> + db->last_handle = MAX(handle + num_handles - 1, db->last_handle);
>>
>> return service->attributes[0];
> Let's split the new test case into its own patch.
Ok
>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>> index f92d860c4..9457d9079 100644
>> --- a/unit/test-gatt.c
>> +++ b/unit/test-gatt.c
>> @@ -1908,6 +1908,67 @@ static struct gatt_db *make_test_spec_large_db_1(void)
>> return make_db(specs);
>> }
>>
>> +/*
>> + * Defined Test database 3:
>> + * Tiny database fits into a single minimum sized-pdu with services unordered.
>> + * Satisfies requirements:
>> + * 5. At least one characteristic at the MAX handle
>> + * 7. at least one service uuid with multiple instances
>> + * 8. Some simple services, some with included services
>> + * 9. an instance where handle of included service comes before the including
>> + * service
>> + * 11. Simple characteristics (no desc) and complex characteristics
>> + * (multiple descriptors)
>> + * 12. Instances of complex chars with 16-bit and 128-bit uuids
>> + * (although not in scrambled order)
>> + */
>> +
>> +static struct gatt_db *make_test_spec_unordered_db(void)
>> +{
>> + const struct att_handle_spec specs[] = {
>> + SECONDARY_SERVICE(0x0003, DEVICE_INFORMATION_UUID, 16),
>> + CHARACTERISTIC_STR(GATT_CHARAC_MANUFACTURER_NAME_STRING,
>> + BT_ATT_PERM_READ |
>> + BT_ATT_PERM_WRITE,
>> + BT_GATT_CHRC_PROP_READ |
>> + BT_GATT_CHRC_PROP_NOTIFY |
>> + BT_GATT_CHRC_PROP_INDICATE |
>> + BT_GATT_CHRC_PROP_EXT_PROP,
>> + "BlueZ"),
>> + DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ |
>> + BT_ATT_PERM_WRITE, 0x00, 0x00),
>> + DESCRIPTOR_STR(GATT_CHARAC_USER_DESC_UUID, BT_ATT_PERM_READ,
>> + "Manufacturer Name"),
>> + DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
>> + 0x00),
>> + CHARACTERISTIC_STR(GATT_CHARAC_SOFTWARE_REVISION_STRING,
>> + BT_ATT_PERM_READ,
>> + BT_GATT_CHRC_PROP_READ |
>> + BT_GATT_CHRC_PROP_INDICATE,
>> + "5.59"),
>> + DESCRIPTOR(GATT_CLIENT_CHARAC_CFG_UUID, BT_ATT_PERM_READ
>> + | BT_ATT_PERM_WRITE, 0x00, 0x00),
>> +
>> + PRIMARY_SERVICE(0xFFFF - 9 + 1, GAP_UUID, 9),
>> + INCLUDE(0x0003),
>> + CHARACTERISTIC_STR(GATT_CHARAC_DEVICE_NAME, BT_ATT_PERM_READ,
>> + BT_GATT_CHRC_PROP_READ,
>> + "BlueZ Unit Tester"),
>> + CHARACTERISTIC(0000B009-0000-0000-0123-456789abcdef,
>> + BT_ATT_PERM_READ | BT_ATT_PERM_WRITE,
>> + BT_GATT_CHRC_PROP_READ |
>> + BT_GATT_CHRC_PROP_EXT_PROP, 0x09),
>> + DESCRIPTOR(GATT_CHARAC_EXT_PROPER_UUID, BT_ATT_PERM_READ, 0x01,
>> + 0x00),
>> + CHARACTERISTIC(GATT_CHARAC_APPEARANCE, BT_ATT_PERM_READ,
>> + BT_GATT_CHRC_PROP_READ, 0x00, 0x00),
>> + PRIMARY_SERVICE(0x0001, DEVICE_INFORMATION_UUID, 1),
>> + { }
>> + };
>> +
>> + return make_db(specs);
>> +}
>> +
>> static void test_client(gconstpointer data)
>> {
>> create_context(512, data);
>> @@ -2345,10 +2406,22 @@ static const struct test_step test_indication_server_1 = {
>> .length = 0x03,
>> };
>>
>> +static void test_hash_db(gconstpointer data)
>> +{
>> + struct context *context = create_context(512, data);
>> +
>> + /* test that gatt_db_get_hash is able to manage unordered db and
>> + * doesn't crash
>> + */
>> + gatt_db_get_hash(context->server_db);
>> +
>> + context_quit(context);
>> +}
>> +
>> int main(int argc, char *argv[])
>> {
>> struct gatt_db *service_db_1, *service_db_2, *service_db_3;
>> - struct gatt_db *ts_small_db, *ts_large_db_1;
>> + struct gatt_db *ts_small_db, *ts_large_db_1, *ts_unordered_db;
>>
>> tester_init(&argc, &argv);
>>
>> @@ -2357,6 +2430,7 @@ int main(int argc, char *argv[])
>> service_db_3 = make_service_data_3_db();
>> ts_small_db = make_test_spec_small_db();
>> ts_large_db_1 = make_test_spec_large_db_1();
>> + ts_unordered_db = make_test_spec_unordered_db();
> Afaik the db cannot be unordered, handles always must be in ascending
> order, what can happen is to have gaps in between, so I'd rename this
> to be something like ts_tail_db or something like that. Is there a
> specific test case in the test spec that does require this db though,
> looks like you are just introducing a custom one, which is valid, but
> I also want to make use of this db on the standard tests as well if
> that is how they are intended.
Yes, the db is not unordered, the crash occurs when the db is setup in
unordered way, e.g. if handle 0x0001 is added after handle 0xFFFF.
This is not defined in the specs, so I added a custom function to create
the db and will update the comment on it.
>> /*
>> * Server Configuration
>> @@ -4487,5 +4561,9 @@ int main(int argc, char *argv[])
>> raw_pdu(0xff, 0x00),
>> raw_pdu());
>>
>> + define_test_server("/robustness/hash-db",
>> + test_hash_db, ts_unordered_db, NULL,
>> + {});
>> +
>> return tester_run();
>> }
>> --
>> 2.34.1
>>
>>
>
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-11 11:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-05 12:34 [PATCH BlueZ v3] shared/gatt-db: Fix munmap_chunk invalid pointer Frédéric Danis
2024-01-05 13:42 ` [BlueZ,v3] " bluez.test.bot
2024-01-05 15:08 ` [PATCH BlueZ v3] " Luiz Augusto von Dentz
2024-01-11 11:30 ` Frédéric Danis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).