[PATCH] tfork

[Ralph Böhme]

On Wed, Apr 19, 2017 at 01:32:57PM -0700, Jeremy Allison wrote:
> On Tue, Apr 18, 2017 at 09:55:56PM +0200, Ralph Böhme via samba-technical wrote:
> > Hi!
> > 
> > Attached is a patchset that adds a signal and waitpid() avoiding fork()
> > replacement: tfork().
> > 
> > It was based on an older patchset from metze, called double_fork at that time,
> > that I extended to fork three-times in order to return a pollable status fd.
> > 
> > It's intented use is in samba_runcmd_send() where it avoids calling waitpid()
> > and dealing with SIGCHLD to get the exit status of the executed command.
> > 
> > With this change it is safe to call samba_runcmd_send() in smbd code as an async
> > replacement for smbrun().
> > 
> > I have a usecase here where someone wants to run shell scripts from source3
> > code without blocking.
> > 
> > Please review & push if happy.
> 
> OK, there's one comment I want to raise and discuss
> before pushing. Not sure if it matters, but at least it's
> something we need to have considered I think.
> 
> Inside this code you use CatchSignal() to set and remove the
> signal handler for SIGTERM in the parent.

SIGCHLD, but yes, I do use CatchSignal(). :)

> Inside CatchSignal() we have:
> 
>         ZERO_STRUCT(act);
> 
>         act.sa_handler = handler;
> #ifdef SA_RESTART
>         /*
>          * We *want* SIGALRM to interrupt a system call.
>          */
>         if(signum != SIGALRM)
>                 act.sa_flags = SA_RESTART;
> 
> which means that on any signal != SIGALRM we *always* set
> the SA_RESTART flag to restart slow system calls.
> 
> However, inside lib/tevent/tevent_signal.c we have:
> 
> 		struct sigaction act;
>                 ZERO_STRUCT(act);
>                 act.sa_handler = tevent_common_signal_handler;
>                 act.sa_flags = sa_flags;
> ....
> 		if (sigaction(signum, &act, sig_state->oldact[signum]) == -1) {
> 
> where sa_flags is passed in from the caller of
> tevent_add_signal().
> 
> All of the callers to tevent_add_signal() I can
> find don't use SA_RESTART flags. They all pass
> zero in the sa_flags parameter, which means that no slow
> system calls will be restarted if the caught signal
> is received.

SA_RESTART is just screwed. Imho the code must deal with EINTR anyway in most if
not all places, so we should just remove it where applicable. The tevent signal
interface allows passing it, but we should not use it in our code.

I only see two callers: tdb_robust_mutex_handler() who doesn't need it and
heimdal.

> In addition, CatchSignal() only returns the old
> act.sa_handler, *not* the content of the old act.sa_flags.
> 
> Which means when you call CatchSignal() to temporarily
> replace a signal handler added by tevent_add_signal()
> and then call CatchSignal() to put it back, you
> overwrite any sa_flags that were passed into the
> tevent_add_signal() call (not that there are any
> right now :-), and will always add in SA_RESTART.
> 
> So depending on the order of calling CatchSignal()
> or tevent_add_signal() we're going to get different
> signal handling behavior (restart or not) on slow
> system calls.

oh.

> This patch doesn't make it any worse though :-).

ah. *phew* :)

> 
> But long run I think we need to remove all
> #ifdef HAVE_SIGACTION tests and just *require*
> sigaction for Samba to run.

yes, abort configure if it's not available on the system.

> Then we need to look at fixing CatchSignal()
> to take a struct sigaction, and return the existing
> struct sigaction instead of void, so it can
> be used inside library code to temporarily
> replace an existing tevent_add_signal() handler
> and correctly replace it.
>
> Eventually we might want to look at removing
> the use of CatchSignal() and varients entirely.

It's a nice utility function where the caller mustn't prepate the sigaction structure.

Maybe change it to take and additional sa_flags and two addtional out args, ie

int CatchSignal(int signum,
                int sa_flags,
		void (*handler)(int),
                int *old_sa_flags,
		void (**old_handler)(int));

Most existing callers would just call CatchSignal(..., NULL, NULL);

-slow