Re: BZ#15722: create all sockets with SOCK_CLOEXEC

[Alexandre Oliva <aoliva at redhat dot com>]

On Dec 11, 2014, Roland McGrath <roland@hack.frob.com> wrote:

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

Ack, thanks.

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

Thanks, fixed.

>> +/* 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?

Left-over from an earlier version of the patch.  Thanks, fixed.

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

	  /* EINVAL is what __socket returns if it does not support
	     SOCK_CLOEXEC.  */

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

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

*nod*

I see now that this is wrong in this case.  See below.

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

The goal was never to offer a race-free SOCK_CLOEXEC-equivalent.  As far
as I was concerned, initially it was just to attempt to reduce the
internal fd count left over after exec.

Adding the FD_CLOEXEC fallback was just an idea I got from one of the
rpc sources that used it.  Maybe we don't want to do that there either?

Anyway, since it was meant to be best-effort (we can't assume
SOCK_CLOEXEC or even FD_CLOEXEC are supported), it wouldn't make much
sense to require it to be atomic either.  And given that it was
best-effort, under the reasoning that we'd better get a socket without
cloexec than not get one at all, I dropped __tolerant, which was meant
to choose between failing altogether or accepting getting a socket
without cloexec.  The latest patch still had some intolerant code, that
closed the socket if FD_CLOEXEC didn't work, though; I guess that's a
symptom of the patch not having a well-defined goal, shifting about as
suggestions and requests come about ;-)

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

I agree.  I hadn't realized SOCK_CLOEXEC could have this sort of impact;
I'd started out from misunderstanding the goal stated in the bug report,
of having *all* sockets created with SOCK_CLOEXEC, which could be
mechanically addressed with a wrapper just like that for poll, and go to
this.

Now I guess we have to go back to the drawing board.

If the goal is to open all internal (*) sockets with SOCK_CLOEXEC
atomically, or fail, it will take some actual review of the various
hunks in my patch, but we can do without a wrapper and just add
SOCK_CLOEXEC to the socket calls proper.

If the goal is to attempt to set the CLOEXEC flag in all internal (*)
sockets as soon as possible, but to tolerate failures to set the flag,
then something similar to the wrapper I proposed would do.

If it's something else, I suppose it might take yet another different
approach.

(*) internal meaning not intentionally exposed to users, so that adding
CLOEXEC will not amount to an API change.  âIntentionallyâ because
ultimately all fds are directly accessible by users.

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

'k

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer