bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling

[Pádraig Brady]

On 05/02/17 04:23, Tobias Stoeckmann wrote:
> It is possible to trigger a signal race in timeout if the alarm is
> triggered AFTER the child process already finished and was waited for
> by the parent.


> If a child terminates, it becomes a zombie process until the parent
> called waitpid() (or an equivalent system call) on it. This means that
> the process id cannot ge given to a new process. But if waitpid() has
> been called, the process id stored in the variable monitored_pid could
> already be assigned to another one. If the alarm -- due to timeout --
> is triggered afterwards, the tool can send a signal to that new process
> instead.
> 
> To fix this, I have introduced a critical section around waitpid(),
> which blocks SIGALRM and resets monitored_pid to 0 if waitpid() returned
> anything except 0 (be it a success or a failure). This way, a later
> alarm won't send any signal.

> diff --git a/src/timeout.c b/src/timeout.c
>  settimeout (double duration, bool warn)
>  {
>  
> -  /* We configure timers below so that SIGALRM is sent on expiry.
> -     Therefore ensure we don't inherit a mask blocking SIGALRM.  */
> -  unblock_signal (SIGALRM);

Removing the call above causes this test to fail:
tests/misc/timeout-blocked.pl
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=tests/misc/timeout-blocked.pl;hb=HEAD

> -      while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0
> -             && errno == EINTR)
> -        continue;
> +      block_signal (SIGALRM, &old_set);
> +      while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0)
> +        sigsuspend (&old_set);
> +      monitored_pid = 0;
> +      unblock_signal (SIGALRM);

I think we'd need some conditional code to allow
sigsuspend() to compile on mingw, MSVC 9.

In general this is a largely theoretical race right?
I.E. pids would need to be recycled between the waitpid() and exit()?
Just trying to get an idea of the practicality of the issue.

thanks!
Pádraig