[PATCH] tfork

[Jeremy Allison]

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.

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.

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.

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

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

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.

Thoughts ?

I'm still reviewing the code, and this discussion
can go on in parallel with pushing this - this
code doesn't make the problem any worse than it
already is :-).

Cheers,

Jeremy.