I suspect I haven't gone far enough.
Simplify and correct signal handling in tcpdump:
* RETSIGTYPE == void, so kill the define for it
* simplify and strengthen setsignal():
* we have sigaction(), so use it
* in general, CLI tools shouldn't catch a signal that was ignored on
entry. Enforce that here, which eliminates the need to return the
previous signal disposition
* for SIGCHLD, use SA_NOCLDSTOP to avoid superfluous signals if the
child is stopped
* with setsignal() Doing The Right Thing, set_slave_signals() is *really*
simple
As I understand it, we're not tracking upstream tcpdump, so I don't think
this is a (new) hinderance for merges.
ok?
Philip Guenther
PS: on OpenBSD, execve(2) resets SIGCHLD to SIG_DFL if it was set to
SIG_IGN. Doing otherwise breaks lots of programs, so it's explicitly
permitted by POSIX...but we don't seem to document that. Expect a manpage
diff after this...
Index: Makefile
===================================================================
RCS file: /data/src/openbsd/src/usr.sbin/tcpdump/Makefile,v
retrieving revision 1.57
diff -u -p -r1.57 Makefile
--- Makefile 10 Oct 2015 07:52:30 -0000 1.57
+++ Makefile 11 Oct 2015 05:21:58 -0000
@@ -28,7 +28,7 @@ CFLAGS+=-Wall -I${.CURDIR}/../../sbin/pf
# for pcap-int.h
CFLAGS+=-I${.CURDIR}/../../lib/libpcap
-CFLAGS+=-DCSLIP -DPPP -DHAVE_FDDI -DETHER_SERVICE -DRETSIGTYPE=void
-DHAVE_NET_SLIP_H -DHAVE_ETHER_NTOHOST -DINET6
+CFLAGS+=-DCSLIP -DPPP -DHAVE_FDDI -DETHER_SERVICE -DHAVE_NET_SLIP_H
-DHAVE_ETHER_NTOHOST -DINET6
LDADD+= -lpcap -ll -lcrypto -lm
DPADD+= ${LIBL} ${LIBPCAP} ${LIBCRYPTO}
Index: setsignal.c
===================================================================
RCS file: /data/src/openbsd/src/usr.sbin/tcpdump/setsignal.c,v
retrieving revision 1.5
diff -u -p -r1.5 setsignal.c
--- setsignal.c 5 Apr 2015 17:02:57 -0000 1.5
+++ setsignal.c 11 Oct 2015 05:47:35 -0000
@@ -23,52 +23,23 @@
#include <sys/types.h>
-#ifdef HAVE_MEMORY_H
-#include <memory.h>
-#endif
#include <signal.h>
-#ifdef HAVE_SIGACTION
#include <string.h>
-#endif
-
-#ifdef HAVE_OS_PROTO_H
-#include "os-proto.h"
-#endif
#include "setsignal.h"
-/*
- * An os independent signal() with BSD semantics, e.g. the signal
- * catcher is restored following service of the signal.
- *
- * When sigset() is available, signal() has SYSV semantics and sigset()
- * has BSD semantics and call interface. Unfortunately, Linux does not
- * have sigset() so we use the more complicated sigaction() interface
- * there.
- *
- * Did I mention that signals suck?
- */
-RETSIGTYPE
-(*setsignal (int sig, RETSIGTYPE (*func)(int)))(int)
+void
+setsignal(int sig, void (*func)(int))
{
-#ifdef HAVE_SIGACTION
- struct sigaction old, new;
-
- memset(&new, 0, sizeof(new));
- new.sa_handler = func;
-#ifdef SA_RESTART
- new.sa_flags |= SA_RESTART;
-#endif
- if (sigaction(sig, &new, &old) < 0)
- return (SIG_ERR);
- return (old.sa_handler);
+ struct sigaction sa;
-#else
-#ifdef HAVE_SIGSET
- return (sigset(sig, func));
-#else
- return (signal(sig, func));
-#endif
-#endif
+ if (sigaction(sig, NULL, &sa) == 0 && sa.sa_handler != SIG_IGN) {
+ sa.sa_handler = func;
+ sa.sa_flags = SA_RESTART;
+ if (sig == SIGCHLD)
+ sa.sa_flags |= SA_NOCLDSTOP;
+ sigemptyset(&sa.sa_mask);
+ sigaction(sig, &sa, NULL);
+ }
}
Index: setsignal.h
===================================================================
RCS file: /data/src/openbsd/src/usr.sbin/tcpdump/setsignal.h,v
retrieving revision 1.3
diff -u -p -r1.3 setsignal.h
--- setsignal.h 7 Oct 2007 16:41:05 -0000 1.3
+++ setsignal.h 11 Oct 2015 05:21:02 -0000
@@ -25,5 +25,5 @@
#ifndef setsignal_h
#define setsignal_h
-RETSIGTYPE (*setsignal(int, RETSIGTYPE (*)(int)))(int);
+void setsignal(int, void (*)(int));
#endif
Index: tcpdump.c
===================================================================
RCS file: /data/src/openbsd/src/usr.sbin/tcpdump/tcpdump.c,v
retrieving revision 1.74
diff -u -p -r1.74 tcpdump.c
--- tcpdump.c 9 Oct 2015 01:37:09 -0000 1.74
+++ tcpdump.c 11 Oct 2015 05:48:13 -0000
@@ -92,8 +92,8 @@ extern void bpf_dump(struct bpf_program
extern int esp_init(char *);
/* Forwards */
-RETSIGTYPE cleanup(int);
-RETSIGTYPE gotchld(int);
+void cleanup(int);
+void gotchld(int);
extern __dead void usage(void);
/* Length of saved portion of packet. */
@@ -503,8 +503,7 @@ main(int argc, char **argv)
}
/* make a clean exit on interrupts */
-/* ARGSUSED */
-RETSIGTYPE
+void
cleanup(int signo)
{
struct pcap_stat stat;
@@ -533,8 +532,7 @@ cleanup(int signo)
_exit(0);
}
-/* ARGSUSED */
-RETSIGTYPE
+void
gotchld(int signo)
{
pid_t pid;
@@ -681,14 +679,10 @@ default_print(register const u_char *bp,
void
set_slave_signals(void)
{
- RETSIGTYPE (*oldhandler)(int);
-
setsignal(SIGTERM, cleanup);
setsignal(SIGINT, cleanup);
setsignal(SIGCHLD, gotchld);
- /* Cooperate with nohup(1) XXX is this still necessary/working? */
- if ((oldhandler = setsignal(SIGHUP, cleanup)) != SIG_DFL)
- (void)setsignal(SIGHUP, oldhandler);
+ setsignal(SIGHUP, cleanup);
}
__dead void
