Date: Wed, 2 Mar 2005 15:16:05 GMT From: Gavin Atkinson <gavin.atkinson@ury.york.ac.uk> To: FreeBSD-gnats-submit@FreeBSD.org Subject: bin/78304: Signal handler abuse in comsat(8) Message-ID: <200503021516.j22FG5nL081678@buffy.york.ac.uk> Resent-Message-ID: <200503021520.j22FKE5Y080986@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 78304 >Category: bin >Synopsis: Signal handler abuse in comsat(8) >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Wed Mar 02 15:20:13 GMT 2005 >Closed-Date: >Last-Modified: >Originator: Gavin Atkinson >Release: FreeBSD 5.3-STABLE i386 >Organization: >Environment: System: FreeBSD buffy.york.ac.uk 5.3-STABLE FreeBSD 5.3-STABLE #0: Sat Feb 12 20:42:16 GMT 2005 root@buffy.york.ac.uk:/usr/obj/usr/src/sys/BUFFY i386 >Description: I've been inspired by the talk given by Henning Brauer given at the UKUUG Winter 2005 on writing safe signal handlers, and have set about fixing some of them in the FreeBSD source tree. Slides for this (well, an almost identical) presentation are at http://www.openbsd.org/papers/opencon04/index.html The comsat daemon hides particularly nasty abuses of signals. In one signal handler, it failes to save and restore errno around a call that can change it, which will cause problems if the main routine is interrupted at the wrong time. In another signal handler, a large number of functions are called which are not guaranteed to be safe to call within the context of a signal handler. Some of these functions (e.g. realloc()) may be particularly dangerous as the handler could possibly be called while the main routine is already in the memory allocator. This latter signal has been fixed to simply set a flag, which is now polled and acted upon in the main loop. >How-To-Repeat: N/A >Fix: --- comsat-sigs.patch begins here --- Index: src/libexec/comsat/comsat.c =================================================================== RCS file: /usr/cvs/src/libexec/comsat/comsat.c,v retrieving revision 1.17 diff -u -r1.17 comsat.c --- src/libexec/comsat/comsat.c 14 Feb 2005 17:42:56 -0000 1.17 +++ src/libexec/comsat/comsat.c 27 Feb 2005 23:04:47 -0000 @@ -77,11 +77,13 @@ struct utmp *utmp = NULL; time_t lastmsgtime; int nutmp, uf; +volatile sig_atomic_t needreadutmp = 0; void jkfprintf(FILE *, char[], char[], off_t); void mailfor(char *); void notify(struct utmp *, char[], off_t, int); void onalrm(int); +void readutmp(void); void reapchildren(int); int @@ -109,7 +111,7 @@ } (void)time(&lastmsgtime); (void)gethostname(hostname, sizeof(hostname)); - onalrm(0); + readutmp(); (void)signal(SIGALRM, onalrm); (void)signal(SIGTTOU, SIG_IGN); (void)signal(SIGCHLD, reapchildren); @@ -121,6 +123,10 @@ errno = 0; continue; } + if (needreadutmp) { + needreadutmp = 0; + readutmp(); + } if (!nutmp) /* no one has logged in yet */ continue; sigblock(sigmask(SIGALRM)); @@ -134,12 +140,22 @@ void reapchildren(int signo) { - while (wait3(NULL, WNOHANG, NULL) > 0); + int save_errno = errno; + + while (wait3(NULL, WNOHANG, NULL) > 0) + ; + errno = save_errno; } void onalrm(int signo) { + needreadutmp = 1; +} + +void +readutmp(void) +{ static u_int utmpsize; /* last malloced size for utmp */ static u_int utmpmtime; /* last modification time for utmp */ struct stat statbf; --- comsat-sigs.patch ends here --- >Release-Note: >Audit-Trail: >Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200503021516.j22FG5nL081678>