2015-04-30 11:26:15

by Mariusz Skamra

[permalink] [raw]
Subject: [PATCH 1/4] unit/eir: Fix G-tag test data

This patch adds SCAN_RSP data to gigaset_gtag_data.
Since MGMT 1.6 advertising and scan response data are merged.
So eir parser shall be able to parse this data.
---
unit/test-eir.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/unit/test-eir.c b/unit/test-eir.c
index 421d0db..53abfa4 100644
--- a/unit/test-eir.c
+++ b/unit/test-eir.c
@@ -621,7 +621,9 @@ static const unsigned char gigaset_gtag_data[] = {
0x02, 0x01, 0x06, 0x0d, 0xff, 0x80, 0x01, 0x02,
0x15, 0x12, 0x34, 0x80, 0x91, 0xd0, 0xf2, 0xbb,
0xc5, 0x03, 0x02, 0x0f, 0x18, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0e, 0x09,
+ 0x47, 0x69, 0x67, 0x61, 0x73, 0x65, 0x74, 0x20,
+ 0x47, 0x2d, 0x74, 0x61, 0x67
};

static const char *gigaset_gtag_uuid[] = {
@@ -635,6 +637,8 @@ static const struct test_data gigaset_gtag_test = {
.flags = 0x06,
.tx_power = 127,
.uuid = gigaset_gtag_uuid,
+ .name = "Gigaset G-tag",
+ .name_complete = true,
};

static const char *uri_beacon_uuid[] = {
--
1.9.1



2015-04-30 14:32:26

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 2/4] src/eir: Fix eir parser

Hi Mariusz,

On Thu, Apr 30, 2015 at 2:26 PM, Mariusz Skamra
<[email protected]> wrote:
> Some devices (like Gigaset's G-Tag) sends Advertising Report with significant part
> set to 0. According specification (Core 4.2, Vol 3, Part C, Sec 11) it is proper
> behaviour. So if ADV and SCAN_RSP data are merged, there is a possibility to lose
> SCAN_RSP data if the parser found early terminator.

This sounds like a bug in the code merging ADV and SCAN_RSP together,
it should have taken care of eliminating the early terminator
otherwise we are changing the format which breaks userspace.

> This patch fixes this issue by checking the address type. For LE, early termination
> won't be not used.
> ---
> android/bluetooth.c | 2 +-
> src/adapter.c | 4 ++--
> src/eir.c | 19 +++++++++++++------
> src/eir.h | 3 ++-
> unit/test-eir.c | 6 ++++--
> 5 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/android/bluetooth.c b/android/bluetooth.c
> index 4d0cd48..09a31f8 100644
> --- a/android/bluetooth.c
> +++ b/android/bluetooth.c
> @@ -1920,7 +1920,7 @@ static void update_found_device(const bdaddr_t *bdaddr, uint8_t bdaddr_type,
>
> memset(&eir, 0, sizeof(eir));
>
> - eir_parse(&eir, data, data_len);
> + eir_parse(&eir, data, data_len, bdaddr_type);
>
> dev = get_device(bdaddr, bdaddr_type);
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 8ee5b5b..762fb19 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -5384,7 +5384,7 @@ static void update_found_devices(struct btd_adapter *adapter,
> char addr[18];
>
> memset(&eir_data, 0, sizeof(eir_data));
> - eir_parse(&eir_data, data, data_len);
> + eir_parse(&eir_data, data, data_len, bdaddr_type);
>
> if (bdaddr_type == BDADDR_BREDR)
> discoverable = true;
> @@ -7467,7 +7467,7 @@ static void connected_callback(uint16_t index, uint16_t length,
>
> memset(&eir_data, 0, sizeof(eir_data));
> if (eir_len > 0)
> - eir_parse(&eir_data, ev->eir, eir_len);
> + eir_parse(&eir_data, ev->eir, eir_len, ev->addr.type);
>
> if (eir_data.class != 0)
> device_set_class(device, eir_data.class);
> diff --git a/src/eir.c b/src/eir.c
> index c984fa5..423a37f 100644
> --- a/src/eir.c
> +++ b/src/eir.c
> @@ -226,7 +226,8 @@ static void eir_parse_uuid128_data(struct eir_data *eir, const uint8_t *data,
> eir_parse_sd(eir, &service, data + 16, len - 16);
> }
>
> -void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
> +void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len,
> + uint8_t bdaddr_type)
> {
> uint16_t len = 0;
>
> @@ -242,15 +243,21 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
> const uint8_t *data;
> uint8_t data_len;
>
> - /* Check for the end of EIR */
> - if (field_len == 0)
> + /* Do not continue EIR Data parsing if got incorrect length */
> + if (len > eir_len)
> break;
>
> len += field_len + 1;
>
> - /* Do not continue EIR Data parsing if got incorrect length */
> - if (len > eir_len)
> + if (field_len == 0) {
> + /* Ignore redundant advertising data over LE*/
> + if (bdaddr_type != BDADDR_BREDR) {
> + eir_data += field_len + 1;
> + continue;
> + }
> + /* field_len == 0 for BREDR means the end of EIR */
> break;
> + }
>
> data = &eir_data[2];
> data_len = field_len - 1;
> @@ -370,7 +377,7 @@ int eir_parse_oob(struct eir_data *eir, uint8_t *eir_data, uint16_t eir_len)
>
> /* optional OOB EIR data */
> if (eir_len > 0)
> - eir_parse(eir, eir_data, eir_len);
> + eir_parse(eir, eir_data, eir_len, 0);
>
> return 0;
> }
> diff --git a/src/eir.h b/src/eir.h
> index 219ee79..944413d 100644
> --- a/src/eir.h
> +++ b/src/eir.h
> @@ -95,7 +95,8 @@ struct eir_data {
> };
>
> void eir_data_free(struct eir_data *eir);
> -void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len);
> +void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len,
> + uint8_t bdaddr_type);
> int eir_parse_oob(struct eir_data *eir, uint8_t *eir_data, uint16_t eir_len);
> int eir_create_oob(const bdaddr_t *addr, const char *name, uint32_t cod,
> const uint8_t *hash, const uint8_t *randomizer,
> diff --git a/unit/test-eir.c b/unit/test-eir.c
> index 53abfa4..09d9865 100644
> --- a/unit/test-eir.c
> +++ b/unit/test-eir.c
> @@ -44,6 +44,7 @@ struct test_data {
> bool name_complete;
> int8_t tx_power;
> const char **uuid;
> + uint8_t bdaddr_type;
> };
>
> static const unsigned char macbookair_data[] = {
> @@ -536,7 +537,7 @@ static void test_basic(const void *data)
> memset(buf, 0, sizeof(buf));
> memset(&eir, 0, sizeof(eir));
>
> - eir_parse(&eir, buf, HCI_MAX_EIR_LENGTH);
> + eir_parse(&eir, buf, HCI_MAX_EIR_LENGTH, BDADDR_BREDR);
> g_assert(eir.services == NULL);
> g_assert(eir.name == NULL);
>
> @@ -560,7 +561,7 @@ static void test_parsing(gconstpointer data)
>
> memset(&eir, 0, sizeof(eir));
>
> - eir_parse(&eir, test->eir_data, test->eir_size);
> + eir_parse(&eir, test->eir_data, test->eir_size, test->bdaddr_type);
>
> tester_debug("Flags: %d", eir.flags);
> tester_debug("Name: %s", eir.name);
> @@ -639,6 +640,7 @@ static const struct test_data gigaset_gtag_test = {
> .uuid = gigaset_gtag_uuid,
> .name = "Gigaset G-tag",
> .name_complete = true,
> + .bdaddr_type = BDADDR_LE_PUBLIC,
> };
>
> static const char *uri_beacon_uuid[] = {
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html



--
Luiz Augusto von Dentz

2015-04-30 11:26:18

by Mariusz Skamra

[permalink] [raw]
Subject: [PATCH 4/4] android/gatt: Fix autoconnect

This patch fixes autoconnect issue, however there is also race hazard in
Android Gatt framework. If app uses autoconnect while connecting,
direct flag is set to false. Then bt_gatt_add_autoconnect should be called
to add this app's id to autoconnect_apps queue.
---
android/gatt.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/android/gatt.c b/android/gatt.c
index 4da959f..72a27ab 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1972,6 +1972,7 @@ static bool trigger_connection(struct app_connection *conn, bool direct)
if (direct)
return connect_le(conn->device) == 0;

+ bt_gatt_add_autoconnect(conn->app->id, &conn->device->bdaddr);
return auto_connect_le(conn->device);
case DEVICE_CONNECTED:
notify_app_connect_status(conn, GATT_SUCCESS);
--
1.9.1


2015-04-30 11:26:16

by Mariusz Skamra

[permalink] [raw]
Subject: [PATCH 2/4] src/eir: Fix eir parser

Some devices (like Gigaset's G-Tag) sends Advertising Report with significant part
set to 0. According specification (Core 4.2, Vol 3, Part C, Sec 11) it is proper
behaviour. So if ADV and SCAN_RSP data are merged, there is a possibility to lose
SCAN_RSP data if the parser found early terminator.

This patch fixes this issue by checking the address type. For LE, early termination
won't be not used.
---
android/bluetooth.c | 2 +-
src/adapter.c | 4 ++--
src/eir.c | 19 +++++++++++++------
src/eir.h | 3 ++-
unit/test-eir.c | 6 ++++--
5 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index 4d0cd48..09a31f8 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -1920,7 +1920,7 @@ static void update_found_device(const bdaddr_t *bdaddr, uint8_t bdaddr_type,

memset(&eir, 0, sizeof(eir));

- eir_parse(&eir, data, data_len);
+ eir_parse(&eir, data, data_len, bdaddr_type);

dev = get_device(bdaddr, bdaddr_type);

diff --git a/src/adapter.c b/src/adapter.c
index 8ee5b5b..762fb19 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -5384,7 +5384,7 @@ static void update_found_devices(struct btd_adapter *adapter,
char addr[18];

memset(&eir_data, 0, sizeof(eir_data));
- eir_parse(&eir_data, data, data_len);
+ eir_parse(&eir_data, data, data_len, bdaddr_type);

if (bdaddr_type == BDADDR_BREDR)
discoverable = true;
@@ -7467,7 +7467,7 @@ static void connected_callback(uint16_t index, uint16_t length,

memset(&eir_data, 0, sizeof(eir_data));
if (eir_len > 0)
- eir_parse(&eir_data, ev->eir, eir_len);
+ eir_parse(&eir_data, ev->eir, eir_len, ev->addr.type);

if (eir_data.class != 0)
device_set_class(device, eir_data.class);
diff --git a/src/eir.c b/src/eir.c
index c984fa5..423a37f 100644
--- a/src/eir.c
+++ b/src/eir.c
@@ -226,7 +226,8 @@ static void eir_parse_uuid128_data(struct eir_data *eir, const uint8_t *data,
eir_parse_sd(eir, &service, data + 16, len - 16);
}

-void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
+void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len,
+ uint8_t bdaddr_type)
{
uint16_t len = 0;

@@ -242,15 +243,21 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
const uint8_t *data;
uint8_t data_len;

- /* Check for the end of EIR */
- if (field_len == 0)
+ /* Do not continue EIR Data parsing if got incorrect length */
+ if (len > eir_len)
break;

len += field_len + 1;

- /* Do not continue EIR Data parsing if got incorrect length */
- if (len > eir_len)
+ if (field_len == 0) {
+ /* Ignore redundant advertising data over LE*/
+ if (bdaddr_type != BDADDR_BREDR) {
+ eir_data += field_len + 1;
+ continue;
+ }
+ /* field_len == 0 for BREDR means the end of EIR */
break;
+ }

data = &eir_data[2];
data_len = field_len - 1;
@@ -370,7 +377,7 @@ int eir_parse_oob(struct eir_data *eir, uint8_t *eir_data, uint16_t eir_len)

/* optional OOB EIR data */
if (eir_len > 0)
- eir_parse(eir, eir_data, eir_len);
+ eir_parse(eir, eir_data, eir_len, 0);

return 0;
}
diff --git a/src/eir.h b/src/eir.h
index 219ee79..944413d 100644
--- a/src/eir.h
+++ b/src/eir.h
@@ -95,7 +95,8 @@ struct eir_data {
};

void eir_data_free(struct eir_data *eir);
-void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len);
+void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len,
+ uint8_t bdaddr_type);
int eir_parse_oob(struct eir_data *eir, uint8_t *eir_data, uint16_t eir_len);
int eir_create_oob(const bdaddr_t *addr, const char *name, uint32_t cod,
const uint8_t *hash, const uint8_t *randomizer,
diff --git a/unit/test-eir.c b/unit/test-eir.c
index 53abfa4..09d9865 100644
--- a/unit/test-eir.c
+++ b/unit/test-eir.c
@@ -44,6 +44,7 @@ struct test_data {
bool name_complete;
int8_t tx_power;
const char **uuid;
+ uint8_t bdaddr_type;
};

static const unsigned char macbookair_data[] = {
@@ -536,7 +537,7 @@ static void test_basic(const void *data)
memset(buf, 0, sizeof(buf));
memset(&eir, 0, sizeof(eir));

- eir_parse(&eir, buf, HCI_MAX_EIR_LENGTH);
+ eir_parse(&eir, buf, HCI_MAX_EIR_LENGTH, BDADDR_BREDR);
g_assert(eir.services == NULL);
g_assert(eir.name == NULL);

@@ -560,7 +561,7 @@ static void test_parsing(gconstpointer data)

memset(&eir, 0, sizeof(eir));

- eir_parse(&eir, test->eir_data, test->eir_size);
+ eir_parse(&eir, test->eir_data, test->eir_size, test->bdaddr_type);

tester_debug("Flags: %d", eir.flags);
tester_debug("Name: %s", eir.name);
@@ -639,6 +640,7 @@ static const struct test_data gigaset_gtag_test = {
.uuid = gigaset_gtag_uuid,
.name = "Gigaset G-tag",
.name_complete = true,
+ .bdaddr_type = BDADDR_LE_PUBLIC,
};

static const char *uri_beacon_uuid[] = {
--
1.9.1


2015-04-30 11:26:17

by Mariusz Skamra

[permalink] [raw]
Subject: [PATCH 3/4] unit/eir: Add WahooHRM test

---
unit/test-eir.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/unit/test-eir.c b/unit/test-eir.c
index 09d9865..0a8d0ce 100644
--- a/unit/test-eir.c
+++ b/unit/test-eir.c
@@ -660,6 +660,28 @@ static const struct test_data uri_beacon_test = {
.uuid = uri_beacon_uuid,
};

+static const char *wahoo_hrm_uuid[] = {
+ "0000180d-0000-1000-8000-00805f9b34fb",
+ NULL
+};
+
+static const unsigned char wahoo_hrm_data[] = {
+ 0x02, 0x01, 0x06, 0x03, 0x02, 0x0d, 0x18, 0x0f, 0x09,
+ 0x57, 0x61, 0x68, 0x6f, 0x6f, 0x20, 0x48, 0x52, 0x4d,
+ 0x20, 0x76, 0x32, 0x2e, 0x31
+};
+
+static const struct test_data wahoo_hrm_test = {
+ .eir_data = wahoo_hrm_data,
+ .eir_size = sizeof(wahoo_hrm_data),
+ .flags = 0x06,
+ .tx_power = 127,
+ .uuid = wahoo_hrm_uuid,
+ .name = "Wahoo HRM v2.1",
+ .name_complete = true,
+ .bdaddr_type = BDADDR_LE_RANDOM,
+};
+
int main(int argc, char *argv[])
{
tester_init(&argc, &argv);
@@ -685,6 +707,7 @@ int main(int argc, char *argv[])
NULL);
tester_add("ad/g-tag", &gigaset_gtag_test, NULL, test_parsing, NULL);
tester_add("ad/uri-beacon", &uri_beacon_test, NULL, test_parsing, NULL);
+ tester_add("ad/wahoo-hrm", &wahoo_hrm_test, NULL, test_parsing, NULL);

return tester_run();
}
--
1.9.1


2015-05-08 12:46:22

by Szymon Janc

[permalink] [raw]
Subject: Re: [PATCH 4/4] android/gatt: Fix autoconnect

Hi Mariusz,

On Thursday 30 of April 2015 13:26:18 Mariusz Skamra wrote:
> This patch fixes autoconnect issue, however there is also race hazard in
> Android Gatt framework. If app uses autoconnect while connecting,
> direct flag is set to false. Then bt_gatt_add_autoconnect should be called
> to add this app's id to autoconnect_apps queue.
> ---
> android/gatt.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 4da959f..72a27ab 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1972,6 +1972,7 @@ static bool trigger_connection(struct app_connection
> *conn, bool direct) if (direct)
> return connect_le(conn->device) == 0;
>
> + bt_gatt_add_autoconnect(conn->app->id, &conn->device->bdaddr);
> return auto_connect_le(conn->device);
> case DEVICE_CONNECTED:
> notify_app_connect_status(conn, GATT_SUCCESS);

This patch is now applied. Thanks.

--
BR
Szymon Janc