[PATCH 1/6] android/socket: Make profile struct const

[PATCH 1/6] android/socket: Make profile struct const

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

We are not supposed to change profile structure, make it const.
---
 android/socket.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/android/socket.c b/android/socket.c
index 83e6996..184deae 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -335,7 +335,7 @@ static sdp_record_t *create_spp_record(uint8_t chan, const char *svc_name)
 	return record;
 }
 
-static struct profile_info {
+static const struct profile_info {
 	uint8_t		uuid[16];
 	uint8_t		channel;
 	uint8_t		svc_hint;
@@ -381,7 +381,7 @@ static struct profile_info {
 	},
 };
 
-static uint32_t sdp_service_register(struct profile_info *profile,
+static uint32_t sdp_service_register(const struct profile_info *profile,
 							const void *svc_name)
 {
 	sdp_record_t *record;
@@ -444,7 +444,7 @@ static int bt_sock_send_fd(int sock_fd, const void *buf, int len, int send_fd)
 	return ret;
 }
 
-static struct profile_info *get_profile_by_uuid(const uint8_t *uuid)
+static const struct profile_info *get_profile_by_uuid(const uint8_t *uuid)
 {
 	unsigned int i;
 
@@ -651,7 +651,7 @@ static void accept_cb(GIOChannel *io, GError *err, gpointer user_data)
 static int handle_listen(void *buf)
 {
 	struct hal_cmd_sock_listen *cmd = buf;
-	struct profile_info *profile;
+	const struct profile_info *profile;
 	struct rfcomm_sock *rfsock;
 	BtIOSecLevel sec_level;
 	GIOChannel *io;
-- 
1.8.3.2

[PATCH 2/6] android/socket: Use security level for connect / listen

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Use MEDIUM security level for connections without profile and default
sec_level for others. rfsock now has pointer to profile info.
---
 android/socket.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/android/socket.c b/android/socket.c
index 184deae..20dbc5e 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -70,6 +70,8 @@ struct rfcomm_sock {
 
 	bdaddr_t dst;
 	uint32_t service_handle;
+
+	const struct profile_info *profile;
 };
 
 static struct rfcomm_sock *create_rfsock(int sock, int *hal_fd)
@@ -667,7 +669,7 @@ static int handle_listen(void *buf)
 			return -1;
 		else {
 			chan = cmd->channel;
-			sec_level = BT_IO_SEC_LOW;
+			sec_level = BT_IO_SEC_MEDIUM;
 		}
 	} else {
 		chan = profile->channel;
@@ -786,6 +788,7 @@ fail:
 static void sdp_search_cb(sdp_list_t *recs, int err, gpointer data)
 {
 	struct rfcomm_sock *rfsock = data;
+	BtIOSecLevel sec_level = BT_IO_SEC_MEDIUM;
 	GError *gerr = NULL;
 	sdp_list_t *list;
 	GIOChannel *io;
@@ -829,11 +832,14 @@ static void sdp_search_cb(sdp_list_t *recs, int err, gpointer data)
 
 	DBG("Got RFCOMM channel %d", chan);
 
+	if (rfsock->profile)
+		sec_level = rfsock->profile->sec_level;
+
 	io = bt_io_connect(connect_cb, rfsock, NULL, &gerr,
 				BT_IO_OPT_SOURCE_BDADDR, &adapter_addr,
 				BT_IO_OPT_DEST_BDADDR, &rfsock->dst,
 				BT_IO_OPT_CHANNEL, chan,
-				BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+				BT_IO_OPT_SEC_LEVEL, sec_level,
 				BT_IO_OPT_INVALID);
 	if (!io) {
 		error("Failed connect: %s", gerr->message);
@@ -875,6 +881,8 @@ static int handle_connect(void *buf)
 	uuid.type = SDP_UUID128;
 	memcpy(&uuid.value.uuid128, cmd->uuid, sizeof(uint128_t));
 
+	rfsock->profile = get_profile_by_uuid(cmd->uuid);
+
 	if (bt_search_service(&adapter_addr, &dst, &uuid, sdp_search_cb, rfsock,
 								NULL) < 0) {
 		error("Failed to search SDP records");
-- 
1.8.3.2

[PATCH 3/6] android/socket: Handle Android events for server socket

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Add watch for tracking events from Android framework for server socket.
---
 android/socket.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/android/socket.c b/android/socket.c
index 20dbc5e..c4f14ab 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -587,6 +587,24 @@ static bool sock_send_accept(struct rfcomm_sock *rfsock, bdaddr_t *bdaddr,
 	return true;
 }
 
+static gboolean sock_server_stack_event_cb(GIOChannel *io, GIOCondition cond,
+								gpointer data)
+{
+	struct rfcomm_sock *rfsock = data;
+
+	DBG("");
+
+	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
+		error("Socket error: sock %d cond %d",
+					g_io_channel_unix_get_fd(io), cond);
+		cleanup_rfsock(rfsock);
+
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
 static void accept_cb(GIOChannel *io, GError *err, gpointer user_data)
 {
 	struct rfcomm_sock *rfsock = user_data;
@@ -656,7 +674,8 @@ static int handle_listen(void *buf)
 	const struct profile_info *profile;
 	struct rfcomm_sock *rfsock;
 	BtIOSecLevel sec_level;
-	GIOChannel *io;
+	GIOChannel *io, *io_stack;
+	GIOCondition cond;
 	GError *err = NULL;
 	int hal_fd;
 	int chan;
@@ -701,6 +720,12 @@ static int handle_listen(void *buf)
 	g_io_channel_set_close_on_unref(io, TRUE);
 	g_io_channel_unref(io);
 
+	/* Handle events from Android */
+	cond = G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL;
+	io_stack = g_io_channel_unix_new(rfsock->fd);
+	g_io_add_watch(io_stack, cond, sock_server_stack_event_cb, rfsock);
+	g_io_channel_unref(io_stack);
+
 	DBG("real_sock %d fd %d hal_fd %d", rfsock->real_sock, rfsock->fd,
 								hal_fd);
 
-- 
1.8.3.2

[PATCH 4/6] android/socket: Check create_rfsock returns valid structure

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

---
 android/socket.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/android/socket.c b/android/socket.c
index c4f14ab..772afaa 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -638,6 +638,11 @@ static void accept_cb(GIOChannel *io, GError *err, gpointer user_data)
 
 	sock_acc = g_io_channel_unix_get_fd(io);
 	rfsock_acc = create_rfsock(sock_acc, &hal_fd);
+	if (!rfsock_acc) {
+		g_io_channel_shutdown(io, TRUE, NULL);
+		return;
+	}
+
 	connections = g_list_append(connections, rfsock_acc);
 
 	DBG("rfsock: fd %d real_sock %d chan %u sock %d",
@@ -898,8 +903,11 @@ static int handle_connect(void *buf)
 
 	DBG("");
 
-	android2bdaddr(cmd->bdaddr, &dst);
 	rfsock = create_rfsock(-1, &hal_fd);
+	if (!rfsock)
+		return -1;
+
+	android2bdaddr(cmd->bdaddr, &dst);
 	bacpy(&rfsock->dst, &dst);
 
 	memset(&uuid, 0, sizeof(uuid));
-- 
1.8.3.2

[PATCH 5/6] android/hal-sock: Check uuid before dereference

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

uuid might be NULL and channel might be specified which makes it
valid case for Android. This adds check for uuid and service name.
---
 android/hal-sock.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/android/hal-sock.c b/android/hal-sock.c
index e02a49a..f45be30 100644
--- a/android/hal-sock.c
+++ b/android/hal-sock.c
@@ -34,12 +34,17 @@ static bt_status_t sock_listen_rfcomm(const char *service_name,
 
 	DBG("");
 
+	memset(&cmd, 0, sizeof(cmd));
+
 	cmd.flags = flags;
 	cmd.type = BTSOCK_RFCOMM;
 	cmd.channel = chan;
-	memcpy(cmd.uuid, uuid, sizeof(cmd.uuid));
-	memset(cmd.name, 0, sizeof(cmd.name));
-	memcpy(cmd.name, service_name, strlen(service_name));
+
+	if (uuid)
+		memcpy(cmd.uuid, uuid, sizeof(cmd.uuid));
+
+	if (service_name)
+		memcpy(cmd.name, service_name, strlen(service_name));
 
 	return hal_ipc_cmd(HAL_SERVICE_ID_SOCK, HAL_OP_SOCK_LISTEN,
 				sizeof(cmd), &cmd, NULL, NULL, sock);
@@ -90,10 +95,15 @@ static bt_status_t sock_connect(const bt_bdaddr_t *bdaddr, btsock_type_t type,
 		return BT_STATUS_UNSUPPORTED;
 	}
 
+	memset(&cmd, 0, sizeof(cmd));
+
 	cmd.flags = flags;
 	cmd.type = type;
 	cmd.channel = chan;
-	memcpy(cmd.uuid, uuid, sizeof(cmd.uuid));
+
+	if (uuid)
+		memcpy(cmd.uuid, uuid, sizeof(cmd.uuid));
+
 	memcpy(cmd.bdaddr, bdaddr, sizeof(cmd.bdaddr));
 
 	return hal_ipc_cmd(HAL_SERVICE_ID_SOCK, HAL_OP_SOCK_CONNECT,
-- 
1.8.3.2

[PATCH 6/6] android: Do not dereference possible NULL pointer

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

eir might be NULL, do not derefernce it in debug and print instead
pointer.
---
 android/bluetooth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index aa684bd..77ce519 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -916,8 +916,8 @@ static void mgmt_device_found_event(uint16_t index, uint16_t length,
 	flags = btohl(ev->flags);
 
 	ba2str(&ev->addr.bdaddr, addr);
-	DBG("hci%u addr %s, rssi %d flags 0x%04x eir_len %u eir %u",
-				index, addr, ev->rssi, flags, eir_len, *eir);
+	DBG("hci%u addr %s, rssi %d flags 0x%04x eir_len %u eir %p",
+				index, addr, ev->rssi, flags, eir_len, eir);
 
 	confirm_name = flags & MGMT_DEV_FOUND_CONFIRM_NAME;
 
-- 
1.8.3.2

Re: [PATCH 3/6] android/socket: Handle Android events for server socket

[Johan Hedberg]

Hi Andrei,

On Tue, Nov 26, 2013, Andrei Emeltchenko wrote:
> Add watch for tracking events from Android framework for server socket.
> ---
>  android/socket.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)

I've applied the first two patches, but wanted to ask about this one:

> +static gboolean sock_server_stack_event_cb(GIOChannel *io, GIOCondition cond,
> +								gpointer data)
> +{
> +	struct rfcomm_sock *rfsock = data;
> +
> +	DBG("");
> +
> +	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
> +		error("Socket error: sock %d cond %d",
> +					g_io_channel_unix_get_fd(io), cond);
> +		cleanup_rfsock(rfsock);
> +
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}

I don't see where (in which patch) you'd add code to handle G_IO_IN on
this socket. Aren't you supposed to read data from this socket and write
it to the RFCOMM one?

Johan

Re: [PATCH 3/6] android/socket: Handle Android events for server socket

[Andrei Emeltchenko]

Hi Johan,

On Tue, Nov 26, 2013 at 05:46:02PM +0200, Johan Hedberg wrote:
> Hi Andrei,
> 
> On Tue, Nov 26, 2013, Andrei Emeltchenko wrote:
> > Add watch for tracking events from Android framework for server socket.
> > ---
> >  android/socket.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> I've applied the first two patches, but wanted to ask about this one:
> 
> > +static gboolean sock_server_stack_event_cb(GIOChannel *io, GIOCondition cond,
> > +								gpointer data)
> > +{
> > +	struct rfcomm_sock *rfsock = data;
> > +
> > +	DBG("");
> > +
> > +	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
> > +		error("Socket error: sock %d cond %d",
> > +					g_io_channel_unix_get_fd(io), cond);
> > +		cleanup_rfsock(rfsock);
> > +
> > +		return FALSE;
> > +	}
> > +
> > +	return TRUE;
> > +}
> 
> I don't see where (in which patch) you'd add code to handle G_IO_IN on
> this socket. Aren't you supposed to read data from this socket and write
> it to the RFCOMM one?

At this moment I do not know which data might come from this socket. I
have debug statement to know that something is coming.

I assume this would be primary used to clean up socket structure if
Android decides to stop listen().

Best regards 
Andrei Emeltchenko 

Re: [PATCH 3/6] android/socket: Handle Android events for server socket

[Johan Hedberg]

Hi Andrei,

On Wed, Nov 27, 2013, Andrei Emeltchenko wrote:
> On Tue, Nov 26, 2013 at 05:46:02PM +0200, Johan Hedberg wrote:
> > Hi Andrei,
> > 
> > On Tue, Nov 26, 2013, Andrei Emeltchenko wrote:
> > > Add watch for tracking events from Android framework for server socket.
> > > ---
> > >  android/socket.c | 27 ++++++++++++++++++++++++++-
> > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > I've applied the first two patches, but wanted to ask about this one:
> > 
> > > +static gboolean sock_server_stack_event_cb(GIOChannel *io, GIOCondition cond,
> > > +								gpointer data)
> > > +{
> > > +	struct rfcomm_sock *rfsock = data;
> > > +
> > > +	DBG("");
> > > +
> > > +	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
> > > +		error("Socket error: sock %d cond %d",
> > > +					g_io_channel_unix_get_fd(io), cond);
> > > +		cleanup_rfsock(rfsock);
> > > +
> > > +		return FALSE;
> > > +	}
> > > +
> > > +	return TRUE;
> > > +}
> > 
> > I don't see where (in which patch) you'd add code to handle G_IO_IN on
> > this socket. Aren't you supposed to read data from this socket and write
> > it to the RFCOMM one?
> 
> At this moment I do not know which data might come from this socket. I
> have debug statement to know that something is coming.

The fact that you're adding G_IO_IN as one of the conditions for the
callback tells the reader that you're interested in data. Then, when the
reader looks at the function implementation he thinks "wait a minute, it
looks like there's a bug here since this is never reading anything". The
debug statement wont help you since then you'll end up in an endless
loop if there ever is data (since the data gets never removed from the
incoming socket buffer). So I'd just remove G_IO_IN from the conditions.

> I assume this would be primary used to clean up socket structure if
> Android decides to stop listen().

Which HAL method would "stop listen()" be done with? I don't see such a
method in the HAL (please correct me if I'm wrong though). If there's no
such method it's not possible to stop the listening socket, and hence
you're just adding dead code here.

Johan

Re: [PATCH 3/6] android/socket: Handle Android events for server socket

[Andrei Emeltchenko]

On Wed, Nov 27, 2013 at 10:21:28AM +0200, Johan Hedberg wrote:
> Hi Andrei,
> 
> On Wed, Nov 27, 2013, Andrei Emeltchenko wrote:
> > On Tue, Nov 26, 2013 at 05:46:02PM +0200, Johan Hedberg wrote:
> > > Hi Andrei,
> > > 
> > > On Tue, Nov 26, 2013, Andrei Emeltchenko wrote:
> > > > Add watch for tracking events from Android framework for server socket.
> > > > ---
> > > >  android/socket.c | 27 ++++++++++++++++++++++++++-
> > > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > 
> > > I've applied the first two patches, but wanted to ask about this one:
> > > 
> > > > +static gboolean sock_server_stack_event_cb(GIOChannel *io, GIOCondition cond,
> > > > +								gpointer data)
> > > > +{
> > > > +	struct rfcomm_sock *rfsock = data;
> > > > +
> > > > +	DBG("");
> > > > +
> > > > +	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
> > > > +		error("Socket error: sock %d cond %d",
> > > > +					g_io_channel_unix_get_fd(io), cond);
> > > > +		cleanup_rfsock(rfsock);
> > > > +
> > > > +		return FALSE;
> > > > +	}
> > > > +
> > > > +	return TRUE;
> > > > +}
> > > 
> > > I don't see where (in which patch) you'd add code to handle G_IO_IN on
> > > this socket. Aren't you supposed to read data from this socket and write
> > > it to the RFCOMM one?
> > 
> > At this moment I do not know which data might come from this socket. I
> > have debug statement to know that something is coming.
> 
> The fact that you're adding G_IO_IN as one of the conditions for the
> callback tells the reader that you're interested in data. Then, when the
> reader looks at the function implementation he thinks "wait a minute, it
> looks like there's a bug here since this is never reading anything". The
> debug statement wont help you since then you'll end up in an endless
> loop if there ever is data (since the data gets never removed from the
> incoming socket buffer). So I'd just remove G_IO_IN from the conditions.
> 

OK

> > I assume this would be primary used to clean up socket structure if
> > Android decides to stop listen().
> 
> Which HAL method would "stop listen()" be done with? I don't see such a
> method in the HAL (please correct me if I'm wrong though). If there's no
> such method it's not possible to stop the listening socket, and hence
> you're just adding dead code here.

This is listening on socketpair descriptor, the other end was sent to
Android framework. So we might get some signal or socket close.

Best regards 
Andrei Emeltchenko 

Re: [PATCH 3/6] android/socket: Handle Android events for server socket

[Johan Hedberg]

Hi Andrei,

On Wed, Nov 27, 2013, Andrei Emeltchenko wrote:
> > > I assume this would be primary used to clean up socket structure if
> > > Android decides to stop listen().
> > 
> > Which HAL method would "stop listen()" be done with? I don't see such a
> > method in the HAL (please correct me if I'm wrong though). If there's no
> > such method it's not possible to stop the listening socket, and hence
> > you're just adding dead code here.
> 
> This is listening on socketpair descriptor, the other end was sent to
> Android framework. So we might get some signal or socket close.

You'll need to explain in more detail how this could happen. We should
already have other places looking for the HAL-side going a way (e.g. if
it crashes) and then calling the necessary cleanup functions.

Johan

Re: [PATCH 6/6] android: Do not dereference possible NULL pointer

[Johan Hedberg]

Hi Andrei,

On Tue, Nov 26, 2013, Andrei Emeltchenko wrote:
> eir might be NULL, do not derefernce it in debug and print instead
> pointer.
> ---
>  android/bluetooth.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/android/bluetooth.c b/android/bluetooth.c
> index aa684bd..77ce519 100644
> --- a/android/bluetooth.c
> +++ b/android/bluetooth.c
> @@ -916,8 +916,8 @@ static void mgmt_device_found_event(uint16_t index, uint16_t length,
>  	flags = btohl(ev->flags);
>  
>  	ba2str(&ev->addr.bdaddr, addr);
> -	DBG("hci%u addr %s, rssi %d flags 0x%04x eir_len %u eir %u",
> -				index, addr, ev->rssi, flags, eir_len, *eir);
> +	DBG("hci%u addr %s, rssi %d flags 0x%04x eir_len %u eir %p",
> +				index, addr, ev->rssi, flags, eir_len, eir);
>  
>  	confirm_name = flags & MGMT_DEV_FOUND_CONFIRM_NAME;

I'd just remove printing the eir pointer value completely since it
doesn't really give us anything. Since this is such a trivial thing I
pushed a patch to fix this myself.

Johan

Re: [PATCH 5/6] android/hal-sock: Check uuid before dereference

[Johan Hedberg]

Hi Andrei,

On Tue, Nov 26, 2013, Andrei Emeltchenko wrote:
> uuid might be NULL and channel might be specified which makes it
> valid case for Android. This adds check for uuid and service name.
> ---
>  android/hal-sock.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

This patch has been applied. Thanks.

Johan

Re: [PATCH 4/6] android/socket: Check create_rfsock returns valid structure

[Johan Hedberg]

Hi Andrei,

On Tue, Nov 26, 2013, Andrei Emeltchenko wrote:
> ---
>  android/socket.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

This patch has been applied. Thanks.

Johan