posix_spawn and vfork

posix_spawn and vfork

[Florian Weimer]

Why do we use clone instead of vfork in posix_spawn?

I think we could use vfork and a call to a separate function for the
subprocess code, as a compiler barrier, and therefore avoiding stack
clobbering issues.

As a result, we can reuse the existing stack and would not have to
allocate a new one.

If this reasoning is incorrect, I'll gladly write up the explanation as
a new comment for sysdeps/unix/sysv/linux/spawni.c.

Thanks,
Florian

Re: posix_spawn and vfork

[Adhemerval Zanella]

On 02/05/2019 09:18, Florian Weimer wrote:
> Why do we use clone instead of vfork in posix_spawn?

Besides being deprecated by POSIX, the main issue currently it lacks
a *proper* way to setup a correct stack frame for helper process.
Even with a function, we are relying on undefined semantic that compiler
won't clobber the stack.

> 
> I think we could use vfork and a call to a separate function for the
> subprocess code, as a compiler barrier, and therefore avoiding stack
> clobbering issues.

Why circle back to vfork? Current clone usage already provides all the
required work avoid all the pitfalls we worked to fix for posix_spawn.

> 
> As a result, we can reuse the existing stack and would not have to
> allocate a new one.

Is it really a bottleneck you think work of using a deprecated and
tricky interface instead? Also, we will need to take of the compatibility
handling (maybe_script_execute) which is the main culprit of the stack
requirement (besides -fstack-check).  One possibility is add a compat
symbol and remove maybe_script_execute, but not sure if worth the trouble.

> 
> If this reasoning is incorrect, I'll gladly write up the explanation as
> a new comment for sysdeps/unix/sysv/linux/spawni.c.
> 
> Thanks,
> Florian
> 

Re: posix_spawn and vfork

[Florian Weimer]

* Adhemerval Zanella:

> On 02/05/2019 09:18, Florian Weimer wrote:
>> Why do we use clone instead of vfork in posix_spawn?
>
> Besides being deprecated by POSIX, the main issue currently it lacks
> a *proper* way to setup a correct stack frame for helper process.
> Even with a function, we are relying on undefined semantic that compiler
> won't clobber the stack.

We could specify the returns_twice attribute, then the compiler should
make it work.

I would even go so far as to say that doing this is worthwhile because
it increases test coverage for our vfork implementation.  I think we
need to provide a working vfork.

>> As a result, we can reuse the existing stack and would not have to
>> allocate a new one.
>
> Is it really a bottleneck you think work of using a deprecated and
> tricky interface instead? Also, we will need to take of the compatibility
> handling (maybe_script_execute) which is the main culprit of the stack
> requirement (besides -fstack-check).  One possibility is add a compat
> symbol and remove maybe_script_execute, but not sure if worth the trouble.

If we share the stack, like vfork does, all that stack-management code
isn't necessary anymore.

Thanks,
Florian

Re: posix_spawn and vfork

[Paul Eggert]

Florian Weimer wrote:

> doing this is worthwhile because
> it increases test coverage for our vfork implementation.  I think we
> need to provide a working vfork.

Yes; regardless of whether we use vfork to implement posix_spawn, too many 
programs rely on vfork to withdraw it any time in the foreseeable future.

Re: posix_spawn and vfork

[Adhemerval Zanella]

On 04/05/2019 06:07, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 02/05/2019 09:18, Florian Weimer wrote:
>>> Why do we use clone instead of vfork in posix_spawn?
>>
>> Besides being deprecated by POSIX, the main issue currently it lacks
>> a *proper* way to setup a correct stack frame for helper process.
>> Even with a function, we are relying on undefined semantic that compiler
>> won't clobber the stack.
> 
> We could specify the returns_twice attribute, then the compiler should
> make it work.
> 
> I would even go so far as to say that doing this is worthwhile because
> it increases test coverage for our vfork implementation.  I think we
> need to provide a working vfork.

The return_twice should indeed help the compiler support, but it would
still need to rely in proper compiler compiler support through an extension
which is in most places sketchy (it may fail in some non define scenario or
with a compiler that does not fully support the attribute). 

> 
>>> As a result, we can reuse the existing stack and would not have to
>>> allocate a new one.
>>
>> Is it really a bottleneck you think work of using a deprecated and
>> tricky interface instead? Also, we will need to take of the compatibility
>> handling (maybe_script_execute) which is the main culprit of the stack
>> requirement (besides -fstack-check).  One possibility is add a compat
>> symbol and remove maybe_script_execute, but not sure if worth the trouble.
> 
> If we share the stack, like vfork does, all that stack-management code
> isn't necessary anymore.

