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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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.