confmgt: when a units value is invalid, include a meaningful error.

Part of 33460.

src/lib/confmgt/type_defs.c

@@ -229,11 +229,17 @@ units_parse_u64(void *target, const char *value, char **errmsg,
   tor_assert(table);
   uint64_t *v = (uint64_t*)target;
   int ok=1;
-  *v = config_parse_units(value, table, &ok);
+  char *msg = NULL;
+  *v = config_parse_units(value, table, &ok, &msg);
   if (!ok) {
-    *errmsg = tor_strdup("Provided value is malformed or out of bounds.");
+    tor_asprintf(errmsg, "Provided value is malformed or out of bounds: %s",
+                 msg);
+    tor_free(msg);
     return -1;
   }
+  if (BUG(msg)) {
+    tor_free(msg);
+  }
   return 0;
 }
 
@@ -245,11 +251,17 @@ units_parse_int(void *target, const char *value, char **errmsg,
   tor_assert(table);
   int *v = (int*)target;
   int ok=1;
-  uint64_t u64 = config_parse_units(value, table, &ok);
+  char *msg = NULL;
+  uint64_t u64 = config_parse_units(value, table, &ok, &msg);
   if (!ok) {
-    *errmsg = tor_strdup("Provided value is malformed or out of bounds.");
+    tor_asprintf(errmsg, "Provided value is malformed or out of bounds: %s",
+                 msg);
+    tor_free(msg);
     return -1;
   }
+  if (BUG(msg)) {
+    tor_free(msg);
+  }
   if (u64 > INT_MAX) {
     tor_asprintf(errmsg, "Provided value %s is too large", value);
     return -1;

src/lib/confmgt/unitparse.c

@@ -13,7 +13,9 @@
 #include "lib/confmgt/unitparse.h"
 #include "lib/log/log.h"
 #include "lib/log/util_bug.h"
+#include "lib/malloc/malloc.h"
 #include "lib/string/parse_int.h"
+#include "lib/string/printf.h"
 #include "lib/string/util_string.h"
 #include "lib/intmath/muldiv.h"
 
@@ -110,23 +112,30 @@ const struct unit_table_t time_msec_units[] = {
  * table <b>u</b>, then multiply the number by the unit multiplier.
  * On success, set *<b>ok</b> to 1 and return this product.
  * Otherwise, set *<b>ok</b> to 0.
- * Warns user when overflow or a negative value is detected.
+ *
+ * If an error (like overflow or a negative value is detected), put an error
+ * message in *<b>errmsg_out</b> if that pointer is non-NULL, and otherwise
+ * log a warning.
  */
 uint64_t
-config_parse_units(const char *val, const unit_table_t *u, int *ok)
+config_parse_units(const char *val, const unit_table_t *u, int *ok,
+                   char **errmsg_out)
 {
   uint64_t v = 0;
   double d = 0;
   int use_float = 0;
   char *cp;
+  char *errmsg = NULL;
 
   tor_assert(ok);
 
   v = tor_parse_uint64(val, 10, 0, UINT64_MAX, ok, &cp);
   if (!*ok || (cp && *cp == '.')) {
     d = tor_parse_double(val, 0, (double)UINT64_MAX, ok, &cp);
-    if (!*ok)
+    if (!*ok) {
+      tor_asprintf(&errmsg, "Unable to parse %s as a number", val);
       goto done;
+    }
     use_float = 1;
   }
 
@@ -148,8 +157,8 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
         d = u->multiplier * d;
 
         if (d < 0) {
-          log_warn(LD_CONFIG, "Got a negative value while parsing %s %s",
+          tor_asprintf(&errmsg, "Got a negative value while parsing %s %s",
-                   val, u->unit);
+                       val, u->unit);
           *ok = 0;
           goto done;
         }
@@ -157,8 +166,8 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
         // Some compilers may warn about casting a double to an unsigned type
         // because they don't know if d is >= 0
         if (d >= 0 && (d > (double)INT64_MAX || (uint64_t)d > INT64_MAX)) {
-          log_warn(LD_CONFIG, "Overflow detected while parsing %s %s",
+          tor_asprintf(&errmsg, "Overflow while parsing %s %s",
-                   val, u->unit);
+                       val, u->unit);
           *ok = 0;
           goto done;
         }
@@ -168,8 +177,8 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
         v = tor_mul_u64_nowrap(v, u->multiplier);
 
         if (v > INT64_MAX) {
-          log_warn(LD_CONFIG, "Overflow detected while parsing %s %s",
+          tor_asprintf(&errmsg, "Overflow while parsing %s %s",
-                   val, u->unit);
+                       val, u->unit);
           *ok = 0;
           goto done;
         }
@@ -179,10 +188,20 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
       goto done;
     }
   }
-  log_warn(LD_CONFIG, "Unknown unit '%s'.", cp);
+  tor_asprintf(&errmsg, "Unknown unit in %s", val);
   *ok = 0;
  done:
 
+  if (errmsg) {
+    tor_assert_nonfatal(!*ok);
+    if (errmsg_out) {
+      *errmsg_out = errmsg;
+    } else {
+      log_warn(LD_CONFIG, "%s", errmsg);
+      tor_free(errmsg);
+    }
+  }
+
   if (*ok)
     return v;
   else
@@ -196,7 +215,7 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)
 uint64_t
 config_parse_memunit(const char *s, int *ok)
 {
-  uint64_t u = config_parse_units(s, memory_units, ok);
+  uint64_t u = config_parse_units(s, memory_units, ok, NULL);
   return u;
 }
 
@@ -208,7 +227,7 @@ int
 config_parse_msec_interval(const char *s, int *ok)
 {
   uint64_t r;
-  r = config_parse_units(s, time_msec_units, ok);
+  r = config_parse_units(s, time_msec_units, ok, NULL);
   if (r > INT_MAX) {
     log_warn(LD_CONFIG, "Msec interval '%s' is too long", s);
     *ok = 0;
@@ -225,7 +244,7 @@ int
 config_parse_interval(const char *s, int *ok)
 {
   uint64_t r;
-  r = config_parse_units(s, time_units, ok);
+  r = config_parse_units(s, time_units, ok, NULL);
   if (r > INT_MAX) {
     log_warn(LD_CONFIG, "Interval '%s' is too long", s);
     *ok = 0;

src/lib/confmgt/unitparse.h

@@ -25,7 +25,8 @@ extern const unit_table_t memory_units[];
 extern const unit_table_t time_units[];
 extern const struct unit_table_t time_msec_units[];
 
-uint64_t config_parse_units(const char *val, const unit_table_t *u, int *ok);
+uint64_t config_parse_units(const char *val, const unit_table_t *u, int *ok,
+                            char **errmsg_out);
 
 uint64_t config_parse_memunit(const char *s, int *ok);
 int config_parse_msec_interval(const char *s, int *ok);

src/test/test_options.c

@@ -307,18 +307,10 @@ test_options_validate(void *arg)
   WANT_ERR("BridgeRelay 1\nDirCache 0",
            "We're a bridge but DirCache is disabled.", PH_VALIDATE);
 
-  // XXXX We should replace this with a more full error message once #29211
+  WANT_ERR("HeartbeatPeriod 21 snarks",
-  // XXXX is done.  It is truncated for now because at the current stage
+           "Unknown unit in 21 snarks", PH_ASSIGN);
-  // XXXX of refactoring, we can't give a full error message like before.
+  WANT_ERR("LogTimeGranularity 21 snarks",
-  WANT_ERR_LOG("HeartbeatPeriod 21 snarks",
+           "Unknown unit in 21 snarks", PH_ASSIGN);
-               "malformed or out of bounds", LOG_WARN,
-               "Unknown unit 'snarks'.",
-               PH_ASSIGN);
-  // XXXX As above.
-  WANT_ERR_LOG("LogTimeGranularity 21 snarks",
-               "malformed or out of bounds", LOG_WARN,
-               "Unknown unit 'snarks'.",
-               PH_ASSIGN);
   OK("HeartbeatPeriod 1 hour", PH_VALIDATE);
   OK("LogTimeGranularity 100 milliseconds", PH_VALIDATE);