[PATCH BlueZ 1/2] unit: Add initial tests for sdp_extract_attr()

[PATCH BlueZ 1/2] unit: Add initial tests for sdp_extract_attr()

[Anderson Lizardo]

These tests do not use the full SDP PDU building code because they try
to catch errors on SDP "extraction" code, which may not appear on a
response PDU (but still cause hard to find bugs).
---
 unit/test-sdp.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/unit/test-sdp.c b/unit/test-sdp.c
index 315a5cd..61449aa 100644
--- a/unit/test-sdp.c
+++ b/unit/test-sdp.c
@@ -754,6 +754,31 @@ static void test_sdp(gconstpointer data)
 	g_free(test->pdu_list);
 }
 
+static void test_sdp_extract_attr(void)
+{
+	const struct sdp_pdu pdus[] = {
+		raw_pdu(SDP_DATA_NIL),
+		raw_pdu(SDP_TEXT_STR8, 0x04, 'A', 'B', 'C', 'D'),
+		raw_pdu(SDP_TEXT_STR16, 0x00, 0x04, 'A', 'B', 'C', 'D'),
+		{ },
+	};
+	int i;
+
+	for (i = 0; pdus[i].valid; i++) {
+		sdp_data_t *d;
+		int size = 0;
+
+		if (g_test_verbose() == TRUE)
+			g_print("dtd=0x%02x\n", *(char *) pdus[i].raw_data);
+
+		d = sdp_extract_attr(pdus[i].raw_data, pdus[i].raw_size, &size,
+									NULL);
+		g_assert(d != NULL);
+		g_assert_cmpuint(size, ==, pdus[i].raw_size);
+		sdp_data_free(d);
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -2709,5 +2734,7 @@ int main(int argc, char *argv[])
 			0x08, 0x09, 0x00, 0x01, 0x35, 0x03, 0x19, 0x11,
 			0x06, 0x00));
 
+	g_test_add_func("/MISC/sdp_extract_attr", test_sdp_extract_attr);
+
 	return g_test_run();
 }
-- 
1.7.9.5

[PATCH BlueZ 2/2] lib: Fix SDP_TEXT_STR16/SDP_URL_STR16 parsing

[Anderson Lizardo]

sdp_extract_attr() uses the "size" parameter to return the number of
bytes consumed when parsing SDP Data Elements. This size is used to
advance a buffer pointer to parse next element.

This size was being incorrectly calculated for
SDP_TEXT_STR16/SDP_URL_STR16, where the string length was added twice.

A unit test added on the previous commit should now pass with this fix.
---
 lib/sdp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index ca474cd..b87f392 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -1176,7 +1176,7 @@ static sdp_data_t *extract_str(const void *p, int bufsize, int *len)
 		}
 		n = bt_get_be16(p);
 		p += sizeof(uint16_t);
-		*len += sizeof(uint16_t) + n;
+		*len += sizeof(uint16_t);
 		bufsize -= sizeof(uint16_t);
 		break;
 	default:
-- 
1.7.9.5

Re: [PATCH BlueZ 1/2] unit: Add initial tests for sdp_extract_attr()

[Marcel Holtmann]

Hi Anderson,

> These tests do not use the full SDP PDU building code because they try
> to catch errors on SDP "extraction" code, which may not appear on a
> response PDU (but still cause hard to find bugs).
> ---
>  unit/test-sdp.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/unit/test-sdp.c b/unit/test-sdp.c
> index 315a5cd..61449aa 100644
> --- a/unit/test-sdp.c
> +++ b/unit/test-sdp.c
> @@ -754,6 +754,31 @@ static void test_sdp(gconstpointer data)
>  	g_free(test->pdu_list);
>  }
>  
> +static void test_sdp_extract_attr(void)
> +{
> +	const struct sdp_pdu pdus[] = {
> +		raw_pdu(SDP_DATA_NIL),
> +		raw_pdu(SDP_TEXT_STR8, 0x04, 'A', 'B', 'C', 'D'),
> +		raw_pdu(SDP_TEXT_STR16, 0x00, 0x04, 'A', 'B', 'C', 'D'),
> +		{ },
> +	};
> +	int i;
> +
> +	for (i = 0; pdus[i].valid; i++) {
> +		sdp_data_t *d;
> +		int size = 0;
> +
> +		if (g_test_verbose() == TRUE)
> +			g_print("dtd=0x%02x\n", *(char *) pdus[i].raw_data);
> +
> +		d = sdp_extract_attr(pdus[i].raw_data, pdus[i].raw_size, &size,
> +									NULL);
> +		g_assert(d != NULL);
> +		g_assert_cmpuint(size, ==, pdus[i].raw_size);
> +		sdp_data_free(d);
> +	}
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	g_test_init(&argc, &argv, NULL);
> @@ -2709,5 +2734,7 @@ int main(int argc, char *argv[])
>  			0x08, 0x09, 0x00, 0x01, 0x35, 0x03, 0x19, 0x11,
>  			0x06, 0x00));
>  
> +	g_test_add_func("/MISC/sdp_extract_attr", test_sdp_extract_attr);
> +

