[PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

Here is a patch to clean up the strict-aliasing warning that we get
when compiling resolv/res_hconf.c with the latest top-of-tree GCC.
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.

Is this OK to checkin?

Steve Ellcey
sellcey@imgtec.com


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

	* resolv/res_hconf.c (_res_hconf_reorder_addrs): Split up assignments
	to avoid GCC strict aliasing warning.


diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
index 73942e8..c1c542a 100644
--- a/resolv/res_hconf.c
+++ b/resolv/res_hconf.c
@@ -407,6 +407,7 @@ _res_hconf_reorder_addrs (struct hostent *hp)
   if (num_ifs <= 0)
     {
       struct ifreq *ifr, *cur_ifr;
+      struct sockaddr_in *sin;
       int sd, num, i;
       /* Save errno.  */
       int save = errno;
@@ -443,14 +444,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;
+	      sin = (struct sockaddr_in *) &cur_ifr->ifr_addr;
+	      ifaddrs[new_num_ifs].u.ipv4.addr = sin->sin_addr.s_addr;
 
 	      if (__ioctl (sd, SIOCGIFNETMASK, cur_ifr) < 0)
 		continue;
 
-	      ifaddrs[new_num_ifs].u.ipv4.mask =
-		((struct sockaddr_in *) &cur_ifr->ifr_netmask)->sin_addr.s_addr;
+	      sin = (struct sockaddr_in *) &cur_ifr->ifr_netmask;
+	      ifaddrs[new_num_ifs].u.ipv4.mask = sin->sin_addr.s_addr;
 
 	      /* Now we're committed to this entry.  */
 	      ++new_num_ifs;

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Paul Eggert]

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.

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

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.


diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
index 73942e8..0dbee2e 100644
--- a/resolv/res_hconf.c
+++ b/resolv/res_hconf.c
@@ -407,6 +407,11 @@ _res_hconf_reorder_addrs (struct hostent *hp)
   if (num_ifs <= 0)
     {
       struct ifreq *ifr, *cur_ifr;
+      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;
 
 	      if (__ioctl (sd, SIOCGIFNETMASK, cur_ifr) < 0)
 		continue;
 
-	      ifaddrs[new_num_ifs].u.ipv4.mask =
-		((struct sockaddr_in *) &cur_ifr->ifr_netmask)->sin_addr.s_addr;
+	      ss.sa = &cur_ifr->ifr_netmask;
+	      ifaddrs[new_num_ifs].u.ipv4.mask = ss.sin->sin_addr.s_addr;
 
 	      /* Now we're committed to this entry.  */
 	      ++new_num_ifs;

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Paul Eggert]

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.

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

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

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Roland McGrath]

> 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.

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Paul Eggert]

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.

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

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 / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

On 05/20/2015 10:23 AM, Andreas Schwab wrote:
> 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.

Oh, but then we can add a union member of the appropriate type (just one
is needed):

diff --git a/sysdeps/gnu/net/if.h b/sysdeps/gnu/net/if.h
index 49a048c..39f40de 100644
--- a/sysdeps/gnu/net/if.h
+++ b/sysdeps/gnu/net/if.h
@@ -24,6 +24,7 @@
 #ifdef __USE_MISC
 # include <sys/types.h>
 # include <sys/socket.h>
+# include <netinet/in.h>
 #endif


@@ -139,6 +140,7 @@ struct ifreq
 	struct sockaddr ifru_broadaddr;
 	struct sockaddr ifru_netmask;
 	struct sockaddr ifru_hwaddr;
+	struct sockaddr_in ifru_addr_in;
 	short int ifru_flags;
 	int ifru_ivalue;
 	int ifru_mtu;


This doesn't change ABI.  And then the code in resolv/res_hconf.c could
use that new member, without any casts.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

