fixed signal handler race condition

beep.c

@@ -16,6 +16,7 @@
  * Bug me, I like it:  http://johnath.com/  or johnath@johnath.com
  */
 
+#include <stdbool.h>
 #include <fcntl.h>
 #include <getopt.h>
 #include <signal.h>
@@ -92,12 +93,9 @@ enum { BEEP_TYPE_CONSOLE, BEEP_TYPE_EVDEV };
 
 /* Momma taught me never to use globals, but we need something the signal 
    handlers can get at.*/
-int console_fd = -1;
-int console_type = BEEP_TYPE_CONSOLE;
-char *console_device = NULL;
+volatile sig_atomic_t stop = false;
 
-
-void do_beep(int freq) {
+void do_beep(int freq, int console_fd, int console_type) {
   if (console_type == BEEP_TYPE_CONSOLE) {
     if(ioctl(console_fd, KIOCSOUND, freq != 0
       ? (int)(CLOCK_TICK_RATE/freq)
@@ -121,22 +119,7 @@ void do_beep(int freq) {
 /* If we get interrupted, it would be nice to not leave the speaker beeping in
    perpetuity. */
 void handle_signal(int signum) {
-
-  if(console_device)
-    free(console_device);
-
-  switch(signum) {
-  case SIGINT:
-    if(console_fd >= 0) {
-      /* Kill the sound, quit gracefully */
-      do_beep(0);
-      close(console_fd);
-      exit(signum);
-    } else {
-      /* Just quit gracefully */
-      exit(signum);
-    }
-  }
+  stop = true;
 }
 
 /* print usage and exit */
@@ -171,7 +154,7 @@ void usage_bail(const char *executable_name) {
  * March 29, 2002 - Daniel Eisenbud points out that c should be int, not char,
  * for correctness on platforms with unsigned chars.
  */
-void parse_command_line(int argc, char **argv, beep_parms_t *result) {
+void parse_command_line(int argc, char **argv, beep_parms_t *result, char *console_device) {
   int c;
 
   struct option opt_list[7] = {{"help", 0, NULL, 'h'},
@@ -264,9 +247,14 @@ void parse_command_line(int argc, char **argv, beep_parms_t *result) {
     result->freq = DEFAULT_FREQ;
 }  
 
-void play_beep(beep_parms_t parms) {
+void play_beep(beep_parms_t parms, char *console_device) {
   int i; /* loop counter */
+  int console_fd = -1;
+  int console_type = BEEP_TYPE_CONSOLE;
 
+  if (stop) {
+    return; 
+  }
   if(parms.verbose == 1)
       fprintf(stderr, "[DEBUG] %d times %d ms beeps (%d delay between, "
 	"%d delay after) @ %.2f Hz\n",
@@ -293,13 +281,14 @@ void play_beep(beep_parms_t parms) {
     console_type = BEEP_TYPE_CONSOLE;
 
   /* Beep */
-  for (i = 0; i < parms.reps; i++) {                    /* start beep */
-    do_beep(parms.freq);
+  for (i = 0; i < parms.reps && !stop; i++) {                    /* start beep */
+    do_beep(parms.freq, console_fd, console_type);
     /* Look ma, I'm not ansi C compatible! */
     usleep(1000*parms.length);                          /* wait...    */
-    do_beep(0);                                         /* stop beep  */
-    if(parms.end_delay || (i+1 < parms.reps))
-       usleep(1000*parms.delay);                        /* wait...    */
+    do_beep(0, console_fd, console_type);                                         /* stop beep  */
+    if(parms.end_delay || (i+1 < parms.reps)) {
+      usleep(1000*parms.delay);                        /* wait...    */
+    }
   }                                                     /* repeat.    */
 
   close(console_fd);
@@ -321,12 +310,14 @@ int main(int argc, char **argv) {
   parms->next       = NULL;
 
   signal(SIGINT, handle_signal);
-  parse_command_line(argc, argv, parms);
+  char *console_device = NULL;
+  parse_command_line(argc, argv, parms, console_device);
+
 
   /* this outermost while loop handles the possibility that -n/--new has been
      used, i.e. that we have multiple beeps specified. Each iteration will
      play, then free() one parms instance. */
-  while(parms) {
+  while(parms && !stop) {
     beep_parms_t *next = parms->next;
 
     if(parms->stdin_beep) {
@@ -339,20 +330,20 @@ int main(int argc, char **argv) {
 	 not much to  be done about that. */
       setvbuf(stdin, NULL, _IONBF, 0);
       setvbuf(stdout, NULL, _IONBF, 0);
-      while(fgets(sin, 4096, stdin)) {
+      while(fgets(sin, 4096, stdin) && !stop) {
 	if(parms->stdin_beep==CHAR_STDIN_BEEP) {
-	  for(ptr=sin;*ptr;ptr++) {
+	  for(ptr=sin;*ptr && !stop;ptr++) {
 	    putchar(*ptr);
 	    fflush(stdout);
-	    play_beep(*parms);
+	    play_beep(*parms, console_device);
 	  }
 	} else {
 	  fputs(sin, stdout);
-	  play_beep(*parms);
+	  play_beep(*parms, console_device);
 	}
       }
     } else {
-      play_beep(*parms);
+      play_beep(*parms, console_device);
     }
 
     /* Junk each parms struct after playing it */

debian/README.Debian

@@ -0,0 +1,9 @@
+beep for Debian
+---------------
+
+The code is quite short so it should be clear that it's not exploitable.
+I therefore think that "suid root with only group audio executeable" is an
+acceptable default - you are of course able to change it anytime with
+dpkg-reconfigure beep
+
+ -- Rhonda D'Vine <rhonda@debian.org>, Wed, 27 Feb 2002 12:48:26 +0100

debian/README.source

@@ -0,0 +1,2 @@
+This package uses quilt for its patch management, see
+/usr/share/doc/quilt/README.source if you are unfamiliar with it.

debian/beep-udeb.dirs

@@ -0,0 +1 @@
+usr/bin

debian/beep.1.diff

@@ -0,0 +1,88 @@
+--- beep.1.orig	2009-03-24 19:02:59.000000000 +0100
++++ beep.1	2009-03-24 19:04:30.000000000 +0100
+@@ -1,12 +1,12 @@
+-.TH BEEP 1 "March 2002"
++.TH BEEP 1 "July 2008"
+ .SH NAME
+ beep \- beep the pc speaker any number of ways
+ .SH SYNOPSIS
+ .B beep
+-[\-f N] [\-l N] [\-r N] [\-d N] [\-D N] [\-s] [\-c]
++[\-\-verbose | \-\-debug] [\-e device | \-\-device device] [\-f N] [\-l N] [\-r N] [\-d N] [\-D N] [\-s] [\-c]
+ .HP
+ .B beep
+-[ OPTIONS ] [-n] [--new] [ OPTIONS ]
++[ OPTIONS ] [\-n] [\-\-new] [ OPTIONS ]
+ .HP
+ .B beep
+ [\-h] [\-\-help]
+@@ -20,6 +20,15 @@
+ All options have default values, meaning that just typing '\fBbeep\fR' will work.  If an option is specified more than once on the command line, subsequent options override their predecessors.  So '\fBbeep\fR \-f 200 \-f 300' will beep at 300Hz.
+ .SH OPTIONS
+ .TP
++\fB\-\-verbose\fR, \fB\-\-debug\fR
++enable debug output. This option prints a line like the following before each
++beep:
++
++[DEBUG] 5 times 200 ms beeps (100 delay between, 0 delay after) @ 1000.00 Hz
++.TP
++\fB\-e\fR device, \fB\-\-device\fR device
++use device as event device. If the switch isn't used, /dev/tty0 and /dev/vc/0 are tried in turn.
++.TP
+ \fB\-f\fR N
+ beep at N Hz, where 0 < N < 20000.  As a general ballpark, the regular terminal beep is around 750Hz.  N is not, incidentally, restricted to whole numbers.
+ .TP
+@@ -35,12 +44,12 @@
+ \fB\-n\fR, \fB\-\-new\fR
+ this option allows you to break the command line up into specifying multiple beeps.  Each time this option is used, beep starts treating all further arguments as though they were for a new beep.  So for example:
+
+-\fBbeep\fR -f 1000 -n -f 2000 -n -f 1500
++\fBbeep\fR \-f 1000 \-n \-f 2000 \-n \-f 1500
+
+-would produce a sequence of three beeps, the first with a frequency of 1000Hz (and otherwise default values), then a second beep with a frequency of 2000Hz (again, with things like delay and reps being set to their defaults), then a third beep, at 1500Hz.  This is different from specifying a -r value, since -r repeats the same beep multiple times, whereas -n allows you to specify different beeps.  After a -n, the new beep is created with all the default values, and any of these can be specified without altering values for preceeding (or later) beeps.  See the \fBEXAMPLES\fR section if this managed to confuse you.
++would produce a sequence of three beeps, the first with a frequency of 1000Hz (and otherwise default values), then a second beep with a frequency of 2000Hz (again, with things like delay and reps being set to their defaults), then a third beep, at 1500Hz.  This is different from specifying a \-r value, since \-r repeats the same beep multiple times, whereas \-n allows you to specify different beeps.  After a \-n, the new beep is created with all the default values, and any of these can be specified without altering values for preceding (or later) beeps.  See the \fBEXAMPLES\fR section if this managed to confuse you.
+ .TP
+ \fB\-s\fR, \fB\-c\fR
+-these options put \fBbeep\fR into input-processing mode.  -s tells \fBbeep\fR to read from stdin, and beep after each newline, and -c tells it to do so after every character.  In both cases, the program will also echo the input back out to stdout, which makes it easy to slip \fBbeep\fR into a text-processing pipeline, see the \fBEXAMPLES\fR section.
++these options put \fBbeep\fR into input-processing mode.  \-s tells \fBbeep\fR to read from stdin, and beep after each newline, and \-c tells it to do so after every character.  In both cases, the program will also echo the input back out to stdout, which makes it easy to slip \fBbeep\fR into a text-processing pipeline, see the \fBEXAMPLES\fR section.
+ .TP
+ \fB\-h\fR, \fB\-\-help\fR
+ display usage info and exit
+@@ -55,20 +64,22 @@
+ .TP
+ A more interesting standalone setup
+
+-\fBbeep\fR -f 300.7 -r 2 -d 100 -l 400
++\fBbeep\fR \-f 300.7 \-r 2 \-d 100 \-l 400
+ .TP
+ As part of a log-watching pipeline
+
+-tail -f /var/log/xferlog | grep 'passwd' | \fBbeep\fR -f 1000 -r 5 -s
++tail \-f /var/log/xferlog | grep \-\-line\-buffered passwd | \\
++.br
++\fBbeep\fR \-f 1000 \-r 5 \-s
+ .TP
+-When using -c mode, I recommend using a short -D, and a shorter -l, so that the beeps don't blur together.  Something like this will get you a cheesy 1970's style beep-as-you-type-each-letter effect
++When using \-c mode, I recommend using a short \-D, and a shorter \-l, so that the beeps don't blur together.  Something like this will get you a cheesy 1970's style beep-as-you-type-each-letter effect
+
+-cat file | \fBbeep\fR -c -f 400 -D 50 -l 10
++cat file | \fBbeep\fR \-c \-f 400 \-D 50 \-l 10
+
+ .TP
+-A highly contrived example of -n/--new usage
++A highly contrived example of \-n/\-\-new usage
+
+-\fBbeep\fR -f 1000 -r 2 -n -r 5 -l 10 --new 
++\fBbeep\fR \-f 1000 \-r 2 \-n \-r 5 \-l 10 \-\-new 
+
+ will produce first two 1000Hz beeps, then 5 beeps at the default tone, but only 10ms long each, followed by a third beep using all the default settings (since none are specified).
+ .PP See also the \fBFREQUENCY TABLE\fR below.
+@@ -81,7 +92,7 @@
+ .PP
+ - you own the current tty
+ .PP
+-What this means is that root can always make beep work (to the best of my knowledge!), and that any local user can make beep work, BUT a non-root remote user cannot use beep in it's natural state.  What's worse, an xterm, or other x-session counts, as far as the kernel is concerned, as 'remote', so beep won't work from a non-priviledged xterm either.  I had originally chalked this up to a bug, but there's actually nothing I can do about it, and it really is a Good Thing that the kernel does things this way.  There is also a solution.
++What this means is that root can always make beep work (to the best of my knowledge!), and that any local user can make beep work, BUT a non-root remote user cannot use beep in it's natural state.  What's worse, an xterm, or other x-session counts, as far as the kernel is concerned, as 'remote', so beep won't work from a non-privileged xterm either.  I had originally chalked this up to a bug, but there's actually nothing I can do about it, and it really is a Good Thing that the kernel does things this way.  There is also a solution.
+ .PP
+ By default beep is not installed with the suid bit set, because that would just be zany.  On the other hand, if you do make it suid root, all your problems with beep bailing on ioctl calls will magically vanish, which is pleasant, and the only reason not to is that any suid program is a potential security hole.  Conveniently, beep is very short, so auditing it is pretty straightforward.
+ .PP

debian/beep.dirs

@@ -0,0 +1,2 @@
+usr/bin
+usr/share/man/man1