can we make this a bit more generic with a bit more details on what you
are testing.

Also having a separate test case for str8, str16 and also str32 of
course would be a good idea. Same for url8, url16 and url32. In addition
checking empty strings and really long strings is a good idea.
Especially long strings that match the max len size.

What I actually like to see is that we can specific element sequences in
raw and also what they are suppose to match. So we need to ensure that
we also extract the right string value and types. And not just the size.

Regards

Marcel

Re: [PATCH BlueZ 2/2] lib: Fix SDP_TEXT_STR16/SDP_URL_STR16 parsing

[Marcel Holtmann]

Hi Anderson,

> sdp_extract_attr() uses the "size" parameter to return the number of
> bytes consumed when parsing SDP Data Elements. This size is used to
> advance a buffer pointer to parse next element.
> 
> This size was being incorrectly calculated for
> SDP_TEXT_STR16/SDP_URL_STR16, where the string length was added twice.
> 
> A unit test added on the previous commit should now pass with this fix.
> ---
>  lib/sdp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/sdp.c b/lib/sdp.c
> index ca474cd..b87f392 100644
> --- a/lib/sdp.c
> +++ b/lib/sdp.c
> @@ -1176,7 +1176,7 @@ static sdp_data_t *extract_str(const void *p, int bufsize, int *len)
>  		}
>  		n = bt_get_be16(p);
>  		p += sizeof(uint16_t);
> -		*len += sizeof(uint16_t) + n;
> +		*len += sizeof(uint16_t);

I do not get this fix. Isn't this str8 and url8 part wrong?

Regards

Marcel

Re: [PATCH BlueZ 2/2] lib: Fix SDP_TEXT_STR16/SDP_URL_STR16 parsing

[Anderson Lizardo]

Hi Marcel,

On Tue, Jan 8, 2013 at 3:13 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> diff --git a/lib/sdp.c b/lib/sdp.c
>> index ca474cd..b87f392 100644
>> --- a/lib/sdp.c
>> +++ b/lib/sdp.c
>> @@ -1176,7 +1176,7 @@ static sdp_data_t *extract_str(const void *p, int bufsize, int *len)
>>               }
>>               n = bt_get_be16(p);
>>               p += sizeof(uint16_t);
>> -             *len += sizeof(uint16_t) + n;
>> +             *len += sizeof(uint16_t);
>
> I do not get this fix. Isn't this str8 and url8 part wrong?

On the same function, next to the return, there is:

*len += n;

This is why STR8/URL8 actually are okay. For STR16 the "n" part is
added twice due to the snippet I change in this patch.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH BlueZ 1/2] unit: Add initial tests for sdp_extract_attr()

[Anderson Lizardo]

Hi Marcel,

On Tue, Jan 8, 2013 at 3:10 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> can we make this a bit more generic with a bit more details on what you
> are testing.
>
> Also having a separate test case for str8, str16 and also str32 of
> course would be a good idea. Same for url8, url16 and url32. In addition
> checking empty strings and really long strings is a good idea.
> Especially long strings that match the max len size.

This was supposed to be a set of initial tests, specially to validate
the fix I sent on the other patch. I was going to improve the test
coverage as I read more of the code.

But I can work on a set of tests which cover all reachable cases for
sdp_extract_attr() (I want to focus on this function for now because
it is less used than others and could hide other bugs.) and send them
at once, including corner cases.

> What I actually like to see is that we can specific element sequences in
> raw and also what they are suppose to match. So we need to ensure that
> we also extract the right string value and types. And not just the size.

The "match" data could be raw bytes which I get by converting the
returned sdp_data_t to PDU format using sdp_gen_pdu(). What do you
think?

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH BlueZ 2/2] lib: Fix SDP_TEXT_STR16/SDP_URL_STR16 parsing

[Marcel Holtmann]

Hi Anderson,