On Wed, 2015-05-20 at 10:42 +0200, Florian Weimer wrote:
> On 05/20/2015 10:23 AM, Andreas Schwab wrote:
> > 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.
> 
> Oh, but then we can add a union member of the appropriate type (just one
> is needed):
> 
> diff --git a/sysdeps/gnu/net/if.h b/sysdeps/gnu/net/if.h
> index 49a048c..39f40de 100644
> --- a/sysdeps/gnu/net/if.h
> +++ b/sysdeps/gnu/net/if.h
> @@ -24,6 +24,7 @@
>  #ifdef __USE_MISC
>  # include <sys/types.h>
>  # include <sys/socket.h>
> +# include <netinet/in.h>
>  #endif
> 
> 
> @@ -139,6 +140,7 @@ struct ifreq
>  	struct sockaddr ifru_broadaddr;
>  	struct sockaddr ifru_netmask;
>  	struct sockaddr ifru_hwaddr;
> +	struct sockaddr_in ifru_addr_in;
>  	short int ifru_flags;
>  	int ifru_ivalue;
>  	int ifru_mtu;
> 
> 
> This doesn't change ABI.  And then the code in resolv/res_hconf.c could
> use that new member, without any casts.

I don't know if this change is going to be considered acceptable or not
but here is a complete patch with the new union member, a macro
definition to access it (in order to match the other union members) and
the needed change to resolv/res_hconf.c.

Steve Ellcey
sellcey@imgtec.com


2015-05-20  Steve Ellcey  <sellcey@imgtec.com>
	    Florian Weimer  <fweimer@redhat.com>

	* resolv/res_hconf.c (_res_hconf_reorder_addrs): Use new ifr_addr_in
	name to access address.
	* sysdeps/gnu/net/if.h (struct ifreq): Add new ifru_addr_in union
	member.
	(ifr_addr_in): New macro.


diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
index 73942e8..3b05287 100644
--- a/resolv/res_hconf.c
+++ b/resolv/res_hconf.c
@@ -444,13 +444,13 @@ _res_hconf_reorder_addrs (struct hostent *hp)
 
 	      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;
+		cur_ifr->ifr_addr_in.sin_addr.s_addr;
 
 	      if (__ioctl (sd, SIOCGIFNETMASK, cur_ifr) < 0)
 		continue;
 
 	      ifaddrs[new_num_ifs].u.ipv4.mask =
-		((struct sockaddr_in *) &cur_ifr->ifr_netmask)->sin_addr.s_addr;
+		cur_ifr->ifr_addr_in.sin_addr.s_addr;
 
 	      /* Now we're committed to this entry.  */
 	      ++new_num_ifs;
diff --git a/sysdeps/gnu/net/if.h b/sysdeps/gnu/net/if.h
index 49a048c..b741d14 100644
--- a/sysdeps/gnu/net/if.h
+++ b/sysdeps/gnu/net/if.h
@@ -24,6 +24,7 @@
 #ifdef __USE_MISC
 # include <sys/types.h>
 # include <sys/socket.h>
+# include <netinet/in.h>
 #endif
 
 
@@ -139,6 +140,7 @@ struct ifreq
 	struct sockaddr ifru_broadaddr;
 	struct sockaddr ifru_netmask;
 	struct sockaddr ifru_hwaddr;
+	struct sockaddr_in ifru_addr_in;
 	short int ifru_flags;
 	int ifru_ivalue;
 	int ifru_mtu;
@@ -151,6 +153,7 @@ struct ifreq
 # define ifr_name	ifr_ifrn.ifrn_name	/* interface name 	*/
 # define ifr_hwaddr	ifr_ifru.ifru_hwaddr	/* MAC address 		*/
 # define ifr_addr	ifr_ifru.ifru_addr	/* address		*/
+# define ifr_addr_in	ifr_ifru.ifru_addr_in	/* sockaddr_in address	*/
 # define ifr_dstaddr	ifr_ifru.ifru_dstaddr	/* other end of p-p lnk	*/
 # define ifr_broadaddr	ifr_ifru.ifru_broadaddr	/* broadcast address	*/
 # define ifr_netmask	ifr_ifru.ifru_netmask	/* interface net mask	*/

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

