buglwIP - A Lightweight TCP/IP stack - Bugs: bug #36283, PPP struct used on header size...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #36283: PPP struct used on header size computation and not packed

Submitter:  Sylvain Rochet <gradator>
Submitted:  Tue 24 Apr 2012 01:48:09 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Tue 15 May 2012 07:49:44 PM UTC, comment #7: 

Ok, that's what I thought (and wrote in comment #1). Thanks for clarifying it.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 May 2012 10:27:28 AM UTC, comment #6: 

I finally troubleshooted that and found why pppInputHeader must be packed.

If a structure is not packed, I guess the compiler optimize any use of this structure in a way that the structure must be aligned everywhere it is used.

With PPPoE, if not packed:

Ethernet => +14
PPPoE header => +6
sizeof(*pih) - sizeof(inProtocol) => -6

So, pppInputHeader replaces the PPPoE header, which is good (except that we double the inProtocol field, which is what -sizeof(inProtocol) tried to avoid), but the *pih ptr is not aligned and some uC die because of that.

Of course, if the structure pppInputHeader is not packed, it may work I guess if ETH_PAD_SIZE is set to 2 (if the driver support it, which mine not at the moment), but this is just because the PPPoE header is 6 bytes long and pppInputHeader too.

I am not a compiler expert, I guess if the structure is packed, that the compiler does not add alignment optimisation then the structure can be used on unaligned address.

Sylvain Rochet <gradator>
Group Member
Thu 03 May 2012 07:04:31 PM UTC, comment #5: 

Using a constant '6' is another good way to fix that, feel free to pack the struct or use a constant based on LwIP design rules which I don't know yet ! :)

Sylvain Rochet <gradator>
Group Member
Thu 03 May 2012 07:00:06 PM UTC, comment #4: 


> I have actually see this fail.


OK, I thought so after seeing the 'crash error' item group.

> (int)sizeof(struct pppInputHeader) returns 8 on 32 bits arch with alignment,


I'm with you on that, of course.

> but pbuf_header(pb, ...) should always be reduced to 6.


That's what I wanted to know: why sold it be 6 when size of is used everywhere, and not the constant '6'? Anyway, I've fixed it.


Simon Goldschmidt <goldsimon>
Group administrator
Thu 03 May 2012 06:42:26 PM UTC, comment #3: 

I have actually see this fail.

(int)sizeof(struct pppInputHeader) returns 8 on 32 bits arch with alignment, but pbuf_header(pb, ...) should always be reduced to 6.

Sylvain Rochet <gradator>
Group Member
Thu 03 May 2012 06:40:52 PM UTC, comment #2: 

Fixed in master.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 03 May 2012 06:36:40 PM UTC, comment #1: 

Have you actually seen this fail or is this from reading the code?

I don't see where "headersize computations" should fail, but since struct pppInputHeader is stored at p->payload, I think it must be packed for alignment reasons, anyway.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 Apr 2012 01:48:09 PM UTC, original submission:  

The pppInputHeader size is used in pbuf_header,

struct pppInputHeader *pih;
if (pbuf_header(pb, sizeof(*pih) - sizeof(inProtocol)) != 0) {

if(pbuf_header(nb, -(int)sizeof(struct pppInputHeader))) {

so, pppInputHeader need to be packed, otherwise on 32 bits system the structure is 8 byte long due to alignment, which fail header size computations.

Attached is a patch packing pppInputHeader struct, which fix this issue.

Sylvain Rochet <gradator>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by gradator (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-05-03 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2012-04-24 gradator Attached File- Added pack-pppInputHeader.patch, #25720

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code