March 2020 - tor-commits - lists.torproject.org

[tor/maint-0.4.3] confmgt: when a units value is invalid, include a meaningful error.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 7e7aff9b6a3bcf240fe30320e1bcd5ffdab92f13 Author: Nick Mathewson <nickm(a)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 u, then multiply the number by the unit multiplier. * On success, set *ok to 1 and return this product. * Otherwise, set *ok 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 *errmsg_out 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);

1 0

[tor/maint-0.4.3] In typed_var_kvassign, include config names in error messages.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 2c792d1e0e6f92c33287ba8e87055142745a6532 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 26 14:16:55 2020 -0500 In typed_var_kvassign, include config names in error messages. This should improve the usability of our configuration error messages. --- src/lib/confmgt/typedvar.c | 11 ++++++++++- src/test/test_options.c | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/lib/confmgt/typedvar.c b/src/lib/confmgt/typedvar.c index 3a188db95..1955302cd 100644 --- a/src/lib/confmgt/typedvar.c +++ b/src/lib/confmgt/typedvar.c @@ -24,6 +24,7 @@ #include "lib/log/log.h" #include "lib/log/util_bug.h" #include "lib/malloc/malloc.h" +#include "lib/string/printf.h" #include "lib/string/util_string.h" #include "lib/confmgt/var_type_def_st.h" @@ -75,7 +76,15 @@ typed_var_kvassign(void *target, const config_line_t *line, return def->fns->kv_parse(target, line, errmsg, def->params); } - return typed_var_assign(target, line->value, errmsg, def); + int rv = typed_var_assign(target, line->value, errmsg, def); + if (rv < 0 && *errmsg != NULL) { + /* typed_var_assign() didn't know the line's keyword, but we do. + * Let's add it to the error message. */ + char *oldmsg = *errmsg; + tor_asprintf(errmsg, "Could not parse %s: %s", line->key, oldmsg); + tor_free(oldmsg); + } + return rv; } /** diff --git a/src/test/test_options.c b/src/test/test_options.c index 129a8299e..636d3c0e5 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -4269,7 +4269,8 @@ test_options_trial_assign(void *arg) v = options_trial_assign(lines, 0, &msg); tt_int_op(v, OP_EQ, SETOPT_ERR_PARSE); tt_str_op(msg, OP_EQ, - "Unrecognized value ambidextrous. Allowed values are 0 and 1."); + "Could not parse UseBridges: Unrecognized value ambidextrous. " + "Allowed values are 0 and 1."); tor_free(msg); config_free_lines(lines);

1 0

[tor/maint-0.4.3] changes file for ticket 33460.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 87e08730565d0da3b598b2ecc20ed4c8791414ef Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 26 14:19:53 2020 -0500 changes file for ticket 33460. --- changes/ticket33460 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changes/ticket33460 b/changes/ticket33460 new file mode 100644 index 000000000..21e0fc966 --- /dev/null +++ b/changes/ticket33460 @@ -0,0 +1,4 @@ + o Minor features (usability): + - Include more information when failing to parse a configuration value. + This should make it easier to tell what's going wrong when a + configuration file doesn't parse. Closes ticket 33460.

1 0

[tor/master] confmgt: when a units value is invalid, include a meaningful error.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 7e7aff9b6a3bcf240fe30320e1bcd5ffdab92f13 Author: Nick Mathewson <nickm(a)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 u, then multiply the number by the unit multiplier. * On success, set *ok to 1 and return this product. * Otherwise, set *ok 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 *errmsg_out 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);

1 0

[tor/master] In typed_var_kvassign, include config names in error messages.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 2c792d1e0e6f92c33287ba8e87055142745a6532 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 26 14:16:55 2020 -0500 In typed_var_kvassign, include config names in error messages. This should improve the usability of our configuration error messages. --- src/lib/confmgt/typedvar.c | 11 ++++++++++- src/test/test_options.c | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/lib/confmgt/typedvar.c b/src/lib/confmgt/typedvar.c index 3a188db95..1955302cd 100644 --- a/src/lib/confmgt/typedvar.c +++ b/src/lib/confmgt/typedvar.c @@ -24,6 +24,7 @@ #include "lib/log/log.h" #include "lib/log/util_bug.h" #include "lib/malloc/malloc.h" +#include "lib/string/printf.h" #include "lib/string/util_string.h" #include "lib/confmgt/var_type_def_st.h" @@ -75,7 +76,15 @@ typed_var_kvassign(void *target, const config_line_t *line, return def->fns->kv_parse(target, line, errmsg, def->params); } - return typed_var_assign(target, line->value, errmsg, def); + int rv = typed_var_assign(target, line->value, errmsg, def); + if (rv < 0 && *errmsg != NULL) { + /* typed_var_assign() didn't know the line's keyword, but we do. + * Let's add it to the error message. */ + char *oldmsg = *errmsg; + tor_asprintf(errmsg, "Could not parse %s: %s", line->key, oldmsg); + tor_free(oldmsg); + } + return rv; } /** diff --git a/src/test/test_options.c b/src/test/test_options.c index 129a8299e..636d3c0e5 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -4269,7 +4269,8 @@ test_options_trial_assign(void *arg) v = options_trial_assign(lines, 0, &msg); tt_int_op(v, OP_EQ, SETOPT_ERR_PARSE); tt_str_op(msg, OP_EQ, - "Unrecognized value ambidextrous. Allowed values are 0 and 1."); + "Could not parse UseBridges: Unrecognized value ambidextrous. " + "Allowed values are 0 and 1."); tor_free(msg); config_free_lines(lines);