The current stack allocation thought mmap is only required to handle the
compat handling of script without shebang.  We can optimize it by moving 
it to a compat symbol and providing a simple pre-defined buffer with enough 
stack size so __spawni_child can allocate its automatic variables.  I will
try to spend some time to work on this this week.

> 
> Thanks,
> Florian
> 

Re: posix_spawn and vfork

[Adhemerval Zanella]

On 04/05/2019 12:59, Paul Eggert wrote:
> Florian Weimer wrote:
> 
>> doing this is worthwhile because
>> it increases test coverage for our vfork implementation.  I think we
>> need to provide a working vfork.
> 
> Yes; regardless of whether we use vfork to implement posix_spawn, too many programs rely on vfork to withdraw it any time in the foreseeable future.

That does not mean we should *promote* the interface, which has a lot
of downsides and are tricky to use correctly.  If we do want to support
a vfork-like interface I would work toward Zack Weinberg suggestion to add 
a callback 'pre-fork' routine and extend it to make it possible to user
supply a stack.  

Something like:

pid_t xfork (void *(*pre_fork (void *), void *stack, size_t stacksize);

It will make the required libc handling (disable asynchronous cancellation,
block all signals, call the syscall primitive with required flags, restore
signal handling, re-enable asynchronous cancellation, handle error state).
The stack is optional and using NULL will make it use a automatic allocate
one (not sure if issuing a VLA is worth based on stacksize or use a 
pre-defined value).

With some care it would be possible to use on even signal handling (assuming
the callback only uses async-signal-safe functions), even on sigaltstack
signal handlers.

Re: posix_spawn and vfork

[Florian Weimer]

* Adhemerval Zanella:

> On 04/05/2019 12:59, Paul Eggert wrote:
>> Florian Weimer wrote:
>> 
>>> doing this is worthwhile because
>>> it increases test coverage for our vfork implementation.  I think we
>>> need to provide a working vfork.
>> 
>> Yes; regardless of whether we use vfork to implement posix_spawn, too many programs rely on vfork to withdraw it any time in the foreseeable future.
>
> That does not mean we should *promote* the interface, which has a lot
> of downsides and are tricky to use correctly.  If we do want to support
> a vfork-like interface I would work toward Zack Weinberg suggestion to add 
> a callback 'pre-fork' routine and extend it to make it possible to user
> supply a stack.  
>
> Something like:
>
> pid_t xfork (void *(*pre_fork (void *), void *stack, size_t stacksize);
>
> It will make the required libc handling (disable asynchronous cancellation,
> block all signals, call the syscall primitive with required flags, restore
> signal handling, re-enable asynchronous cancellation, handle error state).
> The stack is optional and using NULL will make it use a automatic allocate
> one (not sure if issuing a VLA is worth based on stacksize or use a 
> pre-defined value).

If we want to go this route, I'd suggest something like this as a
building block:

  pid_t clone_samestack (unsigned int flags, void (*action) (void *)
                         void *closure);

and it would be a fatal error if ACTION (CLOSURE) returns.  The function
would fail with EINVAL if FLAGS contained CLONE_VM without CLONE_VFORK.

Once we have that, the need for explicit stack management goes away.

Thanks,
Florian

Re: posix_spawn and vfork

[Adhemerval Zanella]

On 06/05/2019 09:50, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 04/05/2019 12:59, Paul Eggert wrote:
>>> Florian Weimer wrote:
>>>
>>>> doing this is worthwhile because
>>>> it increases test coverage for our vfork implementation.  I think we
>>>> need to provide a working vfork.
>>>
>>> Yes; regardless of whether we use vfork to implement posix_spawn, too many programs rely on vfork to withdraw it any time in the foreseeable future.
>>
>> That does not mean we should *promote* the interface, which has a lot
>> of downsides and are tricky to use correctly.  If we do want to support
>> a vfork-like interface I would work toward Zack Weinberg suggestion to add 
>> a callback 'pre-fork' routine and extend it to make it possible to user
>> supply a stack.  
>>
>> Something like:
>>
>> pid_t xfork (void *(*pre_fork (void *), void *stack, size_t stacksize);
>>
>> It will make the required libc handling (disable asynchronous cancellation,
>> block all signals, call the syscall primitive with required flags, restore
>> signal handling, re-enable asynchronous cancellation, handle error state).
>> The stack is optional and using NULL will make it use a automatic allocate
>> one (not sure if issuing a VLA is worth based on stacksize or use a 
>> pre-defined value).
> 
> If we want to go this route, I'd suggest something like this as a
> building block:
> 
>   pid_t clone_samestack (unsigned int flags, void (*action) (void *)
>                          void *closure);
> 
> and it would be a fatal error if ACTION (CLOSURE) returns.  The function
> would fail with EINVAL if FLAGS contained CLONE_VM without CLONE_VFORK.
> 
> Once we have that, the need for explicit stack management goes away.

But would it just a shim wrapper over clone (to handle the required kABI)
as vfork currently is or something more sane as posix_spawn to work
correctly along libc? Also, would be just an internal interface or
a possible extension?

Because for a clone wrapper, I think would be simpler to just make using
clone instead by allowing it accept a null stack input.

---
  struct wrapper_argument
  {
    void (*action) (void *) fn;
    void *arg;
  };

  void wrapper (void *input)
  {
    struct wrapper_argument *arg = input;
    arg->fn (arg->arg);
    abort ();
  }

  pid_t clone_samestack (unsigned int flags, void (*action) (void *)
                         void *closure)
  {
    int clone_flags = CLONE_VM | CLONE_VFORK | SIGCHLD;
#ifndef __ia64__
    return clone (wrapper, NULL, clone_flags,
		  &(struct wrapper_argument) { action, closure }));
#else
    return __clone2 (wrapper, NULL, sizeof stack,
                     &(struct wrapper_argument) { action, closure }));
#endif
  }

---

It also wouldn't require potentially another arch-specific assembly 
implementation.

However my main issue with such interface where is reuses the stack from
parent process is how safe this construction is.  That's why I think the
xfork symbol, which either allocates a pre-defined stack or accepts a 
user-define one (as for sigaltstack) should be safer and play along
libc interfaces better (it could either share some code with posix_spawn).

Re: posix_spawn and vfork

[Florian Weimer]

* Adhemerval Zanella:

>> If we want to go this route, I'd suggest something like this as a
>> building block:
>> 
>>   pid_t clone_samestack (unsigned int flags, void (*action) (void *)
>>                          void *closure);
>> 
>> and it would be a fatal error if ACTION (CLOSURE) returns.  The function
>> would fail with EINVAL if FLAGS contained CLONE_VM without CLONE_VFORK.
>> 
>> Once we have that, the need for explicit stack management goes away.
>
> But would it just a shim wrapper over clone (to handle the required kABI)
> as vfork currently is or something more sane as posix_spawn to work
> correctly along libc? Also, would be just an internal interface or
> a possible extension?

I think it would make sense to start out with the thin wrapper, and then
build the safer vfork abstraction on top of that.

> Because for a clone wrapper, I think would be simpler to just make using
> clone instead by allowing it accept a null stack input.
>
> ---
>   struct wrapper_argument
>   {
>     void (*action) (void *) fn;
>     void *arg;
>   };
>
>   void wrapper (void *input)
>   {
>     struct wrapper_argument *arg = input;
>     arg->fn (arg->arg);
>     abort ();
>   }
>
>   pid_t clone_samestack (unsigned int flags, void (*action) (void *)
>                          void *closure)
>   {
>     int clone_flags = CLONE_VM | CLONE_VFORK | SIGCHLD;
> #ifndef __ia64__
>     return clone (wrapper, NULL, clone_flags,
> 		  &(struct wrapper_argument) { action, closure }));
> #else
>     return __clone2 (wrapper, NULL, sizeof stack,
>                      &(struct wrapper_argument) { action, closure }));
> #endif
>   }
>
> ---
>
> It also wouldn't require potentially another arch-specific assembly 
> implementation.