On 05/20/2015 06:27 PM, Steve Ellcey wrote:

> I don't know if this change is going to be considered acceptable or not
> but here is a complete patch with the new union member, a macro
> definition to access it (in order to match the other union members) and
> the needed change to resolv/res_hconf.c.

It would be more conservative to drop the #define (due to the lack of
scope for preprocessor macros).  Maybe also add a comment to header
saying that application code should use the ifru_addr_in member, not the
other struct sockaddr members due to C aliasing issues.  Application
will run into the same issue, the existing definition was likely
impossible to use correctly.  This is the reason why I suggest not to
add a __ prefix to the ifru_addr_in member.

But from a API risk perspective, adding the member is fine—I think,
others might disagree.  There is no ABI risk because of the existing
padding in struct sockaddr.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

On Fri, 2015-05-22 at 13:34 +0200, Florian Weimer wrote:
> On 05/20/2015 06:27 PM, Steve Ellcey wrote:
> 
> > I don't know if this change is going to be considered acceptable or not
> > but here is a complete patch with the new union member, a macro
> > definition to access it (in order to match the other union members) and
> > the needed change to resolv/res_hconf.c.
> 
> It would be more conservative to drop the #define (due to the lack of
> scope for preprocessor macros).  Maybe also add a comment to header
> saying that application code should use the ifru_addr_in member, not the
> other struct sockaddr members due to C aliasing issues.  Application
> will run into the same issue, the existing definition was likely
> impossible to use correctly.  This is the reason why I suggest not to
> add a __ prefix to the ifru_addr_in member.
> 
> But from a API risk perspective, adding the member is fineВ—I think,
> others might disagree.  There is no ABI risk because of the existing
> padding in struct sockaddr.

Skipping the macro may be more conservative but if we think users are
going to need or want to use the ifru_addr member to avoid the strict
aliasing warnings then not having the macro there seems awkward by
making that field different than all the rest.

I would like to get a fix for this checked in, this is the last of the
glibc problems I am having when building with the top-of-tree GCC.

Steve Ellcey
sellcey@imgtec.com

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

On 05/26/2015 08:47 PM, Steve Ellcey wrote:
> On Fri, 2015-05-22 at 13:34 +0200, Florian Weimer wrote:
>> On 05/20/2015 06:27 PM, Steve Ellcey wrote:
>>
>>> I don't know if this change is going to be considered acceptable or not
>>> but here is a complete patch with the new union member, a macro
>>> definition to access it (in order to match the other union members) and
>>> the needed change to resolv/res_hconf.c.
>>
>> It would be more conservative to drop the #define (due to the lack of
>> scope for preprocessor macros).  Maybe also add a comment to header
>> saying that application code should use the ifru_addr_in member, not the
>> other struct sockaddr members due to C aliasing issues.  Application
>> will run into the same issue, the existing definition was likely
>> impossible to use correctly.  This is the reason why I suggest not to
>> add a __ prefix to the ifru_addr_in member.
>>
>> But from a API risk perspective, adding the member is fineВ—I think,
>> others might disagree.  There is no ABI risk because of the existing
>> padding in struct sockaddr.
> 
> Skipping the macro may be more conservative but if we think users are
> going to need or want to use the ifru_addr member to avoid the strict
> aliasing warnings then not having the macro there seems awkward by
> making that field different than all the rest.

Okay, seems reasonable.  But please add a comment about the aliasing issue.

> I would like to get a fix for this checked in, this is the last of the
> glibc problems I am having when building with the top-of-tree GCC.

I think it's okay, but better wait a one or two days to see if anyone
else has objections.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Pedro Alves]

On 05/22/2015 12:34 PM, Florian Weimer wrote:
> 
> But from a API risk perspective, adding the member is fine—I think,
> others might disagree.  There is no ABI risk because of the existing
> padding in struct sockaddr.