1 0

[tor/release-0.4.3] confmgt: when an int/enum value is invalid, say which values are ok.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit b133325a3ecae9f405127d4b6c0406056723029f Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 26 12:40:53 2020 -0500 confmgt: when an int/enum value is invalid, say which values are ok. Part of 33460. --- src/lib/confmgt/type_defs.c | 47 ++++++++++++++++++++++++++++++++------------- src/test/test_options.c | 3 ++- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/lib/confmgt/type_defs.c b/src/lib/confmgt/type_defs.c index 3f66cba61..dd0a846f3 100644 --- a/src/lib/confmgt/type_defs.c +++ b/src/lib/confmgt/type_defs.c @@ -121,8 +121,9 @@ int_parse(void *target, const char *value, char **errmsg, const void *params) int ok=0; *p = (int)tor_parse_long(value, 10, pp->minval, pp->maxval, &ok, NULL); if (!ok) { - tor_asprintf(errmsg, "Integer %s is malformed or out of bounds.", - value); + tor_asprintf(errmsg, "Integer %s is malformed or out of bounds. " + "Allowed values are between %d and %d.", + value, pp->minval, pp->maxval); return -1; } return 0; @@ -348,11 +349,17 @@ typedef struct enumeration_table_t { int value; } enumeration_table_t; +typedef struct enumeration_params_t { + const char *allowed_val_string; + const enumeration_table_t *table; +} enumeration_params_t; + static int enum_parse(void *target, const char *value, char **errmsg, - const void *params) + const void *params_) { - const enumeration_table_t *table = params; + const enumeration_params_t *params = params_; + const enumeration_table_t *table = params->table; int *p = (int *)target; for (; table->name; ++table) { if (!strcasecmp(value, table->name)) { @@ -360,15 +367,17 @@ enum_parse(void *target, const char *value, char **errmsg, return 0; } } - tor_asprintf(errmsg, "Unrecognized value %s.", value); + tor_asprintf(errmsg, "Unrecognized value %s. %s", + value, params->allowed_val_string); return -1; } static char * -enum_encode(const void *value, const void *params) +enum_encode(const void *value, const void *params_) { int v = *(const int*)value; - const enumeration_table_t *table = params; + const enumeration_params_t *params = params_; + const enumeration_table_t *table = params->table; for (; table->name; ++table) { if (v == table->value) return tor_strdup(table->name); @@ -377,19 +386,21 @@ enum_encode(const void *value, const void *params) } static void -enum_clear(void *value, const void *params) +enum_clear(void *value, const void *params_) { int *p = (int*)value; - const enumeration_table_t *table = params; + const enumeration_params_t *params = params_; + const enumeration_table_t *table = params->table; tor_assert(table->name); *p = table->value; } static bool -enum_ok(const void *value, const void *params) +enum_ok(const void *value, const void *params_) { int v = *(const int*)value; - const enumeration_table_t *table = params; + const enumeration_params_t *params = params_; + const enumeration_table_t *table = params->table; for (; table->name; ++table) { if (v == table->value) return true; @@ -403,6 +414,11 @@ static const enumeration_table_t enum_table_bool[] = { { NULL, 0 }, }; +static const enumeration_params_t enum_params_bool = { + "Allowed values are 0 and 1.", + enum_table_bool +}; + static const enumeration_table_t enum_table_autobool[] = { { "0", 0 }, { "1", 1 }, @@ -410,6 +426,11 @@ static const enumeration_table_t enum_table_autobool[] = { { NULL, 0 }, }; +static const enumeration_params_t enum_params_autobool = { + "Allowed values are 0, 1, and auto.", + enum_table_autobool +}; + static const var_type_fns_t enum_fns = { .parse = enum_parse, .encode = enum_encode, @@ -740,10 +761,10 @@ const var_type_def_t DOUBLE_type_defn = { .name="Float", .fns=&double_fns, }; const var_type_def_t BOOL_type_defn = { .name="Boolean", .fns=&enum_fns, - .params=&enum_table_bool }; + .params=&enum_params_bool }; const var_type_def_t AUTOBOOL_type_defn = { .name="Boolean+Auto", .fns=&enum_fns, - .params=&enum_table_autobool }; + .params=&enum_params_autobool }; const var_type_def_t ISOTIME_type_defn = { .name="Time", .fns=&time_fns, }; const var_type_def_t CSV_type_defn = { diff --git a/src/test/test_options.c b/src/test/test_options.c index b6a9a2150..b67c37f50 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -4276,7 +4276,8 @@ test_options_trial_assign(void *arg) tt_int_op(r, OP_EQ, 0); v = options_trial_assign(lines, 0, &msg); tt_int_op(v, OP_EQ, SETOPT_ERR_PARSE); - tt_str_op(msg, OP_EQ, "Unrecognized value ambidextrous."); + tt_str_op(msg, OP_EQ, + "Unrecognized value ambidextrous. Allowed values are 0 and 1."); tor_free(msg); config_free_lines(lines);

1 0

[tor/master] changes file for ticket 33460.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 87e08730565d0da3b598b2ecc20ed4c8791414ef Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 26 14:19:53 2020 -0500 changes file for ticket 33460. --- changes/ticket33460 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changes/ticket33460 b/changes/ticket33460 new file mode 100644 index 000000000..21e0fc966 --- /dev/null +++ b/changes/ticket33460 @@ -0,0 +1,4 @@ + o Minor features (usability): + - Include more information when failing to parse a configuration value. + This should make it easier to tell what's going wrong when a + configuration file doesn't parse. Closes ticket 33460.

1 0

[tor/master] Merge branch 'maint-0.4.3'
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 7177eeddf1df90a28d8f733d551ed1198e1fadb1 Merge: 686494f0f 87e087305 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Mar 5 08:48:40 2020 -0500 Merge branch 'maint-0.4.3' changes/ticket33460 | 4 +++ src/lib/confmgt/type_defs.c | 67 +++++++++++++++++++++++++++++++++------------ src/lib/confmgt/typedvar.c | 11 +++++++- src/lib/confmgt/unitparse.c | 45 +++++++++++++++++++++--------- src/lib/confmgt/unitparse.h | 3 +- src/test/test_options.c | 20 +++++--------- 6 files changed, 105 insertions(+), 45 deletions(-)

1 0

[tor/release-0.4.3] confmgt: when a units value is invalid, include a meaningful error.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 7e7aff9b6a3bcf240fe30320e1bcd5ffdab92f13 Author: Nick Mathewson <nickm(a)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 u, then multiply the number by the unit multiplier. * On success, set *ok to 1 and return this product. * Otherwise, set *ok 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 *errmsg_out 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);

1 0

[tor/release-0.4.3] In typed_var_kvassign, include config names in error messages.
by nickm＠torproject.org 05 Mar '20

05 Mar '20

commit 2c792d1e0e6f92c33287ba8e87055142745a6532 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 26 14:16:55 2020 -0500 In typed_var_kvassign, include config names in error messages. This should improve the usability of our configuration error messages. --- src/lib/confmgt/typedvar.c | 11 ++++++++++- src/test/test_options.c | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/lib/confmgt/typedvar.c b/src/lib/confmgt/typedvar.c index 3a188db95..1955302cd 100644 --- a/src/lib/confmgt/typedvar.c +++ b/src/lib/confmgt/typedvar.c @@ -24,6 +24,7 @@ #include "lib/log/log.h" #include "lib/log/util_bug.h" #include "lib/malloc/malloc.h" +#include "lib/string/printf.h" #include "lib/string/util_string.h" #include "lib/confmgt/var_type_def_st.h" @@ -75,7 +76,15 @@ typed_var_kvassign(void *target, const config_line_t *line, return def->fns->kv_parse(target, line, errmsg, def->params); } - return typed_var_assign(target, line->value, errmsg, def); + int rv = typed_var_assign(target, line->value, errmsg, def); + if (rv < 0 && *errmsg != NULL) { + /* typed_var_assign() didn't know the line's keyword, but we do. + * Let's add it to the error message. */ + char *oldmsg = *errmsg; + tor_asprintf(errmsg, "Could not parse %s: %s", line->key, oldmsg); + tor_free(oldmsg); + } + return rv; } /** diff --git a/src/test/test_options.c b/src/test/test_options.c index 129a8299e..636d3c0e5 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -4269,7 +4269,8 @@ test_options_trial_assign(void *arg) v = options_trial_assign(lines, 0, &msg); tt_int_op(v, OP_EQ, SETOPT_ERR_PARSE); tt_str_op(msg, OP_EQ, - "Unrecognized value ambidextrous. Allowed values are 0 and 1."); + "Could not parse UseBridges: Unrecognized value ambidextrous. " + "Allowed values are 0 and 1."); tor_free(msg); config_free_lines(lines);

1 0