On Mar 13 17:30, Corinna Vinschen via Cygwin wrote:
> On Mar 13 21:31, Takashi Yano via Cygwin wrote:
> > What about following patch instead of your sigdelayed patch?
> > [...]
> > @@ -1834,6 +1841,26 @@ _cygtls::call_signal_handler ()
> > signal handler. */
> > thisfunc (thissig, &thissi, thiscontext);
> >
> > + lock ();
> > + if (stackptr == ptr)
> > + push (retaddr1);
> > + else if (stackptr == ptr + 1)
> > + {
> > + DWORD64 retaddr3 = pop();
> > + push (retaddr1);
> > + push (retaddr3);
> > + }
> > + else if (stackptr == ptr - 1)
> > + {
> > + if (retaddr2)
> > + push (retaddr2);
> > + else
> > + stackptr++;
> > + }
> > + else
> > + api_fatal ("Signal stack corrupted?.");
> > + unlock ();
> > +
>
> This... looks confusing and desperately needs comments (or at least
> I need comments).
>
> stackptr == ptr + 1 occurs if another signal arrived while the handler
> was running, but isn't there a chance that sigdelayed has been pushed
> as well, i.e., stackptr == ptr + 2?
>
> I have no idea how the stackptr == ptr - 1 situation is supposed to
> happen, though. `else stackptr++;' looks weird. If you don't push a
> known address, what do you expect retaddr() pointing to, afterwards?
I have a slighty changed version. This one treats anything other
than 0, 1 or 2 new addresses on the stack as bug. I really made
an effort trying to come up with a situation where the signal
stack underflows, but I just couldn't. If I'm missing something,
please explain how this may happen.
Apart from that, I attached my patch proposal.
Thanks,
Corinna
>From 588d51915e6e49bbec3ab30aff7f9fe3d4e82744 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.y...@nifty.ne.jp>
Date: Thu, 13 Mar 2025 23:28:53 +0100
Subject: [PATCH] Cygwin: signals: pop return address from signal stack earlier
Commit a942476236b5 ("Cygwin: sigdelayed: pop return address from
signal stack earlier") failed to take two facts into account:
- _cygtls::call_signal_handler() potentially needs the return address
as well, and
- the signal handler may be interrupted by another signal.
Revert the change in sigdelayed() and handle the signal stack manipulation
in _cygtls::call_signal_handler() instead.
Fixes: a942476236b5 ("Cygwin: sigdelayed: pop return address from signal stack
earlier")
Co-authored-by: Corinna Vinschen <cori...@vinschen.de>
Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp>
Signed-off-by: Corinna Vinschen <cori...@vinschen.de>
---
winsup/cygwin/exceptions.cc | 27 +++++++++++++++++++++++++++
winsup/cygwin/scripts/gendef | 24 ++++++------------------
2 files changed, 33 insertions(+), 18 deletions(-)
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index c9fe6a38693c..2e25aa214a2c 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1758,6 +1758,12 @@ _cygtls::call_signal_handler ()
reset_signal_arrived ();
incyg = false;
current_sig = 0; /* Flag that we can accept another signal */
+
+ /* We have to fetch the original return address from the signal stack
+ prior to calling the signal handler. This avoids filling up the
+ signal stack if the signal handler longjumps (longjmp/setcontext). */
+ __tlsstack_t orig_retaddr = pop ();
+ __tlsstack_t *orig_stackptr = stackptr;
unlock (); /* unlock signal stack */
/* Alternate signal stack requested for this signal and alternate signal
@@ -1834,6 +1840,27 @@ _cygtls::call_signal_handler ()
signal handler. */
thisfunc (thissig, &thissi, thiscontext);
+ lock ();
+ switch (stackptr - orig_stackptr)
+ {
+ case 2: /* sigdelayed + added retaddr, pop sigdelayed */
+ pop ();
+ fallthrough;
+ case 1: /* added retaddr */
+ {
+ __tlsstack_t added_retaddr = pop();
+ push (orig_retaddr);
+ push (added_retaddr);
+ }
+ break;
+ case 0:
+ push (orig_retaddr);
+ break;
+ default:
+ api_fatal ("Signal stack corrupted (%D)?", stackptr - orig_stackptr);
+ }
+ unlock ();
+
incyg = true;
set_signal_mask (_my_tls.sigmask, (this_sa_flags & SA_SIGINFO)
diff --git a/winsup/cygwin/scripts/gendef b/winsup/cygwin/scripts/gendef
index e3bcae5b7351..521550175f98 100755
--- a/winsup/cygwin/scripts/gendef
+++ b/winsup/cygwin/scripts/gendef
@@ -161,7 +161,7 @@ _sigbe: #
return here after cygwin syscall
jz 2f # if so
pause
jmp 1b # and loop
-2: movq \$-8,%r11 # decrement signal stack
+2: movq \$-8,%r11 # now decrement aux stack
xaddq %r11,_cygtls.stackptr(%r10) # and get pointer
movq -8(%r11),%r11 # get return address from
signal stack
decl _cygtls.incyg(%r10)
@@ -250,16 +250,6 @@ sigdelayed:
movq %gs:8,%r12 # get tls
movl _cygtls.saved_errno(%r12),%r15d # temporarily save saved_errno
-
- # We have to fetch the original return address from the signal stack
- # prior to calling the signal handler. This avoids filling up the
- # signal stack if the signal handler longjumps (longjmp/setcontext).
- # Store the return address in a callee-saved register (r13).
- movq \$-8,%r11 # decrement signal stack
- xaddq %r11,_cygtls.stackptr(%r12) # and get pointer
- xorq %r13,%r13
- xchgq %r13,-8(%r11) # get return address from
signal stack
-
movq \$_cygtls.start_offset,%rcx # point to beginning of tls
block
addq %r12,%rcx # and store as first arg to
method
call _ZN7_cygtls19call_signal_handlerEv # call handler
@@ -270,13 +260,15 @@ sigdelayed:
jz 2f # if so
pause
jmp 1b # and loop
-
2: testl %r15d,%r15d # was saved_errno < 0
jl 3f # yup. ignore it
movq _cygtls.errno_addr(%r12),%r11
movl %r15d,(%r11)
-
-3: xorl %r11d,%r11d
+3: movq \$-8,%r11 # now decrement aux stack
+ xaddq %r11,_cygtls.stackptr(%r12) # and get pointer
+ xorq %r10,%r10
+ xchgq %r10,-8(%r11) # get return address from
signal stack
+ xorl %r11d,%r11d
movl %r11d,_cygtls.incyg(%r12)
movl %r11d,_cygtls.stacklock(%r12) # release lock
@@ -293,10 +285,6 @@ sigdelayed:
movl 0x24(%rsp),%ebx
addq %rbx,%rsp
- # Before restoring callee-saved registers, move return address from
- # callee-saved r13 to caller-saved r10.
- movq %r13, %r10
-
popq %rax
popq %rbx
popq %rcx
--
2.48.1
--
Problem reports: https://cygwin.com/problems.html
FAQ: https://cygwin.com/faq/
Documentation: https://cygwin.com/docs.html
Unsubscribe info: https://cygwin.com/ml/#unsubscribe-simple
