[PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Vinicius Costa Gomes]

From: Bruna Moreira <bruna.moreira@openbossa.org>

---
 src/adapter.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index b1aabbd..8b742b7 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2977,14 +2977,13 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
 	unsigned int i;
 
 	while (len < EIR_DATA_LENGTH - 1) {
-		uint8_t type = eir_data[1];
 		uint8_t field_len = eir_data[0];
 
 		/* Check for the end of EIR */
 		if (field_len == 0)
 			break;
 
-		switch (type) {
+		switch (eir_data[1]) {
 		case EIR_UUID16_SOME:
 		case EIR_UUID16_ALL:
 			uuid16_count = field_len / 2;
-- 
1.7.3.2

[PATCH v2 2/7] Refactor get_eir_uuids() to get EIR data length parameter

[Vinicius Costa Gomes]

From: Anderson Lizardo <anderson.lizardo@openbossa.org>

get_eir_uuids() will be reused to parse LE advertising data as well, as
they share the same format. But for Advertising, maximum data length is
different (31 bytes vs. 240 bytes for EIR), and the radio is not
required to send the non-significant (zero-filled) bytes.

adapter_emit_device_found() now also accepts a EIR data length
parameter, so it can be reused for LE and can propagate the exact data
length.
---
 src/adapter.c |   17 ++++++++++-------
 src/adapter.h |    2 +-
 src/event.c   |    2 +-
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 8b742b7..363ee94 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2962,7 +2962,8 @@ static void emit_device_found(const char *path, const char *address,
 	g_dbus_send_message(connection, signal);
 }
 
-static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
+static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
+							size_t *uuid_count)
 {
 	uint16_t len = 0;
 	char **uuids;
@@ -2976,7 +2977,10 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
 	uuid_t service;
 	unsigned int i;
 
-	while (len < EIR_DATA_LENGTH - 1) {
+	if (eir_data == NULL || eir_length == 0)
+		return NULL;
+
+	while (len < eir_length - 1) {
 		uint8_t field_len = eir_data[0];
 
 		/* Check for the end of EIR */
@@ -3006,7 +3010,7 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
 	}
 
 	/* Bail out if got incorrect length */
-	if (len > EIR_DATA_LENGTH)
+	if (len > eir_length)
 		return NULL;
 
 	total = uuid16_count + uuid32_count + uuid128_count;
@@ -3056,7 +3060,7 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
 }
 
 void adapter_emit_device_found(struct btd_adapter *adapter,
-				struct remote_dev_info *dev, uint8_t *eir_data)
+	struct remote_dev_info *dev, uint8_t *eir_data, size_t eir_length)
 {
 	struct btd_device *device;
 	char peer_addr[18], local_addr[18];
@@ -3086,8 +3090,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 		alias = g_strdup(dev->alias);
 
 	/* Extract UUIDs from extended inquiry response if any*/
-	if (eir_data != NULL)
-		uuids = get_eir_uuids(eir_data, &uuid_count);
+	uuids = get_eir_uuids(eir_data, eir_length, &uuid_count);
 
 	emit_device_found(adapter->path, paddr,
 			"Address", DBUS_TYPE_STRING, &paddr,
@@ -3147,7 +3150,7 @@ done:
 	adapter->found_devices = g_slist_sort(adapter->found_devices,
 						(GCompareFunc) dev_rssi_cmp);
 
-	adapter_emit_device_found(adapter, dev, eir_data);
+	adapter_emit_device_found(adapter, dev, eir_data, EIR_DATA_LENGTH);
 }
 
 int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr)
diff --git a/src/adapter.h b/src/adapter.h
index 8019cfc..89b07d7 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -124,7 +124,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 				name_status_t name_status, uint8_t *eir_data);
 int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr);
 void adapter_emit_device_found(struct btd_adapter *adapter,
-				struct remote_dev_info *dev, uint8_t *eir_data);
+	struct remote_dev_info *dev, uint8_t *eir_data, size_t eir_length);
 void adapter_mode_changed(struct btd_adapter *adapter, uint8_t scan_mode);
 void adapter_setname_complete(bdaddr_t *local, uint8_t status);
 void adapter_update_tx_power(bdaddr_t *bdaddr, uint8_t status, void *ptr);
diff --git a/src/event.c b/src/event.c
index 971bb35..a057306 100644
--- a/src/event.c
+++ b/src/event.c
@@ -510,7 +510,7 @@ void btd_event_remote_name(bdaddr_t *local, bdaddr_t *peer, uint8_t status,
 	if (dev_info) {
 		g_free(dev_info->name);
 		dev_info->name = g_strdup(name);
-		adapter_emit_device_found(adapter, dev_info, NULL);
+		adapter_emit_device_found(adapter, dev_info, NULL, 0);
 	}
 
 	if (device)
-- 
1.7.3.2

Re: [PATCH v2 2/7] Refactor get_eir_uuids() to get EIR data length parameter

[Johan Hedberg]

Hi,

On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> get_eir_uuids() will be reused to parse LE advertising data as well, as
> they share the same format. But for Advertising, maximum data length is
> different (31 bytes vs. 240 bytes for EIR), and the radio is not
> required to send the non-significant (zero-filled) bytes.
> 
> adapter_emit_device_found() now also accepts a EIR data length
> parameter, so it can be reused for LE and can propagate the exact data
> length.
> ---
>  src/adapter.c |   17 ++++++++++-------
>  src/adapter.h |    2 +-
>  src/event.c   |    2 +-
>  3 files changed, 12 insertions(+), 9 deletions(-)

Pushed upstream after I fixed the following coding style issue:

>  void adapter_emit_device_found(struct btd_adapter *adapter,
> -				struct remote_dev_info *dev, uint8_t *eir_data)
> +	struct remote_dev_info *dev, uint8_t *eir_data, size_t eir_length)

