[PATCH 00/16] android/gatt: Improve robustness in Gatt Client

[PATCH 00/16] android/gatt: Improve robustness in Gatt Client

[Lukasz Rymanowski]

This set improves robustness on searching services, characteristics,
descriptors and included services.

Issue found on UPF50.

There is 4 test showing issue and after each test there is a fix for that.

Last 5 patches is a same fix for shared/gatt-helpers. I did not write special
test for it as we will test it once we make Android to use it shared code.
Anyway, this change does not brake gatt unit tests

Lukasz Rymanowski (16):
  android/tester-gatt: Add robustness test for search services
  attrib/gatt: Improve robustness on search primary services
  android/tester-gatt: Add robustness test for get characteristic
  attrib/gatt: Minor refactor in char_discovered_cb
  attrib/gatt: Improve robustness when searching for characteristics
  android/gatt: Check status on get characteristic callback
  android/tester: Add robustness test for get descriptor
  attrib/gatt: Minor refactor in desc_discovered_cb
  attrib/gatt: Improve robustness when searching for descriptors
  android/tester: Add robustness test for get included services
  attrib/gatt: Improve robustness when searching for included services
  shated/gatt-helpers: Improve robustness of search service
  shared/gatt-helpers: Improve robustness of get characteristics
  shared/gatt-helpers: Improve robustness of get include services
  shared/gatt-helpers: Improve robustness read by type request
  shared/gatt-helpers: Improve robustness of get descriptors

 android/gatt.c            |   8 +++
 android/tester-gatt.c     | 128 ++++++++++++++++++++++++++++++++++++++++++++++
 attrib/gatt.c             |  93 +++++++++++++++++++++++++++++----
 src/shared/gatt-helpers.c |  89 +++++++++++++++++++++++++++++---
 4 files changed, 302 insertions(+), 16 deletions(-)

-- 
1.8.4

[PATCH 01/16] android/tester-gatt: Add robustness test for search services

[Lukasz Rymanowski]

This test verifies if BlueZ do not start infinitive service search
when remote device response with incorrect end handle in Read By
Group response
---
 android/tester-gatt.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/android/tester-gatt.c b/android/tester-gatt.c
index eb0ca1e..6c58bc7 100644
--- a/android/tester-gatt.c
+++ b/android/tester-gatt.c
@@ -895,6 +895,12 @@ static struct iovec search_service_3[] = {
 	end_pdu
 };
 
