Fix strict-aliasing warning in resolv/res_hconf.c

  On Tue, 2015-05-19 at 15:12 -0700, Paul Eggert wrote:
> On 05/19/2015 02:09 PM, Steve Ellcey wrote:
> > It uses the same casts as before but splits up the assignment into
> > two parts and that seems to be sufficient to get rid of the GCC
> > warning.
> 
> It's warning about a portability problem that seems to be genuine. Can't 
> we fix the problem using a union as before?  That should be better than 
> trying to fool GCC into not warning about the problem.

OK, how is this version with no casts.

Steve Ellcey
sellcey@imgtec.com



2015-05-19  Steve Ellcey  <sellcey@imgtec.com>

	* resolv/res_hconf.c (_res_hconf_reorder_addrs): Split up assignment
	and use union to avoid GCC strict aliasing warning.
  

On 05/19/2015 03:53 PM, Steve Ellcey wrote:
> +      union
> +	{
> +	struct sockaddr *sa;
> +	struct sockaddr_in *sin;
> +	} ss;
>         int sd, num, i;
>         /* Save errno.  */
>         int save = errno;
> @@ -443,14 +448,14 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>   		continue;
>   
>   	      ifaddrs[new_num_ifs].addrtype = AF_INET;
> -	      ifaddrs[new_num_ifs].u.ipv4.addr =
> -		((struct sockaddr_in *) &cur_ifr->ifr_addr)->sin_addr.s_addr;
> +	      ss.sa = &cur_ifr->ifr_addr;
> +	      ifaddrs[new_num_ifs].u.ipv4.addr = ss.sin->sin_addr.s_addr;

I'm afraid that's no better than casting.  It needs to be a union of 
contents, not of pointers.
  

On Tue, 2015-05-19 at 16:04 -0700, Paul Eggert wrote:
> On 05/19/2015 03:53 PM, Steve Ellcey wrote:
> > +      union
> > +	{
> > +	struct sockaddr *sa;
> > +	struct sockaddr_in *sin;
> > +	} ss;
> >         int sd, num, i;
> >         /* Save errno.  */
> >         int save = errno;
> > @@ -443,14 +448,14 @@ _res_hconf_reorder_addrs (struct hostent *hp)
> >   		continue;
> >   
> >   	      ifaddrs[new_num_ifs].addrtype = AF_INET;
> > -	      ifaddrs[new_num_ifs].u.ipv4.addr =
> > -		((struct sockaddr_in *) &cur_ifr->ifr_addr)->sin_addr.s_addr;
> > +	      ss.sa = &cur_ifr->ifr_addr;
> > +	      ifaddrs[new_num_ifs].u.ipv4.addr = ss.sin->sin_addr.s_addr;
> 
> I'm afraid that's no better than casting.  It needs to be a union of 
> contents, not of pointers.

Hm, I am not sure I know how to do that.  I tried changing
sysdeps/gnu/net/if.h to include a 'struct sockaddr_in ifru_addr_in;'
entry in the ifru_data union but that won't compile.

In file included from ../include/net/if.h:3:0,
                 from ../sysdeps/generic/ifreq.h:22,
                 from ../sysdeps/unix/sysv/linux/ifreq.c:19:
../sysdeps/gnu/net/if.h:142:21: error: field 'ifru_addr_in' has
incomplete type
  struct sockaddr_in ifru_addr_in;

I am not sure if I am just missing an include or if I simply cannot use
the sockaddr_in struct in this header.

Steve Ellcey
sellcey@imgtec.com
  

> I am not sure if I am just missing an include or if I simply cannot use
> the sockaddr_in struct in this header.

You cannot.  It's a public header and it does not (and should not) define
any of the AF-specific struct sockaddr_foo types.  I don't think this
interface can be used in a strictly standard-C-compliant fashion.  We need
to come up with an idiom or helper code to facilitate using it in whatever
fashion is closest to compliant and is in fact thoroughly safe.  Maybe Paul
has some ideas.
  

Roland McGrath wrote:
> We need
> to come up with an idiom or helper code to facilitate using it in whatever
> fashion is closest to compliant and is in fact thoroughly safe.  Maybe Paul
> has some ideas.

How about __attribute__((__may_alias__))?  In general it's better to avoid that 
attribute, but this may be one of the places where it's unavoidable.
  

On 05/20/2015 01:58 AM, Roland McGrath wrote:
>> I am not sure if I am just missing an include or if I simply cannot use
>> the sockaddr_in struct in this header.
> 
> You cannot.  It's a public header and it does not (and should not) define
> any of the AF-specific struct sockaddr_foo types.  I don't think this
> interface can be used in a strictly standard-C-compliant fashion.  We need
> to come up with an idiom or helper code to facilitate using it in whatever
> fashion is closest to compliant and is in fact thoroughly safe.

If the effective type is correct, then using pointer arithmetic to get
from the wrongly typed pointer to the field may help.  See the
_IO_CAST_FIELD_ACCESS macro here:

  <https://sourceware.org/ml/libc-alpha/2015-05/msg00326.html>

But I doubt the effective type is correct here because a generic socket
address is used, which has padding in the form of a char array (which is
not untyped, the char * aliasing rule works only in the opposite direction).

Looking at struct ifreq, it is rather mysterious to me how this is
supposed to work at all.  I mean, struct sockaddr has just 14 bytes
storage for address information, but IPv6 addresses need 16 bytes, and
socket addresses contain even more information than a raw address.
  

Florian Weimer <fweimer@redhat.com> writes:

> Looking at struct ifreq, it is rather mysterious to me how this is
> supposed to work at all.  I mean, struct sockaddr has just 14 bytes
> storage for address information, but IPv6 addresses need 16 bytes, and
> socket addresses contain even more information than a raw address.

This ioctl is only defined for IPv4.

Andreas.