[v2,3/3] posix: New Linux posix_spawn{p} implementation

  On 1 September 2016 at 00:08, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 31 Aug 2016, Rasmus Villemoes wrote:
>
>> Rather late to the party, but I think there's a few bugs here. Most
>> importantly, dup() doesn't preserve the CLOEXEC flag, so if we do move
>> the write end around like this, the fd will not automatically be closed
>> during the exec, and hence the parent won't receive EOF and will block
>> in read() call until the child finally exits. That's easily fixable with
>> fcntl(p, F_DUPFD_CLOEXEC, 0). Pretty annoying to add a test for, though.
>
> In Linux-specific code we can assume the presence of dup3 (which needs to
> be called by the name __dup3 in implementations of POSIX functions).

What I meant was that it is a little hard to write a regression test
for this bug, since we don't know beforehand what fds the pipe2() call
will give us, making it hard to create actions that is guaranteed to
exercise this code.

But, thinking a bit more about this, why do we even need a pipe to
ensure the child is gone, when we already set CLONE_VFORK? Can't we
just exploit the fact that we run in the same VM as the parent and
make the child write a non-zero error code to the spawn_args
structure? That would eliminate this problem entirely. Something like
below (sorry if gmail has whitespace-damaged it).

Rasmus



From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Date: Thu, 1 Sep 2016 11:03:43 +0200
Subject: [PATCH] linux: spawni.c: simplify error reporting to parent

Using VFORK already ensures that the parent does not run until the
child has either exec'ed succesfully or called _exit. Hence we don't
need to read from a CLOEXEC pipe to ensure proper synchronization - we
just make explicit use of the fact the the child and parent run in the
same VM, so the child can write an error code to a field of the
posix_spawn_args struct instead of sending it through a pipe.

This eliminates some annoying bookkeeping that is necessary to avoid
the file actions from clobbering the write end of the pipe, and
getting rid of the pipe creation in the first place means fewer system
calls and fewer chanches for the spawn to fail (e.g. if we're close to
EMFILE).

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 sysdeps/unix/sysv/linux/spawni.c | 63 ++++++++++++----------------------------
 1 file changed, 18 insertions(+), 45 deletions(-)
  

On 01/09/2016 06:28, Rasmus Villemoes wrote:
> On 1 September 2016 at 00:08, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Wed, 31 Aug 2016, Rasmus Villemoes wrote:
>>
>>> Rather late to the party, but I think there's a few bugs here. Most
>>> importantly, dup() doesn't preserve the CLOEXEC flag, so if we do move
>>> the write end around like this, the fd will not automatically be closed
>>> during the exec, and hence the parent won't receive EOF and will block
>>> in read() call until the child finally exits. That's easily fixable with
>>> fcntl(p, F_DUPFD_CLOEXEC, 0). Pretty annoying to add a test for, though.
>>
>> In Linux-specific code we can assume the presence of dup3 (which needs to
>> be called by the name __dup3 in implementations of POSIX functions).
> 
> What I meant was that it is a little hard to write a regression test
> for this bug, since we don't know beforehand what fds the pipe2() call
> will give us, making it hard to create actions that is guaranteed to
> exercise this code.
> 
> But, thinking a bit more about this, why do we even need a pipe to
> ensure the child is gone, when we already set CLONE_VFORK? Can't we
> just exploit the fact that we run in the same VM as the parent and
> make the child write a non-zero error code to the spawn_args
> structure? That would eliminate this problem entirely. Something like
> below (sorry if gmail has whitespace-damaged it).

I think patch is ok and fixes the issues you noted about using the pipe2
call to signal the execv issue.  It just have one remark about it below.


> @@ -280,14 +267,12 @@ __spawni_child (void *arguments)
>       (2.15).  */
>    maybe_script_execute (args);
> 
> -  ret = -errno;
> -
>  fail:
> -  /* Since sizeof errno < PIPE_BUF, the write is atomic. */
> -  ret = -ret;
> -  if (ret)
> -    while (write_not_cancel (p, &ret, sizeof ret) < 0)
> -      continue;
> +  /* errno should have an appropriate non-zero value, but make sure
> +     that's the case so that our parent knows we failed to
> +     exec. There's no EUNKNOWN or EINTERNALBUG, so we use a value
> +     which is clearly bogus.  */
> +  args->err = errno ? : EHOSTDOWN;
>    _exit (SPAWN_ERROR);
>  }

I would prefer an assert call here to ensure errno is non zero for
failure case instead of reporting a bogus errno to program.  Since
this unexpected issue is either something wrong being reported from
kernel or an underlying bug it would be better to fail at once than
instead to document on manuals that this is potentially an unknown
issue.
  

