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

Submitted by:  Sylvain Rochet <gradator>
Submitted on:  Tue 24 Apr 2012 01:48:09 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project 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>
Project AdministratorIn charge of this item.
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>
Project Member
Thu 03 May 2012 06:40:52 PM UTC, comment #2:

Fixed in master.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Member

 

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 03 May 2012 06:40:52 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Tue 24 Apr 2012 01:48:09 PM UTCgradatorAttached File-=>Added pack-pppInputHeader.patch, #25720

    Back to the top


    Powered by Savane 3.1-cleanup1