[PATCH 1/3] device: Split att_connect_cb

[PATCH 1/3] device: Split att_connect_cb

[Andre Guedes]

In order to reduce code complexity, all code related to
device_browse_primary was moved to a new function called
browse_primary_connect_cb. This way att_connect_cb has
only code related to att_connect.
---
 src/device.c |   48 +++++++++++++++++++++++++++++++++---------------
 1 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/src/device.c b/src/device.c
index c7c741c..311dca1 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1818,21 +1818,12 @@ done:
 static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 {
 	struct btd_device *device = user_data;
-	struct browse_req *req = device->browse;
 	GAttrib *attrib;
 
 	if (gerr) {
-		DBusMessage *reply;
-
 		DBG("%s", gerr->message);
 
-		if (req) {
-			reply = btd_error_failed(req->msg, gerr->message);
-			g_dbus_send_message(req->conn, reply);
-
-			device->browse = NULL;
-			browse_request_free(req, TRUE);
-		} else if (device->auto_connect)
+		if (device->auto_connect)
 			device->auto_id = g_timeout_add_seconds_full(
 						G_PRIORITY_DEFAULT_IDLE,
 						AUTO_CONNECTION_INTERVAL,
@@ -1847,10 +1838,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 	if (device->attachid == 0)
 		error("Attribute server attach failure!");
 
-	if (req) {
-		req->attrib = attrib;
-		gatt_discover_primary(req->attrib, NULL, primary_cb, req);
-	} else if (device->attios) {
+	if (device->attios) {
 		device->attrib = attrib;
 		g_attrib_set_disconnect_function(device->attrib,
 						attrib_disconnected, device);
@@ -1902,6 +1890,36 @@ static gboolean att_connect(gpointer user_data)
 	return FALSE;
 }
 
+static void browse_primary_connect_cb(GIOChannel *io, GError *gerr,
+							gpointer user_data)
+{
+	struct btd_device *device = user_data;
+	struct browse_req *req = device->browse;
+	GAttrib *attrib;
+
+	if (gerr) {
+		DBusMessage *reply;
+
+		DBG("%s", gerr->message);
+
+		reply = btd_error_failed(req->msg, gerr->message);
+		g_dbus_send_message(req->conn, reply);
+
+		device->browse = NULL;
+		browse_request_free(req, TRUE);
+
+		return;
+	}
+
+	attrib = g_attrib_new(io);
+	device->attachid = attrib_channel_attach(attrib, TRUE);
+	if (device->attachid == 0)
+		error("Attribute server attach failure!");
+
+	req->attrib = attrib;
+	gatt_discover_primary(req->attrib, NULL, primary_cb, req);
+}
+
 int device_browse_primary(struct btd_device *device, DBusConnection *conn,
 				DBusMessage *msg, gboolean secure)
 {
@@ -1920,7 +1938,7 @@ int device_browse_primary(struct btd_device *device, DBusConnection *conn,
 
 	sec_level = secure ? BT_IO_SEC_HIGH : BT_IO_SEC_LOW;
 
-	req->io = bt_io_connect(BT_IO_L2CAP, att_connect_cb,
+	req->io = bt_io_connect(BT_IO_L2CAP, browse_primary_connect_cb,
 				device, NULL, NULL,
 				BT_IO_OPT_SOURCE_BDADDR, &src,
 				BT_IO_OPT_DEST_BDADDR, &device->bdaddr,
-- 
1.7.9

[PATCH 2/3] device: Fix invalid read in att_connect_cb

[Andre Guedes]

We should keep a reference to GIOChannel created in att_connect
so we can properly shut it down if the device is removed during a
connection attempt. After establishing the connection, we drop the
GIOChannel reference because GAttrib will take the responsability
of disconnect the link based on the registered ATTIO connection
callbacks.

This patch fixes the following invalid read reported by valgrind
when the device is removed and we have a ongoing connection attempt:

Invalid read of size 4
   at 0x1A90D8: att_connect_cb (device.c:1712)
   by 0x17EDB9: connect_cb (btio.c:169)
   by 0x4E6E29C: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.2800.8)
   by 0x4E6EA77: ??? (in /usr/lib/libglib-2.0.so.0.2800.8)
   by 0x4E6F0B9: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.2800.8)
   by 0x18ACB3: main (main.c:485)
 Address 0x68aacb8 is 456 bytes inside a block of size 472 free'd
   at 0x4C2556E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x1A596B: device_free (device.c:248)
   by 0x121EB5: remove_interface (object.c:563)
   by 0x122288: g_dbus_unregister_interface (object.c:715)
   by 0x1AAFD8: btd_device_unref (device.c:2636)
   by 0x1A7912: device_remove (device.c:1058)
   by 0x19F371: adapter_remove_device (adapter.c:1122)
   by 0x1A0AD4: remove_device (adapter.c:1720)
   by 0x1214C4: process_message (object.c:224)
   by 0x121B60: generic_message (object.c:447)
   by 0x51348C0: ??? (in /usr/lib/libdbus-1.so.3.5.7)
   by 0x512679F: dbus_connection_dispatch (in /usr/lib/libdbus-1.so.3.5.7)
---
 src/device.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/src/device.c b/src/device.c
index 311dca1..36d07ca 100644
--- a/src/device.c
+++ b/src/device.c
@@ -160,6 +160,8 @@ struct btd_device {
 
 	gboolean	authorizing;
 	gint		ref;
+
+	GIOChannel      *att_io;
 };
 
 static uint16_t uuid_list[] = {
@@ -247,6 +249,11 @@ static void device_free(gpointer user_data)
 	if (device->auto_id)
 		g_source_remove(device->auto_id);
 
+	if (device->att_io) {
+		g_io_channel_shutdown(device->att_io, FALSE, NULL);
+		g_io_channel_unref(device->att_io);
+	}
+
 	DBG("%p", device);
 
 	g_free(device->authr);
@@ -1820,6 +1827,9 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 	struct btd_device *device = user_data;
 	GAttrib *attrib;
 
+	g_io_channel_unref(device->att_io);
+	device->att_io = NULL;
+
 	if (gerr) {
 		DBG("%s", gerr->message);
 
@@ -1885,7 +1895,7 @@ static gboolean att_connect(gpointer user_data)
 		return FALSE;
 	}
 
-	g_io_channel_unref(io);
+	device->att_io = io;
 
 	return FALSE;
 }
-- 
1.7.9

[PATCH 3/3] device: Reply ATT requests during bonding

[Andre Guedes]

Unlike BR/EDR which has a dynamically allocated L2CAP data
channel, LE has a fix L2CAP channel where data are transmitted
(CID 4). This means that once the LE link is established the
remote device is able to send data (ATT messages) to the local
device.

Due to the ATT sequential request-response protocol, once a
client sends a request to a server, that client shall send
no other request to the same server until a response PDU has
been received. If the response PDU arrives in 30 secs the
remote device disconnects.

This way, in order to be able to reply ATT requests which may
come through channel ID 4 during the pairing, we first
establish an ATT connection and then we start the pairing
process.

That issue was discovered during test sessions in UPF. We
found some LE devices sending ATT Requests in CID 4 while
pairing was still in progress
---
 src/device.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index 36d07ca..763c79c 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2324,6 +2324,29 @@ static void create_bond_req_exit(DBusConnection *conn, void *user_data)
 	}
 }
 
+static void bonding_connect_cb(GIOChannel *io, GError *gerr,
+							gpointer user_data)
+{
+	struct btd_device *device = user_data;
+
+	g_io_channel_unref(device->att_io);
+	device->att_io = NULL;
+
+	if (gerr) {
+		DBusMessage *reply = btd_error_failed(device->bonding->msg,
+								gerr->message);
+
+		g_dbus_send_message(device->bonding->conn, reply);
+		DBG("%s", gerr->message);
+		return;
+	}
+
+	device->attrib = g_attrib_new(io);
+	device->attachid = attrib_channel_attach(device->attrib, TRUE);
+	if (device->attachid == 0)
+		error("Attribute server attach failure!");
+}
+
 DBusMessage *device_create_bonding(struct btd_device *device,
 					DBusConnection *conn,
 					DBusMessage *msg,
@@ -2340,6 +2363,30 @@ DBusMessage *device_create_bonding(struct btd_device *device,
 	if (device_is_bonded(device))
 		return btd_error_already_exists(msg);
 
+	if (device_is_le(device)) {
+		GError *gerr = NULL;
+		bdaddr_t sba;
+
+		adapter_get_address(adapter, &sba);
+
+		device->att_io = bt_io_connect(BT_IO_L2CAP, bonding_connect_cb,
+					device, NULL, &gerr,
+					BT_IO_OPT_SOURCE_BDADDR, &sba,
+					BT_IO_OPT_DEST_BDADDR,&device->bdaddr,
+					BT_IO_OPT_CID, ATT_CID,
+					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+					BT_IO_OPT_INVALID);
+
+		if (device->att_io == NULL) {
+			DBusMessage *reply = btd_error_failed(msg,
+								gerr->message);
+
+			error("Bonding bt_io_connect(): %s", gerr->message);
+			g_error_free(gerr);
+			return reply;
+		}
+	}
+
 	err = adapter_create_bonding(adapter, &device->bdaddr,
 					device->type, capability);
 	if (err < 0)
-- 
1.7.9

Re: [PATCH 1/3] device: Split att_connect_cb

[Johan Hedberg]

Hi Andre,

On Wed, Feb 08, 2012, Andre Guedes wrote:
> In order to reduce code complexity, all code related to
> device_browse_primary was moved to a new function called
> browse_primary_connect_cb. This way att_connect_cb has
> only code related to att_connect.
> ---
>  src/device.c |   48 +++++++++++++++++++++++++++++++++---------------
>  1 files changed, 33 insertions(+), 15 deletions(-)

All three patches have been applied. Thanks.

Johan