> >> diff --git a/lib/sdp.c b/lib/sdp.c
> >> index ca474cd..b87f392 100644
> >> --- a/lib/sdp.c
> >> +++ b/lib/sdp.c
> >> @@ -1176,7 +1176,7 @@ static sdp_data_t *extract_str(const void *p, int bufsize, int *len)
> >>               }
> >>               n = bt_get_be16(p);
> >>               p += sizeof(uint16_t);
> >> -             *len += sizeof(uint16_t) + n;
> >> +             *len += sizeof(uint16_t);
> >
> > I do not get this fix. Isn't this str8 and url8 part wrong?
> 
> On the same function, next to the return, there is:
> 
> *len += n;
> 
> This is why STR8/URL8 actually are okay. For STR16 the "n" part is
> added twice due to the snippet I change in this patch.

good catch. However I like to see that tiny piece of extra information
in the commit message as well. Makes it a lot clearer.

Regards

Marcel

Re: [PATCH BlueZ 1/2] unit: Add initial tests for sdp_extract_attr()

[Marcel Holtmann]

Hi Anderson,

> > can we make this a bit more generic with a bit more details on what you
> > are testing.
> >
> > Also having a separate test case for str8, str16 and also str32 of
> > course would be a good idea. Same for url8, url16 and url32. In addition
> > checking empty strings and really long strings is a good idea.
> > Especially long strings that match the max len size.
> 
> This was supposed to be a set of initial tests, specially to validate
> the fix I sent on the other patch. I was going to improve the test
> coverage as I read more of the code.
> 
> But I can work on a set of tests which cover all reachable cases for
> sdp_extract_attr() (I want to focus on this function for now because
> it is less used than others and could hide other bugs.) and send them
> at once, including corner cases.

I want to make our life easier in the long term. Otherwise you end up
with copy&paste all over the place.

> > What I actually like to see is that we can specific element sequences in
> > raw and also what they are suppose to match. So we need to ensure that
> > we also extract the right string value and types. And not just the size.
> 
> The "match" data could be raw bytes which I get by converting the
> returned sdp_data_t to PDU format using sdp_gen_pdu(). What do you
> think?

A better test case data could look like this:

	struct test_data {
		const void *input_data;
		size_t input_size;
		uint8_t dtd;
	};

And we can add combinations of structs and unions here to represent the
different testes.

Also I would create a nice helper define like I did with define_ssa()
for example to create the test data and the test with a nice name.

Regards

Marcel

[PATCH v2 BlueZ] lib: Fix SDP_TEXT_STR16/SDP_URL_STR16 parsing

[Anderson Lizardo]

sdp_extract_attr() uses the "size" parameter to return the number of
bytes consumed when parsing SDP Data Elements. This size is used to
advance a buffer pointer to parse next element.

This size was being incorrectly calculated for SDP_{TEXT,URL}_STR16 in
extract_str(), where the string length was added twice. The string
length is already added later in the function for {TEXT,URL}_STR{8,16}
by this statement:

*len += n;
---
 lib/sdp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index ca474cd..b87f392 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -1176,7 +1176,7 @@ static sdp_data_t *extract_str(const void *p, int bufsize, int *len)
 		}
 		n = bt_get_be16(p);
 		p += sizeof(uint16_t);
-		*len += sizeof(uint16_t) + n;
+		*len += sizeof(uint16_t);
 		bufsize -= sizeof(uint16_t);
 		break;
 	default:
-- 
1.7.9.5

Re: [PATCH v2 BlueZ] lib: Fix SDP_TEXT_STR16/SDP_URL_STR16 parsing

[Marcel Holtmann]

Hi Anderson,

> sdp_extract_attr() uses the "size" parameter to return the number of
> bytes consumed when parsing SDP Data Elements. This size is used to
> advance a buffer pointer to parse next element.
> 
> This size was being incorrectly calculated for SDP_{TEXT,URL}_STR16 in
> extract_str(), where the string length was added twice. The string
> length is already added later in the function for {TEXT,URL}_STR{8,16}
> by this statement:
> 
> *len += n;
> ---
>  lib/sdp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

patch has been applied.

Regards

Marcel

Re: [PATCH BlueZ 1/2] unit: Add initial tests for sdp_extract_attr()

[Anderson Lizardo]

Hi Marcel,

On Tue, Jan 8, 2013 at 6:46 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> A better test case data could look like this:
>
>         struct test_data {
>                 const void *input_data;
>                 size_t input_size;
>                 uint8_t dtd;
>         };
>
> And we can add combinations of structs and unions here to represent the
> different testes.
>
> Also I would create a nice helper define like I did with define_ssa()
> for example to create the test data and the test with a nice name.

I just sent a v2 of the SDP DE tests based on your suggestions (the 2
other patches are just refactoring of existing code).

Let me know if this new format is fine, so I can proceed with adding
other tests.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil