[PATCH] Cygwin: setjmp/longjmp: decrement incyg after signal handling

[Takashi Yano]

Hi Corinna,

On Thu, 5 Dec 2024 11:51:17 +0100
Corinna Vinschen wrote:
> Hi Takashi,
> 
> On Dec  5 10:14, Takashi Yano wrote:
> > Hi Corinna,
> > 
> > On Wed,  4 Dec 2024 13:54:47 +0100
> > Corinna Vinschen wrote:
> > > From: Corinna Vinschen <corinna@vinschen.de>
> > > 
> > > Commit 0b6fbd396ca2f ("* exceptions.cc (_cygtls::interrupt_now): Revert
> > > to checking for "spinning" when choosing to defer signal.") introduced
> > > a bug in the loop inside the stabilize_sig_stack subroutine:
> > > 
> > > First, stabilize_sig_stack grabs the stacklock. The _cygtls::incyg
> > > flag is then incremented before checking if a signal has to be handled
> > > for the current thread.
> > > 
> > > If no signal waits, the code simply jumps out, decrements _cygtls::incyg
> > > and returns to the caller, which eventually releases the stacklock.
> > > 
> > > However, if a signal is waiting, stabilize_sig_stack releases the
> > > stacklock, calls _cygtls::call_signal_handler(), and returns to
> > > the start of the subroutine, trying to grab the lock.
> > > 
> > > After grabbing the lock, it increments _cygtls::incyg... wait...
> > > again?
> > > 
> > > The loop does not decrement _cygtls::incyg after
> > > _cygtls::call_signal_handler(), which returns with _cygtls::incyg
> > > set to 1.  So it increments incyg to 2.  If no other signal is
> > > waiting, stabilize_sig_stack jumps out and decrements _cygtls::incyg
> > > to 1.  Eventually, setjmp or longjmp both will return to user
> > > code with _cygtls::incyg set to 1.  This *may* be fixed at some later
> > > point when signals arrive, but there will be a time when the application
> > > runs in user code with broken signal handling.
> > > 
> > > Fixes: 0b6fbd396ca2f ("* exceptions.cc (_cygtls::interrupt_now): Revert to checking for "spinning" when choosing to defer signal.")
> > > Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
> > > ---
> > >  winsup/cygwin/scripts/gendef | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/winsup/cygwin/scripts/gendef b/winsup/cygwin/scripts/gendef
> > > index 7e14f69cf71c..377ceb59b2c8 100755
> > > --- a/winsup/cygwin/scripts/gendef
> > > +++ b/winsup/cygwin/scripts/gendef
> > > @@ -344,6 +344,7 @@ stabilize_sig_stack:
> > >  	movq	\$_cygtls.start_offset,%rcx	# point to beginning
> > >  	addq	%r12,%rcx			#  of tls block
> > >  	call	_ZN7_cygtls19call_signal_handlerEv
> > > +	decl	_cygtls.incyg(%r12)
> > >  	jmp	1b
> > >  3:	decl	_cygtls.incyg(%r12)
> > >  	addq	\$0x20,%rsp
> > > -- 
> > > 2.47.0
> > > 
> > 
> > I tested this patch with Christian's longjmp test case, but
> > the problem does not seem to be fixed.
> 
> That was not the intention.  It was just something I found while looking
> into the assembler code.  This looks like a long-standing bug, which,
> if my description above is right, might result in threads missing
> signals, too.  Or at least, signals being defered, because user-space
> is running partially with the "iscyg" flag being set.
> 
> It would be nice if you could check this, too, to be sure I'm not
> totally wrong here.

It looks almost good to me and your commit message sounds reasonable.
One question.
There are three incyg <- 0 lines in gendef that are outside of stacklock.
Is that safe?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>