The parameters on the second line should be indented if possible enough
that they're past the opening ( on the line above (in this case it means
that you need three lines for the parameters).

>  void adapter_emit_device_found(struct btd_adapter *adapter,
> -				struct remote_dev_info *dev, uint8_t *eir_data);
> +	struct remote_dev_info *dev, uint8_t *eir_data, size_t eir_length);

Same here.

Johan

[PATCH v2 3/7] Refactoring adapter_update_found_devices() function

[Vinicius Costa Gomes]

From: Bruna Moreira <bruna.moreira@openbossa.org>

The common code from adapter_update_found_devices() was moved to
update_found_devices().
---
 src/adapter.c |   50 +++++++++++++++++++++++++++++++-------------------
 1 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 363ee94..6f4f2a3 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3108,10 +3108,8 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	g_strfreev(uuids);
 }
 
-void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
-				int8_t rssi, uint32_t class, const char *name,
-				const char *alias, gboolean legacy,
-				name_status_t name_status, uint8_t *eir_data)
+static struct remote_dev_info *get_found_dev(struct btd_adapter *adapter,
+				const bdaddr_t *bdaddr, gboolean *new_dev)
 {
 	struct remote_dev_info *dev, match;
 
@@ -3121,30 +3119,44 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 
 	dev = adapter_search_found_devices(adapter, &match);
 	if (dev) {
+		*new_dev = FALSE;
 		/* Out of range list update */
 		adapter->oor_devices = g_slist_remove(adapter->oor_devices,
 							dev);
+	} else {
+		*new_dev = TRUE;
+		dev = g_new0(struct remote_dev_info, 1);
+		bacpy(&dev->bdaddr, bdaddr);
+		adapter->found_devices = g_slist_prepend(adapter->found_devices,
+									dev);
+	}
 
-		if (rssi == dev->rssi)
-			return;
+	return dev;
+}
 
