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

@@ -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.
 

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 *);

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);
 }

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);