Is the union's alignment before/after the same?

Thanks,
Pedro Alves

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

On 05/26/2015 09:16 PM, Pedro Alves wrote:
> On 05/22/2015 12:34 PM, Florian Weimer wrote:
>>
>> But from a API risk perspective, adding the member is fine—I think,
>> others might disagree.  There is no ABI risk because of the existing
>> padding in struct sockaddr.
> 
> Is the union's alignment before/after the same?

Good point.

As far as I can tell, the alignment inside struct ifaddrs does not
change because there is an unsigned int member which forces 4 byte
alignment.  Otherwise the interface would not have worked on
strict-alignment architectures because struct sockaddr lacks an
alignment specification.  Wow.  This is stuff is *broken*.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Roland McGrath]

As I said before, adding the #include in that public header is not right.

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

On Tue, 2015-05-26 at 14:56 -0700, Roland McGrath wrote:
> As I said before, adding the #include in that public header is not right.

In that case the two solutions I see are to compile res_hconf.c with
-fno-strict-aliasing or to put DIAG_PUSH_NEEDS_COMMENT, etc. macros
around the two casts in res_hconf.c to turn off the strict aliasing
warnings.  Using -fno-strict-aliasing seems safer as I do not know
(based on my own knowledge) that the casts in question do not cause any
problems when compiled with strict aliasing.  How about this patch?

Steve Ellcey
sellcey@imgtec.com



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

	* resolv/Makefile: Use -fno-strict-aliasing when compiling res_hconf.c.



diff --git a/resolv/Makefile b/resolv/Makefile
index f62eea4..453046c 100644
--- a/resolv/Makefile
+++ b/resolv/Makefile
@@ -85,7 +85,12 @@ CPPFLAGS += -Dgethostbyname=res_gethostbyname \
 ifeq (yes,$(have-ssp))
 CFLAGS-libresolv += -fstack-protector
 endif
-CFLAGS-res_hconf.c = -fexceptions
+
+# The casts of cur_ifr->ifr_addr and cur_ifr->ifr_netmask in
+# _res_hconf_reorder_addrs to pointers to sockaddr_in type break
+# GCC's strict aliasing rules and cause GCC 6 to generate a
+# warning unless -fno-strict-aliasing is used.
+CFLAGS-res_hconf.c = -fexceptions -fno-strict-aliasing
 
 # The BIND code elicits some harmless warnings.
 +cflags += -Wno-strict-prototypes -Wno-write-strings

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Pedro Alves]

On 05/20/2015 05:27 PM, Steve Ellcey wrote:

> diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
> index 73942e8..3b05287 100644
> --- a/resolv/res_hconf.c
> +++ b/resolv/res_hconf.c
> @@ -444,13 +444,13 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>  
>  	      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;
> +		cur_ifr->ifr_addr_in.sin_addr.s_addr;

Without adding a new union member, isn't the simplest to just take a
copy step?  The interface already clearly assumes that a sockaddr_in
fits in a sockaddr.  Something like:

              union
                {
                  struct sockaddr sa;
                  struct sockaddr_in sin;
                } ss;

-  	      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;

etc.  Maybe the compiler even elides the copying.  (And if it doesn't,
would it matter here?)

Thanks,
Pedro Alves

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

On 05/27/2015 11:19 AM, Pedro Alves wrote:
> On 05/20/2015 05:27 PM, Steve Ellcey wrote:
> 
>> diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
>> index 73942e8..3b05287 100644
>> --- a/resolv/res_hconf.c
>> +++ b/resolv/res_hconf.c
>> @@ -444,13 +444,13 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>>  
>>  	      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;
>> +		cur_ifr->ifr_addr_in.sin_addr.s_addr;
> 
> Without adding a new union member, isn't the simplest to just take a
> copy step?

I think it's still undefined behavior.

