Extend tor_sscanf so it can replace sscanf in rephist.c

Fixes bug 4195 and Coverity CID 448

changes/bug4195

@@ -0,0 +1,6 @@
+  o Minor features:
+    - Enhance our internal sscanf replacement so that we can eliminate
+      the last remaining uses of the system sscanf. (Though those uses
+      of sscanf were safe, sscanf itself is generally error prone, so
+      we want to eliminate when we can.) Fixes ticket 4195 and Coverity
+      CID 448.

src/common/util.c

@@ -2694,9 +2694,9 @@ digit_to_num(char d)
  * success, store the result in <b>out</b>, advance bufp to the next
  * character, and return 0.  On failure, return -1. */
 static int
-scan_unsigned(const char **bufp, unsigned *out, int width, int base)
+scan_unsigned(const char **bufp, unsigned long *out, int width, int base)
 {
-  unsigned result = 0;
+  unsigned long result = 0;
   int scanned_so_far = 0;
   const int hex = base==16;
   tor_assert(base == 10 || base == 16);
@@ -2708,8 +2708,8 @@ scan_unsigned(const char **bufp, unsigned *out, int width, int base)
   while (**bufp && (hex?TOR_ISXDIGIT(**bufp):TOR_ISDIGIT(**bufp))
          && scanned_so_far < width) {
     int digit = hex?hex_decode_digit(*(*bufp)++):digit_to_num(*(*bufp)++);
-    unsigned new_result = result * base + digit;
+    unsigned long new_result = result * base + digit;
-    if (new_result > UINT32_MAX || new_result < result)
+    if (new_result < result)
       return -1; /* over/underflow. */
     result = new_result;
     ++scanned_so_far;
@@ -2722,6 +2722,89 @@ scan_unsigned(const char **bufp, unsigned *out, int width, int base)
   return 0;
 }
 
+/** Helper: Read an signed int from *<b>bufp</b> of up to <b>width</b>
+ * characters.  (Handle arbitrary width if <b>width</b> is less than 0.)  On
+ * success, store the result in <b>out</b>, advance bufp to the next
+ * character, and return 0.  On failure, return -1. */
+static int
+scan_signed(const char **bufp, long *out, int width)
+{
+  int neg = 0;
+  unsigned long result = 0;
+
+  if (!bufp || !*bufp || !out)
+    return -1;
+  if (width<0)
+    width=MAX_SCANF_WIDTH;
+
+  if (**bufp == '-') {
+    neg = 1;
+    ++*bufp;
+    --width;
+  }
+
+  if (scan_unsigned(bufp, &result, width, 10) < 0)
+    return -1;
+
+  if (neg) {
+    if (result > ((unsigned long)LONG_MAX) + 1)
+      return -1; /* Underflow */
+    *out = -(long)result;
+  } else {
+    if (result > LONG_MAX)
+      return -1; /* Overflow */
+    *out = (long)result;
+  }
+
+  return 0;
+}
+
+/** Helper: Read a decimal-formatted double from *<b>bufp</b> of up to
+ * <b>width</b> characters.  (Handle arbitrary width if <b>width</b> is less
+ * than 0.)  On success, store the result in <b>out</b>, advance bufp to the
+ * next character, and return 0.  On failure, return -1. */
+static int
+scan_double(const char **bufp, double *out, int width)
+{
+  int neg = 0;
+  double result = 0;
+  int scanned_so_far = 0;
+
+  if (!bufp || !*bufp || !out)
+    return -1;
+  if (width<0)
+    width=MAX_SCANF_WIDTH;
+
+  if (**bufp == '-') {
+    neg = 1;
+    ++*bufp;
+  }
+
+  while (**bufp && TOR_ISDIGIT(**bufp) && scanned_so_far < width) {
+    const int digit = digit_to_num(*(*bufp)++);
+    result = result * 10 + digit;
+    ++scanned_so_far;
+  }
+  if (**bufp == '.') {
+    double fracval = 0, denominator = 1;
+    ++*bufp;
+    ++scanned_so_far;
+    while (**bufp && TOR_ISDIGIT(**bufp) && scanned_so_far < width) {
+      const int digit = digit_to_num(*(*bufp)++);
+      fracval = fracval * 10 + digit;
+      denominator *= 10;
+      ++scanned_so_far;
+    }
+    result += fracval / denominator;
+  }
+
+  if (!scanned_so_far) /* No actual digits scanned */
+    return -1;
+
+  *out = neg ? -result : result;
+  return 0;
+}
+
 /** Helper: copy up to <b>width</b> non-space characters from <b>bufp</b> to
  * <b>out</b>.  Make sure <b>out</b> is nul-terminated. Advance <b>bufp</b>
  * to the next non-space character or the EOS. */
@@ -2758,6 +2841,7 @@ tor_vsscanf(const char *buf, const char *pattern, va_list ap)
       }
     } else {
       int width = -1;
+      int longmod = 0;
       ++pattern;
       if (TOR_ISDIGIT(*pattern)) {
         width = digit_to_num(*pattern++);
@@ -2770,17 +2854,57 @@ tor_vsscanf(const char *buf, const char *pattern, va_list ap)
         if (!width) /* No zero-width things. */
           return -1;
       }
+      if (*pattern == 'l') {
+        longmod = 1;
+        ++pattern;
+      }
       if (*pattern == 'u' || *pattern == 'x') {
-        unsigned *u = va_arg(ap, unsigned *);
+        unsigned long u;
         const int base = (*pattern == 'u') ? 10 : 16;
         if (!*buf)
           return n_matched;
-        if (scan_unsigned(&buf, u, width, base)<0)
+        if (scan_unsigned(&buf, &u, width, base)<0)
           return n_matched;
+        if (longmod) {
+          unsigned long *out = va_arg(ap, unsigned long *);
+          *out = u;
+        } else {
+          unsigned *out = va_arg(ap, unsigned *);
+          if (u > UINT_MAX)
+            return n_matched;
+          *out = u;
+        }
+        ++pattern;
+        ++n_matched;
+      } else if (*pattern == 'f') {
+        double *d = va_arg(ap, double *);
+        if (!longmod)
+          return -1; /* float not supported */
+        if (!*buf)
+          return n_matched;
+        if (scan_double(&buf, d, width)<0)
+          return n_matched;
+        ++pattern;
+        ++n_matched;
+      } else if (*pattern == 'd') {
+        long lng=0;
+        if (scan_signed(&buf, &lng, width)<0)
+          return n_matched;
+        if (longmod) {
+          long *out = va_arg(ap, long *);
+          *out = lng;
+        } else {
+          int *out = va_arg(ap, int *);
+          if (lng < INT_MIN || lng > INT_MAX)
+            return n_matched;
+          *out = (int)lng;
+        }
         ++pattern;
         ++n_matched;
       } else if (*pattern == 's') {
         char *s = va_arg(ap, char *);
+        if (longmod)
+          return -1;
         if (width < 0)
           return -1;
         if (scan_string(&buf, s, width)<0)
@@ -2789,6 +2913,8 @@ tor_vsscanf(const char *buf, const char *pattern, va_list ap)
         ++n_matched;
       } else if (*pattern == 'c') {
         char *ch = va_arg(ap, char *);
+        if (longmod)
+          return -1;
         if (width != -1)
           return -1;
         if (!*buf)
@@ -2799,6 +2925,8 @@ tor_vsscanf(const char *buf, const char *pattern, va_list ap)
       } else if (*pattern == '%') {
         if (*buf != '%')
           return n_matched;
+        if (longmod)
+          return -1;
         ++buf;
         ++pattern;
       } else {
@@ -2812,9 +2940,14 @@ tor_vsscanf(const char *buf, const char *pattern, va_list ap)
 
 /** Minimal sscanf replacement: parse <b>buf</b> according to <b>pattern</b>
  * and store the results in the corresponding argument fields.  Differs from
- * sscanf in that it: Only handles %u, %x, %c and %Ns.  Does not handle
+ * sscanf in that:
- * arbitrarily long widths. %u and %x do not consume any space.  Is
+ * <ul><li>It only handles %u, %lu, %x, %lx, %<NUM>s, %d, %ld, %lf, and %c.
- * locale-independent.  Returns -1 on malformed patterns.
+ *     <li>It only handles decimal inputs for %lf. (12.3, not 1.23e1)
+ *     <li>It does not handle arbitrarily long widths.
+ *     <li>Numbers do not consume any space characters.
+ *     <li>It is locale-independent.
+ *     <li>%u and %x do not consume any space.
+ *     <li>It returns -1 on malformed patterns.</ul>
  *
  * (As with other locale-independent functions, we need this to parse data that
  * is in ASCII without worrying that the C library's locale-handling will make

src/or/rephist.c

@@ -1136,7 +1136,7 @@ rep_hist_load_mtbf_data(time_t now)
     wfu_timebuf[0] = '\0';
 
     if (format == 1) {
-      n = sscanf(line, "%40s %ld %lf S=%10s %8s",
+      n = tor_sscanf(line, "%40s %ld %lf S=%10s %8s",
                  hexbuf, &wrl, &trw, mtbf_timebuf, mtbf_timebuf+11);
       if (n != 3 && n != 5) {
         log_warn(LD_HIST, "Couldn't scan line %s", escaped(line));
@@ -1153,7 +1153,7 @@ rep_hist_load_mtbf_data(time_t now)
       wfu_idx = find_next_with(lines, i+1, "+WFU ");
       if (mtbf_idx >= 0) {
         const char *mtbfline = smartlist_get(lines, mtbf_idx);
-        n = sscanf(mtbfline, "+MTBF %lu %lf S=%10s %8s",
+        n = tor_sscanf(mtbfline, "+MTBF %lu %lf S=%10s %8s",
                    &wrl, &trw, mtbf_timebuf, mtbf_timebuf+11);
         if (n == 2 || n == 4) {
           have_mtbf = 1;
@@ -1164,7 +1164,7 @@ rep_hist_load_mtbf_data(time_t now)
       }
       if (wfu_idx >= 0) {
         const char *wfuline = smartlist_get(lines, wfu_idx);
-        n = sscanf(wfuline, "+WFU %lu %lu S=%10s %8s",
+        n = tor_sscanf(wfuline, "+WFU %lu %lu S=%10s %8s",
                    &wt_uptime, &total_wt_time,
                    wfu_timebuf, wfu_timebuf+11);
         if (n == 2 || n == 4) {

src/test/test_util.c

@@ -1461,12 +1461,28 @@ test_util_control_formats(void)
   tor_free(out);
 }
 
+#define test_feq(value1,value2) do {                               \
+    double v1 = (value1), v2=(value2);                             \
+    double tf_diff = v1-v2;                                        \
+    double tf_tolerance = ((v1+v2)/2.0)/1e8;                       \
+    if (tf_diff<0) tf_diff=-tf_diff;                               \
+    if (tf_tolerance<0) tf_tolerance=-tf_tolerance;                \
+    if (tf_diff<tf_tolerance) {                                    \
+      TT_BLATHER(("%s ~~ %s: %f ~~ %f",#value1,#value2,v1,v2));  \
+    } else {                                                       \
+      TT_FAIL(("%s ~~ %s: %f != %f",#value1,#value2,v1,v2)); \
+    }                                                              \
+  } while(0)
+
 static void
 test_util_sscanf(void)
 {
   unsigned u1, u2, u3;
   char s1[20], s2[10], s3[10], ch;
   int r;
+  long lng1,lng2;
+  int int1, int2;
+  double d1,d2,d3,d4;
 
   /* Simple tests (malformed patterns, literal matching, ...) */
   test_eq(-1, tor_sscanf("123", "%i", &r)); /* %i is not supported */
@@ -1595,6 +1611,65 @@ test_util_sscanf(void)
   test_eq(4, tor_sscanf("1.2.3 foobar", "%u.%u.%u%c", &u1, &u2, &u3, &ch));
   test_eq(' ', ch);
 
+  r = tor_sscanf("12345 -67890 -1", "%d %ld %d", &int1, &lng1, &int2);
+  test_eq(r,3);
+  test_eq(int1, 12345);
+  test_eq(lng1, -67890);
+  test_eq(int2, -1);
+
+#if SIZEOF_INT == 4
+  r = tor_sscanf("-2147483648. 2147483647.", "%d. %d.", &int1, &int2);
+  test_eq(r,2);
+  test_eq(int1, -2147483648);
+  test_eq(int2, 2147483647);
+
+  r = tor_sscanf("-2147483679.", "%d.", &int1);
+  test_eq(r,0);
+
+  r = tor_sscanf("2147483678.", "%d.", &int1);
+  test_eq(r,0);
+#elif SIZEOF_INT == 8
+  r = tor_sscanf("-9223372036854775808. 9223372036854775807.",
+                 "%d. %d.", &int1, &int2);
+  test_eq(r,2);
+  test_eq(int1, -9223372036854775808);
+  test_eq(int2, 9223372036854775807);
+
+  r = tor_sscanf("-9223372036854775809.", "%d.", &int1);
+  test_eq(r,0);
+
+  r = tor_sscanf("9223372036854775808.", "%d.", &int1);
+  test_eq(r,0);
+#endif
+
+#if SIZEOF_LONG == 4
+  r = tor_sscanf("-2147483648. 2147483647.", "%ld. %ld.", &lng1, &lng2)
+  test_eq(r,2);
+  test_eq(lng1, -2147483647 - 1);
+  test_eq(lng2, 2147483647);
+#elif SIZEOF_LONG == 8
+  r = tor_sscanf("-9223372036854775808. 9223372036854775807.",
+                 "%ld. %ld.", &lng1, &lng2);
+  test_eq(r,2);
+  test_eq(lng1, -9223372036854775807L - 1);
+  test_eq(lng2, 9223372036854775807L);
+
+  r = tor_sscanf("-9223372036854775808. 9223372036854775808.",
+                 "%ld. %ld.", &lng1, &lng2);
+  test_eq(r,1);
+  r = tor_sscanf("-9223372036854775809. 9223372036854775808.",
+                 "%ld. %ld.", &lng1, &lng2);
+  test_eq(r,0);
+#endif
+
+  r = tor_sscanf("123.456 .000007 -900123123.2000787 00003.2",
+                 "%lf %lf %lf %lf", &d1,&d2,&d3,&d4);
+  test_eq(r,4);
+  test_feq(d1, 123.456);
+  test_feq(d2, .000007);
+  test_feq(d3, -900123123.2000787);
+  test_feq(d4, 3.2);
+
  done:
   ;
 }