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

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@redhat.com>