[PATCH] shared/gatt-client: Cancel discovery read-by-type requests

[Kyle Pepper <cochcoder@proton.me>]

Server Features and Database Hash reads outlive the client when it is
released during discovery. Their destroy callback then dereferences the
freed client while unregistering the discovery database callback.

Retain cancellable request references for both reads and release them
on completion or client cancellation. Preserve the boolean helper for
callers that do not retain requests. Add native ATT teardown regressions
for queued and pending reads, including repeated cancellation.

Fixes: 76d63c91b27a ("shared/gatt-client: Add support for EATT features")
Fixes: 0f67d5e2f19f ("shared/gatt-client: Read database hash if available")
Assisted-by: Codex:gpt-6
---
 src/shared/gatt-client.c  | 32 +++++++++++++++++++++---
 src/shared/gatt-helpers.c | 26 +++++++++++++++++---
 src/shared/gatt-helpers.h |  7 ++++++
 unit/test-gatt.c          | 52 ++++++++++++++++++++++++++++++++++++++-
 4 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 836a59a..d283376 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -105,6 +105,8 @@ struct bt_gatt_client {
 	unsigned int next_request_id;
 
 	struct bt_gatt_request *discovery_req;
+	struct bt_gatt_request *server_feat_req;
+	struct bt_gatt_request *db_hash_req;
 	unsigned int mtu_req_id;
 
 	/* Pending retry operation for DB out of sync handling */
@@ -1472,6 +1474,9 @@ static void db_hash_read_cb(bool success, uint8_t att_ecode,
 	uint16_t len, handle;
 	struct bt_gatt_iter iter;
 
+	bt_gatt_request_unref(client->db_hash_req);
+	client->db_hash_req = NULL;
+
 	if (!success)
 		goto discover;
 
@@ -1542,10 +1547,12 @@ static bool read_db_hash(struct discovery_op *op)
 	if (!op->hash)
 		return false;
 
-	if (!bt_gatt_read_by_type(client->att, 0x0001, 0xffff, &uuid,
+	client->db_hash_req = bt_gatt_read_by_type_request(client->att,
+							0x0001, 0xffff, &uuid,
 							db_hash_read_cb,
 							discovery_op_ref(op),
-							discovery_op_unref)) {
+							discovery_op_unref);
+	if (!client->db_hash_req) {
 		discovery_op_unref(op);
 		return false;
 	}
@@ -1562,6 +1569,9 @@ static void db_server_feat_read(bool success, uint8_t att_ecode,
 	uint16_t len, handle;
 	struct bt_gatt_iter iter;
 
+	bt_gatt_request_unref(client->server_feat_req);
+	client->server_feat_req = NULL;
+
 	if (!result)
 		return;
 
@@ -1606,10 +1616,12 @@ static void read_server_feat(struct discovery_op *op)
 			return;
 	}
 
-	if (!bt_gatt_read_by_type(client->att, 0x0001, 0xffff, &uuid,
+	client->server_feat_req = bt_gatt_read_by_type_request(client->att,
+							0x0001, 0xffff, &uuid,
 							db_server_feat_read,
 							discovery_op_ref(op),
-							discovery_op_unref))
+							discovery_op_unref);
+	if (!client->server_feat_req)
 		discovery_op_unref(op);
 }
 
@@ -2841,6 +2853,15 @@ static void cancel_pending(void *data)
 	cancel_request(data);
 }
 
+static void cancel_gatt_request(struct bt_gatt_request **req)
+{
+	struct bt_gatt_request *pending = *req;
+
+	*req = NULL;
+	bt_gatt_request_cancel(pending);
+	bt_gatt_request_unref(pending);
+}
+
 bool bt_gatt_client_cancel_all(struct bt_gatt_client *client)
 {
 	if (!client || !client->att)
@@ -2848,6 +2869,9 @@ bool bt_gatt_client_cancel_all(struct bt_gatt_client *client)
 
 	queue_remove_all(client->pending_requests, NULL, NULL, cancel_pending);
 
+	cancel_gatt_request(&client->server_feat_req);
+	cancel_gatt_request(&client->db_hash_req);
+
 	if (client->discovery_req) {
 		bt_gatt_request_cancel(client->discovery_req);
 		bt_gatt_request_unref(client->discovery_req);
diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index b3d5d8f..f33d387 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1356,7 +1356,8 @@ done:
 	discovery_op_complete(op, success, att_ecode);
 }
 
-bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
+struct bt_gatt_request *bt_gatt_read_by_type_request(struct bt_att *att,
+					uint16_t start, uint16_t end,
 					const bt_uuid_t *uuid,
 					bt_gatt_request_callback_t callback,
 					void *user_data,
@@ -1366,7 +1367,7 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
 	uint8_t pdu[4 + get_uuid_len(uuid)];
 
 	if (!att || !uuid || uuid->type == BT_UUID_UNSPEC)
-		return false;
+		return NULL;
 
 	op = new0(struct bt_gatt_request, 1);
 	op->att = att;
@@ -1386,10 +1387,27 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
 						bt_gatt_request_ref(op),
 						async_req_unref);
 	if (op->id)
-		return true;
+		return bt_gatt_request_ref(op);
 
 	free(op);
-	return false;
+	return NULL;
+}
+
+bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
+					const bt_uuid_t *uuid,
+					bt_gatt_request_callback_t callback,
+					void *user_data,
+					bt_gatt_destroy_func_t destroy)
+{
+	struct bt_gatt_request *req;
+
+	req = bt_gatt_read_by_type_request(att, start, end, uuid, callback,
+							user_data, destroy);
+	if (!req)
+		return false;
+
+	bt_gatt_request_unref(req);
+	return true;
 }
 
 static void discover_descs_cb(uint8_t opcode, const void *pdu,
diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
index 2bf5aad..528ef21 100644
--- a/src/shared/gatt-helpers.h
+++ b/src/shared/gatt-helpers.h
@@ -102,5 +102,12 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
 					void *user_data,
 					bt_gatt_destroy_func_t destroy);
 
