From: Luiz Augusto von Dentz <[email protected]>
gatt_db_service_insert_characteristic shall not attempt to insert the
characteristic attribute handle on the next available index as there
could be descriptors in between so this changes the way
get_attribute_index calculates the index based on the given handle to
properly skip indexes used by descriptors.
---
monitor/att.c | 7 +--
src/gatt-database.c | 7 +--
src/settings.c | 3 +-
src/shared/gatt-client.c | 3 ++
src/shared/gatt-db.c | 101 +++++++++++++++++++++++----------------
src/shared/gatt-db.h | 2 +
unit/test-gatt.c | 1 +
7 files changed, 76 insertions(+), 48 deletions(-)
diff --git a/monitor/att.c b/monitor/att.c
index 3e5d7f12d182..b3fb3ba6a0ad 100644
--- a/monitor/att.c
+++ b/monitor/att.c
@@ -506,6 +506,7 @@ static struct gatt_db *get_db(const struct l2cap_frame *frame, bool rsp)
static struct gatt_db_attribute *insert_chrc(const struct l2cap_frame *frame,
uint16_t handle,
+ uint16_t value_handle,
bt_uuid_t *uuid, uint8_t prop,
bool rsp)
{
@@ -515,8 +516,8 @@ static struct gatt_db_attribute *insert_chrc(const struct l2cap_frame *frame,
if (!db)
return NULL;
- return gatt_db_insert_characteristic(db, handle, uuid, 0, prop, NULL,
- NULL, NULL);
+ return gatt_db_insert_characteristic(db, handle, value_handle, uuid, 0,
+ prop, NULL, NULL, NULL);
}
static int bt_uuid_from_data(bt_uuid_t *uuid, const void *data, uint16_t size)
@@ -615,7 +616,7 @@ static void print_chrc(const struct l2cap_frame *frame)
print_uuid(" Value UUID", frame->data, frame->size);
bt_uuid_from_data(&uuid, frame->data, frame->size);
- insert_chrc(frame, handle, &uuid, prop, true);
+ insert_chrc(frame, handle - 1, handle, &uuid, prop, true);
}
static void chrc_read(const struct l2cap_frame *frame)
diff --git a/src/gatt-database.c b/src/gatt-database.c
index 7221ffc87f0d..6c11027a79ed 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -3352,9 +3352,10 @@ static bool database_add_chrc(struct external_service *service,
}
chrc->attrib = gatt_db_service_insert_characteristic(service->attrib,
- handle, &uuid, chrc->perm,
- chrc->props, chrc_read_cb,
- chrc_write_cb, chrc);
+ handle - 1, handle, &uuid,
+ chrc->perm, chrc->props,
+ chrc_read_cb, chrc_write_cb,
+ chrc);
if (!chrc->attrib) {
error("Failed to create characteristic entry in database");
return false;
diff --git a/src/settings.c b/src/settings.c
index 85534f2c7aca..033e9670ac40 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -125,7 +125,8 @@ static int load_chrc(struct gatt_db *db, char *handle, char *value,
handle_int, value_handle,
properties, val_len ? val_str : "", uuid_str);
- att = gatt_db_service_insert_characteristic(service, value_handle,
+ att = gatt_db_service_insert_characteristic(service, handle_int,
+ value_handle,
&uuid, 0, properties,
NULL, NULL, NULL);
if (!att || gatt_db_attribute_get_handle(att) != value_handle)
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 6340bcd8508e..dcf6f0211a67 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -735,6 +735,7 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
}
attr = gatt_db_insert_characteristic(client->db,
+ chrc_data->start_handle,
chrc_data->value_handle,
&chrc_data->uuid, 0,
chrc_data->properties,
@@ -829,6 +830,8 @@ done:
return true;
failed:
+ DBG(client, "Failed to discover descriptors");
+
free(chrc_data);
return false;
}
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 9559583d11a7..2c8e7d31eda1 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -825,28 +825,45 @@ bool gatt_db_set_authorize(struct gatt_db *db, gatt_db_authorize_cb_t cb,
return true;
}
-static uint16_t get_attribute_index(struct gatt_db_service *service,
+static uint16_t service_get_attribute_index(struct gatt_db_service *service,
+ uint16_t *handle,
int end_offset)
{
int i = 0;
- /* Here we look for first free attribute index with given offset */
- while (i < (service->num_handles - end_offset) &&
+ if (!service || !service->attributes[0] || !handle)
+ return 0;
+
+ if (*handle) {
+ /* Check if handle is in within service range */
+ if (*handle < service->attributes[0]->handle)
+ return 0;
+
+ /* Return index based on given handle */
+ i = *handle - service->attributes[0]->handle;
+ } else {
+ /* Here we look for first free attribute index with given
+ * offset.
+ */
+ while (i < (service->num_handles - end_offset) &&
service->attributes[i])
- i++;
+ i++;
+ }
- return i == (service->num_handles - end_offset) ? 0 : i;
-}
+ if (i >= (service->num_handles - end_offset))
+ return 0;
-static uint16_t get_handle_at_index(struct gatt_db_service *service,
- int index)
-{
- return service->attributes[index]->handle;
+ /* Set handle based on the index */
+ if (!(*handle))
+ *handle = service->attributes[0]->handle + i;
+
+ return i;
}
static struct gatt_db_attribute *
service_insert_characteristic(struct gatt_db_service *service,
uint16_t handle,
+ uint16_t value_handle,
const bt_uuid_t *uuid,
uint32_t permissions,
uint8_t properties,
@@ -854,6 +871,7 @@ service_insert_characteristic(struct gatt_db_service *service,
gatt_db_write_t write_func,
void *user_data)
{
+ struct gatt_db_attribute **chrc;
uint8_t value[MAX_CHAR_DECL_VALUE_LEN];
uint16_t len = 0;
int i;
@@ -874,37 +892,48 @@ service_insert_characteristic(struct gatt_db_service *service,
if (handle == UINT16_MAX)
return NULL;
- i = get_attribute_index(service, 1);
+ i = service_get_attribute_index(service, &handle, 1);
if (!i)
return NULL;
- if (!handle)
- handle = get_handle_at_index(service, i - 1) + 2;
-
value[0] = properties;
len += sizeof(properties);
/* We set handle of characteristic value, which will be added next */
- put_le16(handle, &value[1]);
+ put_le16(value_handle, &value[1]);
len += sizeof(uint16_t);
len += uuid_to_le(uuid, &value[3]);
- service->attributes[i] = new_attribute(service, handle - 1,
+ service->attributes[i] = new_attribute(service, handle,
&characteristic_uuid,
value, len);
if (!service->attributes[i])
return NULL;
- set_attribute_data(service->attributes[i], NULL, NULL, BT_ATT_PERM_READ, NULL);
+ chrc = &service->attributes[i];
+ set_attribute_data(service->attributes[i], NULL, NULL, BT_ATT_PERM_READ,
+ NULL);
- i++;
-
- service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0);
- if (!service->attributes[i]) {
- free(service->attributes[i - 1]);
+ i = service_get_attribute_index(service, &value_handle, 0);
+ if (!i) {
+ free(*chrc);
+ *chrc = NULL;
return NULL;
}
+ service->attributes[i] = new_attribute(service, value_handle, uuid,
+ NULL, 0);
+ if (!service->attributes[i]) {
+ free(*chrc);
+ *chrc = NULL;
+ return NULL;
+ }
+
+ /* Update handle of characteristic value_handle if it has changed */
+ put_le16(value_handle, &value[1]);
+ if (memcmp((*chrc)->value, value, len))
+ memcpy((*chrc)->value, value, len);
+
set_attribute_data(service->attributes[i], read_func, write_func,
permissions, user_data);
@@ -914,6 +943,7 @@ service_insert_characteristic(struct gatt_db_service *service,
struct gatt_db_attribute *
gatt_db_insert_characteristic(struct gatt_db *db,
uint16_t handle,
+ uint16_t value_handle,
const bt_uuid_t *uuid,
uint32_t permissions,
uint8_t properties,
@@ -927,7 +957,8 @@ gatt_db_insert_characteristic(struct gatt_db *db,
if (!attrib)
return NULL;
- return service_insert_characteristic(attrib->service, handle, uuid,
+ return service_insert_characteristic(attrib->service, handle,
+ value_handle, uuid,
permissions, properties,
read_func, write_func,
user_data);
@@ -936,6 +967,7 @@ gatt_db_insert_characteristic(struct gatt_db *db,
struct gatt_db_attribute *
gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib,
uint16_t handle,
+ uint16_t value_handle,
const bt_uuid_t *uuid,
uint32_t permissions,
uint8_t properties,
@@ -946,7 +978,8 @@ gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib,
if (!attrib)
return NULL;
- return service_insert_characteristic(attrib->service, handle, uuid,
+ return service_insert_characteristic(attrib->service, handle,
+ value_handle, uuid,
permissions, properties,
read_func, write_func,
user_data);
@@ -964,7 +997,7 @@ gatt_db_service_add_characteristic(struct gatt_db_attribute *attrib,
if (!attrib)
return NULL;
- return service_insert_characteristic(attrib->service, 0, uuid,
+ return service_insert_characteristic(attrib->service, 0, 0, uuid,
permissions, properties,
read_func, write_func,
user_data);
@@ -981,17 +1014,10 @@ service_insert_descriptor(struct gatt_db_service *service,
{
int i;
- i = get_attribute_index(service, 0);
+ i = service_get_attribute_index(service, &handle, 0);
if (!i)
return NULL;
- /* Check if handle is in within service range */
- if (handle && handle <= service->attributes[0]->handle)
- return NULL;
-
- if (!handle)
- handle = get_handle_at_index(service, i - 1) + 1;
-
service->attributes[i] = new_attribute(service, handle, uuid, NULL, 0);
if (!service->attributes[i])
return NULL;
@@ -1151,17 +1177,10 @@ service_insert_included(struct gatt_db_service *service, uint16_t handle,
len += include->value_len;
}
- index = get_attribute_index(service, 0);
+ index = service_get_attribute_index(service, &handle, 0);
if (!index)
return NULL;
- /* Check if handle is in within service range */
- if (handle && handle <= service->attributes[0]->handle)
- return NULL;
-
- if (!handle)
- handle = get_handle_at_index(service, index - 1) + 1;
-
service->attributes[index] = new_attribute(service, handle,
&included_service_uuid,
value, len);
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index fb939e40d40e..f7596e33529a 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -63,6 +63,7 @@ gatt_db_service_add_characteristic(struct gatt_db_attribute *attrib,
struct gatt_db_attribute *
gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib,
uint16_t handle,
+ uint16_t value_handle,
const bt_uuid_t *uuid,
uint32_t permissions,
uint8_t properties,
@@ -73,6 +74,7 @@ gatt_db_service_insert_characteristic(struct gatt_db_attribute *attrib,
struct gatt_db_attribute *
gatt_db_insert_characteristic(struct gatt_db *db,
uint16_t handle,
+ uint16_t value_handle,
const bt_uuid_t *uuid,
uint32_t permissions,
uint8_t properties,
diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 5e06d4ed4bf9..1613fbcb5f21 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -1237,6 +1237,7 @@ add_char_with_value(struct gatt_db_attribute *service_att, uint16_t handle,
if (handle)
attrib = gatt_db_service_insert_characteristic(service_att,
+ handle - 1,
handle, uuid,
att_permissions,
char_properties,
--
2.44.0
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=842520
---Test result---
Test Summary:
CheckPatch PASS 0.52 seconds
GitLint PASS 0.31 seconds
BuildEll PASS 24.58 seconds
BluezMake PASS 1681.37 seconds
MakeCheck PASS 13.38 seconds
MakeDistcheck PASS 174.59 seconds
CheckValgrind PASS 250.29 seconds
CheckSmatch WARNING 353.90 seconds
bluezmakeextell PASS 118.80 seconds
IncrementalBuild PASS 1477.58 seconds
ScanBuild WARNING 985.63 seconds
Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
monitor/att.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
src/shared/gatt-client.c:451:21: warning: Use of memory after it is freed
gatt_db_unregister(op->client->db, op->db_id);
^~~~~~~~~~
src/shared/gatt-client.c:696:2: warning: Use of memory after it is freed
discovery_op_complete(op, false, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:996:2: warning: Use of memory after it is freed
discovery_op_complete(op, success, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1102:2: warning: Use of memory after it is freed
discovery_op_complete(op, success, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1294:2: warning: Use of memory after it is freed
discovery_op_complete(op, success, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1359:2: warning: Use of memory after it is freed
discovery_op_complete(op, success, att_ecode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1634:6: warning: Use of memory after it is freed
if (read_db_hash(op)) {
^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1639:2: warning: Use of memory after it is freed
discover_all(op);
^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2143:6: warning: Use of memory after it is freed
if (read_db_hash(op)) {
^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2151:8: warning: Use of memory after it is freed
discovery_op_ref(op),
^~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3240:2: warning: Use of memory after it is freed
complete_write_long_op(req, success, 0, false);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3262:2: warning: Use of memory after it is freed
request_unref(req);
^~~~~~~~~~~~~~~~~~
12 warnings generated.
---
Regards,
Linux Bluetooth
Hello:
This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Mon, 8 Apr 2024 11:59:49 -0400 you wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> gatt_db_service_insert_characteristic shall not attempt to insert the
> characteristic attribute handle on the next available index as there
> could be descriptors in between so this changes the way
> get_attribute_index calculates the index based on the given handle to
> properly skip indexes used by descriptors.
>
> [...]
Here is the summary with links:
- [BlueZ,v1] shared/gatt-db: Fix gatt_db_service_insert_characteristic
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7604a577c9d7
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html