[PATCH 1/6] android/a2dp: Fix possible NULL dereference

[PATCH 1/6] android/a2dp: Fix possible NULL dereference

[Andrei Emeltchenko]

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

Since a2dp_record may return NULL, check return value. This
silences static analysers tools.
---
 android/a2dp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/android/a2dp.c b/android/a2dp.c
index cee4bfa..36a0714 100644
--- a/android/a2dp.c
+++ b/android/a2dp.c
@@ -366,9 +366,10 @@ bool bt_a2dp_register(const bdaddr_t *addr)
 	}
 
 	rec = a2dp_record();
-	if (bt_adapter_add_record(rec, SVC_HINT_CAPTURING) < 0) {
+	if (!rec || bt_adapter_add_record(rec, SVC_HINT_CAPTURING) < 0) {
 		error("Failed to register on A2DP record");
-		sdp_record_free(rec);
+		if (rec)
+			sdp_record_free(rec);
 		g_io_channel_shutdown(server, TRUE, NULL);
 		g_io_channel_unref(server);
 		server = NULL;
-- 
1.8.3.2

[PATCH 2/6] android/pan: Remove unneeded NULL assignment

[Andrei Emeltchenko]

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

---
 android/pan.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/android/pan.c b/android/pan.c
index fe6ee26..87fa4e8 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -81,7 +81,6 @@ static void pan_device_free(struct pan_device *dev)
 
 	devices = g_slist_remove(devices, dev);
 	g_free(dev);
-	dev = NULL;
 }
 
 static void bt_pan_notify_conn_state(struct pan_device *dev, uint8_t state)
-- 
1.8.3.2

[PATCH 3/6] android/pan: Fix no return on error path

[Andrei Emeltchenko]

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

This fixes possible crash in case connect fails.
---
 android/pan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/android/pan.c b/android/pan.c
index 87fa4e8..7e9b3c3 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -172,6 +172,7 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer data)
 		error("%s", err->message);
 		bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
 		pan_device_free(dev);
+		return;
 	}
 
 	src = (local_role == HAL_PAN_ROLE_NAP) ? BNEP_SVC_NAP : BNEP_SVC_PANU;
-- 
1.8.3.2

[PATCH 4/6] android/doc: Add socket-api.txt document

[Andrei Emeltchenko]

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

Document describes how socket HAL is working.
---
 android/socket-api.txt | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 android/socket-api.txt

diff --git a/android/socket-api.txt b/android/socket-api.txt
new file mode 100644
index 0000000..9f622f9
--- /dev/null
+++ b/android/socket-api.txt
@@ -0,0 +1,61 @@
+Android Socket protocol for Bluetooth
+=====================================
+
+Since Android switched from BlueZ (where sockets where nicely implemented) to
+Bluedroid user space stack there is a need to emulate bluetooth sockets.
+
+Android Bluetooth Socket Hardware Abstraction Layer (HAL) bt_sock.h has
+only 2 functions:
+
+static btsock_interface_t sock_if = {
+	sizeof(sock_if),
+	sock_listen,
+	sock_connect
+};
+
+with following parameters:
+
+sock_listen(btsock_type_t type, const char *service_name,
+		const uint8_t *uuid, int chan, int *sock_fd, int flags)
+sock_connect(const bt_bdaddr_t *bdaddr, btsock_type_t type,
+		const uint8_t *uuid, int chan, int *sock_fd, int flags)
+
+socket type RFCOMM is only supported at the moment. uuid and channel used
+to decide where to connect.
+
+sockfd is used to return socket fd to Android framework. It is used to inform
+framework when remote device is connected.
+
+listen()
+========
+
+Listens on RFCOMM socket, socket channel is either found based on uuid or
+channel parameter used directly. Returns sock_fd to Android framework.
+
+Through this sock_fd channel number as (int) needs to be written right after
+listen() succeeds.
+
+When remote device is connected to this socket we shall send accept signal
+through sock_fd
+
+connect()
+=========
+
+Connects to remote device specified in bd_addr parameter. Socket channel is
+found by SDP search of remote device by supplied uuid. Returns sock_fd to
+Android framework.
+
+Through this sock_fd channel number as (int) needs to be written right after
+connects() succeeds.
+
+When remote device is connected to this socket we shall send connect signal
+through sock_fd
+
+The format of connect/accept signal is shown below:
+
+struct hal_sock_connect_signal {
+	short   size;
+	uint8_t bdaddr[6];
+	int     channel;
+	int     status;
+} __attribute__((packed));
-- 
1.8.3.2

