Message ID | 20230731220340.1487276-1-jcmvbkbc@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [uclibc-ng-devel] linuxthreads/signal: improve sigaction behavior | expand |
Hi Max, there is a compiler warning with this patch added: /home/wbx/embedded-test/openadk/toolchain_qemu-arc_uclibc-ng_archs/usr/bin/arc-openadk-linux-uclibc-gcc -c libpthread/linuxthreads/signals.c -o libpthread/linuxthreads/signals.os -Wall -Wstrict-prototypes -Wstrip libpthread/linuxthreads/signals.c: In function 'sigaction': libpthread/linuxthreads/signals.c:168:29: warning: 'save' may be used uninitialized [-Wmaybe-uninitialized] 168 | sighandler[sig].old = save; | ~~~~~~~~~~~~~~~~~~~~^~~~~~ libpthread/linuxthreads/signals.c:137:9: note: 'save' was declared here 137 | void *save; | ^~~~ Can this be avoided? Thanks for the patch. 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> > --- > 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; > > #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; #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> --- libpthread/linuxthreads/signals.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)