> etc.  Maybe the compiler even elides the copying.

If it does, we are back to square one, I fear.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

On 05/26/2015 11:56 PM, Roland McGrath wrote:
> As I said before, adding the #include in that public header is not right.

What do you recommend to get the definition of struct sockaddr_in
instead?  Or is there no way to fix this because we can't define the
struct by including this header?

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Pedro Alves]

On 05/27/2015 11:50 AM, Florian Weimer wrote:
> On 05/27/2015 11:19 AM, Pedro Alves wrote:
>> On 05/20/2015 05:27 PM, Steve Ellcey wrote:
>>
>>> diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
>>> index 73942e8..3b05287 100644
>>> --- a/resolv/res_hconf.c
>>> +++ b/resolv/res_hconf.c
>>> @@ -444,13 +444,13 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>>>  
>>>  	      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;
>>> +		cur_ifr->ifr_addr_in.sin_addr.s_addr;
>>
>> Without adding a new union member, isn't the simplest to just take a
>> copy step?
> 
> I think it's still undefined behavior.

How so?  AFAIK, it's implementation defined, and GCC allows type-punning provided
the memory is accessed through the union type.

 https://gcc.gnu.org/onlinedocs/gcc-5.1.0/gcc/Optimize-Options.html#Type-punning

> 
>> etc.  Maybe the compiler even elides the copying.
> 
> If it does, we are back to square one, I fear.
> 


Thanks,
Pedro Alves

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

On 05/27/2015 01:09 PM, Pedro Alves wrote:
> On 05/27/2015 11:50 AM, Florian Weimer wrote:
>> On 05/27/2015 11:19 AM, Pedro Alves wrote:
>>> On 05/20/2015 05:27 PM, Steve Ellcey wrote:
>>>
>>>> diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
>>>> index 73942e8..3b05287 100644
>>>> --- a/resolv/res_hconf.c
>>>> +++ b/resolv/res_hconf.c
>>>> @@ -444,13 +444,13 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>>>>  
>>>>  	      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;
>>>> +		cur_ifr->ifr_addr_in.sin_addr.s_addr;
>>>
>>> Without adding a new union member, isn't the simplest to just take a
>>> copy step?
>>
>> I think it's still undefined behavior.
> 
> How so?  AFAIK, it's implementation defined, and GCC allows type-punning provided
> the memory is accessed through the union type.
> 
>  https://gcc.gnu.org/onlinedocs/gcc-5.1.0/gcc/Optimize-Options.html#Type-punning

You mean, copy it to a union member of type struct sockaddr, and reading
from that union as struct sockaddr_in?

(Based on my reading of the standard, memcpy does not change effective
type if one has been assigned.)

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Pedro Alves]

On 05/27/2015 12:16 PM, Florian Weimer wrote:
> On 05/27/2015 01:09 PM, Pedro Alves wrote:
>> On 05/27/2015 11:50 AM, Florian Weimer wrote:
>>> On 05/27/2015 11:19 AM, Pedro Alves wrote:
>>>> On 05/20/2015 05:27 PM, Steve Ellcey wrote:
>>>>
>>>>> diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
>>>>> index 73942e8..3b05287 100644
>>>>> --- a/resolv/res_hconf.c
>>>>> +++ b/resolv/res_hconf.c
>>>>> @@ -444,13 +444,13 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>>>>>  
>>>>>  	      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;
>>>>> +		cur_ifr->ifr_addr_in.sin_addr.s_addr;
>>>>
>>>> Without adding a new union member, isn't the simplest to just take a
>>>> copy step?
>>>
>>> I think it's still undefined behavior.
>>
>> How so?  AFAIK, it's implementation defined, and GCC allows type-punning provided
>> the memory is accessed through the union type.
>>
>>  https://gcc.gnu.org/onlinedocs/gcc-5.1.0/gcc/Optimize-Options.html#Type-punning
> 
> You mean, copy it to a union member of type struct sockaddr, and reading
> from that union as struct sockaddr_in?