-		goto done;
-	}
+void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
+				int8_t rssi, uint32_t class, const char *name,
+				const char *alias, gboolean legacy,
+				name_status_t name_status, uint8_t *eir_data)
+{
+	struct remote_dev_info *dev;
+	gboolean new_dev;
 
-	dev = g_new0(struct remote_dev_info, 1);
+	dev = get_found_dev(adapter, bdaddr, &new_dev);
 
-	bacpy(&dev->bdaddr, bdaddr);
-	dev->class = class;
-	if (name)
-		dev->name = g_strdup(name);
-	if (alias)
-		dev->alias = g_strdup(alias);
-	dev->legacy = legacy;
-	dev->name_status = name_status;
+	if (new_dev) {
+		if (name)
+			dev->name = g_strdup(name);
 
-	adapter->found_devices = g_slist_prepend(adapter->found_devices, dev);
+		if (alias)
+			dev->alias = g_strdup(alias);
+
+		dev->class = class;
+		dev->legacy = legacy;
+		dev->name_status = name_status;
+	} else if (dev->rssi == rssi)
+		return;
 
-done:
 	dev->rssi = rssi;
 
 	adapter->found_devices = g_slist_sort(adapter->found_devices,
-- 
1.7.3.2

Re: [PATCH v2 3/7] Refactoring adapter_update_found_devices() function

[Luiz Augusto von Dentz]

Hi,

On Thu, Nov 11, 2010 at 8:51 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> From: Bruna Moreira <bruna.moreira@openbossa.org>
>
> The common code from adapter_update_found_devices() was moved to
> update_found_devices().
> ---
>  src/adapter.c |   50 +++++++++++++++++++++++++++++++-------------------
>  1 files changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 363ee94..6f4f2a3 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -3108,10 +3108,8 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
>        g_strfreev(uuids);
>  }
>
> -void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
> -                               int8_t rssi, uint32_t class, const char *name,
> -                               const char *alias, gboolean legacy,
> -                               name_status_t name_status, uint8_t *eir_data)
> +static struct remote_dev_info *get_found_dev(struct btd_adapter *adapter,
> +                               const bdaddr_t *bdaddr, gboolean *new_dev)
>  {
>        struct remote_dev_info *dev, match;
>
> @@ -3121,30 +3119,44 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
>
>        dev = adapter_search_found_devices(adapter, &match);
>        if (dev) {
> +               *new_dev = FALSE;
>                /* Out of range list update */
>                adapter->oor_devices = g_slist_remove(adapter->oor_devices,
>                                                        dev);
> +       } else {
> +               *new_dev = TRUE;
> +               dev = g_new0(struct remote_dev_info, 1);
> +               bacpy(&dev->bdaddr, bdaddr);
> +               adapter->found_devices = g_slist_prepend(adapter->found_devices,
> +                                                                       dev);
> +       }
>
> -               if (rssi == dev->rssi)
> -                       return;
> +       return dev;
> +}
>
> -               goto done;
> -       }
> +void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
> +                               int8_t rssi, uint32_t class, const char *name,
> +                               const char *alias, gboolean legacy,
> +                               name_status_t name_status, uint8_t *eir_data)
> +{
> +       struct remote_dev_info *dev;
> +       gboolean new_dev;
>
> -       dev = g_new0(struct remote_dev_info, 1);
> +       dev = get_found_dev(adapter, bdaddr, &new_dev);
>
> -       bacpy(&dev->bdaddr, bdaddr);
> -       dev->class = class;
> -       if (name)
> -               dev->name = g_strdup(name);
> -       if (alias)
> -               dev->alias = g_strdup(alias);
> -       dev->legacy = legacy;
> -       dev->name_status = name_status;
> +       if (new_dev) {
> +               if (name)
> +                       dev->name = g_strdup(name);
>
> -       adapter->found_devices = g_slist_prepend(adapter->found_devices, dev);
> +               if (alias)
> +                       dev->alias = g_strdup(alias);
> +
> +               dev->class = class;
> +               dev->legacy = legacy;
> +               dev->name_status = name_status;
> +       } else if (dev->rssi == rssi)
> +               return;
>
> -done:
>        dev->rssi = rssi;
>
>        adapter->found_devices = g_slist_sort(adapter->found_devices,
> --
> 1.7.3.2

Im not so sure this is doing any good to the code, apparently this add
more code without a clear reason. IMO the only thing that could be
really be beneficial is to have some helper functions such as
dev_info_new/dev_info_free, splitting the memory allocation and the
members initialization doesn't sound a good idea too.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH v2 3/7] Refactoring adapter_update_found_devices() function

[Johan Hedberg]

Hi,

On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> The common code from adapter_update_found_devices() was moved to
> update_found_devices().
> ---
>  src/adapter.c |   50 +++++++++++++++++++++++++++++++-------------------
>  1 files changed, 31 insertions(+), 19 deletions(-)

This one has also been pushed upstream after fixing the following coding
style issue:

> -void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
> -				int8_t rssi, uint32_t class, const char *name,
> -				const char *alias, gboolean legacy,
> -				name_status_t name_status, uint8_t *eir_data)
> +static struct remote_dev_info *get_found_dev(struct btd_adapter *adapter,
> +				const bdaddr_t *bdaddr, gboolean *new_dev)

Same thing as with the previous patch: the parameters on the second line
need to be indented past the opening ( on the first line.

Johan

[PATCH v2 4/7] Initial advertising data parsing implementation

[Vinicius Costa Gomes]

From: Bruna Moreira <bruna.moreira@openbossa.org>

Implement adapter_update_adv() function to parse advertising data
received by btd_event_adv() function. Add some fields for advertising
data in remote_device_info struct.
---
 plugins/hciops.c |    9 +++------
 src/adapter.c    |   25 +++++++++++++++++++++++++
 src/adapter.h    |    5 +++++
 src/event.c      |   13 +++++++++++++
 src/event.h      |    1 +
 5 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index fc99275..dc7a657 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -1011,7 +1011,7 @@ static inline void le_metaevent(int index, void *ptr)
 {
 	evt_le_meta_event *meta = ptr;
 	le_advertising_info *info;
-	uint8_t *rssi, num, i;
+	uint8_t num, i;
 
 	DBG("LE Meta Event");
 
@@ -1022,11 +1022,8 @@ static inline void le_metaevent(int index, void *ptr)
 	info = (le_advertising_info *) (meta->data + 1);
 
 	for (i = 0; i < num; i++) {
-		/* RSSI is last byte of the advertising report event */
-		rssi = info->data + info->length;
-		btd_event_inquiry_result(&BDADDR(index), &info->bdaddr, 0,
-								*rssi, NULL);
-		info = (le_advertising_info *) (rssi + 1);
+		btd_event_adv(&BDADDR(index), info);
+		info = (le_advertising_info *) (info->data + info->length + 1);
 	}
 }
 
diff --git a/src/adapter.c b/src/adapter.c
index 6f4f2a3..9c92e22 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3134,6 +3134,30 @@ static struct remote_dev_info *get_found_dev(struct btd_adapter *adapter,
 	return dev;
 }
 
+void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info)
+{
+	struct remote_dev_info *dev;
+	bdaddr_t bdaddr;
+	gboolean new_dev;
+	int8_t rssi = 0;
+
+	rssi = *(info->data + info->length);
+	bdaddr = info->bdaddr;
+
+	dev = get_found_dev(adapter, &bdaddr, &new_dev);
+
+	if (new_dev) {
+		dev->le = TRUE;
+		dev->evt_type = info->evt_type;
+	} else if (dev->rssi == rssi)
+		return;
+
+	dev->rssi = rssi;
+
+	adapter->found_devices = g_slist_sort(adapter->found_devices,
+						(GCompareFunc) dev_rssi_cmp);
+}
+
 void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 				int8_t rssi, uint32_t class, const char *name,
 				const char *alias, gboolean legacy,
@@ -3151,6 +3175,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 		if (alias)
 			dev->alias = g_strdup(alias);
 
+		dev->le = FALSE;
 		dev->class = class;
 		dev->legacy = legacy;
 		dev->name_status = name_status;
diff --git a/src/adapter.h b/src/adapter.h
index 89b07d7..766b079 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -69,6 +69,10 @@ struct remote_dev_info {
 	char *alias;
 	dbus_bool_t legacy;
 	name_status_t name_status;
+	gboolean le;
+	/* LE adv data */
+	uint8_t evt_type;
+	uint8_t bdaddr_type;
 };
 
 struct hci_dev {
@@ -118,6 +122,7 @@ int adapter_get_discover_type(struct btd_adapter *adapter);
 gboolean adapter_is_ready(struct btd_adapter *adapter);
 struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter,
 						struct remote_dev_info *match);
+void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info);
 void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 				int8_t rssi, uint32_t class, const char *name,
 				const char *alias, gboolean legacy,
diff --git a/src/event.c b/src/event.c
index a057306..8b03bc3 100644
--- a/src/event.c
+++ b/src/event.c
@@ -322,6 +322,19 @@ static char *extract_eir_name(uint8_t *data, uint8_t *type)
 	return NULL;
 }
 