+struct bt_gatt_request *bt_gatt_read_by_type_request(struct bt_att *att,
+					uint16_t start, uint16_t end,
+					const bt_uuid_t *uuid,
+					bt_gatt_request_callback_t callback,
+					void *user_data,
+					bt_gatt_destroy_func_t destroy);
+
 uint16_t bt_gatt_req_get_end_handle(struct bt_gatt_request *req);
 uint16_t bt_gatt_req_get_start_handle(struct bt_gatt_request *req);
diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 6d6d0c7..d206a44 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -42,6 +42,7 @@ struct test_pdu {
 enum context_type {
 	ATT,
 	CLIENT,
+	CLIENT_CACHED,
 	SERVER
 };
 
@@ -680,7 +681,13 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
 						"bt_gatt_server:", NULL);
 		break;
 	case CLIENT:
-		context->client_db = gatt_db_new();
+	case CLIENT_CACHED:
+		if (test_data->context_type == CLIENT_CACHED)
+			context->client_db =
+					gatt_db_clone(test_data->source_db);
+		else
+			context->client_db = gatt_db_new();
+
 		g_assert(context->client_db);
 
 		context->client = bt_gatt_client_new(context->client_db,
@@ -1968,6 +1975,22 @@ static void test_client(gconstpointer data)
 	create_context(512, data);
 }
 
+static void test_client_cancel(gconstpointer data)
+{
+	struct context *context = create_context(23, data);
+
+	context_quit(context);
+}
+
+static void test_client_cancel_all(gconstpointer data)
+{
+	struct context *context = create_context(23, data);
+
+	g_assert(bt_gatt_client_cancel_all(context->client));
+	g_assert(bt_gatt_client_cancel_all(context->client));
+	context_quit(context);
+}
+
 static void test_server(gconstpointer data)
 {
 	struct context *context = create_context(512, data);
@@ -2480,9 +2503,36 @@ 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, *ts_tail_db;
+	struct gatt_db *hash_db;
+	const struct att_handle_spec hash_specs[] = {
+		PRIMARY_SERVICE(0x0001, GATT_UUID, 3),
+		CHARACTERISTIC(GATT_CHARAC_DB_HASH, BT_ATT_PERM_READ,
+				BT_GATT_CHRC_PROP_READ, 0x00),
+		{ }
+	};
 
 	tester_init(&argc, &argv);
 
+	define_test_client("/robustness/client-cancel-queued-discovery",
+			test_client_cancel, NULL, NULL, {});
+
+	define_test_client("/robustness/client-cancel-pending-server-features",
+			test_client, NULL, NULL, MTU_EXCHANGE_CLIENT_PDUS,
+			raw_pdu(0x08, 0x01, 0x00, 0xff, 0xff, 0x3a, 0x2b));
+
+	define_test_client("/robustness/client-cancel-all-queued-discovery",
+			test_client_cancel_all, NULL, NULL, {});
+
+	hash_db = make_db(hash_specs);
+	define_test("/robustness/client-cancel-queued-db-hash",
+			test_client_cancel, CLIENT_CACHED, NULL, hash_db,
+			NULL, {});
+
+	define_test("/robustness/client-cancel-pending-db-hash",
+			test_client, CLIENT_CACHED, NULL, hash_db, NULL,
+			CLIENT_INIT_PDUS,
+			raw_pdu(0x08, 0x01, 0x00, 0xff, 0xff, 0x2a, 0x2b));
+
 	service_db_1 = make_service_data_1_db();
 	service_db_2 = make_service_data_2_db();
 	service_db_3 = make_service_data_3_db();
-- 
2.56.0