commit 7e7aff9b6a3bcf240fe30320e1bcd5ffdab92f13 Author: Nick Mathewson nickm@torproject.org Date: Wed Feb 26 12:40:53 2020 -0500

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

Part of 33460. --- src/lib/confmgt/type_defs.c | 20 ++++++++++++++++---- src/lib/confmgt/unitparse.c | 45 ++++++++++++++++++++++++++++++++------------- src/lib/confmgt/unitparse.h | 3 ++- src/test/test_options.c | 16 ++++------------ 4 files changed, 54 insertions(+), 30 deletions(-)

diff --git a/src/lib/confmgt/type_defs.c b/src/lib/confmgt/type_defs.c index dd0a846f3..d9e5e1e4c 100644 --- a/src/lib/confmgt/type_defs.c +++ b/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; diff --git a/src/lib/confmgt/unitparse.c b/src/lib/confmgt/unitparse.c index 52c0eabb6..61edc6069 100644 --- a/src/lib/confmgt/unitparse.c +++ b/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", - val, u->unit); + tor_asprintf(&errmsg, "Got a negative value while parsing %s %s", + 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", - val, u->unit); + tor_asprintf(&errmsg, "Overflow while parsing %s %s", + 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", - val, u->unit); + tor_asprintf(&errmsg, "Overflow while parsing %s %s", + 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; diff --git a/src/lib/confmgt/unitparse.h b/src/lib/confmgt/unitparse.h index 3f4f039a0..047e11b42 100644 --- a/src/lib/confmgt/unitparse.h +++ b/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); diff --git a/src/test/test_options.c b/src/test/test_options.c index b67c37f50..129a8299e 100644 --- a/src/test/test_options.c +++ b/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 - // XXXX is done. It is truncated for now because at the current stage - // XXXX of refactoring, we can't give a full error message like before. - WANT_ERR_LOG("HeartbeatPeriod 21 snarks", - "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); + WANT_ERR("HeartbeatPeriod 21 snarks", + "Unknown unit in 21 snarks", PH_ASSIGN); + WANT_ERR("LogTimeGranularity 21 snarks", + "Unknown unit in 21 snarks", PH_ASSIGN); OK("HeartbeatPeriod 1 hour", PH_VALIDATE); OK("LogTimeGranularity 100 milliseconds", PH_VALIDATE);