bin/78304: Signal handler abuse in comsat(8)

[Gavin Atkinson]

>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 at 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: