2013-12-03 15:53:08

by Andrei Emeltchenko

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

From: Andrei Emeltchenko <[email protected]>

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



2013-12-09 08:01:32

by Andrei Emeltchenko

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

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

2013-12-08 15:39:37

by Luiz Augusto von Dentz

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

Hi Andrei,

On Wed, Dec 4, 2013 at 10:36 AM, Andrei Emeltchenko
<[email protected]> 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
>> <[email protected]> wrote:
>> > From: Andrei Emeltchenko <[email protected]>
>> >
>> > 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 [email protected]
>> > 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

2013-12-04 08:36:25

by Andrei Emeltchenko

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

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
> <[email protected]> wrote:
> > From: Andrei Emeltchenko <[email protected]>
> >
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz

2013-12-03 19:53:43

by Luiz Augusto von Dentz

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

Hi Andrei,

On Tue, Dec 3, 2013 at 5:53 PM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html



--
Luiz Augusto von Dentz

2013-12-03 15:53:12

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCH 5/6] sdp: Remove dead code

From: Andrei Emeltchenko <[email protected]>

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


2013-12-03 15:53:10

by Andrei Emeltchenko

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

From: Andrei Emeltchenko <[email protected]>

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


2013-12-03 15:53:11

by Andrei Emeltchenko

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

From: Andrei Emeltchenko <[email protected]>

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


2013-12-03 15:53:09

by Andrei Emeltchenko

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

From: Andrei Emeltchenko <[email protected]>

---
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


2013-12-03 15:53:13

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCH 6/6] avdtp: Remove unneeded local variable

From: Andrei Emeltchenko <[email protected]>

---
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