+void btd_event_adv(bdaddr_t *local, le_advertising_info *info)
+{
+	struct btd_adapter *adapter;
+
+	adapter = manager_find_adapter(local);
+	if (adapter == NULL) {
+		error("No matching adapter found");
+		return;
+	}
+
+	adapter_update_adv(adapter, info);
+}
+
 void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 				int8_t rssi, uint8_t *data)
 {
diff --git a/src/event.h b/src/event.h
index 4a7b9c9..44e1462 100644
--- a/src/event.h
+++ b/src/event.h
@@ -23,6 +23,7 @@
  */
 
 int btd_event_request_pin(bdaddr_t *sba, struct hci_conn_info *ci);
+void btd_event_adv(bdaddr_t *local, le_advertising_info *info);
 void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class, int8_t rssi, uint8_t *data);
 void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer, gboolean legacy);
 void btd_event_remote_class(bdaddr_t *local, bdaddr_t *peer, uint32_t class);
-- 
1.7.3.2

Re: [PATCH v2 4/7] Initial advertising data parsing implementation

[Luiz Augusto von Dentz]

Hi,

On Thu, Nov 11, 2010 at 8:51 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> From: Bruna Moreira <bruna.moreira@openbossa.org>
>
> Implement adapter_update_adv() function to parse advertising data
> received by btd_event_adv() function. Add some fields for advertising
> data in remote_device_info struct.
> ---
>  plugins/hciops.c |    9 +++------
>  src/adapter.c    |   25 +++++++++++++++++++++++++
>  src/adapter.h    |    5 +++++
>  src/event.c      |   13 +++++++++++++
>  src/event.h      |    1 +
>  5 files changed, 47 insertions(+), 6 deletions(-)
>
> diff --git a/plugins/hciops.c b/plugins/hciops.c
> index fc99275..dc7a657 100644
> --- a/plugins/hciops.c
> +++ b/plugins/hciops.c
> @@ -1011,7 +1011,7 @@ static inline void le_metaevent(int index, void *ptr)
>  {
>        evt_le_meta_event *meta = ptr;
>        le_advertising_info *info;
> -       uint8_t *rssi, num, i;
> +       uint8_t num, i;
>
>        DBG("LE Meta Event");
>
> @@ -1022,11 +1022,8 @@ static inline void le_metaevent(int index, void *ptr)
>        info = (le_advertising_info *) (meta->data + 1);
>
>        for (i = 0; i < num; i++) {
> -               /* RSSI is last byte of the advertising report event */
> -               rssi = info->data + info->length;
> -               btd_event_inquiry_result(&BDADDR(index), &info->bdaddr, 0,
> -                                                               *rssi, NULL);
> -               info = (le_advertising_info *) (rssi + 1);
> +               btd_event_adv(&BDADDR(index), info);
> +               info = (le_advertising_info *) (info->data + info->length + 1);
>        }
>  }
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 6f4f2a3..9c92e22 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -3134,6 +3134,30 @@ static struct remote_dev_info *get_found_dev(struct btd_adapter *adapter,
>        return dev;
>  }
>
> +void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info)
> +{
> +       struct remote_dev_info *dev;
> +       bdaddr_t bdaddr;
> +       gboolean new_dev;
> +       int8_t rssi = 0;
> +
> +       rssi = *(info->data + info->length);
> +       bdaddr = info->bdaddr;
> +
> +       dev = get_found_dev(adapter, &bdaddr, &new_dev);
> +
> +       if (new_dev) {
> +               dev->le = TRUE;
> +               dev->evt_type = info->evt_type;
> +       } else if (dev->rssi == rssi)
> +               return;

Again this does sound like a good idea to do the member initialization
in a different function, also isn't there a possibility that the le
bdaddr random/private clashes with br/edr bdaddr? I would say the
search function need to take the address and its type a least.

> +       dev->rssi = rssi;
> +
> +       adapter->found_devices = g_slist_sort(adapter->found_devices,
> +                                               (GCompareFunc) dev_rssi_cmp);
> +}
> +
>  void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
>                                int8_t rssi, uint32_t class, const char *name,
>                                const char *alias, gboolean legacy,
> @@ -3151,6 +3175,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
>                if (alias)
>                        dev->alias = g_strdup(alias);
>
> +               dev->le = FALSE;
>                dev->class = class;
>                dev->legacy = legacy;
>                dev->name_status = name_status;
> diff --git a/src/adapter.h b/src/adapter.h
> index 89b07d7..766b079 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -69,6 +69,10 @@ struct remote_dev_info {
>        char *alias;
>        dbus_bool_t legacy;
>        name_status_t name_status;
> +       gboolean le;
> +       /* LE adv data */
> +       uint8_t evt_type;
> +       uint8_t bdaddr_type;
>  };

I would use bdaddr_type and create one for br/edr, we can probably
speed up the search if we can check the address type before the
address and make it impossible to mix LE with BR/EDR devices, their
types would simply not match.