Yes, just like the patchlet did:

+             ss.sa = cur_ifr->ifr_addr;
+	      ifaddrs[new_num_ifs].u.ipv4.addr = ss.sin.sin_addr.s_addr;

That was implementation defined in C90 (6.3.2.3) "A member of a union object is
accessed using a member of a different type", and blessed in C99/TC3, 6.5.2.3,
footnote 80:

 "If the member used to access the contents of a union object
 is not the same as the member last used to store a value in the object, the
 appropriate part of the object representation of the value is reinterpreted
 as an object representation in the new type as described in 6.2.6 (a
 process sometimes called "type punning")."

And given that sockaddr_in contains explicit padding and the sizes of the
objects match, C99+TC3 6.2.6.1 points 6 and 7 do not apply.

> 
> (Based on my reading of the standard, memcpy does not change effective
> type if one has been assigned.)

Thanks,
Pedro Alves

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Florian Weimer]

On 05/27/2015 11:19 AM, Pedro Alves wrote:

> Without adding a new union member, isn't the simplest to just take a
> copy step?  The interface already clearly assumes that a sockaddr_in
> fits in a sockaddr.  Something like:
> 
>               union
>                 {
>                   struct sockaddr sa;
>                   struct sockaddr_in sin;
>                 } ss;
> 
> -  	      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;
> 
> etc.

I misread this proposal.  I now think this is a possible fix, and a very
conservative one at that.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

OK, so here is a complete patch based on Pedro's proposal.  It compiles
fine for me on MIPS with the latest GCC.  OK to checkin?

Steve Ellcey
sellcey@imgtec.com


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

	* resolv/res_hconf.c (_res_hconf_reorder_addrs): Use a union to
	copy data from cur_ifr->ifr_addr.


diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
index 73942e8..b9c423e 100644
--- a/resolv/res_hconf.c
+++ b/resolv/res_hconf.c
@@ -410,6 +410,11 @@ _res_hconf_reorder_addrs (struct hostent *hp)
       int sd, num, i;
       /* Save errno.  */
       int save = errno;
+      union
+      {
+	struct sockaddr sa;
+	struct sockaddr_in sin;
+      } ss;
 
       /* Initialize interface table.  */
 
@@ -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;
 
 	      if (__ioctl (sd, SIOCGIFNETMASK, cur_ifr) < 0)
 		continue;
 
-	      ifaddrs[new_num_ifs].u.ipv4.mask =
-		((struct sockaddr_in *) &cur_ifr->ifr_netmask)->sin_addr.s_addr;
+	      ss.sa = cur_ifr->ifr_netmask;
+	      ifaddrs[new_num_ifs].u.ipv4.mask = ss.sin.sin_addr.s_addr;
 
 	      /* Now we're committed to this entry.  */
 	      ++new_num_ifs;

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Pedro Alves]

On 05/28/2015 05:00 PM, Steve Ellcey wrote:
> OK, so here is a complete patch based on Pedro's proposal.  It compiles
> fine for me on MIPS with the latest GCC.  OK to checkin?

Looks good to me, FWIW.

Thanks,
Pedro Alves

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Roland McGrath]

Can you verify what effect this has on the compiled code on x86_64?

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

On Thu, 2015-05-28 at 13:10 -0700, Roland McGrath wrote:
> Can you verify what effect this has on the compiled code on x86_64?

I could, but I would rather not since I normally only build glibc for
MIPS and it always takes me a while to figure out the options and
settings for an x86 build.  I did a build using GCC 4.9.2 for MIPS and
did not see any significant code differences with this patch (i.e. I did
not see an extra data copy on MIPS).

Steve Ellcey
sellcey@imgtec.com

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Roland McGrath]

