diff mbox series

[BlueZ,v1] shared/gatt-db: Fix gatt_db_service_insert_characteristic

Message ID 20240408155949.3622429-1-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Commit 7604a577c9d7bf15c9144b8154842ce277afec90
Headers show
Series [BlueZ,v1] shared/gatt-db: Fix gatt_db_service_insert_characteristic | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch warning CheckSparse WARNING monitor/att.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild warning ScanBuild: 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.

Commit Message

Luiz Augusto von Dentz April 8, 2024, 3:59 p.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

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(-)

Comments

bluez.test.bot@gmail.com April 8, 2024, 5:43 p.m. UTC | #1 
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
patchwork-bot+bluetooth@kernel.org April 8, 2024, 7:30 p.m. UTC | #2 
Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon,  8 Apr 2024 11:59:49 -0400 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> 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!
diff mbox series

Patch

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,