>  struct hci_dev {
> @@ -118,6 +122,7 @@ int adapter_get_discover_type(struct btd_adapter *adapter);
>  gboolean adapter_is_ready(struct btd_adapter *adapter);
>  struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter,
>                                                struct remote_dev_info *match);
> +void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info);
>  void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
>                                int8_t rssi, uint32_t class, const char *name,
>                                const char *alias, gboolean legacy,
> diff --git a/src/event.c b/src/event.c
> index a057306..8b03bc3 100644
> --- a/src/event.c
> +++ b/src/event.c
> @@ -322,6 +322,19 @@ static char *extract_eir_name(uint8_t *data, uint8_t *type)
>        return NULL;
>  }
>
> +void btd_event_adv(bdaddr_t *local, le_advertising_info *info)
> +{
> +       struct btd_adapter *adapter;
> +
> +       adapter = manager_find_adapter(local);
> +       if (adapter == NULL) {
> +               error("No matching adapter found");
> +               return;
> +       }
> +
> +       adapter_update_adv(adapter, info);
> +}
> +
>  void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
>                                int8_t rssi, uint8_t *data)
>  {
> diff --git a/src/event.h b/src/event.h
> index 4a7b9c9..44e1462 100644
> --- a/src/event.h
> +++ b/src/event.h
> @@ -23,6 +23,7 @@
>  */
>
>  int btd_event_request_pin(bdaddr_t *sba, struct hci_conn_info *ci);
> +void btd_event_adv(bdaddr_t *local, le_advertising_info *info);
>  void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class, int8_t rssi, uint8_t *data);
>  void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer, gboolean legacy);
>  void btd_event_remote_class(bdaddr_t *local, bdaddr_t *peer, uint32_t class);
> --
> 1.7.3.2
>
> --
> 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
>



-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH v2 4/7] Initial advertising data parsing implementation

[Johan Hedberg]

Hi,

On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> +	int8_t rssi = 0;
> +
> +	rssi = *(info->data + info->length);

There's no point in initializing rssi upon declaration since the very
next thing you do is assign a new value to it (not to mention that in
general we try to avoid initialization upon declaration in the code).

> +void btd_event_adv(bdaddr_t *local, le_advertising_info *info)

I'm not really a fan of the adv abreviation, and the function name isn't
even in danger of growing horribly long if you don't abreviate it. So
could you come up with something more clear? Maybe
btd_event_advertising_info or btd_event_advertising_report?

I'll stop reviewing this patch-set here since the rest seem to depend on
the name of this function.

Johan

[PATCH v2 5/7] Advertising data: extract local name

[Vinicius Costa Gomes]

From: Bruna Moreira <bruna.moreira@openbossa.org>

Move extract_eir_name() to glib-helper.c file and rename function to
bt_extract_eir_name(). The local name is extracted from the advertising
data.
---
 src/adapter.c     |    7 +++++++
 src/event.c       |   23 +----------------------
 src/glib-helper.c |   22 ++++++++++++++++++++++
 src/glib-helper.h |    1 +
 4 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 9c92e22..7488322 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3140,6 +3140,7 @@ void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info)
 	bdaddr_t bdaddr;
 	gboolean new_dev;
 	int8_t rssi = 0;
+	uint8_t type = 0x00;
 
 	rssi = *(info->data + info->length);
 	bdaddr = info->bdaddr;
@@ -3156,6 +3157,12 @@ void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info)
 
 	adapter->found_devices = g_slist_sort(adapter->found_devices,
 						(GCompareFunc) dev_rssi_cmp);
+
+	if (info->length) {
+		char *tmp_name = bt_extract_eir_name(info->data, &type);
+		if (tmp_name)
+			dev->name = tmp_name;
+	}
 }
 
 void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
diff --git a/src/event.c b/src/event.c
index 8b03bc3..57bdf60 100644
--- a/src/event.c
+++ b/src/event.c
@@ -301,27 +301,6 @@ void btd_event_simple_pairing_complete(bdaddr_t *local, bdaddr_t *peer,
 	device_simple_pairing_complete(device, status);
 }
 
-static char *extract_eir_name(uint8_t *data, uint8_t *type)
-{
-	if (!data || !type)
-		return NULL;
-
-	if (data[0] == 0)
-		return NULL;
-
-	*type = data[1];
-
-	switch (*type) {
-	case 0x08:
-	case 0x09:
-		if (!g_utf8_validate((char *) (data + 2), data[0] - 1, NULL))
-			return strdup("");
-		return strndup((char *) (data + 2), data[0] - 1);
-	}
-
-	return NULL;
-}
-
 void btd_event_adv(bdaddr_t *local, le_advertising_info *info)
 {
 	struct btd_adapter *adapter;
@@ -410,7 +389,7 @@ void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 	} else
 		legacy = TRUE;
 