> I could, but I would rather not since I normally only build glibc for
> MIPS and it always takes me a while to figure out the options and
> settings for an x86 build.  I did a build using GCC 4.9.2 for MIPS and
> did not see any significant code differences with this patch (i.e. I did
> not see an extra data copy on MIPS).

OK, that's good enough for me to assume that there won't be any extra data
copy on any machine with a reasonable compiler.  (I actually asked about
x86_64 specifically because I wouldn't care about the MIPS code being
suboptimal as long as the x86 code was not.)

The only other thing I'd say about the patch is that the temporary
variable should be declared in the innermost possible scope.


Thanks,
Roland

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

On Thu, 2015-05-28 at 15:57 -0700, Roland McGrath wrote:
> > I could, but I would rather not since I normally only build glibc for
> > MIPS and it always takes me a while to figure out the options and
> > settings for an x86 build.  I did a build using GCC 4.9.2 for MIPS and
> > did not see any significant code differences with this patch (i.e. I did
> > not see an extra data copy on MIPS).
> 
> OK, that's good enough for me to assume that there won't be any extra data
> copy on any machine with a reasonable compiler.  (I actually asked about
> x86_64 specifically because I wouldn't care about the MIPS code being
> suboptimal as long as the x86 code was not.)
> 
> The only other thing I'd say about the patch is that the temporary
> variable should be declared in the innermost possible scope.
> 
> 
> Thanks,
> Roland

OK, moving the declaration is easy enough to do, I will put it in the
for loop where it is used.

Steve Ellcey
sellcey@imgtec.com


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

	* resolv/res_hconf.c (_res_hconf_reorder_addrs): Use a union to
	copy data from cur_ifr->ifr_addr.

diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
index 73942e8..b9c229d 100644
--- a/resolv/res_hconf.c
+++ b/resolv/res_hconf.c
@@ -439,18 +439,24 @@ _res_hconf_reorder_addrs (struct hostent *hp)
 	  for (cur_ifr = ifr, i = 0; i < num;
 	       cur_ifr = __if_nextreq (cur_ifr), ++i)
 	    {
+	      union
+	      {
+		struct sockaddr sa;
+		struct sockaddr_in sin;
+	      } ss;
+
 	      if (cur_ifr->ifr_addr.sa_family != AF_INET)
 		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;
 
 	      if (__ioctl (sd, SIOCGIFNETMASK, cur_ifr) < 0)
 		continue;
 
-	      ifaddrs[new_num_ifs].u.ipv4.mask =
-		((struct sockaddr_in *) &cur_ifr->ifr_netmask)->sin_addr.s_addr;
+	      ss.sa = cur_ifr->ifr_netmask;
+	      ifaddrs[new_num_ifs].u.ipv4.mask = ss.sin.sin_addr.s_addr;
 
 	      /* Now we're committed to this entry.  */
 	      ++new_num_ifs;

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Roland McGrath]

> 	* resolv/res_hconf.c (_res_hconf_reorder_addrs): Use a union to
> 	copy data from cur_ifr->ifr_addr.
                       CUR_IFR->ifr_addr and CUR_IFR->ifr_netmask.

OK to commit with the log fix.


Thanks,
Roland

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Steve Ellcey]

On Thu, 2015-05-28 at 16:43 -0700, Roland McGrath wrote:
> > 	* resolv/res_hconf.c (_res_hconf_reorder_addrs): Use a union to
> > 	copy data from cur_ifr->ifr_addr.
>                        CUR_IFR->ifr_addr and CUR_IFR->ifr_netmask.
> 
> OK to commit with the log fix.
> 
> 
> Thanks,
> Roland

Is there a reason you want CUR_IFR in upper case?  It is not that way in
the code.

Steve Ellcey
sellcey@imgtec.com

Re: [PATCH] Fix strict-aliasing warning in resolv/res_hconf.c

[Roland McGrath]

> Is there a reason you want CUR_IFR in upper case?  It is not that way in
> the code.

The convention for comments and log entries is to put local variable and
parameter names in upper case.