[PATCH BlueZ 1/4] shared/att: Fix possible crash on disconnect

[PATCH BlueZ 1/4] shared/att: Fix possible crash on disconnect

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If there are pending request while disconnecting they would be notified
but clients may endup being freed in the proccess which will then be
calling bt_att_cancel to cancal its requests causing the following
trace:

Invalid read of size 4
   at 0x1D894C: enable_ccc_callback (gatt-client.c:1627)
   by 0x1D247B: disc_att_send_op (att.c:417)
   by 0x1CCC17: queue_remove_all (queue.c:354)
   by 0x1D47B7: disconnect_cb (att.c:635)
   by 0x1E0707: watch_callback (io-glib.c:170)
   by 0x48E963B: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.6400.4)
   by 0x48E9AC7: ??? (in /usr/lib/libglib-2.0.so.0.6400.4)
   by 0x48E9ECF: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.6400.4)
   by 0x1E0E97: mainloop_run (mainloop-glib.c:79)
   by 0x1E13B3: mainloop_run_with_signal (mainloop-notify.c:201)
   by 0x12BC3B: main (main.c:770)
 Address 0x7d40a28 is 24 bytes inside a block of size 32 free'd
   at 0x484A2E0: free (vg_replace_malloc.c:540)
   by 0x1CCC17: queue_remove_all (queue.c:354)
   by 0x1CCC83: queue_destroy (queue.c:73)
   by 0x1D7DD7: bt_gatt_client_free (gatt-client.c:2209)
   by 0x16497B: batt_free (battery.c:77)
   by 0x16497B: batt_remove (battery.c:286)
   by 0x1A0013: service_remove (service.c:176)
   by 0x1A9B7B: device_remove_gatt_service (device.c:3691)
   by 0x1A9B7B: gatt_service_removed (device.c:3805)
   by 0x1CC90B: queue_foreach (queue.c:220)
   by 0x1DE27B: notify_service_changed.isra.0.part.0 (gatt-db.c:369)
   by 0x1DE387: notify_service_changed (gatt-db.c:361)
   by 0x1DE387: gatt_db_service_destroy (gatt-db.c:385)
   by 0x1DE3EF: gatt_db_remove_service (gatt-db.c:519)
   by 0x1D674F: discovery_op_complete (gatt-client.c:388)
   by 0x1D6877: discover_primary_cb (gatt-client.c:1260)
   by 0x1E220B: discovery_op_complete (gatt-helpers.c:628)
   by 0x1E249B: read_by_grp_type_cb (gatt-helpers.c:730)
   by 0x1D247B: disc_att_send_op (att.c:417)
   by 0x1CCC17: queue_remove_all (queue.c:354)
   by 0x1D47B7: disconnect_cb (att.c:635)
---
 src/shared/att.c | 46 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index ed3af2920..58f23dfcb 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -84,6 +84,7 @@ struct bt_att {
 	struct queue *req_queue;	/* Queued ATT protocol requests */
 	struct queue *ind_queue;	/* Queued ATT protocol indications */
 	struct queue *write_queue;	/* Queue of PDUs ready to send */
+	bool in_disc;			/* Cleanup queues on disconnect_cb */
 
 	bt_att_timeout_func_t timeout_callback;
 	bt_att_destroy_func_t timeout_destroy;
@@ -222,8 +223,10 @@ static void destroy_att_send_op(void *data)
 	free(op);
 }
 