-	tmp_name = extract_eir_name(data, &name_type);
+	tmp_name = bt_extract_eir_name(data, &name_type);
 	if (tmp_name) {
 		if (name_type == 0x09) {
 			write_device_name(local, peer, tmp_name);
diff --git a/src/glib-helper.c b/src/glib-helper.c
index 9d76626..33668d7 100644
--- a/src/glib-helper.c
+++ b/src/glib-helper.c
@@ -43,6 +43,7 @@
 #include <glib.h>
 
 #include "glib-helper.h"
+#include "sdpd.h"
 
 /* Number of seconds to keep a sdp_session_t in the cache */
 #define CACHE_TIMEOUT 2
@@ -576,3 +577,24 @@ GSList *bt_string2list(const gchar *str)
 
 	return l;
 }
+
+char *bt_extract_eir_name(uint8_t *data, uint8_t *type)
+{
+	if (!data || !type)
+		return NULL;
+
+	if (data[0] == 0)
+		return NULL;
+
+	*type = data[1];
+
+	switch (*type) {
+	case EIR_NAME_SHORT:
+	case EIR_NAME_COMPLETE:
+		if (!g_utf8_validate((char *) (data + 2), data[0] - 1, NULL))
+			return strdup("");
+		return strndup((char *) (data + 2), data[0] - 1);
+	}
+
+	return NULL;
+}
diff --git a/src/glib-helper.h b/src/glib-helper.h
index e89c2c6..dfe4123 100644
--- a/src/glib-helper.h
+++ b/src/glib-helper.h
@@ -38,3 +38,4 @@ char *bt_name2string(const char *string);
 int bt_string2uuid(uuid_t *uuid, const char *string);
 gchar *bt_list2string(GSList *list);
 GSList *bt_string2list(const gchar *str);
+char *bt_extract_eir_name(uint8_t *data, uint8_t *type);
-- 
1.7.3.2

[PATCH v2 6/7] Extract service UUIDs from advertising data

[Vinicius Costa Gomes]

From: Bruna Moreira <bruna.moreira@openbossa.org>

Make get_eir_uuids() return a GSList of strings, so it can be reused to
extract UUIDs from LE advertising data. The bt_strlist2array() helper
function was created to convert a GSList into a plain array of strings
(needed to send through D-Bus).
---
 src/adapter.c |   64 ++++++++++++++++++++++++++++++++++++++++++--------------
 src/adapter.h |    1 +
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 7488322..cd2d8d5 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -202,6 +202,8 @@ static void dev_info_free(struct remote_dev_info *dev)
 {
 	g_free(dev->name);
 	g_free(dev->alias);
+	g_slist_foreach(dev->services, (GFunc) g_free, NULL);
+	g_slist_free(dev->services);
 	g_free(dev);
 }
 
@@ -2962,11 +2964,20 @@ static void emit_device_found(const char *path, const char *address,
 	g_dbus_send_message(connection, signal);
 }
 
-static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
-							size_t *uuid_count)
+static char **strlist2array(GSList *list)
+{
+	char *tmp, **array;
+
+	tmp = bt_list2string(list);
+	array = g_strsplit(tmp, " ", 0);
+	g_free(tmp);
+
+	return array;
+}
+
+static GSList *get_eir_uuids(uint8_t *eir_data, size_t eir_length, GSList *list)
 {
 	uint16_t len = 0;
-	char **uuids;
 	size_t total;
 	size_t uuid16_count = 0;
 	size_t uuid32_count = 0;
@@ -2975,10 +2986,11 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 	uint8_t *uuid32;
 	uint8_t *uuid128;
 	uuid_t service;
+	char *uuid_str;
 	unsigned int i;
 
 	if (eir_data == NULL || eir_length == 0)
-		return NULL;
+		return list;
 
 	while (len < eir_length - 1) {
 		uint8_t field_len = eir_data[0];
@@ -3011,15 +3023,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 
 	/* Bail out if got incorrect length */
 	if (len > eir_length)
-		return NULL;
+		return list;
 
 	total = uuid16_count + uuid32_count + uuid128_count;
-	*uuid_count = total;
 
 	if (!total)
-		return NULL;
-
-	uuids = g_new0(char *, total + 1);
+		return list;
 
 	/* Generate uuids in SDP format (EIR data is Little Endian) */
 	service.type = SDP_UUID16;
@@ -3028,7 +3037,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 
 		val16 = (val16 << 8) + uuid16[0];
 		service.value.uuid16 = val16;
-		uuids[i] = bt_uuid2string(&service);
+		uuid_str = bt_uuid2string(&service);
+		if (g_slist_find_custom(list, uuid_str,
+						(GCompareFunc) strcmp) == NULL)
+			list = g_slist_append(list, uuid_str);
+		else
+			g_free(uuid_str);
 		uuid16 += 2;
 	}
 
@@ -3041,7 +3055,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 			val32 = (val32 << 8) + uuid32[k];
 
 		service.value.uuid32 = val32;
-		uuids[i] = bt_uuid2string(&service);
+		uuid_str = bt_uuid2string(&service);
+		if (g_slist_find_custom(list, uuid_str,
+						(GCompareFunc) strcmp) == NULL)
+			list = g_slist_append(list, uuid_str);
+		else
+			g_free(uuid_str);
 		uuid32 += 4;
 	}
 
@@ -3052,11 +3071,16 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 		for (k = 0; k < 16; k++)
 			service.value.uuid128.data[k] = uuid128[16 - k - 1];
 
-		uuids[i] = bt_uuid2string(&service);
+		uuid_str = bt_uuid2string(&service);
+		if (g_slist_find_custom(list, uuid_str,
+						(GCompareFunc) strcmp) == NULL)
+			list = g_slist_append(list, uuid_str);
+		else
+			g_free(uuid_str);
 		uuid128 += 16;
 	}
 
-	return uuids;
+	return list;
 }
 
 void adapter_emit_device_found(struct btd_adapter *adapter,
@@ -3069,7 +3093,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	dbus_int16_t rssi = dev->rssi;
 	char *alias;
 	char **uuids = NULL;
-	size_t uuid_count = 0;
+	size_t uuid_count;
 
 	ba2str(&dev->bdaddr, peer_addr);
 	ba2str(&adapter->bdaddr, local_addr);
@@ -3089,8 +3113,16 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	} else
 		alias = g_strdup(dev->alias);
 