For the x86-64 and i386 implementations, it's not just about removing
the NULL check.  I think we will need different trampoline functions,
with different unwinding data.  At that point, we can just rewrite the
whole function.

> However my main issue with such interface where is reuses the stack from
> parent process is how safe this construction is.  That's why I think the
> xfork symbol, which either allocates a pre-defined stack or accepts a 
> user-define one (as for sigaltstack) should be safer and play along
> libc interfaces better (it could either share some code with posix_spawn).

I think it boils down to whether the parent thread is reliably stopped
during vfork (if it is not implemented as fork).  I have not seen any
indication so far that the blocking is unreliable in current (3.x)
kernels.

In contrast, when sizing the separate stack, you need to make educated
guesses about ld.so stack space requirements.  Ideally, you would also
add a guard page.  The new stack may need to be communicated to memory
debuggers.  And so on.  All in all, stack switching seems more risky to
me.

Thanks,
Florian

Re: posix_spawn and vfork

[Adhemerval Zanella]

On 07/05/2019 05:50, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> If we want to go this route, I'd suggest something like this as a
>>> building block:
>>>
>>>   pid_t clone_samestack (unsigned int flags, void (*action) (void *)
>>>                          void *closure);
>>>
>>> and it would be a fatal error if ACTION (CLOSURE) returns.  The function
>>> would fail with EINVAL if FLAGS contained CLONE_VM without CLONE_VFORK.
>>>
>>> Once we have that, the need for explicit stack management goes away.
>>
>> But would it just a shim wrapper over clone (to handle the required kABI)
>> as vfork currently is or something more sane as posix_spawn to work
>> correctly along libc? Also, would be just an internal interface or
>> a possible extension?
> 
> I think it would make sense to start out with the thin wrapper, and then
> build the safer vfork abstraction on top of that.
> 
>> Because for a clone wrapper, I think would be simpler to just make using
>> clone instead by allowing it accept a null stack input.
>>
>> ---
>>   struct wrapper_argument
>>   {
>>     void (*action) (void *) fn;
>>     void *arg;
>>   };
>>
>>   void wrapper (void *input)
>>   {
>>     struct wrapper_argument *arg = input;
>>     arg->fn (arg->arg);
>>     abort ();
>>   }
>>
>>   pid_t clone_samestack (unsigned int flags, void (*action) (void *)
>>                          void *closure)
>>   {
>>     int clone_flags = CLONE_VM | CLONE_VFORK | SIGCHLD;
>> #ifndef __ia64__
>>     return clone (wrapper, NULL, clone_flags,
>> 		  &(struct wrapper_argument) { action, closure }));
>> #else
>>     return __clone2 (wrapper, NULL, sizeof stack,
>>                      &(struct wrapper_argument) { action, closure }));
>> #endif
>>   }
>>
>> ---
>>
>> It also wouldn't require potentially another arch-specific assembly 
>> implementation.
> 
> For the x86-64 and i386 implementations, it's not just about removing
> the NULL check.  I think we will need different trampoline functions,
> with different unwinding data.  At that point, we can just rewrite the
> whole function.

