FreeRADIUS 2 not listening on right port

[Alan DeKok]

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.