-	/* Extract UUIDs from extended inquiry response if any*/
-	uuids = get_eir_uuids(eir_data, eir_length, &uuid_count);
+	/* Extract UUIDs from extended inquiry response if any */
+	dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
+	uuid_count = g_slist_length(dev->services);
+
+	if (dev->services) {
+		uuids = strlist2array(dev->services);
+		g_slist_foreach(dev->services, (GFunc) g_free, NULL);
+		g_slist_free(dev->services);
+		dev->services = NULL;
+	}
 
 	emit_device_found(adapter->path, paddr,
 			"Address", DBUS_TYPE_STRING, &paddr,
diff --git a/src/adapter.h b/src/adapter.h
index 766b079..84d691b 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -71,6 +71,7 @@ struct remote_dev_info {
 	name_status_t name_status;
 	gboolean le;
 	/* LE adv data */
+	GSList *services;
 	uint8_t evt_type;
 	uint8_t bdaddr_type;
 };
-- 
1.7.3.2

[PATCH v2 7/7] Emit "DeviceFound" signal for LE devices

[Vinicius Costa Gomes]

From: Bruna Moreira <bruna.moreira@openbossa.org>

The adapter_emit_device_found() function was modified to emit
DeviceFound signal for LE devices as well.
---
 src/adapter.c |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index cd2d8d5..7edec41 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3102,6 +3102,27 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	if (device)
 		paired = device_is_paired(device);
 
+	/* Extract UUIDs from extended inquiry response if any */
+	dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
+	uuid_count = g_slist_length(dev->services);
+
+	if (dev->services)
+		uuids = strlist2array(dev->services);
+
+	if (dev->le) {
+		emit_device_found(adapter->path, paddr,
+				"Address", DBUS_TYPE_STRING, &paddr,
+				"RSSI", DBUS_TYPE_INT16, &rssi,
+				"Name", DBUS_TYPE_STRING, &dev->name,
+				"Paired", DBUS_TYPE_BOOLEAN, &paired,
+				"UUIDs", DBUS_TYPE_ARRAY, &uuids, uuid_count,
+				NULL);
+
+		g_strfreev(uuids);
+
+		return;
+	}
+
 	icon = class_to_icon(dev->class);
 
 	if (!dev->alias) {
@@ -3113,12 +3134,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	} else
 		alias = g_strdup(dev->alias);
 
-	/* Extract UUIDs from extended inquiry response if any */
-	dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
-	uuid_count = g_slist_length(dev->services);
-
 	if (dev->services) {
-		uuids = strlist2array(dev->services);
 		g_slist_foreach(dev->services, (GFunc) g_free, NULL);
 		g_slist_free(dev->services);
 		dev->services = NULL;
@@ -3195,6 +3211,10 @@ void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info)
 		if (tmp_name)
 			dev->name = tmp_name;
 	}
+
+	/* FIXME: check if other information was changed before emitting the
+	 * signal */
+	adapter_emit_device_found(adapter, dev, info->data, info->length);
 }
 
 void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
-- 
1.7.3.2

Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Luiz Augusto von Dentz]

Hi,