-static void cancel_att_send_op(struct att_send_op *op)
+static void cancel_att_send_op(void *data)
 {
+	struct att_send_op *op = data;
+
 	if (op->destroy)
 		op->destroy(op->user_data);
 
@@ -631,11 +634,6 @@ static bool disconnect_cb(struct io *io, void *user_data)
 	/* Dettach channel */
 	queue_remove(att->chans, chan);
 
-	/* Notify request callbacks */
-	queue_remove_all(att->req_queue, NULL, NULL, disc_att_send_op);
-	queue_remove_all(att->ind_queue, NULL, NULL, disc_att_send_op);
-	queue_remove_all(att->write_queue, NULL, NULL, disc_att_send_op);
-
 	if (chan->pending_req) {
 		disc_att_send_op(chan->pending_req);
 		chan->pending_req = NULL;
@@ -654,6 +652,15 @@ static bool disconnect_cb(struct io *io, void *user_data)
 
 	bt_att_ref(att);
 
+	att->in_disc = true;
+
+	/* Notify request callbacks */
+	queue_remove_all(att->req_queue, NULL, NULL, disc_att_send_op);
+	queue_remove_all(att->ind_queue, NULL, NULL, disc_att_send_op);
+	queue_remove_all(att->write_queue, NULL, NULL, disc_att_send_op);
+
+	att->in_disc = false;
+
 	queue_foreach(att->disconn_list, disconn_handler, INT_TO_PTR(err));
 
 	bt_att_unregister_all(att);
@@ -1574,6 +1581,30 @@ bool bt_att_chan_cancel(struct bt_att_chan *chan, unsigned int id)
 	return true;
 }
 
+static bool bt_att_disc_cancel(struct bt_att *att, unsigned int id)
+{
+	struct att_send_op *op;
+
+	op = queue_find(att->req_queue, match_op_id, UINT_TO_PTR(id));
+	if (op)
+		goto done;
+
+	op = queue_find(att->ind_queue, match_op_id, UINT_TO_PTR(id));
+	if (op)
+		goto done;
+
+	op = queue_find(att->write_queue, match_op_id, UINT_TO_PTR(id));
+
+done:
+	if (!op)
+		return false;
+
+	/* Just cancel since disconnect_cb will be cleaning up */
+	cancel_att_send_op(op);
+
+	return true;
+}
+
 bool bt_att_cancel(struct bt_att *att, unsigned int id)
 {
 	const struct queue_entry *entry;
@@ -1591,6 +1622,9 @@ bool bt_att_cancel(struct bt_att *att, unsigned int id)
 			return true;
 	}
 
+	if (att->in_disc)
+		return bt_att_disc_cancel(att, id);
+
 	op = queue_remove_if(att->req_queue, match_op_id, UINT_TO_PTR(id));
 	if (op)
 		goto done;
-- 
2.25.3

[PATCH BlueZ 2/4] shared/gatt-db: Add support for notifying attribute changes

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This enables to get notified when an attribute has been changed e.g.
it is being removed so the code can detect changes changes at attribute
level.
---
 src/shared/gatt-db.c | 103 +++++++++++++++++++++++++++++++++++++++++++
 src/shared/gatt-db.h |   8 ++++
 2 files changed, 111 insertions(+)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 5eccab3b9..e939ddc3a 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -81,6 +81,13 @@ struct notify {
 	void *user_data;
 };
 
+struct attribute_notify {
+	unsigned int id;
+	gatt_db_attribute_cb_t removed;
+	gatt_db_destroy_func_t destroy;
+	void *user_data;
+};
+
 struct pending_read {
 	struct gatt_db_attribute *attrib;
 	unsigned int id;
@@ -114,6 +121,9 @@ struct gatt_db_attribute {
 
 	unsigned int write_id;
 	struct queue *pending_writes;
+
+	unsigned int next_notify_id;
+	struct queue *notify_list;
 };
 
 struct gatt_db_service {
@@ -171,6 +181,16 @@ static void pending_write_free(void *data)
 	pending_write_result(p, -ECANCELED);
 }
 
+static void attribute_notify_destroy(void *data)
+{
+	struct attribute_notify *notify = data;
+
+	if (notify->destroy)
+		notify->destroy(notify->user_data);
+
+	free(notify);
+}
+
 static void attribute_destroy(struct gatt_db_attribute *attribute)
 {
 	/* Attribute was not initialized by user */
@@ -179,6 +199,7 @@ static void attribute_destroy(struct gatt_db_attribute *attribute)
 
 	queue_destroy(attribute->pending_reads, pending_read_free);
 	queue_destroy(attribute->pending_writes, pending_write_free);
+	queue_destroy(attribute->notify_list, attribute_notify_destroy);
 
 	free(attribute->value);
 	free(attribute);
@@ -208,6 +229,7 @@ static struct gatt_db_attribute *new_attribute(struct gatt_db_service *service,
 
 	attribute->pending_reads = queue_new();
 	attribute->pending_writes = queue_new();
+	attribute->notify_list = queue_new();
 
 	return attribute;
 
@@ -352,12 +374,38 @@ static bool db_hash_update(void *user_data)
 	return false;
 }
 
+static void handle_attribute_notify(void *data, void *user_data)
+{
+	struct attribute_notify *notify = data;
+	struct gatt_db_attribute *attrib = user_data;
+
+	if (notify->removed)
+		notify->removed(attrib, notify->user_data);
+}
+
+static void notify_attribute_changed(struct gatt_db_service *service)
+{
+	int i;
+
+	for (i = 0; i < service->num_handles; i++) {
+		struct gatt_db_attribute *attr = service->attributes[i];
+
+		if (!attr)
+			continue;
+
+		queue_foreach(attr->notify_list, handle_attribute_notify, attr);
+	}
+}
+
 static void notify_service_changed(struct gatt_db *db,
 						struct gatt_db_service *service,
 						bool added)
 {
 	struct notify_data data;
 
+	if (!added)
+		notify_attribute_changed(service);
+
 	if (queue_isempty(db->notify_list))
 		return;
 
@@ -1993,3 +2041,58 @@ void *gatt_db_attribute_get_user_data(struct gatt_db_attribute *attrib)
 
 	return attrib->user_data;
 }
+
+static bool match_attribute_notify_id(const void *a, const void *b)
+{
+	const struct attribute_notify *notify = a;
+	unsigned int id = PTR_TO_UINT(b);
+
+	return notify->id == id;
+}
+
+unsigned int gatt_db_attribute_register(struct gatt_db_attribute *attrib,
+					gatt_db_attribute_cb_t removed,
+					void *user_data,
+					gatt_db_destroy_func_t destroy)
+{
+	struct attribute_notify *notify;
+
+	if (!attrib || !removed)
+		return 0;
+
+	notify = new0(struct attribute_notify, 1);
+	notify->removed = removed;
+	notify->destroy = destroy;
+	notify->user_data = user_data;
+
+	if (attrib->next_notify_id < 1)
+		attrib->next_notify_id = 1;
+
+	notify->id = attrib->next_notify_id++;
+
+	if (!queue_push_tail(attrib->notify_list, notify)) {
+		free(notify);
+		return 0;
+	}
+
+	return notify->id;
+}
+
+bool gatt_db_attribute_unregister(struct gatt_db_attribute *attrib,
+						unsigned int id)
+{
+	struct attribute_notify *notify;
+
+	if (!attrib || !id)
+		return false;
+
+	notify = queue_find(attrib->notify_list, match_attribute_notify_id,
+						UINT_TO_PTR(id));
+	if (!notify)
+		return false;
+
+	queue_remove(attrib->notify_list, notify);
+	attribute_notify_destroy(notify);
+
+	return true;
+}
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index a0fd66c53..5bf19d302 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -281,3 +281,11 @@ bool gatt_db_attribute_write_result(struct gatt_db_attribute *attrib,
 bool gatt_db_attribute_reset(struct gatt_db_attribute *attrib);
 
 void *gatt_db_attribute_get_user_data(struct gatt_db_attribute *attrib);
+
+unsigned int gatt_db_attribute_register(struct gatt_db_attribute *attrib,
+					gatt_db_attribute_cb_t removed,
+					void *user_data,
+					gatt_db_destroy_func_t destroy);
+
+bool gatt_db_attribute_unregister(struct gatt_db_attribute *attrib,
+						unsigned int id);
-- 
2.25.3

[PATCH BlueZ 3/4] shared/gatt-client: Remove notification if its attribute is removed

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If the attribute is being removed from the database it means the
notification shall also be dropped, that way users don't have to
cleanup its subscriptions themselves.
---
 src/shared/gatt-client.c | 70 +++++++++++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 18 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 0b81a7a5c..e21aca1f0 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -174,9 +174,12 @@ static void request_unref(void *data)
 }
 
 struct notify_chrc {
+	struct bt_gatt_client *client;
+	struct gatt_db_attribute *attr;
 	uint16_t value_handle;
 	uint16_t ccc_handle;
 	uint16_t properties;
+	unsigned int notify_id;
 	int notify_count;  /* Reference count of registered notify callbacks */
 
 	/* Pending calls to register_notify are queued here so that they can be
@@ -235,6 +238,51 @@ static void find_ccc(struct gatt_db_attribute *attr, void *user_data)
 	*ccc_ptr = attr;
 }
 
+static bool match_notify_chrc(const void *data, const void *user_data)
+{
+	const struct notify_data *notify_data = data;
+	const struct notify_chrc *chrc = user_data;
+
+	return notify_data->chrc == chrc;
+}
+
+static void notify_data_cleanup(void *data)
+{
+	struct notify_data *notify_data = data;
+
+	if (notify_data->att_id)
+		bt_att_cancel(notify_data->client->att, notify_data->att_id);
+
+	notify_data_unref(notify_data);
+}
+
+static void notify_chrc_free(void *data)
+{
+	struct notify_chrc *chrc = data;
+
+	if (chrc->notify_id)
+		gatt_db_attribute_unregister(chrc->attr, chrc->notify_id);
+
+	queue_destroy(chrc->reg_notify_queue, notify_data_unref);
+	free(chrc);
+}
+
+static void chrc_removed(struct gatt_db_attribute *attr, void *user_data)
+{
+	struct notify_chrc *chrc = user_data;
+	struct bt_gatt_client *client = chrc->client;
+	struct notify_data *data;
+
+	chrc->notify_id = 0;
+
+	while ((data = queue_remove_if(client->notify_list, match_notify_chrc,
+								chrc)))
+		notify_data_cleanup(data);
+
+	queue_remove(client->notify_chrcs, chrc);
+	notify_chrc_free(chrc);
+}
+
 static struct notify_chrc *notify_chrc_create(struct bt_gatt_client *client,
 							uint16_t value_handle)
 {
@@ -274,22 +322,18 @@ static struct notify_chrc *notify_chrc_create(struct bt_gatt_client *client,
 	if (ccc)
 		chrc->ccc_handle = gatt_db_attribute_get_handle(ccc);
 
+	chrc->client = client;
+	chrc->attr = attr;
 	chrc->value_handle = value_handle;
 	chrc->properties = properties;
+	chrc->notify_id = gatt_db_attribute_register(attr, chrc_removed, chrc,
+									NULL);
 
 	queue_push_tail(client->notify_chrcs, chrc);
 
 	return chrc;
 }
 
-static void notify_chrc_free(void *data)
-{
-	struct notify_chrc *chrc = data;
-
-	queue_destroy(chrc->reg_notify_queue, notify_data_unref);
-	free(chrc);
-}
-
 static bool match_notify_data_id(const void *a, const void *b)
 {
 	const struct notify_data *notify_data = a;
@@ -303,16 +347,6 @@ struct handle_range {
 	uint16_t end;
 };
 
-static void notify_data_cleanup(void *data)
-{
-	struct notify_data *notify_data = data;
-
-	if (notify_data->att_id)
-		bt_att_cancel(notify_data->client->att, notify_data->att_id);
-
-	notify_data_unref(notify_data);
-}
-
 struct discovery_op;
 
 typedef void (*discovery_op_complete_func_t)(struct discovery_op *op,
-- 
2.25.3

[PATCH BlueZ 4/4] share/gatt-client: Don't remove active services

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Only remove services that have not been fetched completed as
unsuccessful discovery may be the result of an aborted connection the
attributes that have been fetched previously are likely to be valid.
---
 src/shared/gatt-client.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index e21aca1f0..5ba8e83ba 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -407,11 +407,17 @@ static void discovery_op_complete(struct discovery_op *op, bool success,
 	gatt_db_unregister(op->client->db, op->db_id);
 	op->db_id = 0;
 
-	/* Remove services pending */
+	/* Remove staled services */
 	for (svc = queue_get_entries(op->pending_svcs); svc; svc = svc->next) {
 		struct gatt_db_attribute *attr = svc->data;
 		uint16_t start, end;
 
+		/* Don't remove services that already been marked as active
+		 * previously.
+		 */
+		if (gatt_db_service_get_active(attr))
+			continue;
+
 		gatt_db_attribute_get_service_data(attr, &start, &end,
 							NULL, NULL);
 
-- 
2.25.3

RE: [BlueZ,1/4] shared/att: Fix possible crash on disconnect

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1024 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.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
checkpatch Failed

Outputs:
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#17: 
   by 0x48E963B: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.6400.4)

- total: 0 errors, 1 warnings, 83 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.



---
Regards,
Linux Bluetooth

RE: [BlueZ,1/4] shared/att: Fix possible crash on disconnect

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 601 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.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
makecheck Failed

Outputs:
./test-driver: line 107: 14798 Bus error               (core dumped) "$@" > $log_file 2>&1
make[3]: *** [Makefile:9726: test-suite.log] Error 1
make[2]: *** [Makefile:9834: check-TESTS] Error 2
make[1]: *** [Makefile:10228: check-am] Error 2
make: *** [Makefile:10230: check] Error 2



---
Regards,
Linux Bluetooth