[PATCH 0/3] Fix corrupted SDP response for sequence size > 256

[PATCH 0/3] Fix corrupted SDP response for sequence size > 256

[Bart Westgeest]

This patchset includes minor refactoring and a bug fix. The bug
is exposed when a data sequence grows beyond 256 bytes, in this case the
complete sequence data is written, but the size is truncated to one
byte, resulting in a corrupted SDP response.

RE: Feedback on potential fix for issue while advertising Feature List

Bart Westgeest (3):
  sdp: Inlined single use of function sdp_set_data_type
  sdp: Limit side effects of sdp_get_data_type and sdp_get_data_size
  sdp: Upgrade datatype SEQ8 to SEQ16 when data size is greater than
    256

 lib/sdp.c |   56 +++++++++++++++++++++++---------------------------------
 1 file changed, 23 insertions(+), 33 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] sdp: Inlined single use of function sdp_set_data_type

[Bart Westgeest]

Inlining single use of sdp_set_data_type to improve code readability,
since the function was doing more than just setting the data type.
---
 lib/sdp.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index 1a5bf01..dba3f59 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -666,18 +666,6 @@ static int sdp_get_data_type(sdp_buf_t *buf, uint8_t dtd)
 	return data_type;
 }
 
-static int sdp_set_data_type(sdp_buf_t *buf, uint8_t dtd)
-{
-	int data_type = 0;
-	uint8_t *p = buf->data + buf->data_size;
-
-	*p = dtd;
-	data_type = sdp_get_data_type(buf, dtd);
-	buf->data_size += data_type;
-
-	return data_type;
-}
-
 void sdp_set_attrid(sdp_buf_t *buf, uint16_t attr)
 {
 	uint8_t *p = buf->data;
@@ -815,9 +803,13 @@ int sdp_gen_pdu(sdp_buf_t *buf, sdp_data_t *d)
 	uint128_t u128;
 	uint8_t *seqp = buf->data + buf->data_size;
 
-	pdu_size = sdp_set_data_type(buf, dtd);
+	pdu_size = sdp_get_data_type(buf, dtd);
+	buf->data_size += pdu_size;
+
 	data_size = sdp_get_data_size(buf, d);
 
+	*seqp = dtd;
+
 	switch (dtd) {
 	case SDP_DATA_NIL:
 		break;
-- 
1.7.10.4

[PATCH 2/3] sdp: Limit side effects of sdp_get_data_type and sdp_get_data_size

[Bart Westgeest]

Remove modification of buf->buf_size in 'get' functions. Data is
still indirectly modified due to recursive nature of code.

Renamed sdp_get_data_type to sdp_get_data_type_size.
---
 lib/sdp.c |   26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index dba3f59..026163e 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -633,37 +633,32 @@ void sdp_set_seq_len(uint8_t *ptr, uint32_t length)
 	}
 }
 
-static int sdp_get_data_type(sdp_buf_t *buf, uint8_t dtd)
+static int sdp_get_data_type_size(uint8_t dtd)
 {
-	int data_type = 0;
-
-	data_type += sizeof(uint8_t);
+	int size = sizeof(uint8_t);
 
 	switch (dtd) {
 	case SDP_SEQ8:
 	case SDP_TEXT_STR8:
 	case SDP_URL_STR8:
 	case SDP_ALT8:
-		data_type += sizeof(uint8_t);
+		size += sizeof(uint8_t);
 		break;
 	case SDP_SEQ16:
 	case SDP_TEXT_STR16:
 	case SDP_URL_STR16:
 	case SDP_ALT16:
-		data_type += sizeof(uint16_t);
+		size += sizeof(uint16_t);
 		break;
 	case SDP_SEQ32:
 	case SDP_TEXT_STR32:
 	case SDP_URL_STR32:
 	case SDP_ALT32:
-		data_type += sizeof(uint32_t);
+		size += sizeof(uint32_t);
 		break;
 	}
 
-	if (!buf->data)
-		buf->buf_size += data_type;
-
-	return data_type;
+	return size;
 }
 
 void sdp_set_attrid(sdp_buf_t *buf, uint16_t attr)
