Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Date: Sun, 07 Feb 2016 00:22:04 +0000
From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: Philip Martin <philip.martin@wandisco.com>
Cc: dev@subversion.apache.org, commits@subversion.apache.org
Subject: Re: svn commit: r1727621 - in /subversion/trunk/subversion:
 svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c
 svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Message-ID: <20160207002204.GA13224@tarsus.local2>
References: <20160129185522.A2F813A0249@svn01-us-west.apache.org>
 <20160130074708.GA30257@tarsus.local2>
 <87h9hsaf27.fsf@wandisco.com>
 <87d1sgad97.fsf__33268.2802960791$1454325642$gmane$org@wandisco.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: 
 <87d1sgad97.fsf__33268.2802960791$1454325642$gmane$org@wandisco.com>
User-Agent: Mutt/1.5.23 (2014-03-12)

Philip Martin wrote on Mon, Feb 01, 2016 at 11:20:04 +0000:
> Philip Martin <philip.martin@wandisco.com> writes:
> 
> > That change is broken for two reasons:
> >
> >  - the same handler is used for all those signals but we always exit via
> >    SIGINT
> >
> >  - except apr_signal does not return APR_SUCCESS so we never exit via
> >    SIGINT.
> 
> Fixed by r1727916.

> +  if (cancelled)
> +    {
> +      apr_signal(signum_cancelled, SIG_DFL);
> +      /* No APR support for getpid() so cannot use apr_proc_kill(). */
> +      kill(getpid(), signum_cancelled);
> +    }

There seems to be a race condition here: if this block is entered with
(signum_cancelled == SIGINT), and between the apr_signal() call and
evaluating the arguments to the kill() call, a SIGTERM is received and
changes the value of signum_cancelled, the kill() call will send
a SIGTERM signal to current process while the SIGTERM handler will still
be SIG_IGN (the value installed by our handler).

I think we have two options to fix this: either make signum_cancelled
a write-once variable (never write to it if it's nonzero), or have the
handler install SIG_IGN handlers for all signals we catch, not just for
one of them.  Or perhaps both?  The former approach seems more robust
but the latter seems like a good idea in its own right.

> To see the effect of this run the following at a sh prompt:
> 
>   while :;do echo sleep;sleep 3;echo log;svn log http://svn.apache.org/repos/asf;done
> 
> Using control-C to send SIGINT will exit the loop only if sent while
> sleep is running.  A SIGINT during log will cancel the svn command but
> the loop will continue and a second SIGINT during sleep will be needed
> to exit the loop.

Cheers,

Daniel