[PATCH BlueZ 1/7] android: Use 16-bit UUID for SDP search

[PATCH BlueZ 1/7] android: Use 16-bit UUID for SDP search

[Anderson Lizardo]

These UUIDs are assigned by BT-SIG and therefore there is no need to
use full 128-bit UUIDs. This also avoids unnecessary conversion from
string representation.
---
 android/handsfree.c |    3 +--
 android/hidhost.c   |    7 +++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/android/handsfree.c b/android/handsfree.c
index 9482b2e..a869d58 100644
--- a/android/handsfree.c
+++ b/android/handsfree.c
@@ -34,7 +34,6 @@
 #include "lib/bluetooth.h"
 #include "lib/sdp.h"
 #include "lib/sdp_lib.h"
-#include "lib/uuid.h"
 #include "src/sdp-client.h"
 #include "src/uuid-helper.h"
 #include "src/shared/hfp.h"
@@ -294,7 +293,7 @@ static void handle_connect(const void *buf, uint16_t len)
 
 	device_init(&bdaddr);
 
-	bt_string2uuid(&uuid, HFP_HS_UUID);
+	sdp_uuid16_create(&uuid, HANDSFREE_SVCLASS_ID);
 	if (bt_search_service(&adapter_addr, &device.bdaddr, &uuid,
 					sdp_search_cb, NULL, NULL, 0) < 0) {
 		error("handsfree: SDP search failed");
diff --git a/android/hidhost.c b/android/hidhost.c
index 8bd3455..45fe14f 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -37,7 +37,6 @@
 #include "lib/bluetooth.h"
 #include "lib/sdp.h"
 #include "lib/sdp_lib.h"
-#include "lib/uuid.h"
 #include "src/shared/mgmt.h"
 #include "src/sdp-client.h"
 #include "src/uuid-helper.h"
@@ -751,7 +750,7 @@ static void hid_sdp_did_search_cb(sdp_list_t *recs, int err, gpointer data)
 			dev->version = data->val.uint16;
 	}
 
