| Message ID | 20170418184058.04B574126BF74@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
LGTM. So I presume this patch covers all the internal sockets (as described in the patch), correct? On 18/04/2017 15:40, Florian Weimer wrote: > 2017-04-18 Florian Weimer <fweimer@redhat.com> > > [BZ #15722] > * resolv/res_hconf.c (_res_hconf_reorder_addrs): Create socket > with SOCK_CLOEXEC. > * resolv/res_send.c (send_vc, reopen): Likewise. > * sysdeps/posix/getaddrinfo.c (getaddrinfo): Likewise. > * sysdeps/unix/sysv/linux/check_native.c (__check_native): > Likewise. > * sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_open): Likewise. > * inet/rexec.c (rexec_af): Add comment. > * inet/rcmd.c (rresvport_af): Likewise. > > diff --git a/inet/rcmd.c b/inet/rcmd.c > index b7cc7a8..e43d4af 100644 > --- a/inet/rcmd.c > +++ b/inet/rcmd.c > @@ -383,6 +383,7 @@ rresvport_af (int *alport, sa_family_t family) > __set_errno (EAFNOSUPPORT); > return -1; > } > + /* NB: No SOCK_CLOXEC for backwards compatibility. */ Why maintaining backwards is important in this situation? > s = __socket(family, SOCK_STREAM, 0); > if (s < 0) > return -1; > diff --git a/inet/rexec.c b/inet/rexec.c > index 43fb67b..82e15ae 100644 > --- a/inet/rexec.c > +++ b/inet/rexec.c > @@ -86,6 +86,7 @@ rexec_af (char **ahost, int rport, const char *name, const char *pass, > } > ruserpass(res0->ai_canonname, &name, &pass); > retry: > + /* NB: No SOCK_CLOXEC for backwards compatibility. */ > s = __socket(res0->ai_family, res0->ai_socktype, 0); > if (s < 0) { > perror("rexec: socket"); > diff --git a/nis/nis_findserv.c b/nis/nis_findserv.c > index 77f3c7c..8e01164 100644 > --- a/nis/nis_findserv.c > +++ b/nis/nis_findserv.c > @@ -142,7 +142,7 @@ __nis_findfastest_with_timeout (dir_binding *bind, > } > > /* Create RPC handle */ > - sock = socket (AF_INET, SOCK_DGRAM, IPPROTO_UDP); > + sock = socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_UDP); > clnt = clntudp_create (&saved_sin, NIS_PROG, NIS_VERSION, *timeout, &sock); > if (clnt == NULL) > { > diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c > index d0d116d..8fc06e9 100644 > --- a/resolv/res_hconf.c > +++ b/resolv/res_hconf.c > @@ -388,7 +388,7 @@ _res_hconf_reorder_addrs (struct hostent *hp) > /* Initialize interface table. */ > > /* The SIOCGIFNETMASK ioctl will only work on an AF_INET socket. */ > - sd = __socket (AF_INET, SOCK_DGRAM, 0); > + sd = __socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); > if (sd < 0) > return; > > diff --git a/resolv/res_send.c b/resolv/res_send.c > index ffb9a6a..3de0b3e 100644 > --- a/resolv/res_send.c > +++ b/resolv/res_send.c > @@ -692,7 +692,8 @@ send_vc(res_state statp, > if (statp->_vcsock >= 0) > __res_iclose(statp, false); > > - statp->_vcsock = socket(nsap->sa_family, SOCK_STREAM, 0); > + statp->_vcsock = socket > + (nsap->sa_family, SOCK_STREAM | SOCK_CLOEXEC, 0); > if (statp->_vcsock < 0) { > *terrno = errno; > Perror(statp, stderr, "socket(vc)", errno); > @@ -902,14 +903,16 @@ reopen (res_state statp, int *terrno, int ns) > > /* only try IPv6 if IPv6 NS and if not failed before */ > if (nsap->sa_family == AF_INET6 && !statp->ipv6_unavail) { > - EXT(statp).nssocks[ns] > - = socket(PF_INET6, SOCK_DGRAM|SOCK_NONBLOCK, 0); > + EXT(statp).nssocks[ns] = socket > + (PF_INET6, > + SOCK_DGRAM | SOCK_NONBLOCK | SOCK_CLOEXEC, 0); > if (EXT(statp).nssocks[ns] < 0) > statp->ipv6_unavail = errno == EAFNOSUPPORT; > slen = sizeof (struct sockaddr_in6); > } else if (nsap->sa_family == AF_INET) { > - EXT(statp).nssocks[ns] > - = socket(PF_INET, SOCK_DGRAM|SOCK_NONBLOCK, 0); > + EXT(statp).nssocks[ns] = socket > + (PF_INET, > + SOCK_DGRAM | SOCK_NONBLOCK | SOCK_CLOEXEC, 0); > slen = sizeof (struct sockaddr_in); > } > if (EXT(statp).nssocks[ns] < 0) { > diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c > index eed7264..a55cc39 100644 > --- a/sysdeps/posix/getaddrinfo.c > +++ b/sysdeps/posix/getaddrinfo.c > @@ -2472,7 +2472,7 @@ getaddrinfo (const char *name, const char *service, > close_retry: > close_not_cancel_no_status (fd); > af = q->ai_family; > - fd = __socket (af, SOCK_DGRAM, IPPROTO_IP); > + fd = __socket (af, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_IP); > } > else > { > diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c > index 4a16460..7e5a7c9 100644 > --- a/sysdeps/unix/sysv/linux/check_native.c > +++ b/sysdeps/unix/sysv/linux/check_native.c > @@ -41,7 +41,7 @@ void > __check_native (uint32_t a1_index, int *a1_native, > uint32_t a2_index, int *a2_native) > { > - int fd = __socket (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE); > + int fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE); > > struct sockaddr_nl nladdr; > memset (&nladdr, '\0', sizeof (nladdr)); > diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c > index cff12c2..3bc9902 100644 > --- a/sysdeps/unix/sysv/linux/ifaddrs.c > +++ b/sysdeps/unix/sysv/linux/ifaddrs.c > @@ -255,7 +255,7 @@ __netlink_open (struct netlink_handle *h) > { > struct sockaddr_nl nladdr; > > - h->fd = __socket (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE); > + h->fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE); > if (h->fd < 0) > goto out; > >
On 04/18/2017 09:53 PM, Adhemerval Zanella wrote: > LGTM. So I presume this patch covers all the internal sockets (as described > in the patch), correct? As described in the bug? Unfortunately not. There are some holdouts in nss_nis, which may require some new (internal) Sun RPC interfaces, or a little bit of code duplication to create the socket within nss_nis instead. I didn't include that in this patch because these changes will be less obviously correct. > On 18/04/2017 15:40, Florian Weimer wrote: >> 2017-04-18 Florian Weimer <fweimer@redhat.com> >> >> [BZ #15722] >> * resolv/res_hconf.c (_res_hconf_reorder_addrs): Create socket >> with SOCK_CLOEXEC. >> * resolv/res_send.c (send_vc, reopen): Likewise. >> * sysdeps/posix/getaddrinfo.c (getaddrinfo): Likewise. >> * sysdeps/unix/sysv/linux/check_native.c (__check_native): >> Likewise. >> * sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_open): Likewise. >> * inet/rexec.c (rexec_af): Add comment. >> * inet/rcmd.c (rresvport_af): Likewise. >> >> diff --git a/inet/rcmd.c b/inet/rcmd.c >> index b7cc7a8..e43d4af 100644 >> --- a/inet/rcmd.c >> +++ b/inet/rcmd.c >> @@ -383,6 +383,7 @@ rresvport_af (int *alport, sa_family_t family) >> __set_errno (EAFNOSUPPORT); >> return -1; >> } >> + /* NB: No SOCK_CLOXEC for backwards compatibility. */ > > Why maintaining backwards is important in this situation? It's a legacy interface, so I don't think it's worth introducing a compat symbol and switch to O_CLOEXEC for these descriptors. The file descriptor is returned to the caller. Thanks, Florian
On 18/04/2017 17:09, Florian Weimer wrote: > On 04/18/2017 09:53 PM, Adhemerval Zanella wrote: >> LGTM. So I presume this patch covers all the internal sockets (as described >> in the patch), correct? > > As described in the bug? Unfortunately not. There are some holdouts in nss_nis, which may require some new (internal) Sun RPC interfaces, or a little bit of code duplication to create the socket within nss_nis instead. I didn't include that in this patch because these changes will be less obviously correct. Right, so I think we can't close this bug yet then. >> On 18/04/2017 15:40, Florian Weimer wrote: >>> 2017-04-18 Florian Weimer <fweimer@redhat.com> >>> >>> [BZ #15722] >>> * resolv/res_hconf.c (_res_hconf_reorder_addrs): Create socket >>> with SOCK_CLOEXEC. >>> * resolv/res_send.c (send_vc, reopen): Likewise. >>> * sysdeps/posix/getaddrinfo.c (getaddrinfo): Likewise. >>> * sysdeps/unix/sysv/linux/check_native.c (__check_native): >>> Likewise. >>> * sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_open): Likewise. >>> * inet/rexec.c (rexec_af): Add comment. >>> * inet/rcmd.c (rresvport_af): Likewise. >>> >>> diff --git a/inet/rcmd.c b/inet/rcmd.c >>> index b7cc7a8..e43d4af 100644 >>> --- a/inet/rcmd.c >>> +++ b/inet/rcmd.c >>> @@ -383,6 +383,7 @@ rresvport_af (int *alport, sa_family_t family) >>> __set_errno (EAFNOSUPPORT); >>> return -1; >>> } >>> + /* NB: No SOCK_CLOXEC for backwards compatibility. */ >> >> Why maintaining backwards is important in this situation? > > It's a legacy interface, so I don't think it's worth introducing a compat symbol and switch to O_CLOEXEC for these descriptors. The file descriptor is returned to the caller. > Right, it makes sense. > Thanks, > Florian
On Tue, 18 Apr 2017, Florian Weimer wrote: > diff --git a/inet/rcmd.c b/inet/rcmd.c > index b7cc7a8..e43d4af 100644 > --- a/inet/rcmd.c > +++ b/inet/rcmd.c > @@ -383,6 +383,7 @@ rresvport_af (int *alport, sa_family_t family) > __set_errno (EAFNOSUPPORT); > return -1; > } > + /* NB: No SOCK_CLOXEC for backwards compatibility. */ "CLOXEC" typo, repeated elsewhere in this patch.
diff --git a/inet/rcmd.c b/inet/rcmd.c index b7cc7a8..e43d4af 100644 --- a/inet/rcmd.c +++ b/inet/rcmd.c @@ -383,6 +383,7 @@ rresvport_af (int *alport, sa_family_t family) __set_errno (EAFNOSUPPORT); return -1; } + /* NB: No SOCK_CLOXEC for backwards compatibility. */ s = __socket(family, SOCK_STREAM, 0); if (s < 0) return -1; diff --git a/inet/rexec.c b/inet/rexec.c index 43fb67b..82e15ae 100644 --- a/inet/rexec.c +++ b/inet/rexec.c @@ -86,6 +86,7 @@ rexec_af (char **ahost, int rport, const char *name, const char *pass, } ruserpass(res0->ai_canonname, &name, &pass); retry: + /* NB: No SOCK_CLOXEC for backwards compatibility. */ s = __socket(res0->ai_family, res0->ai_socktype, 0); if (s < 0) { perror("rexec: socket"); diff --git a/nis/nis_findserv.c b/nis/nis_findserv.c index 77f3c7c..8e01164 100644 --- a/nis/nis_findserv.c +++ b/nis/nis_findserv.c @@ -142,7 +142,7 @@ __nis_findfastest_with_timeout (dir_binding *bind, } /* Create RPC handle */ - sock = socket (AF_INET, SOCK_DGRAM, IPPROTO_UDP); + sock = socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_UDP); clnt = clntudp_create (&saved_sin, NIS_PROG, NIS_VERSION, *timeout, &sock); if (clnt == NULL) { diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c index d0d116d..8fc06e9 100644 --- a/resolv/res_hconf.c +++ b/resolv/res_hconf.c @@ -388,7 +388,7 @@ _res_hconf_reorder_addrs (struct hostent *hp) /* Initialize interface table. */ /* The SIOCGIFNETMASK ioctl will only work on an AF_INET socket. */ - sd = __socket (AF_INET, SOCK_DGRAM, 0); + sd = __socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); if (sd < 0) return; diff --git a/resolv/res_send.c b/resolv/res_send.c index ffb9a6a..3de0b3e 100644 --- a/resolv/res_send.c +++ b/resolv/res_send.c @@ -692,7 +692,8 @@ send_vc(res_state statp, if (statp->_vcsock >= 0) __res_iclose(statp, false); - statp->_vcsock = socket(nsap->sa_family, SOCK_STREAM, 0); + statp->_vcsock = socket + (nsap->sa_family, SOCK_STREAM | SOCK_CLOEXEC, 0); if (statp->_vcsock < 0) { *terrno = errno; Perror(statp, stderr, "socket(vc)", errno); @@ -902,14 +903,16 @@ reopen (res_state statp, int *terrno, int ns) /* only try IPv6 if IPv6 NS and if not failed before */ if (nsap->sa_family == AF_INET6 && !statp->ipv6_unavail) { - EXT(statp).nssocks[ns] - = socket(PF_INET6, SOCK_DGRAM|SOCK_NONBLOCK, 0); + EXT(statp).nssocks[ns] = socket + (PF_INET6, + SOCK_DGRAM | SOCK_NONBLOCK | SOCK_CLOEXEC, 0); if (EXT(statp).nssocks[ns] < 0) statp->ipv6_unavail = errno == EAFNOSUPPORT; slen = sizeof (struct sockaddr_in6); } else if (nsap->sa_family == AF_INET) { - EXT(statp).nssocks[ns] - = socket(PF_INET, SOCK_DGRAM|SOCK_NONBLOCK, 0); + EXT(statp).nssocks[ns] = socket + (PF_INET, + SOCK_DGRAM | SOCK_NONBLOCK | SOCK_CLOEXEC, 0); slen = sizeof (struct sockaddr_in); } if (EXT(statp).nssocks[ns] < 0) { diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c index eed7264..a55cc39 100644 --- a/sysdeps/posix/getaddrinfo.c +++ b/sysdeps/posix/getaddrinfo.c @@ -2472,7 +2472,7 @@ getaddrinfo (const char *name, const char *service, close_retry: close_not_cancel_no_status (fd); af = q->ai_family; - fd = __socket (af, SOCK_DGRAM, IPPROTO_IP); + fd = __socket (af, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_IP); } else { diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c index 4a16460..7e5a7c9 100644 --- a/sysdeps/unix/sysv/linux/check_native.c +++ b/sysdeps/unix/sysv/linux/check_native.c @@ -41,7 +41,7 @@ void __check_native (uint32_t a1_index, int *a1_native, uint32_t a2_index, int *a2_native) { - int fd = __socket (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE); + int fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE); struct sockaddr_nl nladdr; memset (&nladdr, '\0', sizeof (nladdr)); diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c index cff12c2..3bc9902 100644 --- a/sysdeps/unix/sysv/linux/ifaddrs.c +++ b/sysdeps/unix/sysv/linux/ifaddrs.c @@ -255,7 +255,7 @@ __netlink_open (struct netlink_handle *h) { struct sockaddr_nl nladdr; - h->fd = __socket (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE); + h->fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE); if (h->fd < 0) goto out;