October 2019 - tor-commits - lists.torproject.org

[tor/master] Add a config_validate() function to invoke verify callbacks.
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit 0cb42385378d539185a20ce8899cdc16cf46145c Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Oct 23 15:12:19 2019 -0400 Add a config_validate() function to invoke verify callbacks. As we turn our monolithic configuration objects into suites of smaller module-specific configuration objects, we will want each one to be able to provide its own verification callbacks. That means that instead of invoking these verification callbacks directly, we will want to call them via the configuration manager object. --- src/lib/confmgt/confparse.c | 73 +++++++++++++++++++++++++++++++++++++++++++++ src/lib/confmgt/confparse.h | 3 ++ 2 files changed, 76 insertions(+) diff --git a/src/lib/confmgt/confparse.c b/src/lib/confmgt/confparse.c index 96eba610b..6d272ed04 100644 --- a/src/lib/confmgt/confparse.c +++ b/src/lib/confmgt/confparse.c @@ -1142,6 +1142,79 @@ config_init(const config_mgr_t *mgr, void *options) } SMARTLIST_FOREACH_END(mv); } +/** + * Normalize and validate a single object `options` within a configuration + * suite, according to its format. `options` may be modified as appropriate + * in order to set ancillary data. If `old_options` is provided, make sure + * that the transition from `old_options` to `options` is permitted. + * + * On success return 0; on failure set *msg_out to a newly allocated string + * explaining what is wrong, and return -1. + **/ +static int +config_validate_single(const config_format_t *fmt, + const void *old_options, void *options, + char **msg_out) +{ + tor_assert(fmt); + tor_assert(options); + + if (fmt->legacy_validate_fn) { + if (fmt->legacy_validate_fn(old_options, options, msg_out) < 0) { + return -1; + } + } + + return 0; +} + +/** + * Normalize and validate all the options in configuration object `options` + * and its sub-objects. `options` may be modified as appropriate in order to + * set ancillary data. If `old_options` is provided, make sure that the + * transition from `old_options` to `options` is permitted. + * + * On success return 0; on failure set *msg_out to a newly allocated string + * explaining what is wrong, and return -1. + **/ +int +config_validate(const config_mgr_t *mgr, + const void *old_options, void *options, + char **msg_out) +{ + int rv; + CONFIG_CHECK(mgr, options); + if (old_options) { + CONFIG_CHECK(mgr, old_options); + } + + config_suite_t **suitep_new = config_mgr_get_suite_ptr(mgr, options); + config_suite_t **suitep_old = NULL; + if (old_options) + suitep_old = config_mgr_get_suite_ptr(mgr, (void*) old_options); + + /* Validate the sub-objects */ + if (suitep_new) { + SMARTLIST_FOREACH_BEGIN(mgr->subconfigs, const config_format_t *, fmt) { + void *obj = smartlist_get((*suitep_new)->configs, fmt_sl_idx); + const void *obj_old=NULL; + if (suitep_old) + obj_old = smartlist_get((*suitep_old)->configs, fmt_sl_idx); + + rv = config_validate_single(fmt, obj_old, obj, msg_out); + if (rv < 0) + return rv; + } SMARTLIST_FOREACH_END(fmt); + } + + /* Validate the top-level object. */ + rv = config_validate_single(mgr->toplevel, old_options, options, msg_out); + if (rv < 0) + return rv; + + return 0; +} + /** Allocate and return a new string holding the written-out values of the vars * in 'options'. If 'minimal', do not write out any default-valued vars. * Else, if comment_defaults, write default values as comments. diff --git a/src/lib/confmgt/confparse.h b/src/lib/confmgt/confparse.h index d50d6d814..fea80f916 100644 --- a/src/lib/confmgt/confparse.h +++ b/src/lib/confmgt/confparse.h @@ -169,6 +169,9 @@ int config_is_same(const config_mgr_t *fmt, struct config_line_t *config_get_changes(const config_mgr_t *mgr, const void *options1, const void *options2); void config_init(const config_mgr_t *mgr, void *options); +int config_validate(const config_mgr_t *mgr, + const void *old_options, void *options, + char **msg_out); void *config_dup(const config_mgr_t *mgr, const void *old); char *config_dump(const config_mgr_t *mgr, const void *default_options, const void *options, int minimal,

1 0

[tor/master] Rename validate_fn{, _t} to start with "legacy_".
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit 4a248bafba0136ded8370de672c0b27b912bbede Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Oct 23 14:39:20 2019 -0400 Rename validate_fn{,_t} to start with "legacy_". The current API of this callback mixes responsibilities, including: * validation * transition checking * processing (modifying) the configuration object. These will have to be disentangled piece by piece, so for now, we'll have "legacy" validate functions as well. This is an automated commit, generated by this command: ./scripts/maint/rename_c_identifier.py \ validate_fn_t legacy_validate_fn_t \ validate_fn legacy_validate_fn --- src/app/config/config.c | 2 +- src/app/config/statefile.c | 2 +- src/feature/dirauth/shared_random_state.c | 2 +- src/lib/confmgt/confparse.c | 2 +- src/lib/confmgt/confparse.h | 4 ++-- src/test/test_confparse.c | 4 ++-- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index aafc8c5a7..4e7720b0b 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -873,7 +873,7 @@ static const config_format_t options_format = { .abbrevs = option_abbrevs_, .deprecations = option_deprecation_notes_, .vars = option_vars_, - .validate_fn = options_validate_cb, + .legacy_validate_fn = options_validate_cb, .clear_fn = options_clear_cb, .config_suite_offset = offsetof(or_options_t, subconfigs_), }; diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index 2e5edc41c..e277e722c 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -165,7 +165,7 @@ static const config_format_t state_format = { }, .abbrevs = state_abbrevs_, .vars = state_vars_, - .validate_fn = or_state_validate_cb, + .legacy_validate_fn = or_state_validate_cb, .extra = &state_extra_var, .config_suite_offset = offsetof(or_state_t, substates_), }; diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index 2070e03aa..eeb005d47 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -94,7 +94,7 @@ static const config_format_t state_format = { offsetof(sr_disk_state_t, magic_), }, .vars = state_vars, - .validate_fn = disk_state_validate_cb, + .legacy_validate_fn = disk_state_validate_cb, .extra = &state_extra_var, .config_suite_offset = -1, }; diff --git a/src/lib/confmgt/confparse.c b/src/lib/confmgt/confparse.c index 323c88a31..96eba610b 100644 --- a/src/lib/confmgt/confparse.c +++ b/src/lib/confmgt/confparse.c @@ -1166,7 +1166,7 @@ config_dump(const config_mgr_t *mgr, const void *default_options, /* XXX use a 1 here so we don't add a new log line while dumping */ if (default_options == NULL) { - if (fmt->validate_fn(NULL, defaults_tmp, &msg) < 0) { + if (fmt->legacy_validate_fn(NULL, defaults_tmp, &msg) < 0) { // LCOV_EXCL_START log_err(LD_BUG, "Failed to validate default config: %s", msg); tor_free(msg); diff --git a/src/lib/confmgt/confparse.h b/src/lib/confmgt/confparse.h index 8d7278cb0..d50d6d814 100644 --- a/src/lib/confmgt/confparse.h +++ b/src/lib/confmgt/confparse.h @@ -68,7 +68,7 @@ typedef struct config_deprecation_t { * config_dump(); later in our refactoring, it will be cleaned up and used * more generally. */ -typedef int (*validate_fn_t)(const void *oldval, +typedef int (*legacy_validate_fn_t)(const void *oldval, void *newval, char **msg_out); @@ -98,7 +98,7 @@ typedef struct config_format_t { const config_var_t *vars; /**< List of variables we recognize, their default * values, and where we stick them in the * structure. */ - validate_fn_t validate_fn; /**< Function to validate config. */ + legacy_validate_fn_t legacy_validate_fn; /**< Function to validate config. */ clear_cfg_fn_t clear_fn; /**< Function to clear the configuration. */ /** If present, extra denotes a LINELIST variable for unrecognized * lines. Otherwise, unrecognized lines are an error. */ diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 8cda96ba5..0c12e35f8 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -128,7 +128,7 @@ static const config_format_t test_fmt = { .abbrevs = test_abbrevs, .deprecations = test_deprecation_notes, .vars = test_vars, - .validate_fn = test_validate_cb, + .legacy_validate_fn = test_validate_cb, .config_suite_offset = -1, }; @@ -822,7 +822,7 @@ static config_format_t etest_fmt = { .abbrevs = test_abbrevs, .deprecations = test_deprecation_notes, .vars = test_vars, - .validate_fn = test_validate_cb, + .legacy_validate_fn = test_validate_cb, .extra = &extra, .config_suite_offset = -1, };

1 0

[tor/master] test_options: stop generating old_data entirely.
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit 24ee44df90d12eb98ba5a39638963696ce2f8012 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Oct 23 18:03:24 2019 -0400 test_options: stop generating old_data entirely. Now that we no longer use it anywhere, we can remove this variable from the test data structure. --- src/test/test_options.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/test/test_options.c b/src/test/test_options.c index 941ca94d9..374fa8b18 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -384,7 +384,6 @@ fixed_get_uname(void) } typedef struct { - or_options_t *old_opt; or_options_t *opt; } options_test_data_t; @@ -398,10 +397,8 @@ get_options_test_data(const char *conf) config_line_t *cl=NULL; options_test_data_t *result = tor_malloc(sizeof(options_test_data_t)); result->opt = options_new(); - result->old_opt = options_new(); options_init(result->opt); - options_init(result->old_opt); rv = config_get_lines(conf, &cl, 1); tt_int_op(rv, OP_EQ, 0); @@ -433,7 +430,6 @@ static void free_options_test_data(options_test_data_t *td) { if (!td) return; - or_options_free(td->old_opt); or_options_free(td->opt); tor_free(td); }

1 0

[tor/master] config.c: extract validate, check-transition, and set into a function
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit bd891f517f022ea93a90f0d771bf61842660e978 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Oct 24 09:08:17 2019 -0400 config.c: extract validate, check-transition, and set into a function This eliminates duplicated code. The options_validate() function itself is now tests-only. --- src/app/config/config.c | 108 ++++++++++++++++++++++++++---------------------- src/app/config/config.h | 9 ++-- 2 files changed, 64 insertions(+), 53 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index deed98d1a..ca8f95f82 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -858,6 +858,9 @@ static int options_validate_cb(const void *old_options, void *options, static void cleanup_protocol_warning_severity_level(void); static void set_protocol_warning_severity_level(int warning_severity); static void options_clear_cb(const config_mgr_t *mgr, void *opts); +static setopt_err_t options_validate_and_set(const or_options_t *old_options, + or_options_t *new_options, + char **msg_out); /** Magic value for or_options_t. */ #define OR_OPTIONS_MAGIC 9090909 @@ -2684,37 +2687,9 @@ options_trial_assign(config_line_t *list, unsigned flags, char **msg) or_options_free(trial_options); return r; } + const or_options_t *cur_options = get_options(); - setopt_err_t rv; - or_options_t *cur_options = get_options_mutable(); - - in_option_validation = 1; - - if (options_validate(cur_options, trial_options, - msg) < 0) { - or_options_free(trial_options); - rv = SETOPT_ERR_PARSE; /*XXX make this a separate return value. */ - goto done; - } - - if (options_transition_allowed(cur_options, trial_options, msg) < 0) { - or_options_free(trial_options); - rv = SETOPT_ERR_TRANSITION; - goto done; - } - in_option_validation = 0; - - if (set_options(trial_options, msg)<0) { - or_options_free(trial_options); - rv = SETOPT_ERR_SETTING; - goto done; - } - - /* we liked it. put it in place. */ - rv = SETOPT_OK; - done: - in_option_validation = 0; - return rv; + return options_validate_and_set(cur_options, trial_options, msg); } /** Print a usage message for tor. */ @@ -3240,6 +3215,54 @@ compute_publishserverdescriptor(or_options_t *options) #define RECOMMENDED_MIN_CIRCUIT_BUILD_TIMEOUT (10) /** + * Validate new_options. If it is valid, and it is a reasonable + * replacement for old_options, replace the previous value of the + * global options, and return return SETOPT_OK. + * + * If it is not valid, then free new_options, set *msg_out to a + * newly allocated error message, and return an error code. + */ +static setopt_err_t +options_validate_and_set(const or_options_t *old_options, + or_options_t *new_options, + char **msg_out) +{ + setopt_err_t rv; + validation_status_t vs; + + in_option_validation = 1; + vs = config_validate(get_options_mgr(), old_options, new_options, msg_out); + + if (vs == VSTAT_TRANSITION_ERR) { + rv = SETOPT_ERR_TRANSITION; + goto err; + } else if (vs < 0) { + rv = SETOPT_ERR_PARSE; + goto err; + } + + if (options_transition_allowed(old_options, new_options, msg_out) < 0) { + rv = SETOPT_ERR_TRANSITION; + goto err; + } + in_option_validation = 0; + + if (set_options(new_options, msg_out)) { + rv = SETOPT_ERR_SETTING; + goto err; + } + + rv = SETOPT_OK; + new_options = NULL; /* prevent free */ + err: + in_option_validation = 0; + tor_assert(new_options == NULL || rv != SETOPT_OK); + or_options_free(new_options); + return rv; +} + +#ifdef TOR_UNIT_TESTS +/** * Return 0 if every setting in options is reasonable, is a * permissible transition from old_options, and none of the * testing-only settings differ from default_options unless in @@ -3248,7 +3271,7 @@ compute_publishserverdescriptor(or_options_t *options) * * On error, tor_strdup an error explanation into *msg. */ -STATIC int +int options_validate(const or_options_t *old_options, or_options_t *options, char **msg) { @@ -3256,6 +3279,7 @@ options_validate(const or_options_t *old_options, or_options_t *options, vs = config_validate(get_options_mgr(), old_options, options, msg); return vs < 0 ? -1 : 0; } +#endif #define REJECT(arg) \ STMT_BEGIN *msg = tor_strdup(arg); return -1; STMT_END @@ -5529,25 +5553,12 @@ options_init_from_string(const char *cf_defaults, const char *cf, } newoptions->IncludeUsed = cf_has_include; - in_option_validation = 1; newoptions->FilesOpenedByIncludes = opened_files; + opened_files = NULL; // prevent double-free. - /* Validate newoptions */ - if (options_validate(oldoptions, newoptions, msg) < 0) { - err = SETOPT_ERR_PARSE; /*XXX make this a separate return value.*/ + err = options_validate_and_set(oldoptions, newoptions, msg); + if (err < 0) goto err; - } - - if (options_transition_allowed(oldoptions, newoptions, msg) < 0) { - err = SETOPT_ERR_TRANSITION; - goto err; - } - in_option_validation = 0; - - if (set_options(newoptions, msg)) { - err = SETOPT_ERR_SETTING; - goto err; /* frees and replaces old options */ - } or_options_free(global_default_options); global_default_options = newdefaultoptions; @@ -5560,9 +5571,6 @@ options_init_from_string(const char *cf_defaults, const char *cf, SMARTLIST_FOREACH(opened_files, char *, f, tor_free(f)); smartlist_free(opened_files); } - // may have been set to opened_files, avoid double free - newoptions->FilesOpenedByIncludes = NULL; - or_options_free(newoptions); or_options_free(newdefaultoptions); if (*msg) { char *old_msg = *msg; diff --git a/src/app/config/config.h b/src/app/config/config.h index dbba30e9c..2badc1af9 100644 --- a/src/app/config/config.h +++ b/src/app/config/config.h @@ -277,9 +277,6 @@ STATIC void port_cfg_free_(port_cfg_t *port); STATIC void or_options_free_(or_options_t *options); STATIC int options_validate_single_onion(or_options_t *options, char **msg); -STATIC int options_validate(const or_options_t *old_options, - or_options_t *options, - char **msg); STATIC int parse_transport_line(const or_options_t *options, const char *line, int validate_only, int server); @@ -311,6 +308,12 @@ STATIC int open_and_add_file_log(const log_severity_list_t *severity, STATIC int options_init_logs(const or_options_t *old_options, or_options_t *options, int validate_only); +#ifdef TOR_UNIT_TESTS +int options_validate(const or_options_t *old_options, + or_options_t *options, + char **msg); +#endif + #endif /* defined(CONFIG_PRIVATE) */ #endif /* !defined(TOR_CONFIG_H) */

1 0

[tor/master] tests: set DataDirectory_option as well as DataDirectory.
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit 06475f30e95133e89d58bb42a33cef5627b5f29f Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Oct 24 20:16:44 2019 -0400 tests: set DataDirectory_option as well as DataDirectory. options_validate_cb() derives DataDirectory (which we use) from DataDirectory_option (which the user sets). I want to add a test that will call options_validate_cb(), but it will fail unless it derives the same value. --- src/test/testing_common.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/testing_common.c b/src/test/testing_common.c index e14a16940..c5a4e81fb 100644 --- a/src/test/testing_common.c +++ b/src/test/testing_common.c @@ -326,6 +326,7 @@ main(int c, const char **v) initialize_mainloop_events(); options_init(options); options->DataDirectory = tor_strdup(temp_dir); + options->DataDirectory_option = tor_strdup(temp_dir); tor_asprintf(&options->KeyDirectory, "%s"PATH_SEPARATOR"keys", options->DataDirectory); options->CacheDirectory = tor_strdup(temp_dir);

1 0

[tor/master] Write a new set of config validation callbacks.
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit e0ae137df5ca4849e98cc957fc3081cc3f203d70 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Oct 23 16:00:25 2019 -0400 Write a new set of config validation callbacks. Unlike legacy_validate_fn_t, these callbacks have separation of duties, into: * early normalization and computation. * validation * transition checking * late normalization and computation Only the first and last steps get mutable objects. Only the transition-checking step gets to see the previous values of this object. --- src/app/config/config.c | 4 ++- src/lib/confmgt/confparse.c | 46 ++++++++++++++++++------ src/lib/confmgt/confparse.h | 86 +++++++++++++++++++++++++++++++++++++-------- 3 files changed, 110 insertions(+), 26 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index b372791a4..deed98d1a 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -3252,7 +3252,9 @@ STATIC int options_validate(const or_options_t *old_options, or_options_t *options, char **msg) { - return config_validate(get_options_mgr(), old_options, options, msg); + validation_status_t vs; + vs = config_validate(get_options_mgr(), old_options, options, msg); + return vs < 0 ? -1 : 0; } #define REJECT(arg) \ diff --git a/src/lib/confmgt/confparse.c b/src/lib/confmgt/confparse.c index f1bec76c0..655fc3483 100644 --- a/src/lib/confmgt/confparse.c +++ b/src/lib/confmgt/confparse.c @@ -1148,10 +1148,11 @@ config_init(const config_mgr_t *mgr, void *options) * in order to set ancillary data. If `old_options` is provided, make sure * that the transition from `old_options` to `options` is permitted. * - * On success return 0; on failure set *msg_out to a newly allocated string - * explaining what is wrong, and return -1. + * On success return VSTAT_OK; on failure set *msg_out to a newly allocated + * string explaining what is wrong, and return a different validation_status_t + * to describe which step failed. **/ -static int +static validation_status_t config_validate_single(const config_format_t *fmt, const void *old_options, void *options, char **msg_out) @@ -1159,13 +1160,37 @@ config_validate_single(const config_format_t *fmt, tor_assert(fmt); tor_assert(options); + if (fmt->pre_normalize_fn) { + if (fmt->pre_normalize_fn(options, msg_out) < 0) { + return VSTAT_PRE_NORMALIZE_ERR; + } + } + if (fmt->legacy_validate_fn) { if (fmt->legacy_validate_fn(old_options, options, msg_out) < 0) { - return -1; + return VSTAT_LEGACY_ERR; } } - return 0; + if (fmt->validate_fn) { + if (fmt->validate_fn(options, msg_out) < 0) { + return VSTAT_VALIDATE_ERR; + } + } + + if (fmt->check_transition_fn && old_options) { + if (fmt->check_transition_fn(old_options, options, msg_out) < 0) { + return VSTAT_TRANSITION_ERR; + } + } + + if (fmt->post_normalize_fn) { + if (fmt->post_normalize_fn(options, msg_out) < 0) { + return VSTAT_POST_NORMALIZE_ERR; + } + } + + return VSTAT_OK; } /** @@ -1174,15 +1199,16 @@ config_validate_single(const config_format_t *fmt, * set ancillary data. If `old_options` is provided, make sure that the * transition from `old_options` to `options` is permitted. * - * On success return 0; on failure set *msg_out to a newly allocated string - * explaining what is wrong, and return -1. + * On success return VSTAT_OK; on failure set *msg_out to a newly allocated + * string explaining what is wrong, and return a different validation_status_t + * to describe which step failed. **/ -int +validation_status_t config_validate(const config_mgr_t *mgr, const void *old_options, void *options, char **msg_out) { - int rv; + validation_status_t rv; CONFIG_CHECK(mgr, options); if (old_options) { CONFIG_CHECK(mgr, old_options); @@ -1212,7 +1238,7 @@ config_validate(const config_mgr_t *mgr, if (rv < 0) return rv; - return 0; + return VSTAT_OK; } /** Allocate and return a new string holding the written-out values of the vars diff --git a/src/lib/confmgt/confparse.h b/src/lib/confmgt/confparse.h index fea80f916..7829f1aeb 100644 --- a/src/lib/confmgt/confparse.h +++ b/src/lib/confmgt/confparse.h @@ -52,25 +52,59 @@ typedef struct config_deprecation_t { #define PLURAL(tok) { #tok, #tok "s", 0, 0 } /** - * Type of a callback to validate whether a given configuration is + * Validation function: verify whether a configuation object is well-formed + * and consistent. + * + * On success, return 0. On failure, set msg_out to a newly allocated + * string containing an error message, and return -1. */ +typedef int (*validate_fn_t)(const void *value, char **msg_out); +/** + * Validation function: verify whether a configuration object (`value`) is an + * allowable value given the previous configuration value (`old_value`). + * + * On success, return 0. On failure, set msg_out to a newly allocated + * string containing an error message, and return -1. */ +typedef int (*check_transition_fn_t)(const void *old_value, const void *value, + char **msg_out); +/** + * Validation function: normalize members of `value`, and compute derived + * members. + * + * This function is called before any other validation of `value`, and must + * not assume that validate_fn or check_transition_fn has passed. + * + * On success, return 0. On failure, set msg_out to a newly allocated + * string containing an error message, and return -1. */ +typedef int (*pre_normalize_fn_t)(void *value, char **msg_out); +/** + * Validation function: normalize members of `value`, and compute derived + * members. + * + * This function is called after validation of `value`, and may + * assume that validate_fn or check_transition_fn has passed. + * + * On success, return 0. On failure, set msg_out to a newly allocated + * string containing an error message, and return -1. */ +typedef int (*post_normalize_fn_t)(void *value, char **msg_out); + +/** + * Legacy function to validate whether a given configuration is * well-formed and consistent. * * The configuration to validate is passed as newval. The previous - * configuration, if any, is provided in oldval. The - * default_val argument receives a configuration object initialized - * with default values for all its fields. The from_setconf argument - * is true iff the input comes from a SETCONF controller command. + * configuration, if any, is provided in oldval. + * + * This API is deprecated, since it mixes the responsibilities of + * pre_normalize_fn_t, post_normalize_fn_t, validate_fn_t, and + * check_transition_fn_t. No new instances of this function type should + * be written. * * On success, return 0. On failure, set *msg_out to a newly allocated * error message, and return -1. - * - * REFACTORING NOTE: Currently, this callback type is only used from inside - * config_dump(); later in our refactoring, it will be cleaned up and used - * more generally. */ typedef int (*legacy_validate_fn_t)(const void *oldval, - void *newval, - char **msg_out); + void *newval, + char **msg_out); struct config_mgr_t; @@ -98,7 +132,18 @@ typedef struct config_format_t { const config_var_t *vars; /**< List of variables we recognize, their default * values, and where we stick them in the * structure. */ - legacy_validate_fn_t legacy_validate_fn; /**< Function to validate config. */ + + /** Early-stage normalization callback. Invoked by config_validate(). */ + pre_normalize_fn_t pre_normalize_fn; + /** Configuration validation function. Invoked by config_validate(). */ + validate_fn_t validate_fn; + /** Legacy validation function. Invoked by config_validate(). */ + legacy_validate_fn_t legacy_validate_fn; + /** Transition checking function. Invoked by config_validate(). */ + check_transition_fn_t check_transition_fn; + /** Late-stage normalization callback. Invoked by config_validate(). */ + post_normalize_fn_t post_normalize_fn; + clear_cfg_fn_t clear_fn; /**< Function to clear the configuration. */ /** If present, extra denotes a LINELIST variable for unrecognized * lines. Otherwise, unrecognized lines are an error. */ @@ -169,9 +214,20 @@ int config_is_same(const config_mgr_t *fmt, struct config_line_t *config_get_changes(const config_mgr_t *mgr, const void *options1, const void *options2); void config_init(const config_mgr_t *mgr, void *options); -int config_validate(const config_mgr_t *mgr, - const void *old_options, void *options, - char **msg_out); + +/** An enumeration to report which validation step failed. */ +typedef enum { + VSTAT_PRE_NORMALIZE_ERR = -5, + VSTAT_VALIDATE_ERR = -4, + VSTAT_LEGACY_ERR = -3, + VSTAT_TRANSITION_ERR = -2, + VSTAT_POST_NORMALIZE_ERR = -1, + VSTAT_OK = 0, +} validation_status_t; + +validation_status_t config_validate(const config_mgr_t *mgr, + const void *old_options, void *options, + char **msg_out); void *config_dup(const config_mgr_t *mgr, const void *old); char *config_dump(const config_mgr_t *mgr, const void *default_options, const void *options, int minimal,

1 0

[tor/master] changes tile for ticket 31241
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit 94853b4ab6930b0be92bd3e62ffe761b12493c96 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Oct 24 20:25:36 2019 -0400 changes tile for ticket 31241 --- changes/ticket31241 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/changes/ticket31241 b/changes/ticket31241 new file mode 100644 index 000000000..64852ca4b --- /dev/null +++ b/changes/ticket31241 @@ -0,0 +1,5 @@ + o Minor features (configuration validation): + - Configuration validation can now be done by per-module callbacks, + rather than a global validation function. This will let us reduce the + size of config.c and some of its more cumbersome functions. Closes + ticket 31241.

1 0

[tor/master] test_options: add a test for options_trial_assign()
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit 620e40bcfca294b405b46faf0eaaec70c91cf564 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Oct 24 20:23:30 2019 -0400 test_options: add a test for options_trial_assign() This function is used by controllers to set options. I refactored it a bit when refactoring options validation, so it needs to get tested. --- src/test/test_options.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/src/test/test_options.c b/src/test/test_options.c index 374fa8b18..15fd714f7 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -4045,6 +4045,87 @@ test_options_init_logs_quiet(void *arg) UNMOCK(add_file_log); } +static int mock_options_act_status = 0; +static int +mock_options_act(const or_options_t *old_options) +{ + (void)old_options; + return mock_options_act_status; +} +static int +mock_options_act_reversible(const or_options_t *old_options, char **msg_out) +{ + (void)old_options; + (void)msg_out; + return 0; +} + +static void +test_options_trial_assign(void *arg) +{ + (void)arg; + setopt_err_t v; + config_line_t *lines = NULL; + char *msg = NULL; + int r; + + // replace options_act*() so that we don't actually launch tor here. + MOCK(options_act, mock_options_act); + MOCK(options_act_reversible, mock_options_act_reversible); + + // Try assigning nothing; that should work. + v = options_trial_assign(lines, 0, &msg); + if (msg) + puts(msg); + tt_ptr_op(msg, OP_EQ, NULL); + tt_int_op(v, OP_EQ, SETOPT_OK); + + // Assigning a nickname is okay + r = config_get_lines("Nickname Hemiramphinae", &lines, 0); + tt_int_op(r, OP_EQ, 0); + v = options_trial_assign(lines, 0, &msg); + tt_ptr_op(msg, OP_EQ, NULL); + tt_int_op(v, OP_EQ, SETOPT_OK); + tt_str_op(get_options()->Nickname, OP_EQ, "Hemiramphinae"); + config_free_lines(lines); + + // We can't change the User; that's a transition error. + r = config_get_lines("User Heraclitus", &lines, 0); + tt_int_op(r, OP_EQ, 0); + v = options_trial_assign(lines, 0, &msg); + tt_int_op(v, OP_EQ, SETOPT_ERR_TRANSITION); + tt_str_op(msg, OP_EQ, "While Tor is running, changing User is not allowed"); + tor_free(msg); + config_free_lines(lines); + + // We can't set the ORPort to nonsense: that's a validation error. + r = config_get_lines("ORPort fractabling planished", &lines, 0); + tt_int_op(r, OP_EQ, 0); + v = options_trial_assign(lines, 0, &msg); + tt_int_op(v, OP_EQ, SETOPT_ERR_PARSE); // (same error code for now) + tt_str_op(msg, OP_EQ, "Invalid ORPort configuration"); + tor_free(msg); + config_free_lines(lines); + + // We can't set UseBridges to a non-boolean: that's a parse error. + r = config_get_lines("UseBridges ambidextrous", &lines, 0); + 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."); + tor_free(msg); + config_free_lines(lines); + + // this didn't change. + tt_str_op(get_options()->Nickname, OP_EQ, "Hemiramphinae"); + + done: + config_free_lines(lines); + tor_free(msg); + UNMOCK(options_act); + UNMOCK(options_act_reversible); +} + #ifndef COCCI #define LOCAL_VALIDATE_TEST(name) \ { "validate__" #name, test_options_validate__ ## name, TT_FORK, NULL, NULL } @@ -4098,5 +4179,6 @@ struct testcase_t options_tests[] = { NULL, NULL }, { "init_logs/quiet", test_options_init_logs_quiet, TT_FORK, NULL, NULL }, + { "trial_assign", test_options_trial_assign, TT_FORK, NULL, NULL }, END_OF_TESTCASES /* */ };

1 0

[tor/master] Make foo_validate() functions call config_validate().
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit dc6d7f072d7307c21df80b982c1d1f98130d3286 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Oct 23 15:38:15 2019 -0400 Make foo_validate() functions call config_validate(). The former foo_validate() functions are now toplevel legacy_validate_fn callbacks. The new foo_validate() functions now call them. This change lets us remove the old shared_random disk state validation callback entirely. --- scripts/maint/practracker/exceptions.txt | 2 +- src/app/config/config.c | 37 ++++++++++++++++++------------- src/app/config/statefile.c | 26 +++++++++++++--------- src/feature/dirauth/shared_random_state.c | 19 ---------------- src/lib/confmgt/confparse.c | 2 +- 5 files changed, 39 insertions(+), 47 deletions(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 054bbbdeb..f7bd8287e 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -38,7 +38,7 @@ problem include-count /src/app/config/config.c 89 problem function-size /src/app/config/config.c:options_act_reversible() 296 problem function-size /src/app/config/config.c:options_act() 589 problem function-size /src/app/config/config.c:resolve_my_address() 190 -problem function-size /src/app/config/config.c:options_validate() 1209 +problem function-size /src/app/config/config.c:options_validate_cb() 1209 problem function-size /src/app/config/config.c:options_init_from_torrc() 207 problem function-size /src/app/config/config.c:options_init_from_string() 113 problem function-size /src/app/config/config.c:options_init_logs() 145 diff --git a/src/app/config/config.c b/src/app/config/config.c index 4e7720b0b..b372791a4 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -3239,13 +3239,20 @@ compute_publishserverdescriptor(or_options_t *options) * */ #define RECOMMENDED_MIN_CIRCUIT_BUILD_TIMEOUT (10) -static int -options_validate_cb(const void *old_options, void *options, char **msg) +/** + * Return 0 if every setting in options is reasonable, is a + * permissible transition from old_options, and none of the + * testing-only settings differ from default_options unless in + * testing mode. Else return -1. Should have no side effects, except for + * normalizing the contents of options. + * + * On error, tor_strdup an error explanation into *msg. + */ +STATIC int +options_validate(const or_options_t *old_options, or_options_t *options, + char **msg) { - in_option_validation = 1; - int rv = options_validate(old_options, options, msg); - in_option_validation = 0; - return rv; + return config_validate(get_options_mgr(), old_options, options, msg); } #define REJECT(arg) \ @@ -3431,18 +3438,16 @@ options_validate_single_onion(or_options_t *options, char **msg) return 0; } -/** Return 0 if every setting in options is reasonable, is a - * permissible transition from old_options, and none of the - * testing-only settings differ from default_options unless in - * testing mode. Else return -1. Should have no side effects, except for - * normalizing the contents of options. - * - * On error, tor_strdup an error explanation into *msg. +/** + * Legacy validation/normalization callback for or_options_t. See + * legacy_validate_fn_t for more information. */ -STATIC int -options_validate(const or_options_t *old_options, or_options_t *options, - char **msg) +static int +options_validate_cb(const void *old_options_, void *options_, char **msg) { + const or_options_t *old_options = old_options_; + or_options_t *options = options_; + config_line_t *cl; const char *uname = get_uname(); int n_ports=0; diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index e277e722c..d3a0ec179 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -265,16 +265,6 @@ validate_transports_in_state(or_state_t *state) return 0; } -static int -or_state_validate_cb(const void *old_state, void *state, char **msg) -{ - /* We don't use these; only options do. Still, we need to match that - * signature. */ - (void) old_state; - - return or_state_validate(state, msg); -} - /** Return 0 if every setting in state is reasonable, and a * permissible transition from old_state. Else warn and return -1. * Should have no side effects, except for normalizing the contents of @@ -283,6 +273,22 @@ or_state_validate_cb(const void *old_state, void *state, char **msg) static int or_state_validate(or_state_t *state, char **msg) { + return config_validate(get_state_mgr(), NULL, state, msg); +} + +/** + * Legacy validation/normalization callback for or_state_t. See + * legacy_validate_fn_t for more information. + */ +static int +or_state_validate_cb(const void *old_state, void *state_, char **msg) +{ + /* There is not a meaningful concept of a state-to-state transition, + * since we do not reload the state after we start. */ + (void) old_state; + + or_state_t *state = state_; + if (entry_guards_parse_state(state, 0, msg)<0) return -1; diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index eeb005d47..e548eb402 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -59,9 +59,6 @@ DUMMY_TYPECHECK_INSTANCE(sr_disk_state_t); /** Our persistent state magic number. */ #define SR_DISK_STATE_MAGIC 0x98AB1254 -static int -disk_state_validate_cb(const void *old_state, void *state, char **msg); - /** Array of variables that are saved to disk as a persistent state. */ static const config_var_t state_vars[] = { V(Version, POSINT, "0"), @@ -94,7 +91,6 @@ static const config_format_t state_format = { offsetof(sr_disk_state_t, magic_), }, .vars = state_vars, - .legacy_validate_fn = disk_state_validate_cb, .extra = &state_extra_var, .config_suite_offset = -1, }; @@ -339,21 +335,6 @@ disk_state_validate(const sr_disk_state_t *state) return -1; } -/** Validate the disk state (NOP for now). */ -static int -disk_state_validate_cb(const void *old_state, void *state, char **msg) -{ - /* We don't use these; only options do. */ - (void) old_state; - - /* This is called by config_dump which is just before we are about to - * write it to disk. At that point, our global memory state has been - * copied to the disk state so it's fair to assume it's trustable. */ - (void) state; - (void) msg; - return 0; -} - /** Parse the Commit line(s) in the disk state and translate them to the * the memory state. Return 0 on success else -1 on error. */ static int diff --git a/src/lib/confmgt/confparse.c b/src/lib/confmgt/confparse.c index 6d272ed04..f1bec76c0 100644 --- a/src/lib/confmgt/confparse.c +++ b/src/lib/confmgt/confparse.c @@ -1239,7 +1239,7 @@ config_dump(const config_mgr_t *mgr, const void *default_options, /* XXX use a 1 here so we don't add a new log line while dumping */ if (default_options == NULL) { - if (fmt->legacy_validate_fn(NULL, defaults_tmp, &msg) < 0) { + if (config_validate(mgr, NULL, defaults_tmp, &msg) < 0) { // LCOV_EXCL_START log_err(LD_BUG, "Failed to validate default config: %s", msg); tor_free(msg);

1 0

[tor/master] config.c: Turn options_transition_allowed() into a proper callback
by nickm＠torproject.org 25 Oct '19

25 Oct '19

commit d17aa0930872e391a19480f65545a029e2359dd7 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Oct 24 09:16:50 2019 -0400 config.c: Turn options_transition_allowed() into a proper callback --- src/app/config/config.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index ca8f95f82..1041b6b6a 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -823,9 +823,9 @@ static const config_deprecation_t option_deprecation_notes_[] = { static char *get_windows_conf_root(void); #endif static int options_act_reversible(const or_options_t *old_options, char **msg); -static int options_transition_allowed(const or_options_t *old, - const or_options_t *new, - char **msg); +static int options_check_transition_cb(const void *old, + const void *new, + char **msg); static int options_transition_affects_workers( const or_options_t *old_options, const or_options_t *new_options); static int options_transition_affects_descriptor( @@ -877,6 +877,7 @@ static const config_format_t options_format = { .deprecations = option_deprecation_notes_, .vars = option_vars_, .legacy_validate_fn = options_validate_cb, + .check_transition_fn = options_check_transition_cb, .clear_fn = options_clear_cb, .config_suite_offset = offsetof(or_options_t, subconfigs_), }; @@ -3240,11 +3241,6 @@ options_validate_and_set(const or_options_t *old_options, rv = SETOPT_ERR_PARSE; goto err; } - - if (options_transition_allowed(old_options, new_options, msg_out) < 0) { - rv = SETOPT_ERR_TRANSITION; - goto err; - } in_option_validation = 0; if (set_options(new_options, msg_out)) { @@ -4821,11 +4817,14 @@ opt_streq(const char *s1, const char *s2) /** Check if any of the previous options have changed but aren't allowed to. */ static int -options_transition_allowed(const or_options_t *old, - const or_options_t *new_val, - char **msg) +options_check_transition_cb(const void *old_, + const void *new_val_, + char **msg) { - if (!old) + const or_options_t *old = old_; + const or_options_t *new_val = new_val_; + + if (BUG(!old)) return 0; #define BAD_CHANGE_TO(opt, how) do { \

1 0