[Bluez-devel] [patch] alignment trap in hcid

[Bluez-devel] [patch] alignment trap in hcid

[Frédéric Dalleau]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

Dear all,

I recently met an alignment trap in hcid.
Some device sent me an sdp request and the answer had to be fragmented 
because the device reception buffer was very small.
After that i saw alignment trap.
The last trace I saw was : Continuation state size: 8
The trace is located at sdpd/request.c : static sdp_cont_state_t 
*sdp_cstate_get(uint8_t *buffer)
This function returns an unaligned pointer.

I think this patch is ok, but it has never been tested, and possibly 
other problems are hidden behind.
Instead of returning an unaligned pointer, the patch allocates a buffer.

To test it, i could build a bluez with small sdp reception buffer and 
ask an arm to give me fragmented reply.
This would save me some time if someone could tell me where to change 
this value...

BR,
Frederic


[-- Attachment #2: upf_hcid_align.patch --]
[-- Type: text/x-patch, Size: 1159 bytes --]

diff --git a/sdpd/request.c b/sdpd/request.c
index 20e68b6..f7952f5 100644
--- a/sdpd/request.c
+++ b/sdpd/request.c
@@ -179,7 +179,8 @@ static sdp_cont_state_t *sdp_cstate_get(uint8_t *buffer)
 
 	pdata += sizeof(uint8_t);
 	if (cStateSize != 0) {
-		sdp_cont_state_t *cstate = (sdp_cont_state_t *)pdata;
+		sdp_cont_state_t *cstate = malloc(sizeof(sdp_cont_state_t));
+		memcpy(cstate, (sdp_cont_state_t *)pdata, sizeof(sdp_cont_state_t));
 		debug("Cstate TS : 0x%lx", cstate->timestamp);
 		debug("Bytes sent : %d", cstate->cStateValue.maxBytesSent);
 		return cstate;
@@ -408,6 +409,8 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)
 	}
 
 done:	
+	if(cstate)
+		free(cstate);
 	if (pattern)
 		sdp_list_free(pattern, free);
 
@@ -593,6 +596,8 @@ static int service_attr_req(sdp_req_t *req, sdp_buf_t *buf)
 	buf->buf_size += sizeof(uint16_t);
 
 done:
+	if(cstate)
+		free(cstate);
 	if (seq)
                 sdp_list_free(seq, free);
 	if (status)
@@ -754,6 +759,8 @@ static int service_search_attr_req(sdp_req_t *req, sdp_buf_t *buf)
 	}
 
 done:
+	if(cstate)
+		free(cstate);
 	if (tmpbuf.data)
 		free(tmpbuf.data);
 	if (pattern)

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [patch] alignment trap in hcid

[Frédéric Dalleau]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]


Updated patch, checks NULL return from malloc spotted by jhe

Frédéric Dalleau wrote:
> Dear all,
>
> I recently met an alignment trap in hcid.
> Some device sent me an sdp request and the answer had to be fragmented 
> because the device reception buffer was very small.
> After that i saw alignment trap.
> The last trace I saw was : Continuation state size: 8
> The trace is located at sdpd/request.c : static sdp_cont_state_t 
> *sdp_cstate_get(uint8_t *buffer)
> This function returns an unaligned pointer.
>
> I think this patch is ok, but it has never been tested, and possibly 
> other problems are hidden behind.
> Instead of returning an unaligned pointer, the patch allocates a buffer.
>
> To test it, i could build a bluez with small sdp reception buffer and 
> ask an arm to give me fragmented reply.
> This would save me some time if someone could tell me where to change 
> this value...
>
> BR,
> Frederic
>
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2008.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> ------------------------------------------------------------------------
>
> _______________________________________________
> Bluez-devel mailing list
> Bluez-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bluez-devel


[-- Attachment #2: upf_hcid_align.patch --]
[-- Type: text/x-patch, Size: 1192 bytes --]

diff --git a/sdpd/request.c b/sdpd/request.c
index 20e68b6..566367f 100644
--- a/sdpd/request.c
+++ b/sdpd/request.c
@@ -179,7 +179,10 @@ static sdp_cont_state_t *sdp_cstate_get(uint8_t *buffer)
 
 	pdata += sizeof(uint8_t);
 	if (cStateSize != 0) {
-		sdp_cont_state_t *cstate = (sdp_cont_state_t *)pdata;
+		sdp_cont_state_t *cstate = malloc(sizeof(sdp_cont_state_t));
+		if(!cstate)
+			return NULL;
+		memcpy(cstate, (sdp_cont_state_t *)pdata, sizeof(sdp_cont_state_t));
 		debug("Cstate TS : 0x%lx", cstate->timestamp);
 		debug("Bytes sent : %d", cstate->cStateValue.maxBytesSent);
 		return cstate;
@@ -408,6 +411,8 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)
 	}
 
 done:	
+	if(cstate)
+		free(cstate);
 	if (pattern)
 		sdp_list_free(pattern, free);
 
@@ -593,6 +598,8 @@ static int service_attr_req(sdp_req_t *req, sdp_buf_t *buf)
 	buf->buf_size += sizeof(uint16_t);
 
 done:
