| Message ID | 20230802174947.2496812-1-jcmvbkbc@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [uclibc-ng-devel,v2] linuxthreads/signal: improve sigaction behavior | expand |
Hi Max, Thank you. applied and pushed, best regards Waldemar Max Filippov wrote, > Setting signal handler in the kernel and then updating sighandler[sig] > results in a crash if a signal which handler is being changed from > SIG_DFL to a non-default was pending. Improve the race a little and > update the sighandler[sig] before the sigaction syscall. It doesn't > eliminate the race entirely, but fixes this particular failing case. > > E.g. this fixes the 100% reproducible segfault in the busybox hush shell > built with FEATURE_EDITING_WINCH on ssh client's terminal window resize, > but in that case there's one more even bigger issue: busybox calls > sigaction with both old and new signal pointers pointing to the same > structure instance, as a result act->sa_handler after the sigaction > syscall is not what the user requested, but the previous handler. > > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> > --- > Changes v1 -> v2: > - initialize 'save' with NULL to avoid compiler warning. The code > cannot use uninitialized 'save' value, so no logic change is needed. > > libpthread/linuxthreads/signals.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/libpthread/linuxthreads/signals.c b/libpthread/linuxthreads/signals.c > index 0c0f2b6b1d2a..0cde54a16d27 100644 > --- a/libpthread/linuxthreads/signals.c > +++ b/libpthread/linuxthreads/signals.c > @@ -134,6 +134,7 @@ int sigaction(int sig, const struct sigaction * act, > { > struct sigaction newact; > struct sigaction *newactp; > + void *save = NULL; > > #ifdef DEBUG_PT > printf(__FUNCTION__": pthreads wrapper!\n"); > @@ -142,6 +143,8 @@ printf(__FUNCTION__": pthreads wrapper!\n"); > sig == __pthread_sig_cancel || > (sig == __pthread_sig_debug && __pthread_sig_debug > 0)) > return EINVAL; > + if (sig > 0 && sig < NSIG) > + save = sighandler[sig].old; > if (act) > { > newact = *act; > @@ -154,22 +157,24 @@ printf(__FUNCTION__": pthreads wrapper!\n"); > newact.sa_handler = (__sighandler_t) pthread_sighandler; > } > newactp = &newact; > + if (sig > 0 && sig < NSIG) > + sighandler[sig].old = (arch_sighandler_t) act->sa_handler; > } > else > newactp = NULL; > if (__libc_sigaction(sig, newactp, oact) == -1) > - return -1; > + { > + if (act && sig > 0 && sig < NSIG) > + sighandler[sig].old = save; > + return -1; > + } > #ifdef DEBUG_PT > printf(__FUNCTION__": sighandler installed, sigaction successful\n"); > #endif > if (sig > 0 && sig < NSIG) > { > if (oact != NULL) > - oact->sa_handler = (__sighandler_t) sighandler[sig].old; > - if (act) > - /* For the assignment is does not matter whether it's a normal > - or real-time signal. */ > - sighandler[sig].old = (arch_sighandler_t) act->sa_handler; > + oact->sa_handler = save; > } > return 0; > } > -- > 2.30.2 > > _______________________________________________ > devel mailing list -- devel@uclibc-ng.org > To unsubscribe send an email to devel-leave@uclibc-ng.org >
diff --git a/libpthread/linuxthreads/signals.c b/libpthread/linuxthreads/signals.c index 0c0f2b6b1d2a..0cde54a16d27 100644 --- a/libpthread/linuxthreads/signals.c +++ b/libpthread/linuxthreads/signals.c @@ -134,6 +134,7 @@ int sigaction(int sig, const struct sigaction * act, { struct sigaction newact; struct sigaction *newactp; + void *save = NULL; #ifdef DEBUG_PT printf(__FUNCTION__": pthreads wrapper!\n"); @@ -142,6 +143,8 @@ printf(__FUNCTION__": pthreads wrapper!\n"); sig == __pthread_sig_cancel || (sig == __pthread_sig_debug && __pthread_sig_debug > 0)) return EINVAL; + if (sig > 0 && sig < NSIG) + save = sighandler[sig].old; if (act) { newact = *act; @@ -154,22 +157,24 @@ printf(__FUNCTION__": pthreads wrapper!\n"); newact.sa_handler = (__sighandler_t) pthread_sighandler; } newactp = &newact; + if (sig > 0 && sig < NSIG) + sighandler[sig].old = (arch_sighandler_t) act->sa_handler; } else newactp = NULL; if (__libc_sigaction(sig, newactp, oact) == -1) - return -1; + { + if (act && sig > 0 && sig < NSIG) + sighandler[sig].old = save; + return -1; + } #ifdef DEBUG_PT printf(__FUNCTION__": sighandler installed, sigaction successful\n"); #endif if (sig > 0 && sig < NSIG) { if (oact != NULL) - oact->sa_handler = (__sighandler_t) sighandler[sig].old; - if (act) - /* For the assignment is does not matter whether it's a normal - or real-time signal. */ - sighandler[sig].old = (arch_sighandler_t) act->sa_handler; + oact->sa_handler = save; } return 0; }
Setting signal handler in the kernel and then updating sighandler[sig] results in a crash if a signal which handler is being changed from SIG_DFL to a non-default was pending. Improve the race a little and update the sighandler[sig] before the sigaction syscall. It doesn't eliminate the race entirely, but fixes this particular failing case. E.g. this fixes the 100% reproducible segfault in the busybox hush shell built with FEATURE_EDITING_WINCH on ssh client's terminal window resize, but in that case there's one more even bigger issue: busybox calls sigaction with both old and new signal pointers pointing to the same structure instance, as a result act->sa_handler after the sigaction syscall is not what the user requested, but the previous handler. Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> --- Changes v1 -> v2: - initialize 'save' with NULL to avoid compiler warning. The code cannot use uninitialized 'save' value, so no logic change is needed. libpthread/linuxthreads/signals.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)