On Wed, Sep 14 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> I think patch is ok and fixes the issues you noted about using the pipe2
> call to signal the execv issue.  It just have one remark about it below.
>
>
>> @@ -280,14 +267,12 @@ __spawni_child (void *arguments)
>>       (2.15).  */
>>    maybe_script_execute (args);
>> 
>> -  ret = -errno;
>> -
>>  fail:
>> -  /* Since sizeof errno < PIPE_BUF, the write is atomic. */
>> -  ret = -ret;
>> -  if (ret)
>> -    while (write_not_cancel (p, &ret, sizeof ret) < 0)
>> -      continue;
>> +  /* errno should have an appropriate non-zero value, but make sure
>> +     that's the case so that our parent knows we failed to
>> +     exec. There's no EUNKNOWN or EINTERNALBUG, so we use a value
>> +     which is clearly bogus.  */
>> +  args->err = errno ? : EHOSTDOWN;
>>    _exit (SPAWN_ERROR);
>>  }
>
> I would prefer an assert call here to ensure errno is non zero for
> failure case instead of reporting a bogus errno to program.  Since
> this unexpected issue is either something wrong being reported from
> kernel or an underlying bug it would be better to fail at once than
> instead to document on manuals that this is potentially an unknown
> issue.

But asserting/aborting in the child doesn't really solve the problem; we
still need to write some non-zero value for the parent to pick up once
we're gone. We could of course write -1 to indicate this really
exceptional situation, but that still leaves deciding how to handle that
in the parent. IMO an assert/abort is a little too harsh, but then the
parent has to return _some_ error code to its caller.

Rasmus
  

On 14/09/2016 15:58, Rasmus Villemoes wrote:
> On Wed, Sep 14 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
>> I think patch is ok and fixes the issues you noted about using the pipe2
>> call to signal the execv issue.  It just have one remark about it below.
>>
>>
>>> @@ -280,14 +267,12 @@ __spawni_child (void *arguments)
>>>       (2.15).  */
>>>    maybe_script_execute (args);
>>>
>>> -  ret = -errno;
>>> -
>>>  fail:
>>> -  /* Since sizeof errno < PIPE_BUF, the write is atomic. */
>>> -  ret = -ret;
>>> -  if (ret)
>>> -    while (write_not_cancel (p, &ret, sizeof ret) < 0)
>>> -      continue;
>>> +  /* errno should have an appropriate non-zero value, but make sure
>>> +     that's the case so that our parent knows we failed to
>>> +     exec. There's no EUNKNOWN or EINTERNALBUG, so we use a value
>>> +     which is clearly bogus.  */
>>> +  args->err = errno ? : EHOSTDOWN;
>>>    _exit (SPAWN_ERROR);
>>>  }
>>
>> I would prefer an assert call here to ensure errno is non zero for
>> failure case instead of reporting a bogus errno to program.  Since
>> this unexpected issue is either something wrong being reported from
>> kernel or an underlying bug it would be better to fail at once than
>> instead to document on manuals that this is potentially an unknown
>> issue.
> 
> But asserting/aborting in the child doesn't really solve the problem; we
> still need to write some non-zero value for the parent to pick up once
> we're gone. We could of course write -1 to indicate this really
> exceptional situation, but that still leaves deciding how to handle that
> in the parent. IMO an assert/abort is a little too harsh, but then the
> parent has to return _some_ error code to its caller.

My idea is to in fact not return to parent, but rather terminate program
execution in face of an unknown issue.  However, I do not have a strong
opinion if it should be really the desirable behaviour and thinking twice
it does seems that aborting program is too harsh.  I think -1 would be
suffice.
  

* Rasmus Villemoes:

> +  /* errno should have an appropriate non-zero value, but make sure
> +     that's the case so that our parent knows we failed to
> +     exec. There's no EUNKNOWN or EINTERNALBUG, so we use a value
> +     which is clearly bogus.  */
> +  args->err = errno ? : EHOSTDOWN;
>    _exit (SPAWN_ERROR);
>  }

I think ECHILD is probably a better fake error code.

You should set args->err to 0 on success ...

> +  args.err = 0;

... and initialize it to -1.

>    if (new_pid > 0)
>      {
> -      if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
> -    ec = 0;
> -      else
> +      ec = args.err;
> +      if (ec != 0)
>      __waitpid (new_pid, NULL, 0);
>      }

You should check (assert?) here that args.err is not -1.  Otherwise we
will never notice if the page is not shared between parent and child,
and the error reporting mechanism does not work.
  

On Tue, Sep 20 2016, Florian Weimer <fw@deneb.enyo.de> wrote:

> * Rasmus Villemoes:
>
>> +  /* errno should have an appropriate non-zero value, but make sure
>> +     that's the case so that our parent knows we failed to
>> +     exec. There's no EUNKNOWN or EINTERNALBUG, so we use a value
>> +     which is clearly bogus.  */
>> +  args->err = errno ? : EHOSTDOWN;
>>    _exit (SPAWN_ERROR);
>>  }
>
> I think ECHILD is probably a better fake error code.

Yeah, that's probably ok. It's consistent with its use in pclose() where
the posix description reads "The status of the child process could not
be obtained". We just use it in the sense "something went wrong, we just
don't know what".

I'd really wish EINTERNALBUG existed.

> You should set args->err to 0 on success ...
>
>> +  args.err = 0;
>
> ... and initialize it to -1.
>
>>    if (new_pid > 0)
>>      {
>> -      if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
>> -    ec = 0;
>> -      else
>> +      ec = args.err;
>> +      if (ec != 0)
>>      __waitpid (new_pid, NULL, 0);
>>      }
>
> You should check (assert?) here that args.err is not -1.  Otherwise we
> will never notice if the page is not shared between parent and child,
> and the error reporting mechanism does not work.

Good point. I'll send an updated patch in a moment.

Rasmus