John Dennis wrote:
I wanted to understand the issues surrounding strict aliasing better. I found the following article to be well written, quite readable, and informative:
I found a NetBSD post with similar information: http://mail-index.netbsd.org/tech-kern/2003/08/11/0001.html
However, note that this is exactly what fr_socket is doing, &salocal is a pointer to sockaddr_storage, sa is in one scope is a pointer to sockaddr_in and in another scope sa is a pointer to sockaddr_in6. Each of these 3 pointers point to exact same memory location, according to the above definitions this is a classic case of illegal pointer aliasing.
Under ISO C89, and ISO C99. But it appears to be legal under the Posix description for "struct sockaddr_storage". Aren't standards wonderful.
It's a shame it didn't, but I don't think it changes the overarching issue. Also, in expediency I did not try building with the various GCC warning flags to see if I could get GCC to emit a warning for this case, perhaps GCC at some level of verbosity would have complained.
I've done some builds with "-fstrict-aliasing -Wstrict-aliasing=2". Ignoring the casts on parameters to functions, I've fixed a number of things, through the simple expedient of using memcpy() to copy from (struct sockaddr_storage) to (struct sockaddr_in), and back. It's ugly, but it SHOULD work.
For what it's worth my reading of the above statements is different than yours. I read the above as sockaddr_storage provides a type whose size is sufficient to hold all sockaddr types while respecting the alignment requirement in any given sockaddr type. The discussion of pointer casting fails to make explicit such casting must still conform to the rules of C99. I do not read the second paragraph as meaning pointer aliasing shall be supported, something outside that scope of that document.
At the minimum, the Opengroup specs seem misleading.
Proper type casting is discussed in the above article in the section: "Casting through a union (1 & 2)" which states "The most commonly accepted method of converting one type of object to another is by using a union".
Note this is what both Jakub and Ulrich independently suggested in the bug report.
Which makes me wonder WTF is the use of 'struct sockaddr_storage'. All of the documentation I've read indicates that it's *supposed* to be the preferred structure to cast back and forth to 'struct sockaddr_in'... but that's forbidden by ISO C89. You'd think that if the Posix people read the ISO C specs, they would have created a 'struct sockaddr_union', which was defined to be the union of all types, and which would have avoided these problems. Maybe I'm naive...
I have created a patch for packet.c which uses a union (the patch is attached). I built freeradius 2.0.4 with the patch and -O2 optimization and the random port problem seems to have been resolved. Note, I have not looked though all the source code to see if there are other code constructs which may also require a fix.
There's a number. Please try CVS head. I've committed a whack of memcpy's, which SHOULD avoid these issues. (And avoid umpteen unions).
Also, does anyone (Alan?) have any problems with updating the bug report with some of this email dialog to capture the issues for future readers?
I'd like to say that for me, the Posix spec is clear: casting between 'struct sockaddr_storage' and other 'struct sockaddr_*' is explicitely allowed. Looking at code on the net, there are LOTS of programs doing this. What is a different about FreeRADIUS is that it's *accessing* the pointer after the cast, and not just passing it to a function. This appears to be explicitly required by Posix: ...pointers to it can be cast as pointers to protocol-specific address structures and used to access the fields of those structures without alignment problems... i.e. TO ACCESS the fields. ACCESS to me means ACCESS. R/W ACCESS. Except that this is forbidden by ISO C89, and no one else does it. <sigh> It serves me right for reading only half of the 10's of 1000's of pages of specs. Alan DeKok.