+static struct iovec search_service_4[] = {
+	raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
+	raw_pdu(0x11, 0x06, 0x01, 0x00, 0x00, 0x00, 0x00, 0x18),
+	end_pdu
+};
+
 static struct iovec get_characteristic_1[] = {
 	SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
 	READ_BY_TYPE_SINGLE_CHARACTERISTIC_PDUS,
@@ -2137,6 +2143,27 @@ static struct test_case test_cases[] = {
 		ACTION_SUCCESS(bluetooth_disable_action, NULL),
 		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
 	),
+	TEST_CASE_BREDRLE("Gatt Client - Search Service - Incorrect rsp",
+		ACTION_SUCCESS(init_pdus, search_service_4),
+		ACTION_SUCCESS(bluetooth_enable_action, NULL),
+		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_ON),
+		ACTION_SUCCESS(emu_setup_powered_remote_action, NULL),
+		ACTION_SUCCESS(emu_set_ssp_mode_action, NULL),
+		ACTION_SUCCESS(emu_set_connect_cb_action, gatt_conn_cb),
+		ACTION_SUCCESS(gatt_client_register_action, &app1_uuid),
+		CALLBACK_STATUS(CB_GATTC_REGISTER_CLIENT, BT_STATUS_SUCCESS),
+		ACTION_SUCCESS(gatt_client_start_scan_action, NULL),
+		CLLBACK_GATTC_SCAN_RES(prop_emu_remotes_default_set, 1, TRUE),
+		ACTION_SUCCESS(gatt_client_stop_scan_action, NULL),
+		ACTION_SUCCESS(gatt_client_connect_action, &app1_conn_req),
+		CALLBACK_GATTC_CONNECT(GATT_STATUS_SUCCESS,
+						prop_emu_remotes_default_set,
+						CONN1_ID, APP1_ID),
+		ACTION_SUCCESS(gatt_client_search_services, &search_services_1),
+		CALLBACK_GATTC_SEARCH_COMPLETE(GATT_STATUS_SUCCESS, CONN1_ID),
+		ACTION_SUCCESS(bluetooth_disable_action, NULL),
+		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
+	),
 	TEST_CASE_BREDRLE("Gatt Client - Get Characteristic - Single",
 		ACTION_SUCCESS(init_pdus, get_characteristic_1),
 		ACTION_SUCCESS(bluetooth_enable_action, NULL),
-- 
1.8.4

[PATCH 02/16] attrib/gatt: Improve robustness on search primary services

[Lukasz Rymanowski]

Without this it is possible to put BlueZ into infinite loop when
doing primary service search on remote device.

Issue happens when remote device replies with ending handle which is
lower than start handle we use for search.

This patch fixes that

Found on robustness session on UPF50
---
 attrib/gatt.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index aafd3f7..b8d766c 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -43,6 +43,7 @@ struct discover_primary {
 	GAttrib *attrib;
 	unsigned int id;
 	bt_uuid_t uuid;
+	uint16_t start;
 	GSList *primaries;
 	gatt_cb_t cb;
 	void *user_data;
@@ -255,8 +256,19 @@ static void primary_by_uuid_cb(guint8 status, const guint8 *ipdu,
 	if (range->end == 0xffff)
 		goto done;
 
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (range->end < dp->start) {
+		err = ATT_ECODE_UNLIKELY;
+		goto done;
+	}
+
+	dp->start = range->end + 1;
+
 	buf = g_attrib_get_buffer(dp->attrib, &buflen);
-	oplen = encode_discover_primary(range->end + 1, 0xffff, &dp->uuid,
+	oplen = encode_discover_primary(dp->start, 0xffff, &dp->uuid,
 								buf, buflen);
 
 	if (oplen == 0)
@@ -325,12 +337,24 @@ static void primary_all_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
 	att_data_list_free(list);
 	err = 0;
 
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (end < dp->start) {
+		err = ATT_ECODE_UNLIKELY;
+		goto done;
+	}
+
+	dp->start = end + 1;
+
 	if (end != 0xffff) {
 		size_t buflen;
 		uint8_t *buf = g_attrib_get_buffer(dp->attrib, &buflen);
-		guint16 oplen = encode_discover_primary(end + 1, 0xffff, NULL,
+		guint16 oplen = encode_discover_primary(dp->start, 0xffff, NULL,
 								buf, buflen);
 
+
 		g_attrib_send(dp->attrib, dp->id, buf, oplen, primary_all_cb,
 						discover_primary_ref(dp),
 						discover_primary_unref);
@@ -362,6 +386,7 @@ guint gatt_discover_primary(GAttrib *attrib, bt_uuid_t *uuid, gatt_cb_t func,
 	dp->attrib = g_attrib_ref(attrib);
 	dp->cb = func;
 	dp->user_data = user_data;
+	dp->start = 0x0001;
 
 	if (uuid) {
 		dp->uuid = *uuid;
-- 
1.8.4

[PATCH 03/16] android/tester-gatt: Add robustness test for get characteristic

[Lukasz Rymanowski]

This test makes sure that BlueZ correctly handles incorrect response
from remote device on get characteristic.
---
 android/tester-gatt.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/android/tester-gatt.c b/android/tester-gatt.c
index 6c58bc7..1abfe26 100644
--- a/android/tester-gatt.c
+++ b/android/tester-gatt.c
@@ -907,6 +907,13 @@ static struct iovec get_characteristic_1[] = {
 	end_pdu
 };
 
+static struct iovec get_characteristic_2[] = {
+	SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
+	raw_pdu(0x08, 0x01, 0x00, 0x10, 0x00, 0x03, 0x28),
+	raw_pdu(0x09, 0x07, 0x00, 0x00, 0x04, 0x00, 0x00, 0x19, 0x00),
+	end_pdu
+};
+
 static struct iovec get_descriptor_1[] = {
 	SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
 	READ_BY_TYPE_SINGLE_CHARACTERISTIC_PDUS,
@@ -2189,6 +2196,31 @@ static struct test_case test_cases[] = {
 		ACTION_SUCCESS(bluetooth_disable_action, NULL),
 		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
 	),
+	TEST_CASE_BREDRLE("Gatt Client - Get Characteristic - Incorrect rsp",
+		ACTION_SUCCESS(init_pdus, get_characteristic_2),
+		ACTION_SUCCESS(bluetooth_enable_action, NULL),
+		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_ON),
+		ACTION_SUCCESS(emu_setup_powered_remote_action, NULL),
+		ACTION_SUCCESS(emu_set_ssp_mode_action, NULL),
+		ACTION_SUCCESS(emu_set_connect_cb_action, gatt_conn_cb),
+		ACTION_SUCCESS(gatt_client_register_action, &app1_uuid),
+		CALLBACK_STATUS(CB_GATTC_REGISTER_CLIENT, BT_STATUS_SUCCESS),
+		ACTION_SUCCESS(gatt_client_start_scan_action, NULL),
+		CLLBACK_GATTC_SCAN_RES(prop_emu_remotes_default_set, 1, TRUE),
+		ACTION_SUCCESS(gatt_client_stop_scan_action, NULL),
+		ACTION_SUCCESS(gatt_client_connect_action, &app1_conn_req),
+		CALLBACK_GATTC_CONNECT(GATT_STATUS_SUCCESS,
+						prop_emu_remotes_default_set,
+						CONN1_ID, APP1_ID),
+		ACTION_SUCCESS(gatt_client_search_services, &search_services_1),
+		CALLBACK_GATTC_SEARCH_COMPLETE(GATT_STATUS_SUCCESS, CONN1_ID),
+		ACTION_SUCCESS(gatt_client_get_characteristic_action,
+							&get_char_data_1),
+		CALLBACK_GATTC_GET_CHARACTERISTIC_CB(GATT_STATUS_FAILURE,
+				CONN1_ID, &service_1, NULL, 0),
+		ACTION_SUCCESS(bluetooth_disable_action, NULL),
+		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
+	),
 	TEST_CASE_BREDRLE("Gatt Client - Get Characteristic - None",
 		ACTION_SUCCESS(init_pdus, get_characteristic_1),
 		ACTION_SUCCESS(bluetooth_enable_action, NULL),
-- 
1.8.4

[PATCH 04/16] attrib/gatt: Minor refactor in char_discovered_cb

[Lukasz Rymanowski]

This will be needed by next patch
---
 attrib/gatt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index b8d766c..4bffd64 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -574,10 +574,16 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
 {
 	struct discover_char *dc = user_data;
 	struct att_data_list *list;
-	unsigned int i, err = ATT_ECODE_ATTR_NOT_FOUND;
+	unsigned int i, err = 0;
 	uint16_t last = 0;
 	uint8_t type;
 
+	/* We have all the characteristic now, lets send it up */
+	if (status == ATT_ECODE_ATTR_NOT_FOUND) {
+		err = dc->characteristics ? 0 : status;
+		goto done;
+	}
+
 	if (status) {
 		err = status;
 		goto done;
@@ -647,7 +653,6 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
 	}
 
 done:
-	err = (dc->characteristics ? 0 : err);
 	dc->cb(err, dc->characteristics, dc->user_data);
 }
 
-- 
1.8.4

[PATCH 05/16] attrib/gatt: Improve robustness when searching for characteristics

[Lukasz Rymanowski]

Without this patch it is possible to enter infinite loop when searching
characteristics on remote device.
This patch fixes that.

Issue happens when remote device replies with ending handle which is
lower than start handle we use for search.

Found on robustness session on UPF50
---
 attrib/gatt.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index 4bffd64..2749c46 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -72,6 +72,7 @@ struct discover_char {
 	unsigned int id;
 	bt_uuid_t *uuid;
 	uint16_t end;
+	uint16_t start;
 	GSList *characteristics;
 	gatt_cb_t cb;
 	void *user_data;
@@ -629,7 +630,18 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
 
 	att_data_list_free(list);
 
-	if (last != 0 && (last + 1 < dc->end)) {
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (last < dc->start) {
+		err = ATT_ECODE_UNLIKELY;
+		goto done;
+	}
+
+	dc->start = last + 1;
+
+	if (last != 0 && (dc->start < dc->end)) {
 		bt_uuid_t uuid;
 		guint16 oplen;
 		size_t buflen;
@@ -639,7 +651,7 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
 
 		bt_uuid16_create(&uuid, GATT_CHARAC_UUID);
 
-		oplen = enc_read_by_type_req(last + 1, dc->end, &uuid, buf,
+		oplen = enc_read_by_type_req(dc->start, dc->end, &uuid, buf,
 									buflen);
 
 		if (oplen == 0)
@@ -680,6 +692,7 @@ guint gatt_discover_char(GAttrib *attrib, uint16_t start, uint16_t end,
 	dc->cb = func;
 	dc->user_data = user_data;
 	dc->end = end;
+	dc->start = start;
 	dc->uuid = g_memdup(uuid, sizeof(bt_uuid_t));
 
 	dc->id = g_attrib_send(attrib, 0, buf, plen, char_discovered_cb,
-- 
1.8.4

[PATCH 06/16] android/gatt: Check status on get characteristic callback

[Lukasz Rymanowski]

---
 android/gatt.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index 53b1983..0296788 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -2725,12 +2725,20 @@ static void discover_char_cb(uint8_t status, GSList *characteristics,
 	struct discover_char_data *data = user_data;
 	struct service *srvc = data->service;
 
+	if (status) {
+		error("gatt: Failed to get characteristics: %s",
+							att_ecode2str(status));
+		convert_send_client_char_notify(NULL, data->conn_id, srvc);
+		goto done;
+	}
+
 	if (queue_isempty(srvc->chars))
 		cache_all_srvc_chars(srvc, characteristics);
 
 	convert_send_client_char_notify(queue_peek_head(srvc->chars),
 							data->conn_id, srvc);
 
+done:
 	free(data);
 }
 
-- 
1.8.4

[PATCH 07/16] android/tester: Add robustness test for get descriptor

[Lukasz Rymanowski]

---
 android/tester-gatt.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/android/tester-gatt.c b/android/tester-gatt.c
index 1abfe26..10c09d2 100644
--- a/android/tester-gatt.c
+++ b/android/tester-gatt.c
@@ -914,6 +914,14 @@ static struct iovec get_characteristic_2[] = {
 	end_pdu
 };
 
+static struct iovec get_descriptor_0[] = {
+	SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
+	READ_BY_TYPE_SINGLE_CHARACTERISTIC_PDUS,
+	raw_pdu(0x04, 0x01, 0x00, 0x10, 0x00),
+	raw_pdu(0x05, 0x01, 0x00, 0x00, 0x00, 0x29),
+	end_pdu
+};
+
 static struct iovec get_descriptor_1[] = {
 	SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
 	READ_BY_TYPE_SINGLE_CHARACTERISTIC_PDUS,
@@ -2247,6 +2255,35 @@ static struct test_case test_cases[] = {
 		ACTION_SUCCESS(bluetooth_disable_action, NULL),
 		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
 	),
+	TEST_CASE_BREDRLE("Gatt Client - Get Descriptor - Incorrect rsp",
+		ACTION_SUCCESS(init_pdus, get_descriptor_0),
+		ACTION_SUCCESS(bluetooth_enable_action, NULL),
+		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_ON),
+		ACTION_SUCCESS(emu_setup_powered_remote_action, NULL),
+		ACTION_SUCCESS(emu_set_ssp_mode_action, NULL),
+		ACTION_SUCCESS(emu_set_connect_cb_action, gatt_conn_cb),
+		ACTION_SUCCESS(gatt_client_register_action, &app1_uuid),
+		CALLBACK_STATUS(CB_GATTC_REGISTER_CLIENT, BT_STATUS_SUCCESS),
+		ACTION_SUCCESS(gatt_client_start_scan_action, NULL),
+		CLLBACK_GATTC_SCAN_RES(prop_emu_remotes_default_set, 1, TRUE),
+		ACTION_SUCCESS(gatt_client_stop_scan_action, NULL),
+		ACTION_SUCCESS(gatt_client_connect_action, &app1_conn_req),
+		CALLBACK_GATTC_CONNECT(GATT_STATUS_SUCCESS,
+						prop_emu_remotes_default_set,
+						CONN1_ID, APP1_ID),
+		ACTION_SUCCESS(gatt_client_search_services, &search_services_1),
+		CALLBACK_GATTC_SEARCH_COMPLETE(GATT_STATUS_SUCCESS, CONN1_ID),
+		ACTION_SUCCESS(gatt_client_get_characteristic_action,
+							&get_char_data_1),
+		CALLBACK_GATTC_GET_CHARACTERISTIC_CB(GATT_STATUS_SUCCESS,
+				CONN1_ID, &service_1, &characteristic_1, 4),
+		ACTION_SUCCESS(gatt_client_get_descriptor_action,
+							&get_desc_data_1),
+		CALLBACK_GATTC_GET_DESCRIPTOR(GATT_STATUS_FAILURE, CONN1_ID,
+				&service_1, &characteristic_1, NULL),
+		ACTION_SUCCESS(bluetooth_disable_action, NULL),
+		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
+	),
 	TEST_CASE_BREDRLE("Gatt Client - Get Descriptor - Single",
 		ACTION_SUCCESS(init_pdus, get_descriptor_1),
 		ACTION_SUCCESS(bluetooth_enable_action, NULL),
-- 
1.8.4

[PATCH 08/16] attrib/gatt: Minor refactor in desc_discovered_cb

[Lukasz Rymanowski]

This is needed for next patch
---
 attrib/gatt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index 2749c46..df4e3a7 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -1008,12 +1008,17 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,
 {
 	struct discover_desc *dd = user_data;
 	struct att_data_list *list;
-	unsigned int i, err = ATT_ECODE_ATTR_NOT_FOUND;
+	unsigned int i, err = 0;
 	guint8 format;
 	uint16_t last = 0xffff;
 	uint8_t type;
 	gboolean uuid_found = FALSE;
 
+	if (status == ATT_ECODE_ATTR_NOT_FOUND) {
+		err = dd->descriptors ? 0 : status;
+		goto done;
+	}
+
 	if (status) {
 		err = status;
 		goto done;
@@ -1086,7 +1091,6 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,
 	}
 
 done:
-	err = (dd->descriptors ? 0 : err);
 	dd->cb(err, dd->descriptors, dd->user_data);
 }
 
-- 
1.8.4

[PATCH 09/16] attrib/gatt: Improve robustness when searching for descriptors

[Lukasz Rymanowski]

Without this patch it is possible to enter infinite loop when
searching descriptors on remote device. This patch fixes that.

Issue happens when remote device replies with ending handle which is
lower than start handle we use for search.
---
 attrib/gatt.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index df4e3a7..bc2f92a 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -83,6 +83,7 @@ struct discover_desc {
 	GAttrib *attrib;
 	unsigned int id;
 	bt_uuid_t *uuid;
+	uint16_t start;
 	uint16_t end;
 	GSList *descriptors;
 	gatt_cb_t cb;
@@ -1072,6 +1073,17 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,
 
 	att_data_list_free(list);
 
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (last < dd->start) {
+		err = ATT_ECODE_UNLIKELY;
+		goto done;
+	}
+
+	dd->start = last + 1;
+
 	if (last < dd->end && !uuid_found) {
 		guint16 oplen;
 		size_t buflen;
@@ -1079,7 +1091,7 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,
 
 		buf = g_attrib_get_buffer(dd->attrib, &buflen);
 
-		oplen = enc_find_info_req(last + 1, dd->end, buf, buflen);
+		oplen = enc_find_info_req(dd->start, dd->end, buf, buflen);
 		if (oplen == 0)
 			return;
 
@@ -1114,6 +1126,7 @@ guint gatt_discover_desc(GAttrib *attrib, uint16_t start, uint16_t end,
 	dd->attrib = g_attrib_ref(attrib);
 	dd->cb = func;
 	dd->user_data = user_data;
+	dd->start = start;
 	dd->end = end;
 	dd->uuid = g_memdup(uuid, sizeof(bt_uuid_t));
 
-- 
1.8.4

[PATCH 10/16] android/tester: Add robustness test for get included services

[Lukasz Rymanowski]

---
 android/tester-gatt.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/android/tester-gatt.c b/android/tester-gatt.c
index 10c09d2..b8b088b 100644
--- a/android/tester-gatt.c
+++ b/android/tester-gatt.c
@@ -950,6 +950,13 @@ static struct iovec get_descriptor_3[] = {
 	end_pdu
 };
 
+static struct iovec get_included_0[] = {
+	SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
+	raw_pdu(0x08, 0x01, 0x00, 0x10, 0x00, 0x02, 0x28),
+	raw_pdu(0x09, 0x08, 0x00, 0x00, 0x15, 0x00, 0x19, 0x00, 0xff, 0xfe),
+	end_pdu
+};
+
 static struct iovec get_included_1[] = {
 	SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
 	raw_pdu(0x08, 0x01, 0x00, 0x10, 0x00, 0x02, 0x28),
@@ -2378,6 +2385,31 @@ static struct test_case test_cases[] = {
 		ACTION_SUCCESS(bluetooth_disable_action, NULL),
 		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
 	),
+	TEST_CASE_BREDRLE("Gatt Client - Get Included Services - Incorrect rsp",
+		ACTION_SUCCESS(init_pdus, get_included_0),
+		ACTION_SUCCESS(bluetooth_enable_action, NULL),
+		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_ON),
+		ACTION_SUCCESS(emu_setup_powered_remote_action, NULL),
+		ACTION_SUCCESS(emu_set_ssp_mode_action, NULL),
+		ACTION_SUCCESS(emu_set_connect_cb_action, gatt_conn_cb),
+		ACTION_SUCCESS(gatt_client_register_action, &app1_uuid),
+		CALLBACK_STATUS(CB_GATTC_REGISTER_CLIENT, BT_STATUS_SUCCESS),
+		ACTION_SUCCESS(gatt_client_start_scan_action, NULL),
+		CLLBACK_GATTC_SCAN_RES(prop_emu_remotes_default_set, 1, TRUE),
+		ACTION_SUCCESS(gatt_client_stop_scan_action, NULL),
+		ACTION_SUCCESS(gatt_client_connect_action, &app1_conn_req),
+		CALLBACK_GATTC_CONNECT(GATT_STATUS_SUCCESS,
+						prop_emu_remotes_default_set,
+						CONN1_ID, APP1_ID),
+		ACTION_SUCCESS(gatt_client_search_services, &search_services_1),
+		CALLBACK_GATTC_SEARCH_COMPLETE(GATT_STATUS_SUCCESS, CONN1_ID),
+		ACTION_SUCCESS(gatt_client_get_included_action,
+							&get_incl_data_1),
+		CALLBACK_GATTC_GET_INCLUDED(GATT_STATUS_FAILURE, CONN1_ID,
+							&service_1, NULL),
+		ACTION_SUCCESS(bluetooth_disable_action, NULL),
+		CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
+		),
 	TEST_CASE_BREDRLE("Gatt Client - Get Included Service - 16 UUID",
 		ACTION_SUCCESS(init_pdus, get_included_1),
 		ACTION_SUCCESS(bluetooth_enable_action, NULL),
-- 
1.8.4

[PATCH 11/16] attrib/gatt: Improve robustness when searching for included services

[Lukasz Rymanowski]

Without this patch it is possible to enter infinite loop when
searching included services on remote device. This patch fixes that.

Issue happens when remote device replies with ending handle which is
lower than start handle we use for search
---
 attrib/gatt.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index bc2f92a..ea5c1cf 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -55,6 +55,7 @@ struct included_discovery {
 	unsigned int	id;
 	int		refs;
 	int		err;
+	uint16_t	start_handle;
 	uint16_t	end_handle;
 	GSList		*includes;
 	gatt_cb_t	cb;
@@ -549,8 +550,19 @@ static void find_included_cb(uint8_t status, const uint8_t *pdu, uint16_t len,
 
 	att_data_list_free(list);
 
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (last_handle < isd->start_handle) {
+		isd->err = ATT_ECODE_UNLIKELY;
+		goto done;
+	}
+
+	isd->start_handle = last_handle + 1;
+
 	if (last_handle < isd->end_handle)
-		find_included(isd, last_handle + 1);
+		find_included(isd, isd->start_handle);
 
 done:
 	if (isd->err == 0)
@@ -564,6 +576,7 @@ unsigned int gatt_find_included(GAttrib *attrib, uint16_t start, uint16_t end,
 
 	isd = g_new0(struct included_discovery, 1);
 	isd->attrib = g_attrib_ref(attrib);
+	isd->start_handle = start;
 	isd->end_handle = end;
 	isd->cb = func;
 	isd->user_data = user_data;
-- 
1.8.4

[PATCH 12/16] shated/gatt-helpers: Improve robustness of search service

[Lukasz Rymanowski]

This patch makes sure that we do get into infinite loop when doing
search service request

It could happen if we got bogus read by group or find by type response
---
 src/shared/gatt-helpers.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 2d07a83..1a60f31 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -212,6 +212,7 @@ static bool convert_uuid_le(const uint8_t *src, size_t len, uint8_t dst[16])
 struct discovery_op {
 	struct bt_att *att;
 	unsigned int id;
+	uint16_t start_handle;
 	uint16_t end_handle;
 	int ref_count;
 	bt_uuid_t uuid;
@@ -663,10 +664,22 @@ static void read_by_grp_type_cb(uint8_t opcode, const void *pdu,
 	}
 
 	last_end = get_le16(pdu + length - data_length + 2);
+
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (last_end < op->start_handle) {
+		success = false;
+		goto done;
+	}
+
+	op->start_handle = last_end + 1;
+
 	if (last_end < op->end_handle) {
 		uint8_t pdu[6];
 
-		put_le16(last_end + 1, pdu);
+		put_le16(op->start_handle, pdu);
 		put_le16(op->end_handle, pdu + 2);
 		put_le16(op->service_type, pdu + 4);
 
@@ -738,10 +751,22 @@ static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
 	 * last_end is end handle of last data set
 	 */
 	last_end = get_le16(pdu + length - 2);
+
+	/*
+	* If last handle is lower from previous start handle then it is smth
+	* wrong. Let's stop search, otherwise we might enter infinite loop.
+	*/
+	if (last_end < op->start_handle) {
+		success = false;
+		goto done;
+	}
+
+	op->start_handle = last_end + 1;
+
 	if (last_end < op->end_handle) {
 		uint8_t pdu[6 + get_uuid_len(&op->uuid)];
 
-		put_le16(last_end + 1, pdu);
+		put_le16(op->start_handle, pdu);
 		put_le16(op->end_handle, pdu + 2);
 		put_le16(op->service_type, pdu + 4);
 		bt_uuid_to_le(&op->uuid, pdu + 6);
@@ -782,6 +807,7 @@ static bool discover_services(struct bt_att *att, bt_uuid_t *uuid,
 		return false;
 
 	op->att = att;
+	op->start_handle = start;
 	op->end_handle = end;
 	op->callback = callback;
 	op->user_data = user_data;
-- 
1.8.4

[PATCH 13/16] shared/gatt-helpers: Improve robustness of get characteristics

[Lukasz Rymanowski]

This patch makes sure that we do get into infinite loop when doing
search for characteristics

It could happen if we got bogus read by type response
---
 src/shared/gatt-helpers.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 1a60f31..cd0ffe2 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1203,10 +1203,22 @@ static void discover_chrcs_cb(uint8_t opcode, const void *pdu,
 		goto done;
 	}
 	last_handle = get_le16(pdu + length - data_length);
+
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (last_handle < op->start_handle) {
+		success = false;
+		goto done;
+	}
+
+	op->start_handle = last_handle + 1;
+
 	if (last_handle != op->end_handle) {
 		uint8_t pdu[6];
 
-		put_le16(last_handle + 1, pdu);
+		put_le16(op->start_handle, pdu);
 		put_le16(op->end_handle, pdu + 2);
 		put_le16(GATT_CHARAC_UUID, pdu + 4);
 
@@ -1249,6 +1261,7 @@ bool bt_gatt_discover_characteristics(struct bt_att *att,
 	op->callback = callback;
 	op->user_data = user_data;
 	op->destroy = destroy;
+	op->start_handle = start;
 	op->end_handle = end;
 
 	put_le16(start, pdu);
-- 
1.8.4

[PATCH 14/16] shared/gatt-helpers: Improve robustness of get include services

[Lukasz Rymanowski]

This patch makes sure that we do get into infinite loop when doing
search for included services.

It could happen if we got bogus read by type response
---
 src/shared/gatt-helpers.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index cd0ffe2..8858e58 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1094,10 +1094,21 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
 	}
 
 	last_handle = get_le16(pdu + length - data_length);
+
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (last_handle < op->start_handle) {
+		success = false;
+		goto failed;
+	}
+
+	op->start_handle = last_handle + 1;
 	if (last_handle != op->end_handle) {
 		uint8_t pdu[6];
 
-		put_le16(last_handle + 1, pdu);
+		put_le16(op->start_handle, pdu);
 		put_le16(op->end_handle, pdu + 2);
 		put_le16(GATT_INCLUDE_UUID, pdu + 4);
 
@@ -1140,6 +1151,7 @@ bool bt_gatt_discover_included_services(struct bt_att *att,
 	op->callback = callback;
 	op->user_data = user_data;
 	op->destroy = destroy;
+	op->start_handle = start;
 	op->end_handle = end;
 
 	put_le16(start, pdu);
-- 
1.8.4

[PATCH 15/16] shared/gatt-helpers: Improve robustness read by type request

[Lukasz Rymanowski]

This patch makes sure that we do get into infinite loop when doing
read by type request.

It could happen if we got bogus read by type response
---
 src/shared/gatt-helpers.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 8858e58..6e5cf30 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1331,10 +1331,22 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
 	}
 
 	last_handle = get_le16(pdu + length - data_length);
+
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (last_handle < op->start_handle) {
+		success = true;
+		goto done;
+	}
+
+	op->start_handle = last_handle + 1;
+
 	if (last_handle != op->end_handle) {
 		uint8_t pdu[4 + get_uuid_len(&op->uuid)];
 
-		put_le16(last_handle + 1, pdu);
+		put_le16(op->start_handle, pdu);
 		put_le16(op->end_handle, pdu + 2);
 		bt_uuid_to_le(&op->uuid, pdu + 4);
 
@@ -1376,6 +1388,7 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
 	op->callback = callback;
 	op->user_data = user_data;
 	op->destroy = destroy;
+	op->start_handle = start;
 	op->end_handle = end;
 	op->uuid = *uuid;
 
-- 
1.8.4

Re: [PATCH 15/16] shared/gatt-helpers: Improve robustness read by type request

[Szymon Janc]

Hi Łukasz,

On Thursday 26 of February 2015 10:39:42 Lukasz Rymanowski wrote:
> This patch makes sure that we do get into infinite loop when doing
> read by type request.
> 
> It could happen if we got bogus read by type response
> ---
>  src/shared/gatt-helpers.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index 8858e58..6e5cf30 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -1331,10 +1331,22 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
>  	}
>  
>  	last_handle = get_le16(pdu + length - data_length);
> +
> +	/*
> +	 * If last handle is lower from previous start handle then it is smth
> +	 * wrong. Let's stop search, otherwise we might enter infinite loop.
> +	 */
> +	if (last_handle < op->start_handle) {
> +		success = true;
> +		goto done;
> +	}

Shouldn't this be success = false ?

> +
> +	op->start_handle = last_handle + 1;
> +
>  	if (last_handle != op->end_handle) {
>  		uint8_t pdu[4 + get_uuid_len(&op->uuid)];
>  
> -		put_le16(last_handle + 1, pdu);
> +		put_le16(op->start_handle, pdu);
>  		put_le16(op->end_handle, pdu + 2);
>  		bt_uuid_to_le(&op->uuid, pdu + 4);
>  
> @@ -1376,6 +1388,7 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
>  	op->callback = callback;
>  	op->user_data = user_data;
>  	op->destroy = destroy;
> +	op->start_handle = start;
>  	op->end_handle = end;
>  	op->uuid = *uuid;
>  
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH 15/16] shared/gatt-helpers: Improve robustness read by type request

[Lukasz Rymanowski]

Hi Szymon,

On Fri, Feb 27, 2015 at 4:07 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Thursday 26 of February 2015 10:39:42 Lukasz Rymanowski wrote:
>> This patch makes sure that we do get into infinite loop when doing
>> read by type request.
>>
>> It could happen if we got bogus read by type response
>> ---
>>  src/shared/gatt-helpers.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
>> index 8858e58..6e5cf30 100644
>> --- a/src/shared/gatt-helpers.c
>> +++ b/src/shared/gatt-helpers.c
>> @@ -1331,10 +1331,22 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
>>       }
>>
>>       last_handle = get_le16(pdu + length - data_length);
>> +
>> +     /*
>> +      * If last handle is lower from previous start handle then it is smth
>> +      * wrong. Let's stop search, otherwise we might enter infinite loop.
>> +      */
>> +     if (last_handle < op->start_handle) {
>> +             success = true;
>> +             goto done;
>> +     }
>
> Shouldn't this be success = false ?

True, my bad. Can you fix that or should I send v2?
>
>> +
>> +     op->start_handle = last_handle + 1;
>> +
>>       if (last_handle != op->end_handle) {
>>               uint8_t pdu[4 + get_uuid_len(&op->uuid)];
>>
>> -             put_le16(last_handle + 1, pdu);
>> +             put_le16(op->start_handle, pdu);
>>               put_le16(op->end_handle, pdu + 2);
>>               bt_uuid_to_le(&op->uuid, pdu + 4);
>>
>> @@ -1376,6 +1388,7 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
>>       op->callback = callback;
>>       op->user_data = user_data;
>>       op->destroy = destroy;
>> +     op->start_handle = start;
>>       op->end_handle = end;
>>       op->uuid = *uuid;
>>
>>
>
> --
> Best regards,
> Szymon Janc
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 16/16] shared/gatt-helpers: Improve robustness of get descriptors

[Lukasz Rymanowski]

This patch makes sure that we do get into infinite loop when doing
get descriptors operation.

It could happen if we got bogus find information response
---
 src/shared/gatt-helpers.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 6e5cf30..39ba636 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1460,10 +1460,22 @@ static void discover_descs_cb(uint8_t opcode, const void *pdu,
 	}
 
 	last_handle = get_le16(pdu + length - data_length);
+
+	/*
+	 * If last handle is lower from previous start handle then it is smth
+	 * wrong. Let's stop search, otherwise we might enter infinite loop.
+	 */
+	if (last_handle < op->start_handle) {
+		success = false;
+		goto done;
+	}
+
+	op->start_handle = last_handle + 1;
+
 	if (last_handle != op->end_handle) {
 		uint8_t pdu[4];
 
-		put_le16(last_handle + 1, pdu);
+		put_le16(op->start_handle, pdu);
 		put_le16(op->end_handle, pdu + 2);
 
 		op->id = bt_att_send(op->att, BT_ATT_OP_FIND_INFO_REQ,
@@ -1505,6 +1517,7 @@ bool bt_gatt_discover_descriptors(struct bt_att *att,
 	op->callback = callback;
 	op->user_data = user_data;
 	op->destroy = destroy;
+	op->start_handle = start;
 	op->end_handle = end;
 
 	put_le16(start, pdu);
-- 
1.8.4

Re: [PATCH 00/16] android/gatt: Improve robustness in Gatt Client

[Szymon Janc]

Hi Łukasz,

On Thursday 26 of February 2015 10:39:27 Lukasz Rymanowski wrote:
> This set improves robustness on searching services, characteristics,
> descriptors and included services.
> 
> Issue found on UPF50.
> 
> There is 4 test showing issue and after each test there is a fix for that.
> 
> Last 5 patches is a same fix for shared/gatt-helpers. I did not write special
> test for it as we will test it once we make Android to use it shared code.
> Anyway, this change does not brake gatt unit tests
> 
> Lukasz Rymanowski (16):
>   android/tester-gatt: Add robustness test for search services
>   attrib/gatt: Improve robustness on search primary services
>   android/tester-gatt: Add robustness test for get characteristic
>   attrib/gatt: Minor refactor in char_discovered_cb
>   attrib/gatt: Improve robustness when searching for characteristics
>   android/gatt: Check status on get characteristic callback
>   android/tester: Add robustness test for get descriptor
>   attrib/gatt: Minor refactor in desc_discovered_cb
>   attrib/gatt: Improve robustness when searching for descriptors
>   android/tester: Add robustness test for get included services
>   attrib/gatt: Improve robustness when searching for included services
>   shated/gatt-helpers: Improve robustness of search service
>   shared/gatt-helpers: Improve robustness of get characteristics
>   shared/gatt-helpers: Improve robustness of get include services
>   shared/gatt-helpers: Improve robustness read by type request
>   shared/gatt-helpers: Improve robustness of get descriptors
> 
>  android/gatt.c            |   8 +++
>  android/tester-gatt.c     | 128 ++++++++++++++++++++++++++++++++++++++++++++++
>  attrib/gatt.c             |  93 +++++++++++++++++++++++++++++----
>  src/shared/gatt-helpers.c |  89 +++++++++++++++++++++++++++++---
>  4 files changed, 302 insertions(+), 16 deletions(-)
> 
Patches 1-11 are now applied, thanks.

-- 
Best regards, 
Szymon Janc