@@ -762,9 +757,6 @@ static int sdp_get_data_size(sdp_buf_t *buf, sdp_data_t *d)
 		break;
 	}
 
-	if (!buf->data)
-		buf->buf_size += data_size;
-
 	return data_size;
 }
 
@@ -783,8 +775,8 @@ static int sdp_gen_buffer(sdp_buf_t *buf, sdp_data_t *d)
 	/* attribute length */
 	buf->buf_size += sizeof(uint8_t) + sizeof(uint16_t);
 
-	sdp_get_data_type(buf, d->dtd);
-	sdp_get_data_size(buf, d);
+	buf->buf_size += sdp_get_data_type_size(d->dtd);
+	buf->buf_size += sdp_get_data_size(buf, d);
 
 	if (buf->buf_size > UCHAR_MAX && d->dtd == SDP_SEQ8)
 		buf->buf_size += sizeof(uint8_t);
@@ -803,7 +795,7 @@ int sdp_gen_pdu(sdp_buf_t *buf, sdp_data_t *d)
 	uint128_t u128;
 	uint8_t *seqp = buf->data + buf->data_size;
 
-	pdu_size = sdp_get_data_type(buf, dtd);
+	pdu_size = sdp_get_data_type_size(dtd);
 	buf->data_size += pdu_size;
 
 	data_size = sdp_get_data_size(buf, d);
-- 
1.7.10.4

[PATCH 3/3] sdp: Upgrade datatype SEQ8 to SEQ16 when data size is greater than 256

[Bart Westgeest]

Fixes a bug where the complete sequence data is written, but the size
is truncated to one byte.
---
 lib/sdp.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index 026163e..ceb1192 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -786,23 +786,29 @@ static int sdp_gen_buffer(sdp_buf_t *buf, sdp_data_t *d)
 
 int sdp_gen_pdu(sdp_buf_t *buf, sdp_data_t *d)
 {
-	uint32_t pdu_size = 0, data_size = 0;
+	uint32_t pdu_size, data_size;
 	unsigned char *src = NULL, is_seq = 0, is_alt = 0;
-	uint8_t dtd = d->dtd;
 	uint16_t u16;
 	uint32_t u32;
 	uint64_t u64;
 	uint128_t u128;
 	uint8_t *seqp = buf->data + buf->data_size;
+	uint32_t orig_data_size = buf->data_size;
 
-	pdu_size = sdp_get_data_type_size(dtd);
+recalculate:
+	pdu_size = sdp_get_data_type_size(d->dtd);
 	buf->data_size += pdu_size;
 
 	data_size = sdp_get_data_size(buf, d);
+	if (data_size > UCHAR_MAX && d->dtd == SDP_SEQ8) {
+		buf->data_size = orig_data_size;
+		d->dtd = SDP_SEQ16;
+		goto recalculate;
+	}
 
-	*seqp = dtd;
+	*seqp = d->dtd;
 
-	switch (dtd) {
+	switch (d->dtd) {
 	case SDP_DATA_NIL:
 		break;
 	case SDP_UINT8:
@@ -884,7 +890,7 @@ int sdp_gen_pdu(sdp_buf_t *buf, sdp_data_t *d)
 		if (src && buf->buf_size >= buf->data_size + data_size) {
 			memcpy(buf->data + buf->data_size, src, data_size);
 			buf->data_size += data_size;
-		} else if (dtd != SDP_DATA_NIL) {
+		} else if (d->dtd != SDP_DATA_NIL) {
 			SDPDBG("Gen PDU : Can't copy from invalid source or dest\n");
 		}
 	}
-- 
1.7.10.4

Re: [PATCH 2/3] sdp: Limit side effects of sdp_get_data_type and sdp_get_data_size

[Anderson Lizardo]

Hi Bart,

On Mon, Nov 19, 2012 at 3:04 PM, Bart Westgeest <bart@elbrys.com> wrote:
> @@ -762,9 +757,6 @@ static int sdp_get_data_size(sdp_buf_t *buf, sdp_data_t *d)
>                 break;
>         }
>
> -       if (!buf->data)
> -               buf->buf_size += data_size;
> -
>         return data_size;
>  }
>
> @@ -783,8 +775,8 @@ static int sdp_gen_buffer(sdp_buf_t *buf, sdp_data_t *d)
>         /* attribute length */
>         buf->buf_size += sizeof(uint8_t) + sizeof(uint16_t);
>
> -       sdp_get_data_type(buf, d->dtd);
> -       sdp_get_data_size(buf, d);
> +       buf->buf_size += sdp_get_data_type_size(d->dtd);
> +       buf->buf_size += sdp_get_data_size(buf, d);