It is still better than add *another* assembly required implementation that
would need to be implemented or adjusted for every port.

> 
>> However my main issue with such interface where is reuses the stack from
>> parent process is how safe this construction is.  That's why I think the
>> xfork symbol, which either allocates a pre-defined stack or accepts a 
>> user-define one (as for sigaltstack) should be safer and play along
>> libc interfaces better (it could either share some code with posix_spawn).
> 
> I think it boils down to whether the parent thread is reliably stopped
> during vfork (if it is not implemented as fork).  I have not seen any
> indication so far that the blocking is unreliable in current (3.x)
> kernels.

My problem is the blurry state the new process is created due the CLONE_VM,
where although it stops the calling process, it still needs to handle shared
states being modified by standard functions (such as malloc) or even with
more complicated states (dynamic loading, dynamic binding, etc).

It gets even more complicate when we started to handle more complex scenarios,
such cancellation, unwinding, non async-safe functions, etc.

> 
> In contrast, when sizing the separate stack, you need to make educated
> guesses about ld.so stack space requirements.  Ideally, you would also
> add a guard page.  The new stack may need to be communicated to memory
> debuggers.  And so on.  All in all, stack switching seems more risky to
> me.

For generic case I do agree dynamic binding does impose some issues
(although golang with split-stack already set some precedent that
it somewhat feasible to pick a stack size for this). 

In any case, I think we are deviating to the point of what would be the
real usercases for a vfork-like interface.  Currently AFAIK vfork is
used mainly as a replacement of fork for performance-wise improvement in
process spawning. It is something that I do think it would be better
served with posix_spawn and well-thought extension (as we are doing
in latest glibc releases).

So do you think we will different vfork scenarios that posix_spawn is
not a possible fit (even with extended attribute or file actions)?