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 <[EMAIL PROTECTED]>
diff -u freeradius-server-2.0.4/src/lib/packet.c.orig
freeradius-server-2.0.4/src/lib/packet.c
--- freeradius-server-2.0.4/src/lib/packet.c.orig 2008-05-29
11:53:26.000000000 -0400
+++ freeradius-server-2.0.4/src/lib/packet.c 2008-05-29 12:13:41.000000000
-0400
@@ -175,8 +175,14 @@
int fr_socket(fr_ipaddr_t *ipaddr, int port)
{
int sockfd;
- struct sockaddr_storage salocal;
socklen_t salen;
+ union {
+ struct sockaddr sa;
+ struct sockaddr_storage ss;
+ struct sockaddr_in in4;
+ struct sockaddr_in6 in6;
+ } sa;
+
if ((port < 0) || (port > 65535)) {
librad_log("Port %d is out of allowed bounds", port);
@@ -198,25 +204,19 @@
}
#endif
- memset(&salocal, 0, sizeof(salocal));
+ memset(&sa, 0, sizeof(sa));
if (ipaddr->af == AF_INET) {
- struct sockaddr_in *sa;
-
- sa = (struct sockaddr_in *) &salocal;
- sa->sin_family = AF_INET;
- sa->sin_addr = ipaddr->ipaddr.ip4addr;
- sa->sin_port = htons((uint16_t) port);
- salen = sizeof(*sa);
+ sa.in4.sin_family = AF_INET;
+ sa.in4.sin_addr = ipaddr->ipaddr.ip4addr;
+ sa.in4.sin_port = htons((uint16_t) port);
+ salen = sizeof(sa.in4);
#ifdef HAVE_STRUCT_SOCKADDR_IN6
} else if (ipaddr->af == AF_INET6) {
- struct sockaddr_in6 *sa;
-
- sa = (struct sockaddr_in6 *) &salocal;
- sa->sin6_family = AF_INET6;
- sa->sin6_addr = ipaddr->ipaddr.ip6addr;
- sa->sin6_port = htons((uint16_t) port);
- salen = sizeof(*sa);
+ sa.in6.sin6_family = AF_INET6;
+ sa.in6.sin6_addr = ipaddr->ipaddr.ip6addr;
+ sa.in6.sin6_port = htons((uint16_t) port);
+ salen = sizeof(sa.in6);
#if 1
/*
@@ -240,7 +240,7 @@
return sockfd; /* don't bind it */
}
- if (bind(sockfd, (struct sockaddr *) &salocal, salen) < 0) {
+ if (bind(sockfd, &sa.sa, salen) < 0) {
close(sockfd);
return -1;
}
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html
