Integer overflow in handling of -l (length) and -D (delay) parameter

[hannob]

beep contains integer overflows in the handling of the length and delay parameters.

To test compile beep with ubsan:
clang -fsanitize=undefined beep.c -o beep

And try:
./beep -l 2147483647
beep.c:299:16: runtime error: signed integer overflow: 1000 * 2147483647 cannot be represented in type 'int'

or
./beep -D 2147483647
beep.c:302:19: runtime error: signed integer overflow: 1000 * 2147483647 cannot be represented in type 'int'

The problem is that the value is multiplied by 1000. Integer overflows are undefined behavior and can thus lead to unpredictable outcome due to compiler optimizations.

This could be made safe by using unsigned variables. They could still overflow, but would "just" wrap around and lead to different values being used. Alternatively of course the inputs could be capped to values that can safely be multiplied within the size of an integer.

[ndim]

I think I have a fix for this over at the beep fork I use for packaging beep for Fedora:

From 60a0d26ba7dc9e6c9456c37dbd892cd56d4a2ddb Mon Sep 17 00:00:00 2001
From: Hans Ulrich Niedermann <[email protected]>
Date: Fri, 28 Dec 2018 03:20:59 +0100
Subject: [PATCH 8/9] get rid of a few signed int and unchecked input value
 ranges

---
 beep.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/beep.c b/beep.c
index 2bea6cd..b838b85 100644
--- a/beep.c
+++ b/beep.c
@@ -74,10 +74,10 @@ char *copyright =
 
 typedef struct beep_parms_t {
   unsigned int freq; /* tone frequency (Hz)      */
-  int length;     /* tone length    (ms)      */
-  int reps;       /* # of repetitions         */
-  int delay;      /* delay between reps  (ms) */
-  int end_delay;  /* do we delay after last rep? */
+  unsigned int length;     /* tone length    (ms)      */
+  unsigned int reps;       /* # of repetitions         */
+  unsigned int delay;      /* delay between reps  (ms) */
+  unsigned int end_delay;  /* do we delay after last rep? */
   int stdin_beep; /* are we using stdin triggers?  We have three options:
 		     - just beep and terminate (default)
 		     - beep after a line of input
@@ -202,19 +202,19 @@ void parse_command_line(int argc, char **argv, beep_parms_t *result) {
       }
       break;
     case 'l' : /* length */
-      if(!sscanf(optarg, "%d", &argval) || (argval < 0))
+      if(!sscanf(optarg, "%d", &argval) || (argval < 0) || (argval > 2100000))
 	usage_bail(argv[0]);
       else
 	result->length = argval;
       break;
     case 'r' : /* repetitions */
-      if(!sscanf(optarg, "%d", &argval) || (argval < 0))
+      if(!sscanf(optarg, "%d", &argval) || (argval < 0) || (argval > 2100000))
 	usage_bail(argv[0]);
       else
 	result->reps = argval;
       break;
     case 'd' : /* delay between reps - WITHOUT delay after last beep*/
-      if(!sscanf(optarg, "%d", &argval) || (argval < 0))
+      if(!sscanf(optarg, "%d", &argval) || (argval < 0) || (argval > 2100000))
 	usage_bail(argv[0]);
       else {
 	result->delay = argval;
@@ -222,7 +222,7 @@ void parse_command_line(int argc, char **argv, beep_parms_t *result) {
       }
       break;
     case 'D' : /* delay between reps - WITH delay after last beep */
-      if(!sscanf(optarg, "%d", &argval) || (argval < 0))
+      if(!sscanf(optarg, "%d", &argval) || (argval < 0) || (argval > 2100000))
 	usage_bail(argv[0]);
       else {
 	result->delay = argval;
@@ -270,7 +270,7 @@ void parse_command_line(int argc, char **argv, beep_parms_t *result) {
 }  
 
 void play_beep(beep_parms_t parms) {
-  int i; /* loop counter */
+  unsigned int i; /* loop counter */
 
   if(parms.verbose == 1)
       fprintf(stderr, "[DEBUG] %d times %d ms beeps (%d delay between, "
@@ -281,10 +281,11 @@ void play_beep(beep_parms_t parms) {
   for (i = 0; i < parms.reps; i++) {                    /* start beep */
     do_beep(parms.freq);
     /* Look ma, I'm not ansi C compatible! */
-    usleep(1000*parms.length);                          /* wait...    */
+    usleep(1000U*parms.length);                          /* wait...    */
     do_beep(0);                                         /* stop beep  */
-    if(parms.end_delay || (i+1 < parms.reps))
-       usleep(1000*parms.delay);                        /* wait...    */
+    if (parms.end_delay || ((i+1) < parms.reps)) {
+       usleep(1000U*parms.delay);                        /* wait...    */
+    }
   }                                                     /* repeat.    */
 }
 
-- 
2.17.2

[ndim]

The post-1.4.1 releases over at https://github.com/spkr-beep/beep fix this.