2010/11/11 Vinicius Costa Gomes <vinicius.gomes@openbossa.org>:
> From: Bruna Moreira <bruna.moreira@openbossa.org>
>
> ---
>  src/adapter.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index b1aabbd..8b742b7 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -2977,14 +2977,13 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
>        unsigned int i;
>
>        while (len < EIR_DATA_LENGTH - 1) {
> -               uint8_t type = eir_data[1];
>                uint8_t field_len = eir_data[0];
>
>                /* Check for the end of EIR */
>                if (field_len == 0)
>                        break;
>
> -               switch (type) {
> +               switch (eir_data[1]) {
>                case EIR_UUID16_SOME:
>                case EIR_UUID16_ALL:
>                        uuid16_count = field_len / 2;

IMO type is easier to understand here, we just need to initialize it
latter after the length check.


-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Johan Hedberg]

Hi Luiz,

On Thu, Nov 11, 2010, Luiz Augusto von Dentz wrote:
> >        while (len < EIR_DATA_LENGTH - 1) {
> > -               uint8_t type = eir_data[1];
> >                uint8_t field_len = eir_data[0];
> >
> >                /* Check for the end of EIR */
> >                if (field_len == 0)
> >                        break;
> >
> > -               switch (type) {
> > +               switch (eir_data[1]) {
> >                case EIR_UUID16_SOME:
> >                case EIR_UUID16_ALL:
> >                        uuid16_count = field_len / 2;
> 
> IMO type is easier to understand here, we just need to initialize it
> latter after the length check.

True, however I wasn't bothered enough about this and went ahead and
pushed the patch anyway upstream. If someone feels like it, feel free to
reintroduce the variable ;)

Johan

Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Johan Hedberg]

Hi,

On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> diff --git a/src/adapter.c b/src/adapter.c
> index b1aabbd..8b742b7 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -2977,14 +2977,13 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
>  	unsigned int i;
>  
>  	while (len < EIR_DATA_LENGTH - 1) {
> -		uint8_t type = eir_data[1];
>  		uint8_t field_len = eir_data[0];
>  
>  		/* Check for the end of EIR */
>  		if (field_len == 0)
>  			break;
>  
> -		switch (type) {
> +		switch (eir_data[1]) {
>  		case EIR_UUID16_SOME:
>  		case EIR_UUID16_ALL:
>  			uuid16_count = field_len / 2;

Pushed upstream. Thanks.

Johan

RE: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Inga Stotland]

Hi Johan,

-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org
[mailto:linux-bluetooth-owner@vger.kernel.org] On Behalf Of Johan Hedberg
Sent: Thursday, November 11, 2010 1:07 PM
To: Vinicius Costa Gomes
Cc: linux-bluetooth@vger.kernel.org; Bruna Moreira
Subject: Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length
is zero

Hi,

On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> diff --git a/src/adapter.c b/src/adapter.c
> index b1aabbd..8b742b7 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -2977,14 +2977,13 @@ static char **get_eir_uuids(uint8_t *eir_data,
size_t *uuid_count)
>  	unsigned int i;
>  
>  	while (len < EIR_DATA_LENGTH - 1) {
> -		uint8_t type = eir_data[1];
>  		uint8_t field_len = eir_data[0];
>  
>  		/* Check for the end of EIR */
>  		if (field_len == 0)
>  			break;
>  
> -		switch (type) {
> +		switch (eir_data[1]) {
>  		case EIR_UUID16_SOME:
>  		case EIR_UUID16_ALL:
>  			uuid16_count = field_len / 2;

Pushed upstream. Thanks.

Johan
--

Was there a bug to begin with? :)
The access to eir_data[1] was always valid due to the check (len <
EIR_DATA_LENGTH - 1)
and the fact that eir_data is a buffer of fixed length of EIR_DATA_LENGTH
(240 bytes).
Oh well, it's upstream already.

Inga

Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Johan Hedberg]

Hi Inga,

On Thu, Nov 11, 2010, Inga Stotland wrote:
> Was there a bug to begin with? :)
> The access to eir_data[1] was always valid due to the check (len <
> EIR_DATA_LENGTH - 1)
> and the fact that eir_data is a buffer of fixed length of EIR_DATA_LENGTH
> (240 bytes).

On closer inspection it seems you might be right, however it'd be nice
to get some comments from the original patch author about this (were
there e.g. crashes or some valgrind warnings observed or was this just
speculation based on looking at the code).

Btw, it seems I may need to slow down on my response time to patches so
there's better time for other people to review them too. E.g. both you
and Luiz were a bit late to the game on a couple of recent patches.
Maybe a 24 hour period before I push anything might be good enough?

Johan

Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Gustavo F. Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2010-11-12 18:54:34 +0200]:

> Hi Inga,
> 
> On Thu, Nov 11, 2010, Inga Stotland wrote:
> > Was there a bug to begin with? :)
> > The access to eir_data[1] was always valid due to the check (len <
> > EIR_DATA_LENGTH - 1)
> > and the fact that eir_data is a buffer of fixed length of EIR_DATA_LENGTH
> > (240 bytes).
> 
> On closer inspection it seems you might be right, however it'd be nice
> to get some comments from the original patch author about this (were
> there e.g. crashes or some valgrind warnings observed or was this just
> speculation based on looking at the code).
> 
> Btw, it seems I may need to slow down on my response time to patches so
> there's better time for other people to review them too. E.g. both you
> and Luiz were a bit late to the game on a couple of recent patches.
> Maybe a 24 hour period before I push anything might be good enough?

I would say 48h, give more time to people review, in case you spent a
whole day off the linux-bluetooth.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Anderson Lizardo]

On 11/12/10, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Inga,
>
> On Thu, Nov 11, 2010, Inga Stotland wrote:
>> Was there a bug to begin with? :)
>> The access to eir_data[1] was always valid due to the check (len <
>> EIR_DATA_LENGTH - 1)
>> and the fact that eir_data is a buffer of fixed length of EIR_DATA_LENGTH
>> (240 bytes).
>
> On closer inspection it seems you might be right, however it'd be nice
> to get some comments from the original patch author about this (were
> there e.g. crashes or some valgrind warnings observed or was this just
> speculation based on looking at the code).

There were valgrind "unintialized value" warnings related to this
after we started using it for parsing adv data (which is max 31 bytes
long but may be smaller because spec allows radio to strip non
significant bytes before sending, which explains why we added a length
parameter in a later patch). Actually I dont think the current code is
"safe" as it assumes the EIR data is aways 240 bytes long, which seems
true as per spec , but less data could be sent, causing reading beyond
buffer.

> Btw, it seems I may need to slow down on my response time to patches so
> there's better time for other people to review them too. E.g. both you
> and Luiz were a bit late to the game on a couple of recent patches.
> Maybe a 24 hour period before I push anything might be good enough?

I appreciate if you could send your comments ASAP, so we can fix them
quickly. Pushing upstream can be delayed for non trivial patches.

PS: I'm out of office until next Tuesday so I may not be able answer
quickly until then. thanks to Vinicius for pushing the patches fixed
versions :)

Regards
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

RE: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero

[Inga Stotland]

Hi Anderson,

-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org
[mailto:linux-bluetooth-owner@vger.kernel.org] On Behalf Of Anderson Lizardo
Sent: Friday, November 12, 2010 5:01 PM
To: Inga Stotland; Vinicius Costa Gomes; linux-bluetooth@vger.kernel.org;
Bruna Moreira
Subject: Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length
is zero

On 11/12/10, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Inga,
>
> On Thu, Nov 11, 2010, Inga Stotland wrote:
>> Was there a bug to begin with? :)
>> The access to eir_data[1] was always valid due to the check (len <
>> EIR_DATA_LENGTH - 1)
>> and the fact that eir_data is a buffer of fixed length of EIR_DATA_LENGTH
>> (240 bytes).
>
> On closer inspection it seems you might be right, however it'd be nice
> to get some comments from the original patch author about this (were
> there e.g. crashes or some valgrind warnings observed or was this just
> speculation based on looking at the code).

There were valgrind "unintialized value" warnings related to this
after we started using it for parsing adv data (which is max 31 bytes
long but may be smaller because spec allows radio to strip non
significant bytes before sending, which explains why we added a length
parameter in a later patch). Actually I dont think the current code is
"safe" as it assumes the EIR data is aways 240 bytes long, which seems
true as per spec , but less data could be sent, causing reading beyond
buffer.

---------------
Just a note...
When Extended Inquiry Response HCI event comes to the upper layer parser
from 
HCI layer it is guaranteed to contain 240 bytes where of course the non 
significant part is zeroed out. This is ensured by internal implementation 
in BlueZ and is spec compliant.

The fix is fine since it takes care of warnings, but personally I am a big
fan of 
keeping meaningful variable names :)

Regards, 

Inga Stotland

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.