Re: BZ#15722: create all sockets with SOCK_CLOEXEC

[Roland McGrath <roland at hack dot frob dot com>]

> 	* NEWS: Updated.

There is no NEWS change in your patch.  If the only change was adding to
the list of fixed BZ#s, that does not get mentioned in ChangeLog.

> --- /dev/null
> +++ b/include/socket-cloexec.h
> @@ -0,0 +1,69 @@
> +/* Copyright (C) 2014 Free Software Foundation, Inc.

Top line of every new file must be a descriptive comment.

> +/* Like socket, but with SOCK_CLOEXEC set if available.  If it's not,
> +   try to set the FD_CLOEXEC flag, and if that fails, close the socket
> +   and fail unless __tolerant.  */

What is __tolerant?

> +#ifdef FD_CLOEXEC
> +  if (ret != -1)
> +    {
> +      int flags = __fcntl (ret, F_GETFD, 0);
> +      if (flags == -1
> +	  || __fcntl (ret, F_SETFD, flags | FD_CLOEXEC) == -1)
> +	{
> +	  __close (ret);
> +	  ret = -1;
> +	  errno = EINVAL;

How did you decide on EINVAL for this case?
It needs a comment.

> +#pragma poison __socket
> +#pragma poison socket

cf. a previous review about introducing '#pragma poison'.

> --- a/resolv/res_hconf.c
> +++ b/resolv/res_hconf.c
> @@ -41,6 +41,7 @@
>  #include <sys/ioctl.h>
>  #include <unistd.h>
>  #include <netinet/in.h>
> +#include <socket-cloexec.h>
>  #include <bits/libc-lock.h>
>  #include "ifreq.h"
>  #include "res_hconf.h"
> @@ -410,7 +411,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_cloexec (AF_INET, SOCK_DGRAM, 0);
>        if (sd < 0)
>  	return;

This is a temporary fd used only within this function, never left open.
If there is no underlying SOCK_CLOEXEC support, then calling fcntl after
the fact is no more guarantee of anything than calling close in this
function (and adds some overhead).  My inclination is not to pretend to
offer any race-free leaklessness if we cannot in fact offer it across
the board.

Sure, it's nice to be leakless when it's cheap and race-free to be so.
But it's not part of the API that you can rely on this, and cannot be
unless we require underlying SOCK_CLOEXEC support for every
implementation of the GNU C API.  That would say that for these
ephemeral uses, we use SOCK_CLOEXEC if it's available but do not bother
with the fcntl call if it's not.

Perhaps we actually can require SOCK_CLOEXEC support nowadays, but we
haven't ever explicitly said so before.  (For Linux configurations,
__ASSUME_SOCK_CLOEXEC is always set since we now support only kernels
new enough.  For the Hurd, *_CLOEXEC are actually implemented entirely
in libc, though SOCK_CLOEXEC is not actually implemented yet.  The other
OS port forthcoming is NaCl, which does not have exec at all yet.)

> @@ -146,7 +146,7 @@ clnttcp_create (struct sockaddr_in *raddr, u_long prog, u_long vers,
>     */
>    if (*sockp < 0)
>      {
> -      *sockp = __socket (AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +      *sockp = __socket_cloexec (AF_INET, SOCK_STREAM, IPPROTO_TCP);
>        (void) bindresvport (*sockp, (struct sockaddr_in *) 0);
>        if ((*sockp < 0)
>  	  || (__connect (*sockp, (struct sockaddr *) raddr,

This is a change in user semantics.  It definitely doesn't belong in the
same change with the ephemeral cases, and probably it's an unacceptable
API change.  For this completely deprecated API, it's not worth making
any changes.

> @@ -132,7 +132,7 @@ clntunix_create (struct sockaddr_un *raddr, u_long prog, u_long vers,
>     */
>    if (*sockp < 0)
>      {
> -      *sockp = __socket (AF_UNIX, SOCK_STREAM, 0);
> +      *sockp = __socket_cloexec (AF_UNIX, SOCK_STREAM, 0);
>        len = strlen (raddr->sun_path) + sizeof (raddr->sun_family) + 1;
>        if (*sockp < 0
>  	  || __connect (*sockp, (struct sockaddr *) raddr, len) < 0)

Same here.

> @@ -48,7 +48,7 @@ int
>  internal_function
>  __get_socket (struct sockaddr_in *saddr)
>  {
> -  int so = __socket (AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +  int so = __socket_cloexec (AF_INET, SOCK_STREAM, IPPROTO_TCP);
>    if (so < 0)
>      return -1;

This one is OK as far as being a proper ephemeral case, but I'd still be
inclined just not to touch any sunrpc/ code at all.


Thanks,
Roland