On Mar 14 20:35, Takashi Yano via Cygwin wrote:
> On Fri, 14 Mar 2025 11:01:25 +0100
> Corinna Vinschen wrote:
> > I don't think so. I was mulling in circles over this tonight
> > (don't ask me how I slept!) and came to the same conclusion.
> > But here's the problem:
> >
> > I'm simply not 100% sure.
> >
> > What concerns me is that stackptr points beyond stack if the stack
> > is full (i.e., sigdelayed + return address).
> >
> > That was what happened before I applied a942476236b5: stackptr was
> > incremented until it pointed at _cygtls::initialized, and eventually it
> > overwrote it. Fortunately, that stopped further incrementing due to the
> > isinitialized() test.
> >
> > So, if there *is* a twisted situation which results in pushing another
> > return address onto the stack, a stack size of 2 would again result in
> > initialized being overwritten. So I wonder if we should keep kind of
> > an airbag for an unusual situation. Plus trying to keep stackptr inside
> > stack even if it's full. So that stackptr never grows into initialized:
> >
> > #define TLS_STACK_SIZE 5
> >
> > and
> >
> > void push (__tlsstack_t addr)
> > {
> > if (stackptr < (__tlsstack_t *) &initialized)
> > *stackptr++ = (__tlsstack_t) addr;
> > }
> >
> > What do you think?
>
> Yeah. We do not have to minimize the stack space at the cost of
> taking risks.
>
> One more thing. I am also concerned that pop() lacks a guard.
> If pop() calls when stack is empty, then push() destroys the
> stackptr pointer value.
Good point. I attach a new proposal. It also doesn't check against
&initialized (becasue that doesn't avoid an overflow into initialized,
but against the last slot of the stack. This also moves pop into
the C++ code and drops its assembler counterpart.
Ok?
THanks,
Corinna
>From 17b7ee2a89540e6a8f60539fb3a8e27f61e42646 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/local_includes/cygtls.h | 19 ++++++++++++---
winsup/cygwin/scripts/gendef | 35 +++++----------------------
3 files changed, 49 insertions(+), 32 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/local_includes/cygtls.h
b/winsup/cygwin/local_includes/cygtls.h
index dfd3198435a4..079ada99a762 100644
--- a/winsup/cygwin/local_includes/cygtls.h
+++ b/winsup/cygwin/local_includes/cygtls.h
@@ -26,7 +26,9 @@ details. */
# define UNLEN 256
#endif
-#define TLS_STACK_SIZE 256
+/* Room for two full frames including an extra sigdelayed, plus an
+ empty slot so stackptr never grows beyond the stack. */
+#define TLS_STACK_SIZE 5
#include "cygthread.h"
@@ -206,8 +208,19 @@ public: /* Do NOT remove this public: line, it's a marker
for gentls_offsets. */
void init_thread (void *, DWORD (*) (void *, void *));
static void call (DWORD (*) (void *, void *), void *);
void remove (DWORD);
- void push (__tlsstack_t addr) {*stackptr++ = (__tlsstack_t) addr;}
- __tlsstack_t pop ();
+ void push (__tlsstack_t addr)
+ {
+ /* Make sure stackptr never points beyond stack (to initialized). */
+ if (stackptr < (__tlsstack_t *) stack + TLS_STACK_SIZE - 1)
+ *stackptr++ = (__tlsstack_t) addr;
+ }
+ __tlsstack_t pop ()
+ {
+ /* Make sure stackptr never points below stack (to itself). */
+ if (stackptr > stack)
+ --stackptr;
+ return *stackptr;
+ }
__tlsstack_t retaddr () {return stackptr[-1];}
bool isinitialized () const
{
diff --git a/winsup/cygwin/scripts/gendef b/winsup/cygwin/scripts/gendef
index e3bcae5b7351..a2f0392bc860 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
@@ -320,17 +308,6 @@ sigdelayed:
_sigdelayed_end:
.global _sigdelayed_end
-# _cygtls::pop
- .global _ZN7_cygtls3popEv
- .seh_proc _ZN7_cygtls3popEv
-_ZN7_cygtls3popEv:
- .seh_endprologue
- movq \$-8,%r11
- xaddq %r11,_cygtls.stackptr_p(%rcx)
- movq -8(%r11),%rax
- ret
- .seh_endproc
-
.seh_proc stabilize_sig_stack
stabilize_sig_stack:
pushq %r12
--
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