No need to check for "if (!buf->data)" like in the original code?

>
>         if (buf->buf_size > UCHAR_MAX && d->dtd == SDP_SEQ8)
>                 buf->buf_size += sizeof(uint8_t);
> @@ -803,7 +795,7 @@ int sdp_gen_pdu(sdp_buf_t *buf, sdp_data_t *d)
>         uint128_t u128;
>         uint8_t *seqp = buf->data + buf->data_size;
>
> -       pdu_size = sdp_get_data_type(buf, dtd);
> +       pdu_size = sdp_get_data_type_size(dtd);

Same here. Additionally, buf->buf_size is not being updated.

>         buf->data_size += pdu_size;
>
>         data_size = sdp_get_data_size(buf, d);

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

Re: [PATCH 3/3] sdp: Upgrade datatype SEQ8 to SEQ16 when data size is greater than 256

[Anderson Lizardo]

Hi Bart,

On Mon, Nov 19, 2012 at 3:04 PM, Bart Westgeest <bart@elbrys.com> wrote:
> Fixes a bug where the complete sequence data is written, but the size
> is truncated to one byte.
> ---
>  lib/sdp.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/lib/sdp.c b/lib/sdp.c
> index 026163e..ceb1192 100644
> --- a/lib/sdp.c
> +++ b/lib/sdp.c
> @@ -786,23 +786,29 @@ static int sdp_gen_buffer(sdp_buf_t *buf, sdp_data_t *d)
>
>  int sdp_gen_pdu(sdp_buf_t *buf, sdp_data_t *d)
>  {
> -       uint32_t pdu_size = 0, data_size = 0;
> +       uint32_t pdu_size, data_size;
>         unsigned char *src = NULL, is_seq = 0, is_alt = 0;
> -       uint8_t dtd = d->dtd;
>         uint16_t u16;
>         uint32_t u32;
>         uint64_t u64;
>         uint128_t u128;
>         uint8_t *seqp = buf->data + buf->data_size;
> +       uint32_t orig_data_size = buf->data_size;
>
> -       pdu_size = sdp_get_data_type_size(dtd);
> +recalculate:
> +       pdu_size = sdp_get_data_type_size(d->dtd);
>         buf->data_size += pdu_size;
>
>         data_size = sdp_get_data_size(buf, d);
> +       if (data_size > UCHAR_MAX && d->dtd == SDP_SEQ8) {
> +               buf->data_size = orig_data_size;
> +               d->dtd = SDP_SEQ16;
> +               goto recalculate;

IMHO, using "goto" here is too complicated for 3 lines of code that
will just be run at most twice. So I would simply duplicate them
inside the if(). But others may have different opinion on this regard.

> +       }
>
> -       *seqp = dtd;
> +       *seqp = d->dtd;
>
> -       switch (dtd) {
> +       switch (d->dtd) {
>         case SDP_DATA_NIL:
>                 break;
>         case SDP_UINT8:

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

Re: [PATCH 2/3] sdp: Limit side effects of sdp_get_data_type and sdp_get_data_size

[Bart Westgeest]

Hi Anderson,

>> -       sdp_get_data_type(buf, d->dtd);
>> -       sdp_get_data_size(buf, d);
>> +       buf->buf_size += sdp_get_data_type_size(d->dtd);
>> +       buf->buf_size += sdp_get_data_size(buf, d);
>
> No need to check for "if (!buf->data)" like in the original code?

I do not think there's a need for it, this modification is in 
sdp_gen_buffer. sdp_gen_buffer is called three times from the following 
functions: (1) sdp_attr_size, (2) sdp_append_to_pdu, and (3) 
gen_dataseq_pdu.

For (2) & (3), buf is memset to 0 prior to calling sdp_gen_buffer, thus 
buf->data is always NULL.

For (1), buf is also memset to 0 for the first call to sdp_attr_size, 
which is called repetitively in a foreach statement. Repetitive calls to 
sdp_gen_buffer on the same buffer will not (ever) result in an 
allocation of buf->data as far as I can tell.

>> -       pdu_size = sdp_get_data_type(buf, dtd);
>> +       pdu_size = sdp_get_data_type_size(dtd);
>
> Same here. Additionally, buf->buf_size is not being updated.

This modification is in sdp_gen_pdu. sdp_gen_pdu is never called with 
buf->data == NULL. If it would, then the initialization of seqp in 
sdp_gen_pdu is erroneous, since it uses buf->data without verifying it.

In addition, in the current code, sdp_gen_pdu is only called from three 
locations: (1) get_data_size, (2) sdp_append_to_pdu, (3) 
gen_dataseq_pdu. In all cases the buf->data is either allocated prior to 
calling (2 & 3), or (1) there's an if statement preventing sdp_gen_pdu 
from being called when buf->data == NULL.

Bart

Re: [PATCH 3/3] sdp: Upgrade datatype SEQ8 to SEQ16 when data size is greater than 256

[Bart Westgeest]

Hi Anderson,

>> +recalculate:
>> +       pdu_size = sdp_get_data_type_size(d->dtd);
>>          buf->data_size += pdu_size;
>>
>>          data_size = sdp_get_data_size(buf, d);
>> +       if (data_size > UCHAR_MAX && d->dtd == SDP_SEQ8) {
>> +               buf->data_size = orig_data_size;
>> +               d->dtd = SDP_SEQ16;
>> +               goto recalculate;
>
> IMHO, using "goto" here is too complicated for 3 lines of code that
> will just be run at most twice. So I would simply duplicate them
> inside the if(). But others may have different opinion on this regard.

I don't have strong feelings about this. I can change it.

One could argue that the way I did it shows that the *same* code needs 
to be executed again. (It the same code can also be moved into a 
do-while) - this, as well what I have currently, would allow for a data 
upgrade to SDP_SEQ16 -> SDP_SEQ32, and SDP_ALT8 -> SDP_ALT16-> SDP_ALT32 
as well, which theoretically are also needed as far as I can tell.

For the record, in any regard, I think the fix is rather ugly, goto 
statement or not. This problem is now 'fixed' in two places (the other 
one in sdp_append_to_buf, with the corresponding data length increase in 
sdp_gen_buffer). IMHO, the code requires a bigger cleanup. I actually 
made an attempt at it, but the fix got too big, and I opted for what I 
submitted instead.

To get back on topic, let me know if you or others want me to re-submit.

Bart

Re: [PATCH 0/3] Fix corrupted SDP response for sequence size > 256

[Johan Hedberg]

Hi Bart,

On Mon, Nov 19, 2012, Bart Westgeest wrote:
> This patchset includes minor refactoring and a bug fix. The bug
> is exposed when a data sequence grows beyond 256 bytes, in this case the
> complete sequence data is written, but the size is truncated to one
> byte, resulting in a corrupted SDP response.
> 
> RE: Feedback on potential fix for issue while advertising Feature List
> 
> Bart Westgeest (3):
>   sdp: Inlined single use of function sdp_set_data_type
>   sdp: Limit side effects of sdp_get_data_type and sdp_get_data_size
>   sdp: Upgrade datatype SEQ8 to SEQ16 when data size is greater than
>     256
> 
>  lib/sdp.c |   56 +++++++++++++++++++++++---------------------------------
>  1 file changed, 23 insertions(+), 33 deletions(-)

I'm happy with these as well as your responses to the feedback you got,
so the patches are now pushed upstream.

Regarding the SDP code needing cleanups/fixing/rewriting in general, we
know :) This is one of the oldest pieces of code in the stack which
could definitely use a rewrite. However, no one has come along with the
time to do it yet.

Johan