From 321dfd55d4d37ad7060272abf933adaf9d22f7b6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 10 Nov 2007 05:18:17 +0000 Subject: [PATCH] r16585@catbus: nickm | 2007-11-10 00:16:11 -0500 Patch from Christopher Layne: Make event_del() restore previous signal handlers, not the default. svn:r506 --- ChangeLog | 1 + evsignal.h | 8 +++++ signal.c | 98 +++++++++++++++++++++++++++++++++++++++++--------- test/regress.c | 76 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 166 insertions(+), 17 deletions(-) diff --git a/ChangeLog b/ChangeLog index f2b34b54..dbebaebf 100644 --- a/ChangeLog +++ b/ChangeLog @@ -47,4 +47,5 @@ Changes in current version: o Add new evutil_timer* functions to wrap (or replace) the regular timeval manipulation functions. o Fix many build issues when using the Microsoft C compiler. o Remove a bash-ism in autogen.sh + o When calling event_del on a signal, restore the signal handler's previous value rather than setting it to SIG_DFL. Patch from Christopher Layne. diff --git a/evsignal.h b/evsignal.h index 7efbcabe..0d1e8314 100644 --- a/evsignal.h +++ b/evsignal.h @@ -27,6 +27,8 @@ #ifndef _EVSIGNAL_H_ #define _EVSIGNAL_H_ +typedef void (*ev_sighandler_t)(int); + struct evsignal_info { struct event_list signalqueue; struct event ev_signal; @@ -34,6 +36,12 @@ struct evsignal_info { int ev_signal_added; volatile sig_atomic_t evsignal_caught; sig_atomic_t evsigcaught[NSIG]; +#ifdef HAVE_SIGACTION + struct sigaction **sh_old; +#else + ev_sighandler_t **sh_old; +#endif + int sh_old_max; }; void evsignal_init(struct event_base *); void evsignal_process(struct event_base *); diff --git a/signal.c b/signal.c index 3636b4f4..22acd386 100644 --- a/signal.c +++ b/signal.c @@ -82,7 +82,6 @@ evsignal_cb(int fd, short what, void *arg) n = recv(fd, signals, sizeof(signals), 0); if (n == -1) event_err(1, "%s: read", __func__); - event_add(ev, NULL); } #ifdef HAVE_SETFD @@ -107,13 +106,15 @@ evsignal_init(struct event_base *base) FD_CLOSEONEXEC(base->sig.ev_signal_pair[0]); FD_CLOSEONEXEC(base->sig.ev_signal_pair[1]); + base->sig.sh_old = NULL; + base->sig.sh_old_max = 0; base->sig.evsignal_caught = 0; memset(&base->sig.evsigcaught, 0, sizeof(sig_atomic_t)*NSIG); evutil_make_socket_nonblocking(base->sig.ev_signal_pair[0]); - event_set(&base->sig.ev_signal, base->sig.ev_signal_pair[1], EV_READ, - evsignal_cb, &base->sig.ev_signal); + event_set(&base->sig.ev_signal, base->sig.ev_signal_pair[1], + EV_READ | EV_PERSIST, evsignal_cb, &base->sig.ev_signal); base->sig.ev_signal.ev_base = base; base->sig.ev_signal.ev_flags |= EVLIST_INTERNAL; } @@ -124,32 +125,68 @@ evsignal_add(struct event *ev) int evsignal; #ifdef HAVE_SIGACTION struct sigaction sa; +#else + ev_sighandler_t sh; #endif struct event_base *base = ev->ev_base; + struct evsignal_info *sig = &ev->ev_base->sig; + void *p; if (ev->ev_events & (EV_READ|EV_WRITE)) event_errx(1, "%s: EV_SIGNAL incompatible use", __func__); evsignal = EVENT_SIGNAL(ev); + /* + * resize saved signal handler array up to the highest signal number. + * a dynamic array is used to keep footprint on the low side. + */ + if (evsignal >= sig->sh_old_max) { + event_debug(("%s: evsignal > sh_old_max, resizing array", + __func__, evsignal, sig->sh_old_max)); + sig->sh_old_max = evsignal + 1; + p = realloc(sig->sh_old, sig->sh_old_max * sizeof *sig->sh_old); + if (p == NULL) { + event_warn("realloc"); + return (-1); + } + sig->sh_old = p; + } + + /* allocate space for previous handler out of dynamic array */ + sig->sh_old[evsignal] = malloc(sizeof *sig->sh_old[evsignal]); + if (sig->sh_old[evsignal] == NULL) { + event_warn("malloc"); + return (-1); + } + #ifdef HAVE_SIGACTION + /* setup new handler */ memset(&sa, 0, sizeof(sa)); sa.sa_handler = evsignal_handler; - sigfillset(&sa.sa_mask); sa.sa_flags |= SA_RESTART; + sigfillset(&sa.sa_mask); + + /* save previous handler setup */ + if (sigaction(evsignal, &sa, sig->sh_old[evsignal]) == -1) { + event_warn("sigaction"); + free(sig->sh_old[evsignal]); + return (-1); + } +#else + /* save previous handler setup */ + if ((sh = signal(evsignal, evsignal_handler)) == SIG_ERR) { + event_warn("signal"); + free(sig->sh_old[evsignal]); + return (-1); + } + *sig->sh_old[evsignal] = sh; +#endif /* catch signals if they happen quickly */ evsignal_base = base; - if (sigaction(evsignal, &sa, NULL) == -1) - return (-1); -#else - evsignal_base = base; - if (signal(evsignal, evsignal_handler) == SIG_ERR) - return (-1); -#endif - - if (!base->sig.ev_signal_added) { - base->sig.ev_signal_added = 1; - event_add(&base->sig.ev_signal, NULL); + if (!sig->ev_signal_added) { + sig->ev_signal_added = 1; + event_add(&sig->ev_signal, NULL); } return (0); @@ -158,11 +195,34 @@ evsignal_add(struct event *ev) int evsignal_del(struct event *ev) { + int evsignal, ret = 0; + struct event_base *base = ev->ev_base; + struct evsignal_info *sig = &ev->ev_base->sig; #ifdef HAVE_SIGACTION - return (sigaction(EVENT_SIGNAL(ev),(struct sigaction *)SIG_DFL, NULL)); + struct sigaction *sh; #else - return (signal(EVENT_SIGNAL(ev),SIG_DFL))==SIG_ERR ? -1 : 0; + ev_sighandler_t *sh; #endif + + evsignal = EVENT_SIGNAL(ev); + + /* restore previous handler */ + sh = sig->sh_old[evsignal]; + sig->sh_old[evsignal] = NULL; +#ifdef HAVE_SIGACTION + if (sigaction(evsignal, sh, NULL) == -1) { + event_warn("sigaction"); + ret = -1; + } +#else + if (signal(evsignal, *sh) == SIG_ERR) { + event_warn("signal"); + ret = -1; + } +#endif + free(sh); + + return ret; } static void @@ -220,4 +280,8 @@ evsignal_dealloc(struct event_base *base) base->sig.ev_signal_pair[0] = -1; EVUTIL_CLOSESOCKET(base->sig.ev_signal_pair[1]); base->sig.ev_signal_pair[1] = -1; + base->sig.sh_old_max = 0; + + /* per index frees are handled in evsignal_del() */ + free(base->sig.sh_old); } diff --git a/test/regress.c b/test/regress.c index a95f7d6a..105ac504 100644 --- a/test/regress.c +++ b/test/regress.c @@ -191,6 +191,11 @@ timeout_cb(int fd, short event, void *arg) test_ok = 1; } +void signal_cb_sa(int sig) +{ + test_ok = 2; +} + void signal_cb(int fd, short event, void *arg) { @@ -555,6 +560,75 @@ test_signal_switchbase(void) event_base_free(base2); cleanup_test(); } + +/* + * assert that a signal event removed from the event queue really is + * removed - with no possibility of it's parent handler being fired. + */ +void +test_signal_assert() +{ + struct event ev; + struct event_base *base = event_init(); + printf("Signal handler assert: "); + /* use SIGCONT so we don't kill ourselves when we signal to nowhere */ + signal_set(&ev, SIGCONT, signal_cb, &ev); + signal_add(&ev, NULL); + /* + * if signal_del() fails to reset the handler, it's current handler + * will still point to evsignal_handler(). + */ + signal_del(&ev); + + raise(SIGCONT); + /* only way to verify we were in evsignal_handler() */ + if (base->sig.evsignal_caught) + test_ok = 0; + else + test_ok = 1; + + event_base_free(base); + cleanup_test(); + return; +} + +/* + * assert that we restore our previous signal handler properly. + */ +void +test_signal_restore() +{ + struct event ev; + struct event_base *base = event_init(); +#ifdef HAVE_SIGACTION + struct sigaction sa; +#endif + + test_ok = 0; + printf("Signal handler restore: "); +#ifdef HAVE_SIGACTION + sa.sa_handler = signal_cb_sa; + sa.sa_flags = 0x0; + sigemptyset(&sa.sa_mask); + if (sigaction(SIGUSR1, &sa, NULL) == -1) + goto out; +#else + if (signal(SIGUSR1, signal_cb_sa) == SIG_ERR) + goto out; +#endif + signal_set(&ev, SIGUSR1, signal_cb, &ev); + signal_add(&ev, NULL); + signal_del(&ev); + + raise(SIGUSR1); + /* 1 == signal_cb, 2 == signal_cb_sa, we want our previous handler */ + if (test_ok != 2) + test_ok = 0; +out: + event_base_free(base); + cleanup_test(); + return; +} #endif void @@ -1116,6 +1190,8 @@ main (int argc, char **argv) test_signal_dealloc(); test_signal_pipeloss(); test_signal_switchbase(); + test_signal_restore(); + test_signal_assert(); #endif return (0);