| Message ID | 1432139240.16668.77.camel@ubuntu-sellcey |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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*.
As I said before, adding the #include in that public header is not right.
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
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.
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?
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
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.)
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
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.
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
> 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.
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 */