On Fri, Nov 09, 2007 at 06:23:33PM -0500, Nick Mathewson wrote:
> On Fri, Nov 09, 2007 at 06:27:04AM -0800, Christopher Layne wrote:
> > [ Warning: this is long and detailed, but includes details of a present
> > bug in libevent. ]
>
>
> Hi, Christopher! This all seems very reasonable to me. Would it be
> hard to turn change your proof of concept code into a regresssion
> test? If not, I'd really appreciate that, as it would make applying
> your patch a no-brainer.
>
> Yrs,
> --
> Nick Mathewson
I went ahead and added regression tests for this. I also cleaned up the
patch to fix a couple of things pointed out by Scott Lamb and simplified
the conditional compilation business to use the same named struct member.
Also built with "#define HAVE_SIGACTION 0" and all is good there.
Before:
[...]
Signal dealloc: OK
Signal pipeloss: OK
Signal switchbase: OK
Signal handler restore: FAILED
After:
[...]
Signal dealloc: OK
Signal pipeloss: OK
Signal switchbase: OK
Signal handler restore: OK
Signal handler assert: OK
BTW: Should we change the regress test to not exit(1) immediately on
failure and instead exit with error at the end if any test failed?
-cl
--
Index: libevent/test/regress.c
===================================================================
--- libevent/test/regress.c (revision 504)
+++ libevent/test/regress.c (working copy)
@@ -191,6 +191,11 @@
test_ok = 1;
}
+void signal_cb_sa(int sig)
+{
+ test_ok = 2;
+}
+
void
signal_cb(int fd, short event, void *arg)
{
@@ -555,8 +560,77 @@
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
test_free_active_base(void)
{
@@ -1116,6 +1190,8 @@
test_signal_dealloc();
test_signal_pipeloss();
test_signal_switchbase();
+ test_signal_restore();
+ test_signal_assert();
#endif
return (0);
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 **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 *);
Index: libevent/signal.c
===================================================================
--- libevent/signal.c (revision 504)
+++ libevent/signal.c (working copy)
@@ -82,7 +82,6 @@
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 @@
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 @@
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;
+ sa.sa_flags |= SA_RESTART;
sigfillset(&sa.sa_mask);
- sa.sa_flags |= SA_RESTART;
- /* catch signals if they happen quickly */
- evsignal_base = base;
- if (sigaction(evsignal, &sa, NULL) == -1)
+ /* save previous handler setup */
+ if (sigaction(evsignal, &sa, sig->sh_old[evsignal]) == -1) {
+ event_warn("sigaction");
+ free(sig->sh_old[evsignal]);
return (-1);
+ }
#else
- evsignal_base = base;
- if (signal(evsignal, evsignal_handler) == SIG_ERR)
+ /* 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 (!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 @@
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 @@
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);
}
_______________________________________________
Libevent-users mailing list
[email protected]
http://monkeymail.org/mailman/listinfo/libevent-users
