Create more sockets with SOCK_CLOEXEC [BZ #15722]

Message ID 20170418184058.04B574126BF74@oldenburg.str.redhat.com
State New
Headers show

Commit Message

Florian Weimer April 18, 2017, 6:40 p.m.
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.

Comments

Adhemerval Zanella April 18, 2017, 7:53 p.m. | #1
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;
>  
>
Florian Weimer April 18, 2017, 8:09 p.m. | #2
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
Adhemerval Zanella April 18, 2017, 8:28 p.m. | #3
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
Joseph Myers April 19, 2017, 3:19 p.m. | #4
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.

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;