-	bt_string2uuid(&uuid, HID_UUID);
+	sdp_uuid16_create(&uuid, HID_SVCLASS_ID);
 	if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
 				hid_sdp_search_cb, dev, NULL, 0) < 0) {
 		error("failed to search sdp details");
@@ -792,7 +791,7 @@ static void bt_hid_connect(const void *buf, uint16_t len)
 	ba2str(&dev->dst, addr);
 	DBG("connecting to %s", addr);
 
-	bt_string2uuid(&uuid, PNP_UUID);
+	sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
 	if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
 					hid_sdp_did_search_cb, dev, NULL, 0) < 0) {
 		error("Failed to search DeviceID SDP details");
@@ -1293,7 +1292,7 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 		dev->ctrl_io = g_io_channel_ref(chan);
 		dev->uhid_fd = -1;
 
-		bt_string2uuid(&uuid, PNP_UUID);
+		sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
 		if (bt_search_service(&src, &dev->dst, &uuid,
 					hid_sdp_did_search_cb, dev, NULL, 0) < 0) {
 			error("failed to search did sdp details");
-- 
1.7.9.5

[PATCH BlueZ 2/7] android/tester: Update SDP PDU after UUID change

[Anderson Lizardo]

Now it is expected to receive a 16-bit UUID for PNP_INFO.
---
 android/android-tester.c |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/android/android-tester.c b/android/android-tester.c
index 870ad8d..d635732 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -3307,16 +3307,10 @@ static void hid_ctrl_connect_cb(uint16_t handle, uint16_t cid, void *user_data)
 
 static const uint8_t did_req_pdu[] = { 0x06, /* PDU id */
 			0x00, 0x00, /* Transaction id */
-			0x00, 0x1d, /* Req length */
-			0x35, 0x11, /* Attributes length */
-			0x1c, 0x00, 0x00, 0x12, 0x00, 0x00, 0x00, 0x10, 0x00,
-			0x80, 0x00, 0x00, 0x80, 0x5f, 0x9b, 0x34, 0xfb, 0xff,
-			0xff, 0x35, 0x05, 0x0a, 0x00, 0x00, 0xff, 0xff, 0x00,
-			0x06, 0x00, 0x01, 0x00, 0x1d, 0x35, 0x11, 0x1c, 0x00,
-			0x00, 0x11, 0x24, 0x00, 0x00, 0x10, 0x00, 0x80, 0x00,
-			0x00, 0x80, 0x5f, 0x9b, 0x34, 0xfb, 0xff, 0xff, 0x35,
-			0x05, 0x0a, 0x00, 0x0, 0xff, 0xff,
-			0x00 }; /* no continuation */
+			0x00, 0x0f, /* Req length */
+			0x35, 0x03, /* Attributes length */
+			0x19, 0x12, 0x00, 0xff, 0xff, 0x35, 0x05, 0x0a, 0x00,
+			0x00, 0xff, 0xff, 0x00 }; /* no continuation */
 
 static const uint8_t did_rsp_pdu[] = { 0x07, /* PDU id */
 			0x00, 0x00, /* Transaction id */
-- 
1.7.9.5

[PATCH BlueZ 3/7] android/hidhost: Trivial coding style fix

[Anderson Lizardo]

Fix two lines over 80 columns.
---
 android/hidhost.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index 45fe14f..f54ca6d 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -793,7 +793,7 @@ static void bt_hid_connect(const void *buf, uint16_t len)
 
 	sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
 	if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
-					hid_sdp_did_search_cb, dev, NULL, 0) < 0) {
+				hid_sdp_did_search_cb, dev, NULL, 0) < 0) {
 		error("Failed to search DeviceID SDP details");
 		hid_device_remove(dev);
 		status = HAL_STATUS_FAILED;
@@ -1294,7 +1294,7 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 
 		sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
 		if (bt_search_service(&src, &dev->dst, &uuid,
-					hid_sdp_did_search_cb, dev, NULL, 0) < 0) {
+				hid_sdp_did_search_cb, dev, NULL, 0) < 0) {
 			error("failed to search did sdp details");
 			hid_device_remove(dev);
 			return;
-- 
1.7.9.5

[PATCH BlueZ 4/7] android/client: Fix set_info command

[Anderson Lizardo]

Although this command is not implemented by BlueZ, make sure it is
callable from haltest so at least the IPC can be tested.

Also memset() the hid_info parameter to not pass uninitialized data
around.
---
 android/client/if-hh.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/android/client/if-hh.c b/android/client/if-hh.c
index 56eb698..0341d25 100644
--- a/android/client/if-hh.c
+++ b/android/client/if-hh.c
@@ -229,6 +229,9 @@ static void virtual_unplug_p(int argc, const char **argv)
 
 /* set_info */
 
+/* Same completion as connect_c */
+#define set_info_c connect_c
+
 static void set_info_p(int argc, const char **argv)
 {
 	bt_bdaddr_t addr;
@@ -236,8 +239,11 @@ static void set_info_p(int argc, const char **argv)
 
 	RETURN_IF_NULL(if_hh);
 	VERIFY_ADDR_ARG(2, &addr);
-	/* TODO: set_info does not seem to be called anywhere */
 
+	memset(&hid_info, 0, sizeof(hid_info));
+
+	/* This command is intentionally not supported. See comment from
+	 * bt_hid_info() in android/hidhost.c */
 	EXEC(if_hh->set_info, &addr, hid_info);
 }
 
@@ -416,7 +422,7 @@ static struct method methods[] = {
 	STD_METHODCH(connect, "<addr>"),
 	STD_METHODCH(disconnect, "<addr>"),
 	STD_METHODCH(virtual_unplug, "<addr>"),
-	STD_METHOD(set_info),
+	STD_METHODCH(set_info, "<addr>"),
 	STD_METHODCH(get_protocol, "<addr> <mode>"),
 	STD_METHODCH(set_protocol, "<addr> <mode>"),
 	STD_METHODCH(get_report, "<addr> <type> <report_id> <size>"),
-- 
1.7.9.5

[PATCH BlueZ 5/7] android/test-ipc: Fix crash due to invalid ipc_register() parameter

[Anderson Lizardo]

This test checks for proper handling of invalid Service ID on a IPC
message, but it was attempting to register handlers for this invalid ID,
which on current ipc_register() implementation was causing a buffer
overrun.

The fix was to use a valid ID during registration, but still attempt to
use an invalid one when sending the message.
---
 android/test-ipc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/test-ipc.c b/android/test-ipc.c
index 3a0729e..7318251 100644
--- a/android/test-ipc.c
+++ b/android/test-ipc.c
@@ -526,7 +526,7 @@ static const struct hal_hdr test_cmd_service_offrange_hdr = {
 static const struct test_data test_cmd_service_offrange = {
 	.cmd = &test_cmd_service_offrange_hdr,
 	.cmd_size = sizeof(struct hal_hdr),
-	.service = HAL_SERVICE_ID_MAX + 1,
+	.service = 0,
 	.handlers = cmd_handlers,
 	.handlers_size = 1,
 	.expected_signal = SIGTERM
-- 
1.7.9.5

[PATCH BlueZ 6/7] android: Add assertion for ID parameter in ipc_register/ipc_unregister

[Anderson Lizardo]

It is a programming error to pass a invalid ID for these functions (i.e.
the ID is statically defined on the code). Given that this ID is used
for indexing an array, adding an assertion will ensure that improper API
usage will not cause random crashes due to memory corruption.
---
 android/ipc.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/android/ipc.c b/android/ipc.c
index ab0f1a4..373345c 100644
--- a/android/ipc.c
+++ b/android/ipc.c
@@ -336,12 +336,16 @@ void ipc_send_notif(uint8_t service_id, uint8_t opcode,  uint16_t len,
 void ipc_register(uint8_t service, const struct ipc_handler *handlers,
 								uint8_t size)
 {
+	g_assert(service <= HAL_SERVICE_ID_MAX);
+
 	services[service].handler = handlers;
 	services[service].size = size;
 }
 
 void ipc_unregister(uint8_t service)
 {
+	g_assert(service <= HAL_SERVICE_ID_MAX);
+
 	services[service].handler = NULL;
 	services[service].size = 0;
 }
-- 
1.7.9.5

[PATCH BlueZ 7/7] android: Add test-ipc to "make check"

[Anderson Lizardo]

---
 Makefile.am         |    3 ++-
 android/Makefile.am |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 1a44a9f..11f2aa1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -178,6 +178,7 @@ EXTRA_DIST += src/genbuiltin src/bluetooth.conf \
 			profiles/input/input.conf profiles/proximity/proximity.conf
 
 test_scripts =
+unit_tests =
 
 include Makefile.tools
 include Makefile.obexd
@@ -222,7 +223,7 @@ AM_CFLAGS += @DBUS_CFLAGS@ @GLIB_CFLAGS@
 AM_CPPFLAGS = -I$(builddir)/lib -I$(srcdir)/gdbus
 
 
-unit_tests = unit/test-eir unit/test-uuid unit/test-textfile unit/test-crc
+unit_tests += unit/test-eir unit/test-uuid unit/test-textfile unit/test-crc
 
 unit_test_eir_SOURCES = unit/test-eir.c src/eir.c src/uuid-helper.c
 unit_test_eir_LDADD = lib/libbluetooth-internal.la @GLIB_LIBS@
diff --git a/android/Makefile.am b/android/Makefile.am
index 5baa8db..1913b42 100644
--- a/android/Makefile.am
+++ b/android/Makefile.am
@@ -153,7 +153,7 @@ android_audio_a2dp_default_la_SOURCES = android/audio-msg.h \
 					android/hardware/hardware.h \
 					android/system/audio.h
 
-noinst_PROGRAMS += android/test-ipc
+unit_tests += android/test-ipc
 
 android_test_ipc_SOURCES = android/test-ipc.c \
 				src/shared/util.h src/shared/util.c \
-- 
1.7.9.5

Re: [PATCH BlueZ 1/7] android: Use 16-bit UUID for SDP search

[Luiz Augusto von Dentz]

Hi Anderson,

On Tue, Feb 11, 2014 at 8:32 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> These UUIDs are assigned by BT-SIG and therefore there is no need to
> use full 128-bit UUIDs. This also avoids unnecessary conversion from
> string representation.
> ---
>  android/handsfree.c |    3 +--
>  android/hidhost.c   |    7 +++----
>  2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/android/handsfree.c b/android/handsfree.c
> index 9482b2e..a869d58 100644
> --- a/android/handsfree.c
> +++ b/android/handsfree.c
> @@ -34,7 +34,6 @@
>  #include "lib/bluetooth.h"
>  #include "lib/sdp.h"
>  #include "lib/sdp_lib.h"
> -#include "lib/uuid.h"
>  #include "src/sdp-client.h"
>  #include "src/uuid-helper.h"
>  #include "src/shared/hfp.h"
> @@ -294,7 +293,7 @@ static void handle_connect(const void *buf, uint16_t len)
>
>         device_init(&bdaddr);
>
> -       bt_string2uuid(&uuid, HFP_HS_UUID);
> +       sdp_uuid16_create(&uuid, HANDSFREE_SVCLASS_ID);
>         if (bt_search_service(&adapter_addr, &device.bdaddr, &uuid,
>                                         sdp_search_cb, NULL, NULL, 0) < 0) {
>                 error("handsfree: SDP search failed");
> diff --git a/android/hidhost.c b/android/hidhost.c
> index 8bd3455..45fe14f 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -37,7 +37,6 @@
>  #include "lib/bluetooth.h"
>  #include "lib/sdp.h"
>  #include "lib/sdp_lib.h"
> -#include "lib/uuid.h"
>  #include "src/shared/mgmt.h"
>  #include "src/sdp-client.h"
>  #include "src/uuid-helper.h"
> @@ -751,7 +750,7 @@ static void hid_sdp_did_search_cb(sdp_list_t *recs, int err, gpointer data)
>                         dev->version = data->val.uint16;
>         }
>
> -       bt_string2uuid(&uuid, HID_UUID);
> +       sdp_uuid16_create(&uuid, HID_SVCLASS_ID);
>         if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
>                                 hid_sdp_search_cb, dev, NULL, 0) < 0) {
>                 error("failed to search sdp details");
> @@ -792,7 +791,7 @@ static void bt_hid_connect(const void *buf, uint16_t len)
>         ba2str(&dev->dst, addr);
>         DBG("connecting to %s", addr);
>
> -       bt_string2uuid(&uuid, PNP_UUID);
> +       sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
>         if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
>                                         hid_sdp_did_search_cb, dev, NULL, 0) < 0) {
>                 error("Failed to search DeviceID SDP details");
> @@ -1293,7 +1292,7 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>                 dev->ctrl_io = g_io_channel_ref(chan);
>                 dev->uhid_fd = -1;
>
> -               bt_string2uuid(&uuid, PNP_UUID);
> +               sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
>                 if (bt_search_service(&src, &dev->dst, &uuid,
>                                         hid_sdp_did_search_cb, dev, NULL, 0) < 0) {
>                         error("failed to search did sdp details");
> --
> 1.7.9.5

Applied all except 6/7, I think we should probably return an error if
there is an attempt to register a service out of range, then the
caller can assert so we can have a proper test for it that expect an
error in such case.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/7] android: Use 16-bit UUID for SDP search

[Szymon Janc]

Hi,

On Wednesday 12 of February 2014 11:58:16 Luiz Augusto von Dentz wrote:
> Hi Anderson,
> 
> On Tue, Feb 11, 2014 at 8:32 PM, Anderson Lizardo
> 
> <anderson.lizardo@openbossa.org> wrote:
> > These UUIDs are assigned by BT-SIG and therefore there is no need to
> > use full 128-bit UUIDs. This also avoids unnecessary conversion from
> > string representation.
> > ---
> > 
> >  android/handsfree.c |    3 +--
> >  android/hidhost.c   |    7 +++----
> >  2 files changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/android/handsfree.c b/android/handsfree.c
> > index 9482b2e..a869d58 100644
> > --- a/android/handsfree.c
> > +++ b/android/handsfree.c
> > @@ -34,7 +34,6 @@
> > 
> >  #include "lib/bluetooth.h"
> >  #include "lib/sdp.h"
> >  #include "lib/sdp_lib.h"
> > 
> > -#include "lib/uuid.h"
> > 
> >  #include "src/sdp-client.h"
> >  #include "src/uuid-helper.h"
> >  #include "src/shared/hfp.h"
> > 
> > @@ -294,7 +293,7 @@ static void handle_connect(const void *buf, uint16_t
> > len)> 
> >         device_init(&bdaddr);
> > 
> > -       bt_string2uuid(&uuid, HFP_HS_UUID);
> > +       sdp_uuid16_create(&uuid, HANDSFREE_SVCLASS_ID);
> > 
> >         if (bt_search_service(&adapter_addr, &device.bdaddr, &uuid,
> >         
> >                                         sdp_search_cb, NULL, NULL, 0) < 0)
> >                                         {
> >                 
> >                 error("handsfree: SDP search failed");
> > 
> > diff --git a/android/hidhost.c b/android/hidhost.c
> > index 8bd3455..45fe14f 100644
> > --- a/android/hidhost.c
> > +++ b/android/hidhost.c
> > @@ -37,7 +37,6 @@
> > 
> >  #include "lib/bluetooth.h"
> >  #include "lib/sdp.h"
> >  #include "lib/sdp_lib.h"
> > 
> > -#include "lib/uuid.h"
> > 
> >  #include "src/shared/mgmt.h"
> >  #include "src/sdp-client.h"
> >  #include "src/uuid-helper.h"
> > 
> > @@ -751,7 +750,7 @@ static void hid_sdp_did_search_cb(sdp_list_t *recs,
> > int err, gpointer data)> 
> >                         dev->version = data->val.uint16;
> >         
> >         }
> > 
> > -       bt_string2uuid(&uuid, HID_UUID);
> > +       sdp_uuid16_create(&uuid, HID_SVCLASS_ID);
> > 
> >         if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
> >         
> >                                 hid_sdp_search_cb, dev, NULL, 0) < 0) {
> >                 
> >                 error("failed to search sdp details");
> > 
> > @@ -792,7 +791,7 @@ static void bt_hid_connect(const void *buf, uint16_t
> > len)> 
> >         ba2str(&dev->dst, addr);
> >         DBG("connecting to %s", addr);
> > 
> > -       bt_string2uuid(&uuid, PNP_UUID);
> > +       sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
> > 
> >         if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
> >         
> >                                         hid_sdp_did_search_cb, dev, NULL,
> >                                         0) < 0) {
> >                 
> >                 error("Failed to search DeviceID SDP details");
> > 
> > @@ -1293,7 +1292,7 @@ static void connect_cb(GIOChannel *chan, GError
> > *err, gpointer user_data)> 
> >                 dev->ctrl_io = g_io_channel_ref(chan);
> >                 dev->uhid_fd = -1;
> > 
> > -               bt_string2uuid(&uuid, PNP_UUID);
> > +               sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
> > 
> >                 if (bt_search_service(&src, &dev->dst, &uuid,
> >                 
> >                                         hid_sdp_did_search_cb, dev, NULL,
> >                                         0) < 0) {
> >                         
> >                         error("failed to search did sdp details");
> > 
> > --
> > 1.7.9.5
> 
> Applied all except 6/7, I think we should probably return an error if
> there is an attempt to register a service out of range, then the
> caller can assert so we can have a proper test for it that expect an
> error in such case.

Well, currently IPC depends on hal-msg.h which defines max allowed service ID 
and using something bigger is a code bug. We could have make IPC independent 
of hal-msg.h and just verify registered ID in runtime but that would add extra 
code for each caller with no profit as IDs are fixed anyway.

That test fix is invalid as we actually want to test if IPC handles out-of-
bound service ID correctly (when receiving register command).
And I'm not sure why this actually was causing any problems since that test is 
not registering handlers for out-of-bound service ID, just sends ipc message 
with such ID.

(BTW I think we should pass max service id on ipc_init, as currently audio ipc 
is using max service id from bt ipc)

-- 
BR
Szymon Janc

Re: [PATCH BlueZ 1/7] android: Use 16-bit UUID for SDP search

[Anderson Lizardo]

Hi Szymon,

On Wed, Feb 12, 2014 at 6:59 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
>> Applied all except 6/7, I think we should probably return an error if
>> there is an attempt to register a service out of range, then the
>> caller can assert so we can have a proper test for it that expect an
>> error in such case.
>
> Well, currently IPC depends on hal-msg.h which defines max allowed service ID
> and using something bigger is a code bug. We could have make IPC independent
> of hal-msg.h and just verify registered ID in runtime but that would add extra
> code for each caller with no profit as IDs are fixed anyway.

Personally, I don't have strong opinions on using assert() versus
raise(SIGTERM) (which is how runtime error conditions seem to be
handled). Initially I went with raise(SIGTERM), but then I noticed the
IDs are statically defined, and there is no way to give a invalid ID
to ipc_register(),  unless due to programming error (for which
assert() is ideal, due to low overhead and no introduction of dead
code).

That said, this patch is not critical, but only a check so future
users of ipc_register() don't reintroduce a similar crash fixed by the
other commit.

>
> That test fix is invalid as we actually want to test if IPC handles out-of-
> bound service ID correctly (when receiving register command).
> And I'm not sure why this actually was causing any problems since that test is
> not registering handlers for out-of-bound service ID, just sends ipc message
> with such ID.

I forgot to clarify this on the commit message: the "out of bound"
service ID is still passed on the IPC message. What I fixed is the
service ID field used solely for registering the handlers (i.e. passed
to ipc_register()). If you check the changed struct, there is another
field for the IPC headers where there is still the expected (out of
bound) ID.

In the current format, ipc_register() must not receive an "out of
bound" ID otherwise memory corruptions occur, which introduces subtle
bugs (in my case the crash happened in very specific compilation
parameters and valgrind didn't help because the corrupted structures
were static).

Best Regards,
-- 
Anderson Lizardo
http://www.indt.org/?lang=en
INdT - Manaus - Brazil

Re: [PATCH BlueZ 1/7] android: Use 16-bit UUID for SDP search

[Szymon Janc]

Hi Anderson,

On Wednesday 12 of February 2014 07:28:24 Anderson Lizardo wrote:
> Hi Szymon,
> 
> On Wed, Feb 12, 2014 at 6:59 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> >> Applied all except 6/7, I think we should probably return an error if
> >> there is an attempt to register a service out of range, then the
> >> caller can assert so we can have a proper test for it that expect an
> >> error in such case.
> > 
> > Well, currently IPC depends on hal-msg.h which defines max allowed service
> > ID and using something bigger is a code bug. We could have make IPC
> > independent of hal-msg.h and just verify registered ID in runtime but
> > that would add extra code for each caller with no profit as IDs are fixed
> > anyway.
> 
> Personally, I don't have strong opinions on using assert() versus
> raise(SIGTERM) (which is how runtime error conditions seem to be
> handled). Initially I went with raise(SIGTERM), but then I noticed the
> IDs are statically defined, and there is no way to give a invalid ID
> to ipc_register(),  unless due to programming error (for which
> assert() is ideal, due to low overhead and no introduction of dead
> code).
> 
> That said, this patch is not critical, but only a check so future
> users of ipc_register() don't reintroduce a similar crash fixed by the
> other commit.
> 
> > That test fix is invalid as we actually want to test if IPC handles
> > out-of-
> > bound service ID correctly (when receiving register command).
> > And I'm not sure why this actually was causing any problems since that
> > test is not registering handlers for out-of-bound service ID, just sends
> > ipc message with such ID.
> 
> I forgot to clarify this on the commit message: the "out of bound"
> service ID is still passed on the IPC message. What I fixed is the
> service ID field used solely for registering the handlers (i.e. passed
> to ipc_register()). If you check the changed struct, there is another
> field for the IPC headers where there is still the expected (out of
> bound) ID.
> 
> In the current format, ipc_register() must not receive an "out of
> bound" ID otherwise memory corruptions occur, which introduces subtle
> bugs (in my case the crash happened in very specific compilation
> parameters and valgrind didn't help because the corrupted structures
> were static).

Yes, I was looking at service ID in wrong line. This seems ok now.

-- 
BR
Szymon Janc

Re: [PATCH BlueZ 1/7] android: Use 16-bit UUID for SDP search

[Luiz Augusto von Dentz]

Hi Szymon,

On Wed, Feb 12, 2014 at 12:59 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi,
>
> On Wednesday 12 of February 2014 11:58:16 Luiz Augusto von Dentz wrote:
>> Hi Anderson,
>>
>> On Tue, Feb 11, 2014 at 8:32 PM, Anderson Lizardo
>>
>> <anderson.lizardo@openbossa.org> wrote:
>> > These UUIDs are assigned by BT-SIG and therefore there is no need to
>> > use full 128-bit UUIDs. This also avoids unnecessary conversion from
>> > string representation.
>> > ---
>> >
>> >  android/handsfree.c |    3 +--
>> >  android/hidhost.c   |    7 +++----
>> >  2 files changed, 4 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/android/handsfree.c b/android/handsfree.c
>> > index 9482b2e..a869d58 100644
>> > --- a/android/handsfree.c
>> > +++ b/android/handsfree.c
>> > @@ -34,7 +34,6 @@
>> >
>> >  #include "lib/bluetooth.h"
>> >  #include "lib/sdp.h"
>> >  #include "lib/sdp_lib.h"
>> >
>> > -#include "lib/uuid.h"
>> >
>> >  #include "src/sdp-client.h"
>> >  #include "src/uuid-helper.h"
>> >  #include "src/shared/hfp.h"
>> >
>> > @@ -294,7 +293,7 @@ static void handle_connect(const void *buf, uint16_t
>> > len)>
>> >         device_init(&bdaddr);
>> >
>> > -       bt_string2uuid(&uuid, HFP_HS_UUID);
>> > +       sdp_uuid16_create(&uuid, HANDSFREE_SVCLASS_ID);
>> >
>> >         if (bt_search_service(&adapter_addr, &device.bdaddr, &uuid,
>> >
>> >                                         sdp_search_cb, NULL, NULL, 0) < 0)
>> >                                         {
>> >
>> >                 error("handsfree: SDP search failed");
>> >
>> > diff --git a/android/hidhost.c b/android/hidhost.c
>> > index 8bd3455..45fe14f 100644
>> > --- a/android/hidhost.c
>> > +++ b/android/hidhost.c
>> > @@ -37,7 +37,6 @@
>> >
>> >  #include "lib/bluetooth.h"
>> >  #include "lib/sdp.h"
>> >  #include "lib/sdp_lib.h"
>> >
>> > -#include "lib/uuid.h"
>> >
>> >  #include "src/shared/mgmt.h"
>> >  #include "src/sdp-client.h"
>> >  #include "src/uuid-helper.h"
>> >
>> > @@ -751,7 +750,7 @@ static void hid_sdp_did_search_cb(sdp_list_t *recs,
>> > int err, gpointer data)>
>> >                         dev->version = data->val.uint16;
>> >
>> >         }
>> >
>> > -       bt_string2uuid(&uuid, HID_UUID);
>> > +       sdp_uuid16_create(&uuid, HID_SVCLASS_ID);
>> >
>> >         if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
>> >
>> >                                 hid_sdp_search_cb, dev, NULL, 0) < 0) {
>> >
>> >                 error("failed to search sdp details");
>> >
>> > @@ -792,7 +791,7 @@ static void bt_hid_connect(const void *buf, uint16_t
>> > len)>
>> >         ba2str(&dev->dst, addr);
>> >         DBG("connecting to %s", addr);
>> >
>> > -       bt_string2uuid(&uuid, PNP_UUID);
>> > +       sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
>> >
>> >         if (bt_search_service(&adapter_addr, &dev->dst, &uuid,
>> >
>> >                                         hid_sdp_did_search_cb, dev, NULL,
>> >                                         0) < 0) {
>> >
>> >                 error("Failed to search DeviceID SDP details");
>> >
>> > @@ -1293,7 +1292,7 @@ static void connect_cb(GIOChannel *chan, GError
>> > *err, gpointer user_data)>
>> >                 dev->ctrl_io = g_io_channel_ref(chan);
>> >                 dev->uhid_fd = -1;
>> >
>> > -               bt_string2uuid(&uuid, PNP_UUID);
>> > +               sdp_uuid16_create(&uuid, PNP_INFO_SVCLASS_ID);
>> >
>> >                 if (bt_search_service(&src, &dev->dst, &uuid,
>> >
>> >                                         hid_sdp_did_search_cb, dev, NULL,
>> >                                         0) < 0) {
>> >
>> >                         error("failed to search did sdp details");
>> >
>> > --
>> > 1.7.9.5
>>
>> Applied all except 6/7, I think we should probably return an error if
>> there is an attempt to register a service out of range, then the
>> caller can assert so we can have a proper test for it that expect an
>> error in such case.
>
> Well, currently IPC depends on hal-msg.h which defines max allowed service ID
> and using something bigger is a code bug. We could have make IPC independent
> of hal-msg.h and just verify registered ID in runtime but that would add extra
> code for each caller with no profit as IDs are fixed anyway.

Perhaps we should ignore and not register service is initialized with
e.g. -1 just skip ipc_register.

> That test fix is invalid as we actually want to test if IPC handles out-of-
> bound service ID correctly (when receiving register command).
> And I'm not sure why this actually was causing any problems since that test is
> not registering handlers for out-of-bound service ID, just sends ipc message
> with such ID.

Im not sure we have the array of HAL_SERVICE_ID_MAX + 1 I thought that
would be HAL_SERVICE_ID_MAX, nevertheless every test seems to be
calling test_cmd_reg which does call ipc_register which IMO should be
invalid for HAL_SERVICE_ID_MAX + 1.

> (BTW I think we should pass max service id on ipc_init, as currently audio ipc
> is using max service id from bt ipc)

Im did not get the comment above.


-- 
Luiz Augusto von Dentz