[ Warning: this is long and detailed, but includes details of a present bug in libevent. ]
While building libevent with -W -Wall -pedantic, I came across this: signal.c: In function 'evsignal_del': signal.c:162: warning: ISO C forbids conversion of function pointer to object pointer type Which was: 158 int 159 evsignal_del(struct event *ev) 160 { 161 #ifdef HAVE_SIGACTION 162 return (sigaction(EVENT_SIGNAL(ev),(struct sigaction *)SIG_DFL, NULL)); 163 #else 164 return (signal(EVENT_SIGNAL(ev),SIG_DFL))==SIG_ERR ? -1 : 0; 165 #endif 166 } On a hunch, I setup a temporary sigaction struct with it's handler set to SIG_DFL, other flags/masks cleared, and pass it to sigaction like so: 162 return (sigaction(EVENT_SIGNAL(ev), &sa, NULL)); So I run a regression test and strangely start seeing: Signal dealloc: OK Signal pipeloss: OK Signal switchbase: Program terminated with signal SIGUSR1 I then look up the default action (atleast for glibc) for SIGUSR1 and find in glibc's documentation: "The default action is to terminate the process." http://www.gnu.org/software/libc/manual/html_node/Miscellaneous-Signals.html But the original passes the regression tests. I then look up the source for sigaction() in glibc's source and find, as per POSIX, it will not modify the signal handler whatsoever if the 2nd argument is a NULL pointer. But we're originally passing "(struct sigaction *)SIG_DFL" to sigaction(), and with that the regression passes right? $ egrep -r SIG_DFL /usr/include /usr/include/bits/signum.h:#define SIG_DFL ((__sighandler_t) 0) /* Default action. */ Actually we're really passing "(struct sigaction *)0". So sigaction(), as presently called from evsignal_del(), isn't doing anything at all, even though it looks like we're reassigning the default action for a given signal, sigaction() is just getting the equivalent of NULL on probably all platforms. However, even if we succeeded in assigning the default action we should really be assigning the previous action before evsignal_add() ever got it's hands on it. This patch aims to save a given signal's previous handler state, and appropriately restore said state upon removal of the signal from libevent's watch. Due to the nature of struct sigaction being around 16+ bytes on a 32-bit platform, I chose to handle it with a dynamically sized array (even just "struct sigaction *sa_old[NSIG]" is another 260-520 bytes for every event_base used) and a max index variable. Allocations within the array are done by evsignal_add(), and free'd by evsignal_del(). As far as I can tell, given sane management of events flagged as EV_SIGNAL, one should not be able to double-add, or miss a delete. If that becomes true - there are bigger problems. -- Proof of concept example: #include <signal.h> #include <stdlib.h> #include <unistd.h> #include "event.h" void sig_h(int sig) { write(2, "sig_h\n", 6); return; } void sig_ev(int sig, short event, void *a) { write(2, "sig_ev\n", 7); return; } int main(int argc, char **argv) { struct event ev_s; struct event_base *eb; signal(SIGUSR1, sig_h); raise(SIGUSR1); eb = event_init(); event_set(&ev_s, SIGUSR1, EV_SIGNAL, sig_ev, NULL); event_add(&ev_s, NULL); raise(SIGUSR1); event_dispatch(); if (argc > 1) { write(2, "dealloc\n", 8); event_base_free(eb); } raise(SIGUSR1); write(2, "exit\n", 5); return 0; } $ LD_LIBRARY_PATH=.libs ./sig_test # this shows it ignoring our original sig_h sig_ev exit $ LD_LIBRARY_PATH=.libs ./sig_test 1 # this shows it stomping free()d memory sig_h sig_ev dealloc exit valgrind: ==00:00:00:01.420 27461== 1 errors in context 1 of 4: ==00:00:00:01.420 27461== Invalid read of size 4 ==00:00:00:01.420 27461== at 0x401E3A9: evsignal_handler (signal.c:188) ==00:00:00:01.421 27461== by 0x41040897: (within /lib/tls/libc-2.3.4.so) ==00:00:00:01.421 27461== by 0x8048721: main (sig_test.c:37) ==00:00:00:01.421 27461== Address 0x4038094 is 108 bytes inside a block of size 412 free'd ==00:00:00:01.422 27461== at 0x400529F: free (vg_replace_malloc.c:320) ==00:00:00:01.422 27461== by 0x400D48A: event_base_free (event.c:239) ==00:00:00:01.422 27461== by 0x8048715: main (sig_test.c:35) ==00:00:00:01.269 27461== Invalid write of size 4 ==00:00:00:01.269 27461== at 0x401E38E: evsignal_handler (signal.c:180) ==00:00:00:01.269 27461== by 0x41040897: (within /lib/tls/libc-2.3.4.so) ==00:00:00:01.270 27461== by 0x8048721: main (sig_test.c:37) ==00:00:00:01.270 27461== Address 0x40380cc is 164 bytes inside a block of size 412 free'd ==00:00:00:01.270 27461== at 0x400529F: free (vg_replace_malloc.c:320) ==00:00:00:01.270 27461== by 0x400D48A: event_base_free (event.c:239) ==00:00:00:01.271 27461== by 0x8048715: main (sig_test.c:35) [etc] strace: write(2, "dealloc\n", 8dealloc ) = 8 epoll_ctl(3, EPOLL_CTL_DEL, 5, {EPOLLIN, {u32=134533600, u64=134533600}}) = 0 close(4) = 0 close(5) = 0 close(3) = 0 tgkill(27466, 27466, SIGUSR1) = 0 --- SIGUSR1 (User defined signal 1) @ 0 (0) --- send(-1, "a", 1, 0) = -1 EBADF (Bad file descriptor) sigreturn() = ? (mask now []) write(2, "exit\n", 5exit ) = 5 [etc] As you can see, evsignal_handler still going strong even after we're done with any events. [patch libevent, clean, remake] $ LD_LIBRARY_PATH=.libs ./sig_test # now uses original after we remove signal sig_h sig_ev sig_h exit $ LD_LIBRARY_PATH=.libs ./sig_test 1 # still uses original, and doesn't stomp memory sig_h sig_ev dealloc sig_h exit ==00:00:00:00.956 305== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 18 from 1) write(2, "dealloc\n", 8dealloc ) = 8 epoll_ctl(3, EPOLL_CTL_DEL, 5, {EPOLLIN, {u32=134533608, u64=134533608}}) = 0 close(4) = 0 close(5) = 0 close(3) = 0 tgkill(309, 309, SIGUSR1) = 0 --- SIGUSR1 (User defined signal 1) @ 0 (0) --- write(2, "sig_h\n", 6sig_h ) = 6 sigreturn() = ? (mask now []) write(2, "exit\n", 5exit ) = 5 -cl -- Patch: Index: libevent/evsignal.h =================================================================== --- libevent/evsignal.h (revision 504) +++ libevent/evsignal.h (working copy) @@ -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 @@ int ev_signal_added; volatile sig_atomic_t evsignal_caught; sig_atomic_t evsigcaught[NSIG]; +#ifdef HAVE_SIGACTION + struct sigaction **sa_old; +#else + ev_sighandler_t **sh_old; +#endif + int sig_max; }; void evsignal_init(struct event_base *); void evsignal_process(struct event_base *); Index: libevent/signal.c =================================================================== --- libevent/signal.c (revision 504) +++ libevent/signal.c (working copy) @@ -107,6 +107,12 @@ FD_CLOSEONEXEC(base->sig.ev_signal_pair[0]); FD_CLOSEONEXEC(base->sig.ev_signal_pair[1]); +#ifdef HAVE_SIGACTION + base->sig.sa_old = NULL; +#else + base->sig.sh_old = NULL; +#endif + base->sig.sig_max = 0; base->sig.evsignal_caught = 0; memset(&base->sig.evsigcaught, 0, sizeof(sig_atomic_t)*NSIG); @@ -126,12 +132,39 @@ struct sigaction sa; #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->sig_max) { + event_debug(("%s: evsignal > sig_max, resizing array", + __func__, evsignal, sig->sig_max)); + sig->sig_max = evsignal + 1; #ifdef HAVE_SIGACTION + p = realloc(sig->sa_old, sig->sig_max * sizeof *sig->sa_old); +#else + p = realloc(sig->sh_old, sig->sig_max * sizeof *sig->sh_old); +#endif + if (p == NULL) { + event_warn("realloc"); + return (-1); + } +#ifdef HAVE_SIGACTION + sig->sa_old = p; +#else + sig->sh_old = p; +#endif + } + +#ifdef HAVE_SIGACTION + /* setup new handler */ memset(&sa, 0, sizeof(sa)); sa.sa_handler = evsignal_handler; sigfillset(&sa.sa_mask); @@ -139,17 +172,41 @@ /* catch signals if they happen quickly */ evsignal_base = base; - if (sigaction(evsignal, &sa, NULL) == -1) + /* allocate space for previous handler out of dynamic array */ + sig->sa_old[evsignal] = malloc(sizeof *sig->sa_old[evsignal]); + if (sig->sa_old[evsignal] == NULL) { + event_warn("malloc"); return (-1); + } + /* save previous handler setup */ + if (sigaction(evsignal, NULL, sig->sa_old[evsignal]) == -1 + || sigaction(evsignal, &sa, NULL) == -1) + { + event_warn("sigaction"); + free(sig->sa_old[evsignal]); + return (-1); + } #else evsignal_base = base; - if (signal(evsignal, evsignal_handler) == SIG_ERR) + + /* 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); + } + /* save previous handler setup */ + *sig->sh_old[evsignal] = signal(evsignal, evsignal_handler); + if (*sig->sh_old[evsignal] == SIG_ERR) { + event_warn("signal"); + free(sig->sh_old[evsignal]); + 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 +215,35 @@ 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 *sa; #else - return (signal(EVENT_SIGNAL(ev),SIG_DFL))==SIG_ERR ? -1 : 0; + evsighandler_t *sh; #endif + + evsignal = EVENT_SIGNAL(ev); + +#ifdef HAVE_SIGACTION + /* restore previous handler */ + sa = sig->sa_old[evsignal]; + sig->sa_old[evsignal] = NULL; + if ((ret = sigaction(evsignal, sa, NULL)) == -1) + event_warn("sigaction"); + free(sa); +#else + /* restore previous handler */ + sh = sig->sh_old[evsignal]; + sig->sh_old[evsignal] = NULL; + if (signal(evsignal, sh) == SIG_ERR) { + event_warn("signal"); + ret = -1; + } + free(sh); +#endif + return ret; } static void @@ -220,4 +301,12 @@ base->sig.ev_signal_pair[0] = -1; EVUTIL_CLOSESOCKET(base->sig.ev_signal_pair[1]); base->sig.ev_signal_pair[1] = -1; + base->sig.sig_max = 0; + + /* per index frees are handled in evsignal_del() */ +#ifdef HAVE_SIGACTION + free(base->sig.sa_old); +#else + free(base->sig.sh_old); +#endif } _______________________________________________ Libevent-users mailing list Libevent-users@monkey.org http://monkeymail.org/mailman/listinfo/libevent-users