FreeRADIUS 2 not listening on right port

[John Dennis]

Alan DeKok wrote:
> Phil Mayers wrote:
>   
>> For those not following the Fedora bug, it (or rather, it's dependency)
>> has been closed by Ulrich Drepper. He seems to be saying that the
>> FreeRadius code is incorrect and specifically that an invalid typecast
>> is triggering the compiler to generate bad code:
>>     
>
>   
I wanted to understand the issues surrounding strict aliasing better. I 
found the following article to be well written, quite readable, and 
informative:

Understanding Strict Aliasing 
<http://www.cellperformance.com/mike_acton/2006/06/understanding_strict_aliasing.html>
http://www.cellperformance.com/mike_acton/2006/06/understanding_strict_aliasing.html

If one is to take away any simple concept and/or rule from the article 
it would have to be these 3 statements:

"One pointer is said to /alias/ another pointer when both refer to the 
same location or object. "
"In C99, it is /illegal/ to create an alias of a different type than the 
original."
"Strict aliasing means that two objects of different types cannot refer 
to the same location in memory."

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.

if (ipaddr->af == AF_INET) {
struct sockaddr_in *sa;

sa = (struct sockaddr_in *) &salocal;
sa->sin_port = htons((uint16_t) port);
} else if (ipaddr->af == AF_INET6) {
struct sockaddr_in6 *sa;

sa = (struct sockaddr_in6 *) &salocal;
sa->sin6_port = htons((uint16_t) port);
}


>   Interesting.  So GCC doesn't complain, and it generates bad assembly
> for C code that it thinks is perfectly valid.
>
>   
Agreed, GCC should have flagged this. However, for what it's worth the 
above article states the compiler may not always be able to recognize 
various illegal constructs and programmers should not depend it. If and 
when GCC should emit warnings is a subject for debate, but I think we 
can all agree it would have been nicer if GCC had emitted a warning. 
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.
>> https://bugzilla.redhat.com/show_bug.cgi?id=448743#c6
>>
>> Summary (as far as I can make out): Because the code looks like this:
>>     
> ...
>   
>> ...the compiler can't detect that the "i" value is "used", and optimises
>> the code touching it away.
>>     
>
>   
Yup, this is weird, but as I understand it illegal constructs produce 
undefined results, perhaps the fact GCC stored some LHS values but not 
others is just an example of "undefined".
>   i.e.  GCC doesn't properly analyze the code that it optimizes, so it
> generates the wrong optimizations.  GCC has a history of doing this...
>
>   
>> I'm not expert enough in the C99 standard to judge whether the comments
>> at the above URL are correct or not; I suspect it's a matter of some
>> controversy, and will back slowly away now... ;o)
>>     
>
>   I think your comments about the underlying cause are correct.
> Ulrich's comments about Posix are interesting:
>
>   
>>>> The invalid casts are forbidden by ISO C.  POSIX
>>>> does not and cannot guarantee anything about this type of use of
>>>> sockaddr_storage.
>>>>         
>
>   To quote the Opengroup page:
>
> http://www.opengroup.org/onlinepubs/009695399/basedefs/sys/socket.h.html
>
> ...
> The <sys/socket.h> header shall define the sockaddr_storage structure.
> This structure shall be:
>
>     * Large enough to accommodate all supported protocol-specific
> address structures
>
>     * Aligned at an appropriate boundary so that 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. the code in FreeRADIUS is correct.
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.

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.

Also note that the suggested union contains "a compatible member", which 
as I understand is necessary to satisfy the rules as well as providing 
enough information for the compiler.
> It uses sockaddr_stoage as a
> generic container for protocol-specific address structures.  It casts a
> pointer to sockaddr_storage to a pointer to protocol-specific address
> structures.  The recommendation to use a union to hold both
> sockaddr_storage && sockaddr_in is... interesting.  If you have to do
> that, WTF is the use of sockaddr_storage?
>
>   Looking on the net, I also found a fair amount of code using
> sockaddr_storage in the recommended Posix way.  It appears that
> FreeRADIUS is OK, and just got hit by a GCC bug.
>
>   i.e. for ANY code like this:
>
> 	foo = (type_t *) bar;
> 	switch (x) {
> 	case 1:
> 		foo->a = 1;
> 		break;
> 	...
> 	}
> 	function(&bar);
>
>   GCC will "optimize away" the line doing "foo->a = 1;".  Nice.
>
>   Alan DeKok.
> -
> List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html
>   
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. I expect I'll push a 2.0.4 
version of FreeRADIUS with full -O2 optimization into the F-9 
updates-testing stream with the patch. However, I'll wait a bit to see 
if others chime in with any other known problem areas first which should 
be folded into any such patch.

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?

-- 
John Dennis <jdennis at redhat.com>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freeradius.org/pipermail/freeradius-users/attachments/20080529/3032c73f/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: freeradius-sockaddr-alias.patch
URL: <http://lists.freeradius.org/pipermail/freeradius-users/attachments/20080529/3032c73f/attachment.ksh>