+	if(cstate)
+		free(cstate);
 	if (seq)
                 sdp_list_free(seq, free);
 	if (status)
@@ -754,6 +761,8 @@ static int service_search_attr_req(sdp_req_t *req, sdp_buf_t *buf)
 	}
 
 done:
+	if(cstate)
+		free(cstate);
 	if (tmpbuf.data)
 		free(tmpbuf.data);
 	if (pattern)

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [patch] alignment trap in hcid

[Marcel Holtmann]

Hi Frederic,

> I recently met an alignment trap in hcid.
> Some device sent me an sdp request and the answer had to be  
> fragmented because the device reception buffer was very small.
> After that i saw alignment trap.
> The last trace I saw was : Continuation state size: 8
> The trace is located at sdpd/request.c : static sdp_cont_state_t  
> *sdp_cstate_get(uint8_t *buffer)
> This function returns an unaligned pointer.

this is so funny since I know it was there, but the new qualification  
tests don't find it anymore :)

Please fix the coding style. You are missing some spaces after the "if".

Johan, please have second look at the patch. It looks good to me.

Regards

Marcel


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [patch] alignment trap in hcid

[Johan Hedberg]

On Feb 29, 2008, at 20:27, Marcel Holtmann wrote:
>> I recently met an alignment trap in hcid.
>> Some device sent me an sdp request and the answer had to be
>> fragmented because the device reception buffer was very small.
>> After that i saw alignment trap.
>> The last trace I saw was : Continuation state size: 8
>> The trace is located at sdpd/request.c : static sdp_cont_state_t
>> *sdp_cstate_get(uint8_t *buffer)
>> This function returns an unaligned pointer.
>
> this is so funny since I know it was there, but the new qualification
> tests don't find it anymore :)
>
> Please fix the coding style. You are missing some spaces after the  
> "if".
>
> Johan, please have second look at the patch. It looks good to me.

Looks good to me too.

Johan

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [patch] alignment trap in hcid

[Frédéric Dalleau]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]


Damn me, I always forget this space after ifs...
Is there any way to test this, or hope it's ok ?

Frederic

Johan Hedberg wrote:
> On Feb 29, 2008, at 20:27, Marcel Holtmann wrote:
>   
>>> I recently met an alignment trap in hcid.
>>> Some device sent me an sdp request and the answer had to be
>>> fragmented because the device reception buffer was very small.
>>> After that i saw alignment trap.
>>> The last trace I saw was : Continuation state size: 8
>>> The trace is located at sdpd/request.c : static sdp_cont_state_t
>>> *sdp_cstate_get(uint8_t *buffer)
>>> This function returns an unaligned pointer.
>>>       
>> this is so funny since I know it was there, but the new qualification
>> tests don't find it anymore :)
>>
>> Please fix the coding style. You are missing some spaces after the  
>> "if".
>>
>> Johan, please have second look at the patch. It looks good to me.
>>     
>
> Looks good to me too.
>
> Johan
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2008.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> Bluez-devel mailing list
> Bluez-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bluez-devel
>   


[-- Attachment #2: upf_hcid_align.patch --]
[-- Type: text/x-patch, Size: 1196 bytes --]

diff --git a/sdpd/request.c b/sdpd/request.c
index 20e68b6..5e3c715 100644
--- a/sdpd/request.c
+++ b/sdpd/request.c
@@ -179,7 +179,10 @@ static sdp_cont_state_t *sdp_cstate_get(uint8_t *buffer)
 
 	pdata += sizeof(uint8_t);
 	if (cStateSize != 0) {
-		sdp_cont_state_t *cstate = (sdp_cont_state_t *)pdata;
+		sdp_cont_state_t *cstate = malloc(sizeof(sdp_cont_state_t));
+		if (!cstate)
+			return NULL;
+		memcpy(cstate, (sdp_cont_state_t *)pdata, sizeof(sdp_cont_state_t));
 		debug("Cstate TS : 0x%lx", cstate->timestamp);
 		debug("Bytes sent : %d", cstate->cStateValue.maxBytesSent);
 		return cstate;
@@ -408,6 +411,8 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)
 	}
 
 done:	
+	if (cstate)
+		free(cstate);
 	if (pattern)
 		sdp_list_free(pattern, free);
 
@@ -593,6 +598,8 @@ static int service_attr_req(sdp_req_t *req, sdp_buf_t *buf)
 	buf->buf_size += sizeof(uint16_t);
 
 done:
+	if (cstate)
+		free(cstate);
 	if (seq)
                 sdp_list_free(seq, free);
 	if (status)
@@ -754,6 +761,8 @@ static int service_search_attr_req(sdp_req_t *req, sdp_buf_t *buf)
 	}
 
 done:
+	if (cstate)
+		free(cstate);
 	if (tmpbuf.data)
 		free(tmpbuf.data);
 	if (pattern)

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [patch] alignment trap in hcid

[Johan Hedberg]

Hi Fr=E9d=E9ric,

On Mar 3, 2008, at 6:29, Fr=E9d=E9ric Dalleau wrote:
> Damn me, I always forget this space after ifs...
> Is there any way to test this, or hope it's ok ?

The patch was fine and is now commited to CVS. Thanks.

Johan
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel