buglwIP - A Lightweight TCP/IP stack - Bugs: bug #33485, SOCK_ADDR_TYPE_MATCH and...

 
 

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

bug #33485: SOCK_ADDR_TYPE_MATCH and SOCK_ADDR_TYPE_MATCH_OR_UNSPEC use error

Submitted by:  hanhui <hanhui03>
Submitted on:  Mon 06 Jun 2011 02:18:30 PM UTC  
 
Category: sockets/netconnSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: 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 07 Jun 2011 03:09:01 PM UTC, comment #9:

I don't like the usage of alignment members myself, but it was the only solution I saw, since the structure is only allocated on the stack to get correctly aliglned memory.

However, I think this can be solved by adding a typedef that is defined either to struck sockaddr_in if IPv6 is disabled or to struct sockaddr_in6 if it is enabled would also do the trick and would be less confusing.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 07 Jun 2011 02:50:18 PM UTC, comment #8:

>alignment1 and alignment2 will give the required result on most compilers,...


I agree and I would say all compilers. My point was there could be a lot of compiler-added padding with the extra alignments and if the sa_* members are each aligned. alignment1 isn't necessary. If a compiler is going to align members, alignment2 will align the follow u8_t array. And it will also align alignment1 wasting space.

But the statement by Simon that this is an internal type changed my worries of a bloated ip_addr type. I thought it might be copied a good bit in the code if it were a global type.

Bill Auerbach <billauerbach>
Tue 07 Jun 2011 02:17:51 PM UTC, comment #7:

> From the draft C99 standard (6.7.2.1), you'll note the compiler
> can add any padding is chooses between members. So the
> alignment1 and alignment2 are also second guessing the compiler, > aren't they?


Yes, they are, which is why I think if getting particular alignment of fields in a structure is important, padding is not the way to do it: "If we really need to avoid padding between fields in a structure then we must mark it as packed or specify the alignment of fields using compiler-specific directives"

alignment1 and alignment2 will give the required result on most compilers, and so as they are more portable than the compiler-specific directives there is an argument in favour of doing it that way, as long as we have suitable assertions in place to highlight when this assumption about the compiler behavior is broken.

I can't see a case for adding more padding however because it would neither increase our portability nor produce the correct result more often, and would just bloat the size of the structures on most compilers.

Kieran Mansley <kieranm>
Project Administrator
Tue 07 Jun 2011 02:00:44 PM UTC, comment #6:

From the draft C99 standard (6.7.2.1), you'll note the compiler can add any padding is chooses between members. So the alignment1 and alignment2 are also second guessing the compiler, aren't they?

12 Each non-bit-field member of a structure or union object is aligned in an implementationdefined
manner appropriate to its type.

13 Within a structure object, the non-bit-field members and the units in which bit-fields reside have addresses that increase in the order in which they are declared. A pointer to a structure object, suitably converted, points to its initial member (or if that member is a bit-field, then to the unit in which it resides), and vice versa. There may be unnamed padding within a structure object, but not at its beginning.

Bill Auerbach <billauerbach>
Tue 07 Jun 2011 09:01:21 AM UTC, comment #5:

While a compiler could do that, I think it is unlikely, as it would normally align a field to a boundary of the size of that type. Without the "char pad[]" fields alignment1 is 16 bit aligned, and alignment2 is 32 bit aligned, i.e they are already on their natural alignment so why insert padding?

Perhaps the compiler would rather store alignment1 as a 32 bit int as that is more efficient, but if we take that view then why stop at 2 bytes padding between alignment1 and alignment2? A compiler could in theory put them both on 64 bit boundaries (e.g. on a 64 system) if it thought that was better.

Trying to second-guess the compiler and insert padding into structures is never going to work well across all compilers, apart from in some very limited cases. If we really need to avoid padding between fields in a structure then we must mark it as packed or specify the alignment of fields using compiler-specific directives.

I'm always pleased to see debug code to check assumptions, so the assertion idea is fine by me, but I agree with Simon that when the struct sockaddr_in[6] was allocated by the caller it must have had the correct alignment then, so although it's come to us via a struct sockaddr the aligment shouldn't have changed and the compiler's warning (while technically correct) can be thought of as wrong in this case.

