[ 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