[PATCH 5/6] sdp: Remove dead code

[Andrei Emeltchenko]

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

sdp_next_handle always returns value >= 0x10000.
---
 src/sdpd-service.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/sdpd-service.c b/src/sdpd-service.c
index 38bf808..448c76b 100644
--- a/src/sdpd-service.c
+++ b/src/sdpd-service.c
@@ -424,10 +424,6 @@ int service_register_req(sdp_req_t *req, sdp_buf_t *rsp)
 
 	if (rec->handle == 0xffffffff) {
 		rec->handle = sdp_next_handle();
-		if (rec->handle < 0x10000) {
-			sdp_record_free(rec);
-			goto invalid;
-		}
 	} else {
 		if (sdp_record_find(rec->handle)) {
 			/* extract_pdu_server will add the record handle
-- 
1.8.3.2

[PATCH 6/6] avdtp: Remove unneeded local variable

[Andrei Emeltchenko]

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

---
 profiles/audio/avdtp.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index f866b39..e12ad9d 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -773,10 +773,9 @@ static int get_send_buffer_size(int sk)
 	socklen_t optlen = sizeof(size);
 
 	if (getsockopt(sk, SOL_SOCKET, SO_SNDBUF, &size, &optlen) < 0) {
-		int err = -errno;
-		error("getsockopt(SO_SNDBUF) failed: %s (%d)", strerror(-err),
-									-err);
-		return err;
+		error("getsockopt(SO_SNDBUF) failed: %s (%d)", strerror(errno),
+									errno);
+		return -errno;
 	}
 
 	/*
@@ -792,10 +791,9 @@ static int set_send_buffer_size(int sk, int size)
 	socklen_t optlen = sizeof(size);
 
 	if (setsockopt(sk, SOL_SOCKET, SO_SNDBUF, &size, optlen) < 0) {
-		int err = -errno;
-		error("setsockopt(SO_SNDBUF) failed: %s (%d)", strerror(-err),
-									-err);
-		return err;
+		error("setsockopt(SO_SNDBUF) failed: %s (%d)", strerror(errno),
+									errno);
+		return -errno;
 	}
 
 	return 0;
-- 
1.8.3.2

Re: [PATCH 1/6] android/a2dp: Fix possible NULL dereference

[Luiz Augusto von Dentz]

Hi Andrei,

On Tue, Dec 3, 2013 at 5:53 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Since a2dp_record may return NULL, check return value. This
> silences static analysers tools.
> ---
>  android/a2dp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/android/a2dp.c b/android/a2dp.c
> index cee4bfa..36a0714 100644
> --- a/android/a2dp.c
> +++ b/android/a2dp.c
> @@ -366,9 +366,10 @@ bool bt_a2dp_register(const bdaddr_t *addr)
>         }
>
>         rec = a2dp_record();
> -       if (bt_adapter_add_record(rec, SVC_HINT_CAPTURING) < 0) {
> +       if (!rec || bt_adapter_add_record(rec, SVC_HINT_CAPTURING) < 0) {

Usually we check the return individually, that means you do if (rec)
and perhaps handle the error path with goto, but first make sure that
a2dp_record can actually fail otherwise this is pointless.

>                 error("Failed to register on A2DP record");
> -               sdp_record_free(rec);
> +               if (rec)
> +                       sdp_record_free(rec);
>                 g_io_channel_shutdown(server, TRUE, NULL);
>                 g_io_channel_unref(server);
>                 server = NULL;
> --
> 1.8.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

Re: [PATCH 1/6] android/a2dp: Fix possible NULL dereference

[Andrei Emeltchenko]

Hi Luiz,

On Tue, Dec 03, 2013 at 09:53:43PM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
> 
> On Tue, Dec 3, 2013 at 5:53 PM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Since a2dp_record may return NULL, check return value. This
> > silences static analysers tools.
> > ---
> >  android/a2dp.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/android/a2dp.c b/android/a2dp.c
> > index cee4bfa..36a0714 100644
> > --- a/android/a2dp.c
> > +++ b/android/a2dp.c
> > @@ -366,9 +366,10 @@ bool bt_a2dp_register(const bdaddr_t *addr)
> >         }
> >
> >         rec = a2dp_record();
> > -       if (bt_adapter_add_record(rec, SVC_HINT_CAPTURING) < 0) {
> > +       if (!rec || bt_adapter_add_record(rec, SVC_HINT_CAPTURING) < 0) {
> 
> Usually we check the return individually, that means you do if (rec)
> and perhaps handle the error path with goto, but first make sure that
> a2dp_record can actually fail otherwise this is pointless.

It might return NULL if malloc fails, do you think that we need to change
malloc to g_malloc in sdp code. Otherwise every tools warns about NULL
dereference.

Best regards 
Andrei Emeltchenko 

>
> >                 error("Failed to register on A2DP record");
> > -               sdp_record_free(rec);
> > +               if (rec)
> > +                       sdp_record_free(rec);
> >                 g_io_channel_shutdown(server, TRUE, NULL);
> >                 g_io_channel_unref(server);
> >                 server = NULL;
> > --
> > 1.8.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

Re: [PATCH 1/6] android/a2dp: Fix possible NULL dereference

[Luiz Augusto von Dentz]

Hi Andrei,

On Wed, Dec 4, 2013 at 10:36 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> Hi Luiz,
>
> On Tue, Dec 03, 2013 at 09:53:43PM +0200, Luiz Augusto von Dentz wrote:
>> Hi Andrei,
>>
>> On Tue, Dec 3, 2013 at 5:53 PM, Andrei Emeltchenko
>> <Andrei.Emeltchenko.news@gmail.com> wrote:
>> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> >
>> > Since a2dp_record may return NULL, check return value. This
>> > silences static analysers tools.
>> > ---
>> >  android/a2dp.c | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/android/a2dp.c b/android/a2dp.c
>> > index cee4bfa..36a0714 100644
>> > --- a/android/a2dp.c
>> > +++ b/android/a2dp.c
>> > @@ -366,9 +366,10 @@ bool bt_a2dp_register(const bdaddr_t *addr)
>> >         }
>> >
>> >         rec = a2dp_record();
>> > -       if (bt_adapter_add_record(rec, SVC_HINT_CAPTURING) < 0) {
>> > +       if (!rec || bt_adapter_add_record(rec, SVC_HINT_CAPTURING) < 0) {
>>
>> Usually we check the return individually, that means you do if (rec)
>> and perhaps handle the error path with goto, but first make sure that
>> a2dp_record can actually fail otherwise this is pointless.
>
> It might return NULL if malloc fails, do you think that we need to change
> malloc to g_malloc in sdp code. Otherwise every tools warns about NULL
> dereference.
>
> Best regards
> Andrei Emeltchenko
>
>>
>> >                 error("Failed to register on A2DP record");
>> > -               sdp_record_free(rec);
>> > +               if (rec)
>> > +                       sdp_record_free(rec);
>> >                 g_io_channel_shutdown(server, TRUE, NULL);
>> >                 g_io_channel_unref(server);
>> >                 server = NULL;
>> > --
>> > 1.8.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

I fixed this myself and applied 1-4, patch 5 is actually wrong since
sdp_next_handle may return values bellow 0x10000 if we run out of
handles and patch 6 is not necessary since what is in android/avdtp.c
is what we will be using in the future.


-- 
Luiz Augusto von Dentz

Re: [PATCH 1/6] android/a2dp: Fix possible NULL dereference

[Andrei Emeltchenko]

Hi Luiz,

On Sun, Dec 08, 2013 at 05:39:37PM +0200, Luiz Augusto von Dentz wrote:
> I fixed this myself and applied 1-4, patch 5 is actually wrong since
> sdp_next_handle may return values bellow 0x10000 if we run out of

OK

> handles and patch 6 is not necessary since what is in android/avdtp.c
> is what we will be using in the future.

How are we going to use those local vars? If they would be global it probably
make some (little) sense ...


@@ -773,10 +773,9 @@ static int get_send_buffer_size(int sk)
 	socklen_t optlen = sizeof(size);
 
 	if (getsockopt(sk, SOL_SOCKET, SO_SNDBUF, &size, &optlen) < 0) {
-		int err = -errno;
-		error("getsockopt(SO_SNDBUF) failed: %s (%d)", strerror(-err),
-									-err);
-		return err;
+		error("getsockopt(SO_SNDBUF) failed: %s (%d)", strerror(errno),
+									errno);
+		return -errno;
 	}
 
 	/*


Regards,
Andrei