Kieran Mansley <kieranm>
Project Administrator
Mon 06 Jun 2011 08:41:24 PM UTC, comment #4:

>I don't see why I would end up with 16 bytes unless IPv6 is turned on...


Regardless of IPV6 being on or off, this is because alignment1 and alignment2 could become 32-bit aligned. The compiler might well generate this with padding:

I get it better now that it's an internal thing only. I guess it will fall out in the end. Maybe until sometime later and it's gotten some mileage it's worth an LWIP_ASSERT to be sure it's always is as you require? I wouldn't benchmark with the asserts though if this is often used.

I know you're well aware that there are several platforms and several compilers and the combinations are possibly P*C. I'm trying to think about it from this angle.

Bill Auerbach <billauerbach>
Mon 06 Jun 2011 08:15:51 PM UTC, comment #3:

I don't see why I would end up with 16 bytes unless IPv6 is turned on, which then is what I want. That structure is supposed to be big enough to hold both struct sockaddr_in and struct sockaddr_in6 in that case.

However, using a dummy[2] would mean the required alignment would be 1, not 4. In that case the compiler warns about increased alignment requirements when casting from sockaddr (alignment 1) to sockaddr_in (alignment 4). By using sockaddr_aligned (only for instantiation internally), I make sure the RAM I get is aligned for both types.

> Casting throws away what a compiler knows about alignment of an
> expression.


That's exactly what we need here: we do get a sockadr-pointer, which has strictly spoken has alignment requirements of 1 but we know it is either a sockaddr_in or a sockaddr_in6 (or a bug in the application as these are the only types you may use when allocating a variable). So in this situation we do know more than the compiler. In fact, this scheme is used throughout the stack (and has been used in sockets.c before, also).

From everything I know, I'm quite sure this should be good everywhere. The only option left would be to use memcpy() to copy from the pointer passed in to a structure on the stack (of the actual type) which is aligned correctly, but this should be avoided.

> I believe a cast of (struct sockaddr_in) will cause the compiler to assume the
> address is aligned (since any instance of a sockaddr it would have aligned)


This is exactly the problem: the compiler warns that struct sockaddr is not aligned but the target (sockaddr_in) requires alignment.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 06 Jun 2011 06:09:25 PM UTC, comment #2:

From patch:

On some 32-bit platforms you could wind up with 16 bytes taken up before the sa_data member. This might be more portable and smaller:

or use a u8_t dummy[2]; for the :16.

---

Casting throws away what a compiler knows about alignment of an expression. So I'm not sure the cast to void * is sound across all platforms. I believe a cast of (struct sockaddr_in) will cause the compiler to assume the address is aligned (since any instance of a sockaddr it would have aligned). If you know it to be aligned, then the (void ) isn't needed, is it?

The summary is what you find and fix for your platform(s) might not necessarily work everywhere. I think the less casting that is done the better.

But I don't know for sure - what you have could be fine everywhere.

Bill Auerbach <billauerbach>
Mon 06 Jun 2011 04:01:24 PM UTC, comment #1:

Fixed, thanks for reporting. I'm just in progress of testing sockets with/without IPv6 after adding the new code, but help like this is always appreciated!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 06 Jun 2011 02:18:30 PM UTC, original submission:

SOCK_ADDR_TYPE_MATCH and SOCK_ADDR_TYPE_MATCH_OR_UNSPEC use error!

in sockets.c for example:

int
lwip_bind(int s, const struct sockaddr *name, socklen_t namelen)
{
......
if (SOCK_ADDR_TYPE_MATCH(name, sock)) {
.......
}
......
}

should be:

int
lwip_bind(int s, const struct sockaddr *name, socklen_t namelen)
{
......
if (!SOCK_ADDR_TYPE_MATCH(name, sock)) {
.......
}
......
}

hanhui <hanhui03>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by billauerbach (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by hanhui03 (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 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 06 Jun 2011 04:01:24 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1