From e8dc513bd012c9d7e1a5908c72056d5be52e760e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 19 Jul 2019 06:45:54 -0400 Subject: [PATCH 01/28] Add a config_mgr_t type to wrap config_format_t Remember that our goal in the present refactoring is to allow each subsystem to declare its own configuration structure and variables. To do this, each module will get its own config_format_t, and so we'll want a different structure that wraps several config_format_t objects. This is a "config_mgr_t". --- src/app/config/config.c | 60 ++++--- src/app/config/config.h | 5 +- src/app/config/confparse.c | 145 +++++++++++------ src/app/config/confparse.h | 47 ++++-- src/app/config/statefile.c | 33 ++-- src/feature/dirauth/shared_random_state.c | 27 +++- src/test/test_confparse.c | 183 +++++++++++++--------- src/test/test_helpers.c | 2 +- src/test/test_options.c | 14 +- 9 files changed, 327 insertions(+), 189 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 4bc807a6fb..76feed0005 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -843,7 +843,7 @@ static void config_maybe_load_geoip_files_(const or_options_t *options, static int options_validate_cb(void *old_options, void *options, void *default_options, int from_setconf, char **msg); -static void options_free_cb(void *options); +static void options_free_cb(const config_mgr_t *, void *options); static void cleanup_protocol_warning_severity_level(void); static void set_protocol_warning_severity_level(int warning_severity); @@ -851,7 +851,7 @@ static void set_protocol_warning_severity_level(int warning_severity); #define OR_OPTIONS_MAGIC 9090909 /** Configuration format for or_options_t. */ -STATIC const config_format_t options_format = { +static const config_format_t options_format = { sizeof(or_options_t), { "or_options_t", @@ -895,6 +895,19 @@ static int in_option_validation = 0; /* True iff we've initialized libevent */ static int libevent_initialized = 0; +/* A global configuration manager to handle all configuration objects. */ +static config_mgr_t *options_mgr = NULL; + +/** Return the global configuration manager object for torrc options. */ +STATIC const config_mgr_t * +get_options_mgr(void) +{ + if (PREDICT_UNLIKELY(options_mgr == NULL)) { + options_mgr = config_mgr_new(&options_format); + } + return options_mgr; +} + /** Return the contents of our frontpage string, or NULL if not configured. */ MOCK_IMPL(const char*, get_dirportfrontpage, (void)) @@ -977,14 +990,14 @@ set_options(or_options_t *new_val, char **msg) if (old_options && old_options != global_options) { elements = smartlist_new(); for (i=0; options_format.vars[i].member.name; ++i) { - const config_var_t *var = &options_format.vars[i]; + const config_var_t *var = &options_format.vars[i]; // XXXX 29211 const char *var_name = var->member.name; if (config_var_is_contained(var)) { /* something else will check this var, or it doesn't need checking */ continue; } - if (!config_is_same(&options_format, new_val, old_options, var_name)) { - line = config_get_assigned_option(&options_format, new_val, + if (!config_is_same(get_options_mgr(), new_val, old_options, var_name)) { + line = config_get_assigned_option(get_options_mgr(), new_val, var_name, 1); if (line) { @@ -1046,7 +1059,7 @@ or_options_free_(or_options_t *options) tor_free(options->command_arg); tor_free(options->master_key_fname); config_free_lines(options->MyFamily); - config_free(&options_format, options); + config_free(get_options_mgr(), options); } /** Release all memory and resources held by global configuration structures. @@ -1080,6 +1093,8 @@ config_free_all(void) have_parsed_cmdline = 0; libevent_initialized = 0; + + config_mgr_free(options_mgr); } /** Make address -- a piece of information related to our operation as @@ -2547,7 +2562,7 @@ config_parse_commandline(int argc, char **argv, int ignore_errors, param = tor_malloc_zero(sizeof(config_line_t)); param->key = is_cmdline ? tor_strdup(argv[i]) : - tor_strdup(config_expand_abbrev(&options_format, s, 1, 1)); + tor_strdup(config_expand_abbrev(get_options_mgr(), s, 1, 1)); param->value = arg; param->command = command; param->next = NULL; @@ -2573,7 +2588,7 @@ config_parse_commandline(int argc, char **argv, int ignore_errors, int option_is_recognized(const char *key) { - const config_var_t *var = config_find_option(&options_format, key); + const config_var_t *var = config_find_option(get_options_mgr(), key); return (var != NULL); } @@ -2582,7 +2597,7 @@ option_is_recognized(const char *key) const char * option_get_canonical_name(const char *key) { - const config_var_t *var = config_find_option(&options_format, key); + const config_var_t *var = config_find_option(get_options_mgr(), key); return var ? var->member.name : NULL; } @@ -2591,7 +2606,7 @@ option_get_canonical_name(const char *key) config_line_t * option_get_assignment(const or_options_t *options, const char *key) { - return config_get_assigned_option(&options_format, options, key, 1); + return config_get_assigned_option(get_options_mgr(), options, key, 1); } /** Try assigning list to the global options. You do this by duping @@ -2607,9 +2622,9 @@ setopt_err_t options_trial_assign(config_line_t *list, unsigned flags, char **msg) { int r; - or_options_t *trial_options = config_dup(&options_format, get_options()); + or_options_t *trial_options = config_dup(get_options_mgr(), get_options()); - if ((r=config_assign(&options_format, trial_options, + if ((r=config_assign(get_options_mgr(), trial_options, list, flags, msg)) < 0) { or_options_free(trial_options); return r; @@ -2992,7 +3007,7 @@ is_local_addr, (const tor_addr_t *addr)) or_options_t * options_new(void) { - return config_new(&options_format); + return config_new(get_options_mgr()); } /** Set options to hold reasonable defaults for most options. @@ -3000,10 +3015,10 @@ options_new(void) void options_init(or_options_t *options) { - config_init(&options_format, options); + config_init(get_options_mgr(), options); config_line_t *dflts = get_options_defaults(); char *msg=NULL; - if (config_assign(&options_format, options, dflts, + if (config_assign(get_options_mgr(), options, dflts, CAL_WARN_DEPRECATIONS, &msg)<0) { log_err(LD_BUG, "Unable to set default options: %s", msg); tor_free(msg); @@ -3040,7 +3055,7 @@ options_dump(const or_options_t *options, int how_to_dump) return NULL; } - return config_dump(&options_format, use_defaults, options, minimal, 0); + return config_dump(get_options_mgr(), use_defaults, options, minimal, 0); } /** Return 0 if every element of sl is a string holding a decimal @@ -3174,8 +3189,9 @@ options_validate_cb(void *old_options, void *options, void *default_options, /** Callback to free an or_options_t */ static void -options_free_cb(void *options) +options_free_cb(const config_mgr_t *mgr, void *options) { + (void)mgr; or_options_free_(options); } @@ -4435,7 +4451,7 @@ options_validate(or_options_t *old_options, or_options_t *options, STMT_BEGIN \ if (!options->TestingTorNetwork && \ !options->UsingTestNetworkDefaults_ && \ - !config_is_same(&options_format,options, \ + !config_is_same(get_options_mgr(),options, \ default_options,#arg)) { \ REJECT(#arg " may only be changed in testing Tor " \ "networks!"); \ @@ -5431,7 +5447,7 @@ options_init_from_string(const char *cf_defaults, const char *cf, err = SETOPT_ERR_PARSE; goto err; } - retval = config_assign(&options_format, newoptions, cl, + retval = config_assign(get_options_mgr(), newoptions, cl, CAL_WARN_DEPRECATIONS, msg); config_free_lines(cl); if (retval < 0) { @@ -5439,15 +5455,15 @@ options_init_from_string(const char *cf_defaults, const char *cf, goto err; } if (i==0) - newdefaultoptions = config_dup(&options_format, newoptions); + newdefaultoptions = config_dup(get_options_mgr(), newoptions); } if (newdefaultoptions == NULL) { - newdefaultoptions = config_dup(&options_format, global_default_options); + newdefaultoptions = config_dup(get_options_mgr(), global_default_options); } /* Go through command-line variables too */ - retval = config_assign(&options_format, newoptions, + retval = config_assign(get_options_mgr(), newoptions, global_cmdline_options, CAL_WARN_DEPRECATIONS, msg); if (retval < 0) { err = SETOPT_ERR_PARSE; diff --git a/src/app/config/config.h b/src/app/config/config.h index c6feb89fe7..44f09e5ee9 100644 --- a/src/app/config/config.h +++ b/src/app/config/config.h @@ -247,9 +247,8 @@ int options_any_client_port_set(const or_options_t *options); #define CL_PORT_DFLT_GROUP_WRITABLE (1u<<7) STATIC int options_act(const or_options_t *old_options); -#ifdef TOR_UNIT_TESTS -extern const struct config_format_t options_format; -#endif +struct config_mgr_t; +STATIC const struct config_mgr_t *get_options_mgr(void); STATIC port_cfg_t *port_cfg_new(size_t namelen); #define port_cfg_free(port) \ diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 699fc0e3dd..9c4c6a2778 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -37,13 +37,44 @@ #include "lib/string/printf.h" #include "lib/string/util_string.h" -static void config_reset(const config_format_t *fmt, void *options, +static void config_reset(const config_mgr_t *fmt, void *options, const config_var_t *var, int use_defaults); +struct config_mgr_t { + /** The 'top-level' configuration format. This one is used for legacy + * options that have not yet been assigned to different sub-modules. + * + * (NOTE: for now, this is the only config_format_t that a config_mgr_t + * contains. A subsequent commit will add more. XXXX) + */ + const config_format_t *toplevel; +}; + +/** Create a new config_mgr_t to manage a set of configuration objects to be + * wrapped under toplevel_fmt. */ +config_mgr_t * +config_mgr_new(const config_format_t *toplevel_fmt) +{ + config_mgr_t *mgr = tor_malloc_zero(sizeof(config_mgr_t)); + mgr->toplevel = toplevel_fmt; + return mgr; +} + +/** Release all storage held in mgr */ +void +config_mgr_free_(config_mgr_t *mgr) +{ + if (!mgr) + return; + memset(mgr, 0, sizeof(*mgr)); + tor_free(mgr); +} + /** Allocate an empty configuration object of a given format type. */ void * -config_new(const config_format_t *fmt) +config_new(const config_mgr_t *mgr) { + const config_format_t *fmt = mgr->toplevel; void *opts = tor_malloc_zero(fmt->size); struct_set_magic(opts, &fmt->magic); CONFIG_CHECK(fmt, opts); @@ -60,9 +91,10 @@ config_new(const config_format_t *fmt) * apply abbreviations that work for the config file and the command line. * If warn_obsolete is set, warn about deprecated names. */ const char * -config_expand_abbrev(const config_format_t *fmt, const char *option, +config_expand_abbrev(const config_mgr_t *mgr, const char *option, int command_line, int warn_obsolete) { + const config_format_t *fmt = mgr->toplevel; int i; if (! fmt->abbrevs) return option; @@ -90,8 +122,9 @@ config_expand_abbrev(const config_format_t *fmt, const char *option, * explaining why it is deprecated (which may be an empty string). Return NULL * if it is not deprecated. The key field must be fully expanded. */ const char * -config_find_deprecation(const config_format_t *fmt, const char *key) +config_find_deprecation(const config_mgr_t *mgr, const char *key) { + const config_format_t *fmt = mgr->toplevel; if (BUG(fmt == NULL) || BUG(key == NULL)) return NULL; // LCOV_EXCL_LINE if (fmt->deprecations == NULL) @@ -112,8 +145,9 @@ config_find_deprecation(const config_format_t *fmt, const char *key) * NULL. */ const config_var_t * -config_find_option(const config_format_t *fmt, const char *key) +config_find_option(const config_mgr_t *mgr, const char *key) { + const config_format_t *fmt = mgr->toplevel; int i; size_t keylen = strlen(key); if (!keylen) @@ -139,8 +173,9 @@ config_find_option(const config_format_t *fmt, const char *key) /** Return the number of option entries in fmt. */ static int -config_count_options(const config_format_t *fmt) +config_count_options(const config_mgr_t *mgr) { + const config_format_t *fmt = mgr->toplevel; int i; for (i=0; fmt->vars[i].member.name; ++i) ; @@ -175,14 +210,15 @@ config_var_is_contained(const config_var_t *var) * Called from config_assign_line() and option_reset(). */ static int -config_assign_value(const config_format_t *fmt, void *options, +config_assign_value(const config_mgr_t *mgr, void *options, config_line_t *c, char **msg) { const config_var_t *var; + const config_format_t *fmt = mgr->toplevel; CONFIG_CHECK(fmt, options); - var = config_find_option(fmt, c->key); + var = config_find_option(mgr, c->key); tor_assert(var); tor_assert(!strcmp(c->key, var->member.name)); @@ -192,9 +228,11 @@ config_assign_value(const config_format_t *fmt, void *options, /** Mark every linelist in options "fragile", so that fresh assignments * to it will replace old ones. */ static void -config_mark_lists_fragile(const config_format_t *fmt, void *options) +config_mark_lists_fragile(const config_mgr_t *mgr, void *options) { int i; + const config_format_t *fmt = mgr->toplevel; + tor_assert(fmt); tor_assert(options); @@ -224,10 +262,11 @@ warn_deprecated_option(const char *what, const char *why) * Called from config_assign(). */ static int -config_assign_line(const config_format_t *fmt, void *options, +config_assign_line(const config_mgr_t *mgr, void *options, config_line_t *c, unsigned flags, bitarray_t *options_seen, char **msg) { + const config_format_t *fmt = mgr->toplevel; const unsigned use_defaults = flags & CAL_USE_DEFAULTS; const unsigned clear_first = flags & CAL_CLEAR_FIRST; const unsigned warn_deprecations = flags & CAL_WARN_DEPRECATIONS; @@ -235,7 +274,7 @@ config_assign_line(const config_format_t *fmt, void *options, CONFIG_CHECK(fmt, options); - var = config_find_option(fmt, c->key); + var = config_find_option(mgr, c->key); if (!var) { if (fmt->extra) { void *lvalue = STRUCT_VAR_P(options, fmt->extra->offset); @@ -258,7 +297,7 @@ config_assign_line(const config_format_t *fmt, void *options, const char *deprecation_msg; if (warn_deprecations && - (deprecation_msg = config_find_deprecation(fmt, var->member.name))) { + (deprecation_msg = config_find_deprecation(mgr, var->member.name))) { warn_deprecated_option(var->member.name, deprecation_msg); } @@ -271,14 +310,14 @@ config_assign_line(const config_format_t *fmt, void *options, log_warn(LD_CONFIG, "Linelist option '%s' has no value. Skipping.", c->key); } else { /* not already cleared */ - config_reset(fmt, options, var, use_defaults); + config_reset(mgr, options, var, use_defaults); } } return 0; } else if (c->command == CONFIG_LINE_CLEAR && !clear_first) { // XXXX This is unreachable, since a CLEAR line always has an // XXXX empty value. - config_reset(fmt, options, var, use_defaults); // LCOV_EXCL_LINE + config_reset(mgr, options, var, use_defaults); // LCOV_EXCL_LINE } if (options_seen && ! config_var_is_cumulative(var)) { @@ -292,7 +331,7 @@ config_assign_line(const config_format_t *fmt, void *options, bitarray_set(options_seen, var_index); } - if (config_assign_value(fmt, options, c, msg) < 0) + if (config_assign_value(mgr, options, c, msg) < 0) return -2; return 0; } @@ -300,18 +339,19 @@ config_assign_line(const config_format_t *fmt, void *options, /** Restore the option named key in options to its default value. * Called from config_assign(). */ STATIC void -config_reset_line(const config_format_t *fmt, void *options, +config_reset_line(const config_mgr_t *mgr, void *options, const char *key, int use_defaults) { + const config_format_t *fmt = mgr->toplevel; const config_var_t *var; CONFIG_CHECK(fmt, options); - var = config_find_option(fmt, key); + var = config_find_option(mgr, key); if (!var) return; /* give error on next pass. */ - config_reset(fmt, options, var, use_defaults); + config_reset(mgr, options, var, use_defaults); } /** Return true iff value needs to be quoted and escaped to be used in @@ -345,16 +385,18 @@ config_value_needs_escape(const char *value) * value needs to be quoted before it's put in a config file, quote and * escape that value. Return NULL if no such key exists. */ config_line_t * -config_get_assigned_option(const config_format_t *fmt, const void *options, +config_get_assigned_option(const config_mgr_t *mgr, const void *options, const char *key, int escape_val) { const config_var_t *var; config_line_t *result; + const config_format_t *fmt = mgr->toplevel; + tor_assert(options && key); CONFIG_CHECK(fmt, options); - var = config_find_option(fmt, key); + var = config_find_option(mgr, key); if (!var) { log_warn(LD_CONFIG, "Unknown option '%s'. Failing.", key); return NULL; @@ -432,12 +474,13 @@ options_trial_assign() calls config_assign(1, 1) returns. */ int -config_assign(const config_format_t *fmt, void *options, config_line_t *list, +config_assign(const config_mgr_t *mgr, void *options, config_line_t *list, unsigned config_assign_flags, char **msg) { config_line_t *p; bitarray_t *options_seen; - const int n_options = config_count_options(fmt); + const config_format_t *fmt = mgr->toplevel; + const int n_options = config_count_options(mgr); const unsigned clear_first = config_assign_flags & CAL_CLEAR_FIRST; const unsigned use_defaults = config_assign_flags & CAL_USE_DEFAULTS; @@ -445,7 +488,7 @@ config_assign(const config_format_t *fmt, void *options, config_line_t *list, /* pass 1: normalize keys */ for (p = list; p; p = p->next) { - const char *full = config_expand_abbrev(fmt, p->key, 0, 1); + const char *full = config_expand_abbrev(mgr, p->key, 0, 1); if (strcmp(full,p->key)) { tor_free(p->key); p->key = tor_strdup(full); @@ -456,14 +499,14 @@ config_assign(const config_format_t *fmt, void *options, config_line_t *list, * mentioned config options, and maybe set to their defaults. */ if (clear_first) { for (p = list; p; p = p->next) - config_reset_line(fmt, options, p->key, use_defaults); + config_reset_line(mgr, options, p->key, use_defaults); } options_seen = bitarray_init_zero(n_options); /* pass 3: assign. */ while (list) { int r; - if ((r=config_assign_line(fmt, options, list, config_assign_flags, + if ((r=config_assign_line(mgr, options, list, config_assign_flags, options_seen, msg))) { bitarray_free(options_seen); return r; @@ -475,7 +518,7 @@ config_assign(const config_format_t *fmt, void *options, config_line_t *list, /** Now we're done assigning a group of options to the configuration. * Subsequent group assignments should _replace_ linelists, not extend * them. */ - config_mark_lists_fragile(fmt, options); + config_mark_lists_fragile(mgr, options); return 0; } @@ -483,12 +526,9 @@ config_assign(const config_format_t *fmt, void *options, config_line_t *list, /** Reset config option var to 0, 0.0, NULL, or the equivalent. * Called from config_reset() and config_free(). */ static void -config_clear(const config_format_t *fmt, void *options, +config_clear(void *options, const config_var_t *var) { - - (void)fmt; /* unused */ - struct_var_free(options, &var->member); } @@ -496,20 +536,21 @@ config_clear(const config_format_t *fmt, void *options, * use_defaults, set it to its default value. * Called by config_init() and option_reset_line() and option_assign_line(). */ static void -config_reset(const config_format_t *fmt, void *options, +config_reset(const config_mgr_t *mgr, void *options, const config_var_t *var, int use_defaults) { + const config_format_t *fmt = mgr->toplevel; config_line_t *c; char *msg = NULL; CONFIG_CHECK(fmt, options); - config_clear(fmt, options, var); /* clear it first */ + config_clear(options, var); /* clear it first */ if (!use_defaults) return; /* all done */ if (var->initvalue) { c = tor_malloc_zero(sizeof(config_line_t)); c->key = tor_strdup(var->member.name); c->value = tor_strdup(var->initvalue); - if (config_assign_value(fmt, options, c, &msg) < 0) { + if (config_assign_value(mgr, options, c, &msg) < 0) { // LCOV_EXCL_START log_warn(LD_BUG, "Failed to assign default: %s", msg); tor_free(msg); /* if this happens it's a bug */ @@ -521,9 +562,10 @@ config_reset(const config_format_t *fmt, void *options, /** Release storage held by options. */ void -config_free_(const config_format_t *fmt, void *options) +config_free_(const config_mgr_t *mgr, void *options) { int i; + const config_format_t *fmt = mgr->toplevel; if (!options) return; @@ -531,7 +573,7 @@ config_free_(const config_format_t *fmt, void *options) tor_assert(fmt); for (i=0; fmt->vars[i].member.name; ++i) - config_clear(fmt, options, &(fmt->vars[i])); + config_clear(options, &(fmt->vars[i])); if (fmt->extra) { config_line_t **linep = STRUCT_VAR_P(options, fmt->extra->offset); @@ -545,14 +587,15 @@ config_free_(const config_format_t *fmt, void *options) * and o2. Must not be called for LINELIST_S or OBSOLETE options. */ int -config_is_same(const config_format_t *fmt, +config_is_same(const config_mgr_t *mgr, const void *o1, const void *o2, const char *name) { + const config_format_t *fmt = mgr->toplevel; CONFIG_CHECK(fmt, o1); CONFIG_CHECK(fmt, o2); - const config_var_t *var = config_find_option(fmt, name); + const config_var_t *var = config_find_option(mgr, name); if (!var) { return true; } @@ -562,12 +605,13 @@ config_is_same(const config_format_t *fmt, /** Copy storage held by old into a new or_options_t and return it. */ void * -config_dup(const config_format_t *fmt, const void *old) +config_dup(const config_mgr_t *mgr, const void *old) { + const config_format_t *fmt = mgr->toplevel; void *newopts; int i; - newopts = config_new(fmt); + newopts = config_new(mgr); for (i=0; fmt->vars[i].member.name; ++i) { if (config_var_is_contained(&fmt->vars[i])) { // Something else will copy this option, or it doesn't need copying. @@ -586,9 +630,10 @@ config_dup(const config_format_t *fmt, const void *old) /** Set all vars in the configuration object options to their default * values. */ void -config_init(const config_format_t *fmt, void *options) +config_init(const config_mgr_t *mgr, void *options) { int i; + const config_format_t *fmt = mgr->toplevel; const config_var_t *var; CONFIG_CHECK(fmt, options); @@ -596,7 +641,7 @@ config_init(const config_format_t *fmt, void *options) var = &fmt->vars[i]; if (!var->initvalue) continue; /* defaults to NULL or 0 */ - config_reset(fmt, options, var, 1); + config_reset(mgr, options, var, 1); } } @@ -605,11 +650,12 @@ config_init(const config_format_t *fmt, void *options) * Else, if comment_defaults, write default values as comments. */ char * -config_dump(const config_format_t *fmt, const void *default_options, +config_dump(const config_mgr_t *mgr, const void *default_options, const void *options, int minimal, int comment_defaults) { smartlist_t *elements; + const config_format_t *fmt = mgr->toplevel; const void *defaults = default_options; void *defaults_tmp = NULL; config_line_t *line, *assigned; @@ -618,8 +664,8 @@ config_dump(const config_format_t *fmt, const void *default_options, char *msg = NULL; if (defaults == NULL) { - defaults = defaults_tmp = config_new(fmt); - config_init(fmt, defaults_tmp); + defaults = defaults_tmp = config_new(mgr); + config_init(mgr, defaults_tmp); } /* XXX use a 1 here so we don't add a new log line while dumping */ @@ -643,15 +689,15 @@ config_dump(const config_format_t *fmt, const void *default_options, /* Don't save 'hidden' control variables. */ if (fmt->vars[i].flags & CVFLAG_NODUMP) continue; - if (minimal && config_is_same(fmt, options, defaults, + if (minimal && config_is_same(mgr, options, defaults, fmt->vars[i].member.name)) continue; else if (comment_defaults && - config_is_same(fmt, options, defaults, fmt->vars[i].member.name)) + config_is_same(mgr, options, defaults, fmt->vars[i].member.name)) comment_option = 1; line = assigned = - config_get_assigned_option(fmt, options, fmt->vars[i].member.name, 1); + config_get_assigned_option(mgr, options, fmt->vars[i].member.name, 1); for (; line; line = line->next) { if (!strcmpstart(line->key, "__")) { @@ -677,7 +723,7 @@ config_dump(const config_format_t *fmt, const void *default_options, SMARTLIST_FOREACH(elements, char *, cp, tor_free(cp)); smartlist_free(elements); if (defaults_tmp) { - fmt->free_fn(defaults_tmp); + fmt->free_fn(mgr, defaults_tmp); } return result; } @@ -687,8 +733,9 @@ config_dump(const config_format_t *fmt, const void *default_options, * Return false otherwise. Log errors at level severity. */ bool -config_check_ok(const config_format_t *fmt, const void *options, int severity) +config_check_ok(const config_mgr_t *mgr, const void *options, int severity) { + const config_format_t *fmt = mgr->toplevel; bool all_ok = true; for (int i=0; fmt->vars[i].member.name; ++i) { if (!struct_var_ok(options, &fmt->vars[i].member)) { diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 3633c2a80b..a6a2450aee 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -39,8 +39,10 @@ typedef struct config_deprecation_t { * of arguments. */ typedef int (*validate_fn_t)(void*,void*,void*,int,char**); +struct config_mgr_t; + /** Callback to free a configuration object. */ -typedef void (*free_cfg_fn_t)(void*); +typedef void (*free_cfg_fn_t)(const struct config_mgr_t *mgr, void*); /** Information on the keys, value types, key-to-struct-member mappings, * variable descriptions, validation functions, and abbreviations for a @@ -61,6 +63,19 @@ typedef struct config_format_t { const struct_member_t *extra; } config_format_t; +/** + * A collection of config_format_t objects to describe several objects + * that are all configured with the same configuration file. + * + * (NOTE: for now, this only handles a single config_format_t.) + **/ +typedef struct config_mgr_t config_mgr_t; + +config_mgr_t *config_mgr_new(const config_format_t *toplevel_fmt); +void config_mgr_free_(config_mgr_t *mgr); +#define config_mgr_free(mgr) \ + FREE_AND_NULL(config_mgr_t, config_mgr_free_, (mgr)) + /** Macro: assert that cfg has the right magic field for format * fmt. */ #define CONFIG_CHECK(fmt, cfg) STMT_BEGIN \ @@ -72,34 +87,34 @@ typedef struct config_format_t { #define CAL_CLEAR_FIRST (1u<<1) #define CAL_WARN_DEPRECATIONS (1u<<2) -void *config_new(const config_format_t *fmt); -void config_free_(const config_format_t *fmt, void *options); -#define config_free(fmt, options) do { \ - config_free_((fmt), (options)); \ +void *config_new(const config_mgr_t *fmt); +void config_free_(const config_mgr_t *fmt, void *options); +#define config_free(mgr, options) do { \ + config_free_((mgr), (options)); \ (options) = NULL; \ } while (0) -struct config_line_t *config_get_assigned_option(const config_format_t *fmt, +struct config_line_t *config_get_assigned_option(const config_mgr_t *mgr, const void *options, const char *key, int escape_val); -int config_is_same(const config_format_t *fmt, +int config_is_same(const config_mgr_t *fmt, const void *o1, const void *o2, const char *name); -void config_init(const config_format_t *fmt, void *options); -void *config_dup(const config_format_t *fmt, const void *old); -char *config_dump(const config_format_t *fmt, const void *default_options, +void config_init(const config_mgr_t *mgr, void *options); +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, int comment_defaults); -bool config_check_ok(const config_format_t *fmt, const void *options, +bool config_check_ok(const config_mgr_t *mgr, const void *options, int severity); -int config_assign(const config_format_t *fmt, void *options, +int config_assign(const config_mgr_t *mgr, void *options, struct config_line_t *list, unsigned flags, char **msg); -const char *config_find_deprecation(const config_format_t *fmt, +const char *config_find_deprecation(const config_mgr_t *mgr, const char *key); -const config_var_t *config_find_option(const config_format_t *fmt, +const config_var_t *config_find_option(const config_mgr_t *mgr, const char *key); -const char *config_expand_abbrev(const config_format_t *fmt, +const char *config_expand_abbrev(const config_mgr_t *mgr, const char *option, int command_line, int warn_obsolete); void warn_deprecated_option(const char *what, const char *why); @@ -117,7 +132,7 @@ bool config_var_is_contained(const config_var_t *var); #define CFG_EQ_ROUTERSET(a,b,opt) routerset_equal((a)->opt, (b)->opt) #ifdef CONFPARSE_PRIVATE -STATIC void config_reset_line(const config_format_t *fmt, void *options, +STATIC void config_reset_line(const config_mgr_t *mgr, void *options, const char *key, int use_defaults); #endif diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index d997d3932e..a44bcf6fb5 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -145,7 +145,7 @@ static int or_state_validate_cb(void *old_options, void *options, void *default_options, int from_setconf, char **msg); -static void or_state_free_cb(void *state); +static void or_state_free_cb(const config_mgr_t *mgr, void *state); /** Magic value for or_state_t. */ #define OR_STATE_MAGIC 0x57A73f57 @@ -174,6 +174,19 @@ static const config_format_t state_format = { &state_extra_var, }; +/* A global configuration manager for state-file objects */ +static config_mgr_t *state_mgr = NULL; + +/** Return the configuration manager for state-file objects. */ +static const config_mgr_t * +get_state_mgr(void) +{ + if (PREDICT_UNLIKELY(state_mgr == NULL)) { + state_mgr = config_mgr_new(&state_format); + } + return state_mgr; +} + /** Persistent serialized state. */ static or_state_t *global_state = NULL; @@ -269,8 +282,9 @@ or_state_validate_cb(void *old_state, void *state, void *default_state, } static void -or_state_free_cb(void *state) +or_state_free_cb(const config_mgr_t *mgr, void *state) { + (void)mgr; or_state_free_(state); } @@ -298,7 +312,7 @@ or_state_set(or_state_t *new_state) char *err = NULL; int ret = 0; tor_assert(new_state); - config_free(&state_format, global_state); + config_free(get_state_mgr(), global_state); global_state = new_state; if (entry_guards_parse_state(global_state, 1, &err)<0) { log_warn(LD_GENERAL,"%s",err); @@ -363,7 +377,7 @@ or_state_new(void) { or_state_t *new_state = tor_malloc_zero(sizeof(or_state_t)); new_state->magic_ = OR_STATE_MAGIC; - config_init(&state_format, new_state); + config_init(get_state_mgr(), new_state); return new_state; } @@ -404,7 +418,7 @@ or_state_load(void) int assign_retval; if (config_get_lines(contents, &lines, 0)<0) goto done; - assign_retval = config_assign(&state_format, new_state, + assign_retval = config_assign(get_state_mgr(), new_state, lines, 0, &errmsg); config_free_lines(lines); if (assign_retval<0) @@ -431,7 +445,7 @@ or_state_load(void) or_state_save_broken(fname); tor_free(contents); - config_free(&state_format, new_state); + config_free(get_state_mgr(), new_state); new_state = or_state_new(); } else if (contents) { @@ -464,7 +478,7 @@ or_state_load(void) tor_free(fname); tor_free(contents); if (new_state) - config_free(&state_format, new_state); + config_free(get_state_mgr(), new_state); return r; } @@ -517,7 +531,7 @@ or_state_save(time_t now) tor_free(global_state->TorVersion); tor_asprintf(&global_state->TorVersion, "Tor %s", get_version()); - state = config_dump(&state_format, NULL, global_state, 1, 0); + state = config_dump(get_state_mgr(), NULL, global_state, 1, 0); format_local_iso_time(tbuf, now); tor_asprintf(&contents, "# Tor state file last generated on %s local time\n" @@ -727,7 +741,7 @@ or_state_free_(or_state_t *state) if (!state) return; - config_free(&state_format, state); + config_free(get_state_mgr(), state); } void @@ -735,4 +749,5 @@ or_state_free_all(void) { or_state_free(global_state); global_state = NULL; + config_mgr_free(state_mgr); } diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index c2ad3e7cca..a552e621ca 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -62,7 +62,7 @@ DUMMY_TYPECHECK_INSTANCE(sr_disk_state_t); static int disk_state_validate_cb(void *old_state, void *state, void *default_state, int from_setconf, char **msg); -static void disk_state_free_cb(void *); +static void disk_state_free_cb(const config_mgr_t *mgr, void *); /* Array of variables that are saved to disk as a persistent state. */ static const config_var_t state_vars[] = { @@ -103,6 +103,19 @@ static const config_format_t state_format = { &state_extra_var, }; +/* Global configuration manager for the shared-random state file */ +static config_mgr_t *shared_random_state_mgr = NULL; + +/** Return the configuration manager for the shared-random state file. */ +static const config_mgr_t * +get_srs_mgr(void) +{ + if (PREDICT_UNLIKELY(shared_random_state_mgr == NULL)) { + shared_random_state_mgr = config_mgr_new(&state_format); + } + return shared_random_state_mgr; +} + static void state_query_del_(sr_state_object_t obj_type, void *data); /* Return a string representation of a protocol phase. */ @@ -264,7 +277,7 @@ disk_state_free_(sr_disk_state_t *state) if (state == NULL) { return; } - config_free(&state_format, state); + config_free(get_srs_mgr(), state); } /* Allocate a new disk state, initialize it and return it. */ @@ -280,7 +293,7 @@ disk_state_new(time_t now) new_state->ValidAfter = now; /* Init config format. */ - config_init(&state_format, new_state); + config_init(get_srs_mgr(), new_state); return new_state; } @@ -349,8 +362,9 @@ disk_state_validate_cb(void *old_state, void *state, void *default_state, } static void -disk_state_free_cb(void *state) +disk_state_free_cb(const config_mgr_t *mgr, void *state) { + (void)mgr; disk_state_free_(state); } @@ -683,7 +697,7 @@ disk_state_load_from_disk_impl(const char *fname) } disk_state = disk_state_new(time(NULL)); - config_assign(&state_format, disk_state, lines, 0, &errmsg); + config_assign(get_srs_mgr(), disk_state, lines, 0, &errmsg); config_free_lines(lines); if (errmsg) { log_warn(LD_DIR, "SR: Reading state error: %s", errmsg); @@ -736,7 +750,7 @@ disk_state_save_to_disk(void) /* Make sure that our disk state is up to date with our memory state * before saving it to disk. */ disk_state_update(); - state = config_dump(&state_format, NULL, sr_disk_state, 0, 0); + state = config_dump(get_srs_mgr(), NULL, sr_disk_state, 0, 0); format_local_iso_time(tbuf, now); tor_asprintf(&content, "# Tor shared random state file last generated on %s " @@ -1278,6 +1292,7 @@ sr_state_free_all(void) /* Nullify our global state. */ sr_state = NULL; sr_disk_state = NULL; + config_mgr_free(shared_random_state_mgr); } /* Save our current state in memory to disk. */ diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index ec018f0c52..8a921531fa 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -119,7 +119,7 @@ test_validate_cb(void *old_options, void *options, void *default_options, return 0; } -static void test_free_cb(void *options); +static void test_free_cb(const config_mgr_t *mgr, void *options); #define TEST_MAGIC 0x1337 @@ -139,12 +139,12 @@ static const config_format_t test_fmt = { }; static void -test_free_cb(void *options) +test_free_cb(const config_mgr_t *mgr, void *options) { if (!options) return; - config_free(&test_fmt, options); + config_free(mgr, options); } /* Make sure that config_init sets everything to the right defaults. */ @@ -152,8 +152,9 @@ static void test_confparse_init(void *arg) { (void)arg; - test_struct_t *tst = config_new(&test_fmt); - config_init(&test_fmt, tst); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = config_new(mgr); + config_init(mgr, tst); // Make sure that options are initialized right. */ tt_uint_op(tst->magic, OP_EQ, TEST_MAGIC); @@ -178,7 +179,8 @@ test_confparse_init(void *arg) tt_int_op(tst->hidden_int, OP_EQ, 0); done: - config_free(&test_fmt, tst); + config_free(mgr, tst); + config_mgr_free(mgr); } static const char simple_settings[] = @@ -207,18 +209,18 @@ static const char simple_settings[] = /* Return a configuration object set up from simple_settings above. */ static test_struct_t * -get_simple_config(void) +get_simple_config(const config_mgr_t *mgr) { test_struct_t *result = NULL; - test_struct_t *tst = config_new(&test_fmt); + test_struct_t *tst = config_new(mgr); config_line_t *lines = NULL; char *msg = NULL; - config_init(&test_fmt, tst); + config_init(mgr, tst); int r = config_get_lines(simple_settings, &lines, 0); tt_int_op(r, OP_EQ, 0); - r = config_assign(&test_fmt, tst, lines, 0, &msg); + r = config_assign(mgr, tst, lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); @@ -227,7 +229,7 @@ get_simple_config(void) done: tor_free(msg); config_free_lines(lines); - config_free(&test_fmt, tst); + config_free(mgr, tst); return result; } @@ -236,7 +238,8 @@ static void test_confparse_assign_simple(void *arg) { (void)arg; - test_struct_t *tst = get_simple_config(); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); tt_str_op(tst->s, OP_EQ, "this is a"); tt_str_op(tst->fn, OP_EQ, "/simple/test of the"); @@ -284,10 +287,11 @@ test_confparse_assign_simple(void *arg) tt_str_op(tst->mixed_hidden_lines->next->value, OP_EQ, "ABC"); tt_assert(!tst->mixed_hidden_lines->next->next); - tt_assert(config_check_ok(&test_fmt, tst, LOG_ERR)); + tt_assert(config_check_ok(mgr, tst, LOG_ERR)); done: - config_free(&test_fmt, tst); + config_free(mgr, tst); + config_mgr_free(mgr); } /* Try to assign to an obsolete option, and make sure we get a warning. */ @@ -295,26 +299,28 @@ static void test_confparse_assign_obsolete(void *arg) { (void)arg; - test_struct_t *tst = config_new(&test_fmt); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; - config_init(&test_fmt, tst); + config_init(mgr, tst); int r = config_get_lines("obsolete option here", &lines, 0); tt_int_op(r, OP_EQ, 0); setup_capture_of_logs(LOG_WARN); - r = config_assign(&test_fmt, tst, lines, 0, &msg); + r = config_assign(mgr, tst, lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); expect_single_log_msg_containing("Skipping obsolete configuration option"); done: teardown_capture_of_logs(); - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); tor_free(msg); + config_mgr_free(mgr); } /* Try to assign to an deprecated option, and make sure we get a warning @@ -323,30 +329,32 @@ static void test_confparse_assign_deprecated(void *arg) { (void)arg; - test_struct_t *tst = config_new(&test_fmt); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; - config_init(&test_fmt, tst); + config_init(mgr, tst); int r = config_get_lines("deprecated_int 7", &lines, 0); tt_int_op(r, OP_EQ, 0); setup_capture_of_logs(LOG_WARN); - r = config_assign(&test_fmt, tst, lines, CAL_WARN_DEPRECATIONS, &msg); + r = config_assign(mgr, tst, lines, CAL_WARN_DEPRECATIONS, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); expect_single_log_msg_containing("This integer is deprecated."); tt_int_op(tst->deprecated_int, OP_EQ, 7); - tt_assert(config_check_ok(&test_fmt, tst, LOG_ERR)); + tt_assert(config_check_ok(mgr, tst, LOG_ERR)); done: teardown_capture_of_logs(); - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); tor_free(msg); + config_mgr_free(mgr); } /* Try to re-assign an option name that has been depreacted in favor of @@ -355,16 +363,17 @@ static void test_confparse_assign_replaced(void *arg) { (void)arg; - test_struct_t *tst = config_new(&test_fmt); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; - config_init(&test_fmt, tst); + config_init(mgr, tst); int r = config_get_lines("float 1000\n", &lines, 0); tt_int_op(r, OP_EQ, 0); setup_capture_of_logs(LOG_WARN); - r = config_assign(&test_fmt, tst, lines, CAL_WARN_DEPRECATIONS, &msg); + r = config_assign(mgr, tst, lines, CAL_WARN_DEPRECATIONS, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); expect_single_log_msg_containing("use 'dbl' instead."); @@ -374,9 +383,10 @@ test_confparse_assign_replaced(void *arg) done: teardown_capture_of_logs(); - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); tor_free(msg); + config_mgr_free(mgr); } /* Try to set a linelist value with no option. */ @@ -384,25 +394,27 @@ static void test_confparse_assign_emptystring(void *arg) { (void)arg; - test_struct_t *tst = config_new(&test_fmt); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; - config_init(&test_fmt, tst); + config_init(mgr, tst); int r = config_get_lines("lines\n", &lines, 0); tt_int_op(r, OP_EQ, 0); setup_capture_of_logs(LOG_WARN); - r = config_assign(&test_fmt, tst, lines, 0, &msg); + r = config_assign(mgr, tst, lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); expect_single_log_msg_containing("has no value"); done: teardown_capture_of_logs(); - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); tor_free(msg); + config_mgr_free(mgr); } /* Try to set a the same option twice; make sure we get a warning. */ @@ -410,26 +422,28 @@ static void test_confparse_assign_twice(void *arg) { (void)arg; - test_struct_t *tst = config_new(&test_fmt); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; - config_init(&test_fmt, tst); + config_init(mgr, tst); int r = config_get_lines("pos 10\n" "pos 99\n", &lines, 0); tt_int_op(r, OP_EQ, 0); setup_capture_of_logs(LOG_WARN); - r = config_assign(&test_fmt, tst, lines, 0, &msg); + r = config_assign(mgr, tst, lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); expect_single_log_msg_containing("used more than once"); done: teardown_capture_of_logs(); - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); tor_free(msg); + config_mgr_free(mgr); } typedef struct badval_test_t { @@ -443,16 +457,17 @@ static void test_confparse_assign_badval(void *arg) { const badval_test_t *bt = arg; - test_struct_t *tst = config_new(&test_fmt); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; - config_init(&test_fmt, tst); + config_init(mgr, tst); int r = config_get_lines(bt->cfg, &lines, 0); tt_int_op(r, OP_EQ, 0); setup_capture_of_logs(LOG_WARN); - r = config_assign(&test_fmt, tst, lines, 0, &msg); + r = config_assign(mgr, tst, lines, 0, &msg); tt_int_op(r, OP_LT, 0); tt_ptr_op(msg, OP_NE, NULL); if (! strstr(msg, bt->expect_msg)) { @@ -461,9 +476,10 @@ test_confparse_assign_badval(void *arg) done: teardown_capture_of_logs(); - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); tor_free(msg); + config_mgr_free(mgr); } /* Various arguments for badval test. @@ -495,11 +511,12 @@ static void test_confparse_dump(void *arg) { (void)arg; - test_struct_t *tst = get_simple_config(); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); char *dumped = NULL; /* Minimal version. */ - dumped = config_dump(&test_fmt, NULL, tst, 1, 0); + dumped = config_dump(mgr, NULL, tst, 1, 0); tt_str_op(dumped, OP_EQ, "s this is a\n" "fn /simple/test of the\n" @@ -524,7 +541,7 @@ test_confparse_dump(void *arg) /* Maximal */ tor_free(dumped); - dumped = config_dump(&test_fmt, NULL, tst, 0, 0); + dumped = config_dump(mgr, NULL, tst, 0, 0); tt_str_op(dumped, OP_EQ, "s this is a\n" "fn /simple/test of the\n" @@ -550,7 +567,7 @@ test_confparse_dump(void *arg) /* commented */ tor_free(dumped); - dumped = config_dump(&test_fmt, NULL, tst, 0, 1); + dumped = config_dump(mgr, NULL, tst, 0, 1); tt_str_op(dumped, OP_EQ, "s this is a\n" "fn /simple/test of the\n" @@ -575,8 +592,9 @@ test_confparse_dump(void *arg) "VisibleLineB ABC\n"); done: - config_free(&test_fmt, tst); + config_free(mgr, tst); tor_free(dumped); + config_mgr_free(mgr); } /* Try confparse_reset_line(), and make sure it behaves correctly */ @@ -584,16 +602,18 @@ static void test_confparse_reset(void *arg) { (void)arg; - test_struct_t *tst = get_simple_config(); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); - config_reset_line(&test_fmt, tst, "interval", 0); + config_reset_line(mgr, tst, "interval", 0); tt_int_op(tst->interval, OP_EQ, 0); - config_reset_line(&test_fmt, tst, "interval", 1); + config_reset_line(mgr, tst, "interval", 1); tt_int_op(tst->interval, OP_EQ, 10); done: - config_free(&test_fmt, tst); + config_free(mgr, tst); + config_mgr_free(mgr); } /* Try setting options a second time on a config object, and make sure @@ -602,7 +622,8 @@ static void test_confparse_reassign(void *arg) { (void)arg; - test_struct_t *tst = get_simple_config(); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL, *rs = NULL; @@ -613,7 +634,7 @@ test_confparse_reassign(void *arg) "csv 14,15\n" "routerset 127.0.0.1\n", &lines, 0); - r = config_assign(&test_fmt, tst,lines, 0, &msg); + r = config_assign(mgr, tst,lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); @@ -633,7 +654,7 @@ test_confparse_reassign(void *arg) tt_str_op(rs, OP_EQ, "127.0.0.1"); // Try again with the CLEAR_FIRST and USE_DEFAULTS flags - r = config_assign(&test_fmt, tst, lines, + r = config_assign(mgr, tst, lines, CAL_CLEAR_FIRST|CAL_USE_DEFAULTS, &msg); tt_int_op(r, OP_EQ, 0); @@ -644,10 +665,11 @@ test_confparse_reassign(void *arg) tt_int_op(tst->i, OP_EQ, 12); done: - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); tor_free(msg); tor_free(rs); + config_mgr_free(mgr); } /* Try setting options a second time on a config object, using the +foo @@ -656,7 +678,8 @@ static void test_confparse_reassign_extend(void *arg) { (void)arg; - test_struct_t *tst = get_simple_config(); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; @@ -664,7 +687,7 @@ test_confparse_reassign_extend(void *arg) "+lines 13\n", &lines, 1); // allow extended format. tt_int_op(r, OP_EQ, 0); - r = config_assign(&test_fmt, tst,lines, 0, &msg); + r = config_assign(mgr, tst,lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); @@ -684,27 +707,28 @@ test_confparse_reassign_extend(void *arg) "/lines\n", &lines, 1); // allow extended format. tt_int_op(r, OP_EQ, 0); - r = config_assign(&test_fmt, tst, lines, 0, &msg); + r = config_assign(mgr, tst, lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); tt_assert(tst->lines == NULL); config_free_lines(lines); - config_free(&test_fmt, tst); - tst = get_simple_config(); + config_free(mgr, tst); + tst = get_simple_config(mgr); r = config_get_lines( "/lines away!\n", &lines, 1); // allow extended format. tt_int_op(r, OP_EQ, 0); - r = config_assign(&test_fmt, tst, lines, 0, &msg); + r = config_assign(mgr, tst, lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); tt_assert(tst->lines == NULL); done: - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); tor_free(msg); + config_mgr_free(mgr); } /* Test out confparse_get_assigned(). */ @@ -712,30 +736,32 @@ static void test_confparse_get_assigned(void *arg) { (void)arg; - test_struct_t *tst = get_simple_config(); + + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; - lines = config_get_assigned_option(&test_fmt, tst, "I", 1); + lines = config_get_assigned_option(mgr, tst, "I", 1); tt_assert(lines); tt_str_op(lines->key, OP_EQ, "i"); tt_str_op(lines->value, OP_EQ, "3"); tt_assert(lines->next == NULL); config_free_lines(lines); - lines = config_get_assigned_option(&test_fmt, tst, "s", 1); + lines = config_get_assigned_option(mgr, tst, "s", 1); tt_assert(lines); tt_str_op(lines->key, OP_EQ, "s"); tt_str_op(lines->value, OP_EQ, "this is a"); tt_assert(lines->next == NULL); config_free_lines(lines); - lines = config_get_assigned_option(&test_fmt, tst, "obsolete", 1); + lines = config_get_assigned_option(mgr, tst, "obsolete", 1); tt_assert(!lines); - lines = config_get_assigned_option(&test_fmt, tst, "nonesuch", 1); + lines = config_get_assigned_option(mgr, tst, "nonesuch", 1); tt_assert(!lines); - lines = config_get_assigned_option(&test_fmt, tst, "mixedlines", 1); + lines = config_get_assigned_option(mgr, tst, "mixedlines", 1); tt_assert(lines); tt_str_op(lines->key, OP_EQ, "LineTypeA"); tt_str_op(lines->value, OP_EQ, "i d"); @@ -745,7 +771,7 @@ test_confparse_get_assigned(void *arg) tt_assert(lines->next->next == NULL); config_free_lines(lines); - lines = config_get_assigned_option(&test_fmt, tst, "linetypeb", 1); + lines = config_get_assigned_option(mgr, tst, "linetypeb", 1); tt_assert(lines); tt_str_op(lines->key, OP_EQ, "LineTypeB"); tt_str_op(lines->value, OP_EQ, "i c"); @@ -754,7 +780,7 @@ test_confparse_get_assigned(void *arg) tor_free(tst->s); tst->s = tor_strdup("Hello\nWorld"); - lines = config_get_assigned_option(&test_fmt, tst, "s", 1); + lines = config_get_assigned_option(mgr, tst, "s", 1); tt_assert(lines); tt_str_op(lines->key, OP_EQ, "s"); tt_str_op(lines->value, OP_EQ, "\"Hello\\nWorld\""); @@ -762,8 +788,9 @@ test_confparse_get_assigned(void *arg) config_free_lines(lines); done: - config_free(&test_fmt, tst); + config_free(mgr, tst); config_free_lines(lines); + config_mgr_free(mgr); } /* Another variant, which accepts and stores unrecognized lines.*/ @@ -796,24 +823,25 @@ static void test_confparse_extra_lines(void *arg) { (void)arg; - test_struct_t *tst = config_new(&etest_fmt); + config_mgr_t *mgr = config_mgr_new(&etest_fmt); + test_struct_t *tst = config_new(mgr); config_line_t *lines = NULL; char *msg = NULL, *dump = NULL; - config_init(&etest_fmt, tst); + config_init(mgr, tst); int r = config_get_lines( "unknotty addita\n" "pos 99\n" "wombat knish\n", &lines, 0); tt_int_op(r, OP_EQ, 0); - r = config_assign(&etest_fmt, tst, lines, 0, &msg); + r = config_assign(mgr, tst, lines, 0, &msg); tt_int_op(r, OP_EQ, 0); tt_ptr_op(msg, OP_EQ, NULL); tt_assert(tst->extra_lines); - dump = config_dump(&etest_fmt, NULL, tst, 1, 0); + dump = config_dump(mgr, NULL, tst, 1, 0); tt_str_op(dump, OP_EQ, "pos 99\n" "unknotty addita\n" @@ -823,7 +851,8 @@ test_confparse_extra_lines(void *arg) tor_free(msg); tor_free(dump); config_free_lines(lines); - config_free(&etest_fmt, tst); + config_free(mgr, tst); + config_mgr_free(mgr); } static void @@ -889,12 +918,14 @@ static void test_confparse_check_ok_fail(void *arg) { (void)arg; - test_struct_t *tst = config_new(&test_fmt); + config_mgr_t *mgr = config_mgr_new(&test_fmt); + test_struct_t *tst = config_new(mgr); tst->pos = -10; - tt_assert(! config_check_ok(&test_fmt, tst, LOG_INFO)); + tt_assert(! config_check_ok(mgr, tst, LOG_INFO)); done: - config_free(&test_fmt, tst); + config_free(mgr, tst); + config_mgr_free(mgr); } #define CONFPARSE_TEST(name, flags) \ diff --git a/src/test/test_helpers.c b/src/test/test_helpers.c index b4389f2d17..31a6540efc 100644 --- a/src/test/test_helpers.c +++ b/src/test/test_helpers.c @@ -295,7 +295,7 @@ helper_parse_options(const char *conf) if (ret != 0) { goto done; } - ret = config_assign(&options_format, opt, line, 0, &msg); + ret = config_assign(get_options_mgr(), opt, line, 0, &msg); if (ret != 0) { goto done; } diff --git a/src/test/test_options.c b/src/test/test_options.c index b39cd4f1e4..b6ab97b4ae 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -96,7 +96,7 @@ clear_log_messages(void) opt->command = CMD_RUN_TOR; \ options_init(opt); \ \ - dflt = config_dup(&options_format, opt); \ + dflt = config_dup(get_options_mgr(), opt); \ clear_log_messages(); \ } while (0) @@ -196,7 +196,7 @@ test_options_validate_impl(const char *configuration, if (r) goto done; - r = config_assign(&options_format, opt, cl, 0, &msg); + r = config_assign(get_options_mgr(), opt, cl, 0, &msg); if (phase == PH_ASSIGN) { if (test_options_checkmsgs(configuration, expect_errmsg, expect_log_severity, @@ -304,7 +304,7 @@ test_have_enough_mem_for_dircache(void *arg) r = config_get_lines(configuration, &cl, 1); tt_int_op(r, OP_EQ, 0); - r = config_assign(&options_format, opt, cl, 0, &msg); + r = config_assign(get_options_mgr(), opt, cl, 0, &msg); tt_int_op(r, OP_EQ, 0); /* 300 MB RAM available, DirCache enabled */ @@ -327,7 +327,7 @@ test_have_enough_mem_for_dircache(void *arg) r = config_get_lines(configuration, &cl, 1); tt_int_op(r, OP_EQ, 0); - r = config_assign(&options_format, opt, cl, 0, &msg); + r = config_assign(get_options_mgr(), opt, cl, 0, &msg); tt_int_op(r, OP_EQ, 0); /* 300 MB RAM available, DirCache enabled, Bridge */ @@ -350,7 +350,7 @@ test_have_enough_mem_for_dircache(void *arg) r = config_get_lines(configuration, &cl, 1); tt_int_op(r, OP_EQ, 0); - r = config_assign(&options_format, opt, cl, 0, &msg); + r = config_assign(get_options_mgr(), opt, cl, 0, &msg); tt_int_op(r, OP_EQ, 0); /* 200 MB RAM available, DirCache disabled */ @@ -438,7 +438,7 @@ get_options_test_data(const char *conf) rv = config_get_lines(conf, &cl, 1); tt_int_op(rv, OP_EQ, 0); - rv = config_assign(&options_format, result->opt, cl, 0, &msg); + rv = config_assign(get_options_mgr(), result->opt, cl, 0, &msg); if (msg) { /* Display the parse error message by comparing it with an empty string */ tt_str_op(msg, OP_EQ, ""); @@ -449,7 +449,7 @@ get_options_test_data(const char *conf) result->opt->TokenBucketRefillInterval = 1; rv = config_get_lines(TEST_OPTIONS_OLD_VALUES, &cl, 1); tt_int_op(rv, OP_EQ, 0); - rv = config_assign(&options_format, result->def_opt, cl, 0, &msg); + rv = config_assign(get_options_mgr(), result->def_opt, cl, 0, &msg); if (msg) { /* Display the parse error message by comparing it with an empty string */ tt_str_op(msg, OP_EQ, ""); From 769fe817174d238c2c4a615872d6df9a0379dfc6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 22 Jul 2019 13:53:36 -0400 Subject: [PATCH 02/28] Start teaching config_mgr_t to handle sub-objects and sub-formats The eventual design here will be that multiple config_format_t objects get registered with a single config_mgr_t. That config_mgr_t manages a "top-level" object, which has a pointer to the other objects. I had earlier thought of a different design, where there would be no top-level object, and config_mgr_t would deal with a container instead. But this would require a bunch of invasive refactoring that I don't think we should do just yet. --- src/app/config/confparse.c | 371 +++++++++++++++++++++++++------------ 1 file changed, 252 insertions(+), 119 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 9c4c6a2778..0964321ad0 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -37,8 +37,38 @@ #include "lib/string/printf.h" #include "lib/string/util_string.h" +/** + * A managed_var_t is an internal wrapper around a config_var_t in + * a config_format_t structure. It is used by config_mgr_t to + * keep track of which option goes with which structure. */ +typedef struct managed_var_t { + /** + * A pointer to the config_var_t for this option. + */ + const config_var_t *cvar; + /** + * The index of the object in which this option is stored. It is + * IDX_TOPLEVEL to indicate that the object is the top-level object. + **/ + int object_idx; +} managed_var_t; + static void config_reset(const config_mgr_t *fmt, void *options, - const config_var_t *var, int use_defaults); + const managed_var_t *var, int use_defaults); +static void config_mgr_register_fmt(config_mgr_t *mgr, + const config_format_t *fmt, + int object_idx); + +/** Release all storage held in a managed_var_t. */ +static void +managed_var_free_(managed_var_t *mv) +{ + if (!mv) + return; + tor_free(mv); +} +#define managed_var_free(mv) \ + FREE_AND_NULL(managed_var_t, managed_var_free_, (mv)) struct config_mgr_t { /** The 'top-level' configuration format. This one is used for legacy @@ -48,8 +78,17 @@ struct config_mgr_t { * contains. A subsequent commit will add more. XXXX) */ const config_format_t *toplevel; + /** A smartlist of managed_var_t objects for all configuration formats. */ + smartlist_t *all_vars; + /** A smartlist of config_abbrev_t objects for all abbreviations. These + * objects are */ + smartlist_t *all_abbrevs; + /** A smartlist of config_deprecation_t for all configuration formats. */ + smartlist_t *all_deprecations; }; +#define IDX_TOPLEVEL (-1) + /** Create a new config_mgr_t to manage a set of configuration objects to be * wrapped under toplevel_fmt. */ config_mgr_t * @@ -57,15 +96,81 @@ config_mgr_new(const config_format_t *toplevel_fmt) { config_mgr_t *mgr = tor_malloc_zero(sizeof(config_mgr_t)); mgr->toplevel = toplevel_fmt; + mgr->all_vars = smartlist_new(); + mgr->all_abbrevs = smartlist_new(); + mgr->all_deprecations = smartlist_new(); + + config_mgr_register_fmt(mgr, toplevel_fmt, IDX_TOPLEVEL); return mgr; } +/** Add a config_format_t to a manager, with a specified (unique) index. */ +static void +config_mgr_register_fmt(config_mgr_t *mgr, + const config_format_t *fmt, + int object_idx) +{ + int i; + + /* register variables */ + for (i = 0; fmt->vars[i].member.name; ++i) { + managed_var_t *mv = tor_malloc_zero(sizeof(managed_var_t)); + mv->cvar = &fmt->vars[i]; + mv->object_idx = object_idx; + smartlist_add(mgr->all_vars, mv); + } + + /* register abbrevs */ + if (fmt->abbrevs) { + for (i = 0; fmt->abbrevs[i].abbreviated; ++i) { + smartlist_add(mgr->all_abbrevs, (void*)&fmt->abbrevs[i]); + } + } + + /* register deprecations. */ + if (fmt->deprecations) { + const config_deprecation_t *d; + for (d = fmt->deprecations; d->name; ++d) { + smartlist_add(mgr->all_deprecations, (void*)d); + } + } +} + +/** + * Return a pointer to the configuration object within toplevel whose + * index is idx. + * + * NOTE: XXXX Eventually, there will be multiple objects supported within the + * toplevel object. For example, the or_options_t will contain pointers + * to configuration objects for other modules. This function gets + * the sub-object for a particular modules. + */ +static void * +config_mgr_get_obj_mutable(const config_mgr_t *mgr, void *toplevel, int idx) +{ + (void)mgr; + tor_assert(idx == IDX_TOPLEVEL); // nothing else is implemented yet XXXX + tor_assert(toplevel); + return toplevel; +} + +/** As config_mgr_get_obj_mutable(), but return a const pointer. */ +static const void * +config_mgr_get_obj(const config_mgr_t *mgr, const void *toplevel, int idx) +{ + return config_mgr_get_obj_mutable(mgr, (void*)toplevel, idx); +} + /** Release all storage held in mgr */ void config_mgr_free_(config_mgr_t *mgr) { if (!mgr) return; + SMARTLIST_FOREACH(mgr->all_vars, managed_var_t *, mv, managed_var_free(mv)); + smartlist_free(mgr->all_vars); + smartlist_free(mgr->all_abbrevs); + smartlist_free(mgr->all_deprecations); memset(mgr, 0, sizeof(*mgr)); tor_free(mgr); } @@ -94,27 +199,23 @@ const char * config_expand_abbrev(const config_mgr_t *mgr, const char *option, int command_line, int warn_obsolete) { - const config_format_t *fmt = mgr->toplevel; - int i; - if (! fmt->abbrevs) - return option; - for (i=0; fmt->abbrevs[i].abbreviated; ++i) { + SMARTLIST_FOREACH_BEGIN(mgr->all_abbrevs, const config_abbrev_t *, abbrev) { /* Abbreviations are case insensitive. */ - if (!strcasecmp(option,fmt->abbrevs[i].abbreviated) && - (command_line || !fmt->abbrevs[i].commandline_only)) { - if (warn_obsolete && fmt->abbrevs[i].warn) { + if (!strcasecmp(option, abbrev->abbreviated) && + (command_line || !abbrev->commandline_only)) { + if (warn_obsolete && abbrev->warn) { log_warn(LD_CONFIG, "The configuration option '%s' is deprecated; " "use '%s' instead.", - fmt->abbrevs[i].abbreviated, - fmt->abbrevs[i].full); + abbrev->abbreviated, + abbrev->full); } /* Keep going through the list in case we want to rewrite it more. * (We could imagine recursing here, but I don't want to get the * user into an infinite loop if we craft our list wrong.) */ - option = fmt->abbrevs[i].full; + option = abbrev->full; } - } + } SMARTLIST_FOREACH_END(abbrev); return option; } @@ -124,18 +225,66 @@ config_expand_abbrev(const config_mgr_t *mgr, const char *option, const char * config_find_deprecation(const config_mgr_t *mgr, const char *key) { - const config_format_t *fmt = mgr->toplevel; - if (BUG(fmt == NULL) || BUG(key == NULL)) + if (BUG(mgr == NULL) || BUG(key == NULL)) return NULL; // LCOV_EXCL_LINE - if (fmt->deprecations == NULL) - return NULL; - const config_deprecation_t *d; - for (d = fmt->deprecations; d->name; ++d) { + SMARTLIST_FOREACH_BEGIN(mgr->all_deprecations, const config_deprecation_t *, + d) { if (!strcasecmp(d->name, key)) { return d->why_deprecated ? d->why_deprecated : ""; } - } + } SMARTLIST_FOREACH_END(d); + return NULL; +} + +/** + * Find the managed_var_t object for a variable whose name is name + * according to mgr. Return that object, or NULL if none exists. + * + * If allow_truncated is true, then accept any variable whose + * name begins with name. + * + * If idx_out is not NULL, set *idx_out to the position of + * that variable within mgr->all_vars, or to -1 if the variable is + * not found. + */ +static const managed_var_t * +config_mgr_find_var(const config_mgr_t *mgr, + const char *key, + bool allow_truncated, int *idx_out) +{ + const size_t keylen = strlen(key); + if (idx_out) + *idx_out = -1; + + if (!keylen) + return NULL; /* if they say "--" on the command line, it's not an option */ + + /* First, check for an exact (case-insensitive) match */ + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { + if (!strcasecmp(mv->cvar->member.name, key)) { + if (idx_out) + *idx_out = mv_sl_idx; + return mv; + } + } SMARTLIST_FOREACH_END(mv); + + if (!allow_truncated) + return NULL; + + /* If none, check for an abbreviated match */ + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { + if (!strncasecmp(key, mv->cvar->member.name, keylen)) { + log_warn(LD_CONFIG, "The abbreviation '%s' is deprecated. " + "Please use '%s' instead", + key, mv->cvar->member.name); + if (idx_out) + *idx_out = mv_sl_idx; + return mv; + } + } SMARTLIST_FOREACH_END(mv); + + /* Okay, unrecognized option */ return NULL; } @@ -147,39 +296,18 @@ config_find_deprecation(const config_mgr_t *mgr, const char *key) const config_var_t * config_find_option(const config_mgr_t *mgr, const char *key) { - const config_format_t *fmt = mgr->toplevel; - int i; - size_t keylen = strlen(key); - if (!keylen) - return NULL; /* if they say "--" on the command line, it's not an option */ - /* First, check for an exact (case-insensitive) match */ - for (i=0; fmt->vars[i].member.name; ++i) { - if (!strcasecmp(key, fmt->vars[i].member.name)) { - return &fmt->vars[i]; - } - } - /* If none, check for an abbreviated match */ - for (i=0; fmt->vars[i].member.name; ++i) { - if (!strncasecmp(key, fmt->vars[i].member.name, keylen)) { - log_warn(LD_CONFIG, "The abbreviation '%s' is deprecated. " - "Please use '%s' instead", - key, fmt->vars[i].member.name); - return &fmt->vars[i]; - } - } - /* Okay, unrecognized option */ - return NULL; + const managed_var_t *mv = config_mgr_find_var(mgr, key, true, NULL); + if (mv) + return mv->cvar; + else + return NULL; } /** Return the number of option entries in fmt. */ static int config_count_options(const config_mgr_t *mgr) { - const config_format_t *fmt = mgr->toplevel; - int i; - for (i=0; fmt->vars[i].member.name; ++i) - ; - return i; + return smartlist_len(mgr->all_vars); } bool @@ -213,16 +341,17 @@ static int config_assign_value(const config_mgr_t *mgr, void *options, config_line_t *c, char **msg) { - const config_var_t *var; + const managed_var_t *var; const config_format_t *fmt = mgr->toplevel; CONFIG_CHECK(fmt, options); - var = config_find_option(mgr, c->key); + var = config_mgr_find_var(mgr, c->key, true, NULL); tor_assert(var); - tor_assert(!strcmp(c->key, var->member.name)); + tor_assert(!strcmp(c->key, var->cvar->member.name)); + void *object = config_mgr_get_obj_mutable(mgr, options, var->object_idx); - return struct_var_kvassign(options, c, msg, &var->member); + return struct_var_kvassign(object, c, msg, &var->cvar->member); } /** Mark every linelist in options "fragile", so that fresh assignments @@ -230,16 +359,13 @@ config_assign_value(const config_mgr_t *mgr, void *options, static void config_mark_lists_fragile(const config_mgr_t *mgr, void *options) { - int i; - const config_format_t *fmt = mgr->toplevel; - - tor_assert(fmt); + tor_assert(mgr); tor_assert(options); - for (i = 0; fmt->vars[i].member.name; ++i) { - const config_var_t *var = &fmt->vars[i]; - struct_var_mark_fragile(options, &var->member); - } + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { + void *object = config_mgr_get_obj_mutable(mgr, options, mv->object_idx); + struct_var_mark_fragile(object, &mv->cvar->member); + } SMARTLIST_FOREACH_END(mv); } void @@ -270,12 +396,13 @@ config_assign_line(const config_mgr_t *mgr, void *options, const unsigned use_defaults = flags & CAL_USE_DEFAULTS; const unsigned clear_first = flags & CAL_CLEAR_FIRST; const unsigned warn_deprecations = flags & CAL_WARN_DEPRECATIONS; - const config_var_t *var; + const managed_var_t *mvar; CONFIG_CHECK(fmt, options); - var = config_find_option(mgr, c->key); - if (!var) { + int var_index = -1; + mvar = config_mgr_find_var(mgr, c->key, true, &var_index); + if (!mvar) { if (fmt->extra) { void *lvalue = STRUCT_VAR_P(options, fmt->extra->offset); log_info(LD_CONFIG, @@ -289,44 +416,47 @@ config_assign_line(const config_mgr_t *mgr, void *options, } } + const config_var_t *cvar = mvar->cvar; + tor_assert(cvar); + /* Put keyword into canonical case. */ - if (strcmp(var->member.name, c->key)) { + if (strcmp(cvar->member.name, c->key)) { tor_free(c->key); - c->key = tor_strdup(var->member.name); + c->key = tor_strdup(cvar->member.name); } const char *deprecation_msg; if (warn_deprecations && - (deprecation_msg = config_find_deprecation(mgr, var->member.name))) { - warn_deprecated_option(var->member.name, deprecation_msg); + (deprecation_msg = config_find_deprecation(mgr, cvar->member.name))) { + warn_deprecated_option(cvar->member.name, deprecation_msg); } if (!strlen(c->value)) { /* reset or clear it, then return */ if (!clear_first) { - if (config_var_is_cumulative(var) && c->command != CONFIG_LINE_CLEAR) { + if (config_var_is_cumulative(cvar) && c->command != CONFIG_LINE_CLEAR) { /* We got an empty linelist from the torrc or command line. As a special case, call this an error. Warn and ignore. */ log_warn(LD_CONFIG, "Linelist option '%s' has no value. Skipping.", c->key); } else { /* not already cleared */ - config_reset(mgr, options, var, use_defaults); + config_reset(mgr, options, mvar, use_defaults); } } return 0; } else if (c->command == CONFIG_LINE_CLEAR && !clear_first) { // XXXX This is unreachable, since a CLEAR line always has an // XXXX empty value. - config_reset(mgr, options, var, use_defaults); // LCOV_EXCL_LINE + config_reset(mgr, options, mvar, use_defaults); // LCOV_EXCL_LINE } - if (options_seen && ! config_var_is_cumulative(var)) { + if (options_seen && ! config_var_is_cumulative(cvar)) { /* We're tracking which options we've seen, and this option is not * supposed to occur more than once. */ - int var_index = (int)(var - fmt->vars); + tor_assert(var_index >= 0); if (bitarray_is_set(options_seen, var_index)) { log_warn(LD_CONFIG, "Option '%s' used more than once; all but the last " - "value will be ignored.", var->member.name); + "value will be ignored.", cvar->member.name); } bitarray_set(options_seen, var_index); } @@ -343,11 +473,11 @@ config_reset_line(const config_mgr_t *mgr, void *options, const char *key, int use_defaults) { const config_format_t *fmt = mgr->toplevel; - const config_var_t *var; + const managed_var_t *var; CONFIG_CHECK(fmt, options); - var = config_find_option(mgr, key); + var = config_mgr_find_var(mgr, key, true, NULL); if (!var) return; /* give error on next pass. */ @@ -388,7 +518,7 @@ config_line_t * config_get_assigned_option(const config_mgr_t *mgr, const void *options, const char *key, int escape_val) { - const config_var_t *var; + const managed_var_t *var; config_line_t *result; const config_format_t *fmt = mgr->toplevel; @@ -396,13 +526,14 @@ config_get_assigned_option(const config_mgr_t *mgr, const void *options, CONFIG_CHECK(fmt, options); - var = config_find_option(mgr, key); + var = config_mgr_find_var(mgr, key, true, NULL); if (!var) { log_warn(LD_CONFIG, "Unknown option '%s'. Failing.", key); return NULL; } + const void *object = config_mgr_get_obj(mgr, options, var->object_idx); - result = struct_var_kvencode(options, &var->member); + result = struct_var_kvencode(object, &var->cvar->member); if (escape_val) { config_line_t *line; @@ -526,10 +657,10 @@ config_assign(const config_mgr_t *mgr, void *options, config_line_t *list, /** Reset config option var to 0, 0.0, NULL, or the equivalent. * Called from config_reset() and config_free(). */ static void -config_clear(void *options, - const config_var_t *var) +config_clear(const config_mgr_t *mgr, void *options, const managed_var_t *var) { - struct_var_free(options, &var->member); + void *object = config_mgr_get_obj_mutable(mgr, options, var->object_idx); + struct_var_free(object, &var->cvar->member); } /** Clear the option indexed by var in options. Then if @@ -537,19 +668,21 @@ config_clear(void *options, * Called by config_init() and option_reset_line() and option_assign_line(). */ static void config_reset(const config_mgr_t *mgr, void *options, - const config_var_t *var, int use_defaults) + const managed_var_t *var, int use_defaults) { const config_format_t *fmt = mgr->toplevel; config_line_t *c; char *msg = NULL; CONFIG_CHECK(fmt, options); - config_clear(options, var); /* clear it first */ + config_clear(mgr, options, var); /* clear it first */ + if (!use_defaults) return; /* all done */ - if (var->initvalue) { + + if (var->cvar->initvalue) { c = tor_malloc_zero(sizeof(config_line_t)); - c->key = tor_strdup(var->member.name); - c->value = tor_strdup(var->initvalue); + c->key = tor_strdup(var->cvar->member.name); + c->value = tor_strdup(var->cvar->initvalue); if (config_assign_value(mgr, options, c, &msg) < 0) { // LCOV_EXCL_START log_warn(LD_BUG, "Failed to assign default: %s", msg); @@ -564,7 +697,6 @@ config_reset(const config_mgr_t *mgr, void *options, void config_free_(const config_mgr_t *mgr, void *options) { - int i; const config_format_t *fmt = mgr->toplevel; if (!options) @@ -572,8 +704,9 @@ config_free_(const config_mgr_t *mgr, void *options) tor_assert(fmt); - for (i=0; fmt->vars[i].member.name; ++i) - config_clear(options, &(fmt->vars[i])); + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { + config_clear(mgr, options, mv); + } SMARTLIST_FOREACH_END(mv); if (fmt->extra) { config_line_t **linep = STRUCT_VAR_P(options, fmt->extra->offset); @@ -595,36 +728,39 @@ config_is_same(const config_mgr_t *mgr, CONFIG_CHECK(fmt, o1); CONFIG_CHECK(fmt, o2); - const config_var_t *var = config_find_option(mgr, name); + const managed_var_t *var = config_mgr_find_var(mgr, name, true, NULL); if (!var) { return true; } + const void *obj1 = config_mgr_get_obj(mgr, o1, var->object_idx); + const void *obj2 = config_mgr_get_obj(mgr, o2, var->object_idx); - return struct_var_eq(o1, o2, &var->member); + return struct_var_eq(obj1, obj2, &var->cvar->member); } /** Copy storage held by old into a new or_options_t and return it. */ void * config_dup(const config_mgr_t *mgr, const void *old) { - const config_format_t *fmt = mgr->toplevel; void *newopts; - int i; newopts = config_new(mgr); - for (i=0; fmt->vars[i].member.name; ++i) { - if (config_var_is_contained(&fmt->vars[i])) { + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, managed_var_t *, mv) { + if (config_var_is_contained(mv->cvar)) { // Something else will copy this option, or it doesn't need copying. continue; } - if (struct_var_copy(newopts, old, &fmt->vars[i].member) < 0) { + const void *oldobj = config_mgr_get_obj(mgr, old, mv->object_idx); + void *newobj = config_mgr_get_obj_mutable(mgr, newopts, mv->object_idx); + if (struct_var_copy(newobj, oldobj, &mv->cvar->member) < 0) { // LCOV_EXCL_START log_err(LD_BUG, "Unable to copy value for %s.", - fmt->vars[i].member.name); + mv->cvar->member.name); tor_assert_unreached(); // LCOV_EXCL_STOP } - } + } SMARTLIST_FOREACH_END(mv); + return newopts; } /** Set all vars in the configuration object options to their default @@ -632,17 +768,14 @@ config_dup(const config_mgr_t *mgr, const void *old) void config_init(const config_mgr_t *mgr, void *options) { - int i; const config_format_t *fmt = mgr->toplevel; - const config_var_t *var; CONFIG_CHECK(fmt, options); - for (i=0; fmt->vars[i].member.name; ++i) { - var = &fmt->vars[i]; - if (!var->initvalue) + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { + if (!mv->cvar->initvalue) continue; /* defaults to NULL or 0 */ - config_reset(mgr, options, var, 1); - } + config_reset(mgr, options, mv, 1); + } SMARTLIST_FOREACH_END(mv); } /** Allocate and return a new string holding the written-out values of the vars @@ -654,13 +787,12 @@ config_dump(const config_mgr_t *mgr, const void *default_options, const void *options, int minimal, int comment_defaults) { - smartlist_t *elements; const config_format_t *fmt = mgr->toplevel; + smartlist_t *elements; const void *defaults = default_options; void *defaults_tmp = NULL; config_line_t *line, *assigned; char *result; - int i; char *msg = NULL; if (defaults == NULL) { @@ -680,24 +812,24 @@ config_dump(const config_mgr_t *mgr, const void *default_options, } elements = smartlist_new(); - for (i=0; fmt->vars[i].member.name; ++i) { + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, managed_var_t *, mv) { int comment_option = 0; - if (config_var_is_contained(&fmt->vars[i])) { + if (config_var_is_contained(mv->cvar)) { // Something else will dump this option, or it doesn't need dumping. continue; } /* Don't save 'hidden' control variables. */ - if (fmt->vars[i].flags & CVFLAG_NODUMP) + if (mv->cvar->flags & CVFLAG_NODUMP) continue; - if (minimal && config_is_same(mgr, options, defaults, - fmt->vars[i].member.name)) + const char *name = mv->cvar->member.name; + if (minimal && config_is_same(mgr, options, defaults, name)) continue; else if (comment_defaults && - config_is_same(mgr, options, defaults, fmt->vars[i].member.name)) + config_is_same(mgr, options, defaults, name)) comment_option = 1; line = assigned = - config_get_assigned_option(mgr, options, fmt->vars[i].member.name, 1); + config_get_assigned_option(mgr, options, name, 1); for (; line; line = line->next) { if (!strcmpstart(line->key, "__")) { @@ -710,7 +842,7 @@ config_dump(const config_mgr_t *mgr, const void *default_options, line->key, line->value); } config_free_lines(assigned); - } + } SMARTLIST_FOREACH_END(mv); if (fmt->extra) { line = *(config_line_t**)STRUCT_VAR_P(options, fmt->extra->offset); @@ -735,14 +867,15 @@ config_dump(const config_mgr_t *mgr, const void *default_options, bool config_check_ok(const config_mgr_t *mgr, const void *options, int severity) { - const config_format_t *fmt = mgr->toplevel; bool all_ok = true; - for (int i=0; fmt->vars[i].member.name; ++i) { - if (!struct_var_ok(options, &fmt->vars[i].member)) { + + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { + if (!struct_var_ok(options, &mv->cvar->member)) { log_fn(severity, LD_BUG, "Invalid value for %s", - fmt->vars[i].member.name); + mv->cvar->member.name); all_ok = false; } - } + } SMARTLIST_FOREACH_END(mv); + return all_ok; } From 7e91d4f57298fc08e4aad8ca90ecfad7cf9de206 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 22 Jul 2019 15:01:57 -0400 Subject: [PATCH 03/28] Replace config_find_option with a variant that exposes less --- src/app/config/config.c | 6 ++---- src/app/config/confparse.c | 14 +++++++------- src/app/config/confparse.h | 6 +++--- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 76feed0005..444ce481c1 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -2588,8 +2588,7 @@ config_parse_commandline(int argc, char **argv, int ignore_errors, int option_is_recognized(const char *key) { - const config_var_t *var = config_find_option(get_options_mgr(), key); - return (var != NULL); + return config_find_option_name(get_options_mgr(), key) != NULL; } /** Return the canonical name of a configuration option, or NULL @@ -2597,8 +2596,7 @@ option_is_recognized(const char *key) const char * option_get_canonical_name(const char *key) { - const config_var_t *var = config_find_option(get_options_mgr(), key); - return var ? var->member.name : NULL; + return config_find_option_name(get_options_mgr(), key); } /** Return a canonical list of the options assigned for key. diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 0964321ad0..16fb6dbc58 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -288,17 +288,17 @@ config_mgr_find_var(const config_mgr_t *mgr, return NULL; } -/** If key is a configuration option, return the corresponding const - * config_var_t. Otherwise, if key is a non-standard abbreviation, - * warn, and return the corresponding const config_var_t. Otherwise return - * NULL. +/** + * If key is a name or an abbreviation configuration option, return + * the corresponding canonical name for it. Warn if the abbreviation is + * non-standard. Return NULL if the option does not exist. */ -const config_var_t * -config_find_option(const config_mgr_t *mgr, const char *key) +const char * +config_find_option_name(const config_mgr_t *mgr, const char *key) { const managed_var_t *mv = config_mgr_find_var(mgr, key, true, NULL); if (mv) - return mv->cvar; + return mv->cvar->member.name; else return NULL; } diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index a6a2450aee..6e1e8cb3e0 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -111,9 +111,9 @@ int config_assign(const config_mgr_t *mgr, void *options, struct config_line_t *list, unsigned flags, char **msg); const char *config_find_deprecation(const config_mgr_t *mgr, - const char *key); -const config_var_t *config_find_option(const config_mgr_t *mgr, - const char *key); + const char *key); +const char *config_find_option_name(const config_mgr_t *mgr, + const char *key); const char *config_expand_abbrev(const config_mgr_t *mgr, const char *option, int command_line, int warn_obsolete); From 89a3051365a9a089397e64a18053d80337034ff8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 22 Jul 2019 15:27:26 -0400 Subject: [PATCH 04/28] Lower responsibility for listing changed options into confparse.c --- src/app/config/config.c | 36 +++++++--------------------------- src/app/config/confparse.c | 40 ++++++++++++++++++++++++++++++++++++++ src/app/config/confparse.h | 2 ++ 3 files changed, 49 insertions(+), 29 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 444ce481c1..1fe4bd9c5d 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -964,9 +964,6 @@ get_options_defaults(void) int set_options(or_options_t *new_val, char **msg) { - int i; - smartlist_t *elements; - config_line_t *line; or_options_t *old_options = global_options; global_options = new_val; /* Note that we pass the *old* options below, for comparison. It @@ -988,35 +985,16 @@ set_options(or_options_t *new_val, char **msg) /* Issues a CONF_CHANGED event to notify controller of the change. If Tor is * just starting up then the old_options will be undefined. */ if (old_options && old_options != global_options) { - elements = smartlist_new(); - for (i=0; options_format.vars[i].member.name; ++i) { - const config_var_t *var = &options_format.vars[i]; // XXXX 29211 - const char *var_name = var->member.name; - if (config_var_is_contained(var)) { - /* something else will check this var, or it doesn't need checking */ - continue; - } - if (!config_is_same(get_options_mgr(), new_val, old_options, var_name)) { - line = config_get_assigned_option(get_options_mgr(), new_val, - var_name, 1); - - if (line) { - config_line_t *next; - for (; line; line = next) { - next = line->next; - smartlist_add(elements, line->key); - smartlist_add(elements, line->value); - tor_free(line); - } - } else { - smartlist_add_strdup(elements, options_format.vars[i].member.name); - smartlist_add(elements, NULL); - } - } + smartlist_t *elements = smartlist_new(); + config_line_t *changes = + config_get_changes(get_options_mgr(), old_options, new_val); + for (config_line_t *line = changes; line; line = line->next) { + smartlist_add(elements, line->key); + smartlist_add(elements, line->value); } control_event_conf_changed(elements); - SMARTLIST_FOREACH(elements, char *, cp, tor_free(cp)); smartlist_free(elements); + config_free_lines(changes); } if (old_options != global_options) { diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 16fb6dbc58..32899a5591 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -738,6 +738,46 @@ config_is_same(const config_mgr_t *mgr, return struct_var_eq(obj1, obj2, &var->cvar->member); } +/** + * Return a list of the options which have changed between options1 and + * options2. If an option has reverted to its default value, it has a + * value entry of NULL. + **/ +config_line_t * +config_get_changes(const config_mgr_t *mgr, + const void *options1, const void *options2) +{ + config_line_t *result = NULL; + config_line_t **next = &result; + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, managed_var_t *, mv) { + if (config_var_is_contained(mv->cvar)) { + /* something else will check this var, or it doesn't need checking */ + continue; + } + const void *obj1 = config_mgr_get_obj(mgr, options1, mv->object_idx); + const void *obj2 = config_mgr_get_obj(mgr, options2, mv->object_idx); + + if (struct_var_eq(obj1, obj2, &mv->cvar->member)) { + continue; + } + + const char *varname = mv->cvar->member.name; + config_line_t *line = + config_get_assigned_option(mgr, options2, varname, 1); + + if (line) { + *next = line; + } else { + *next = tor_malloc_zero(sizeof(config_line_t)); + (*next)->key = tor_strdup(varname); + } + while (*next) + next = &(*next)->next; + } SMARTLIST_FOREACH_END(mv); + + return result; +} + /** Copy storage held by old into a new or_options_t and return it. */ void * config_dup(const config_mgr_t *mgr, const void *old) diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 6e1e8cb3e0..e2647637a3 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -100,6 +100,8 @@ struct config_line_t *config_get_assigned_option(const config_mgr_t *mgr, int config_is_same(const config_mgr_t *fmt, const void *o1, const void *o2, const char *name); +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); void *config_dup(const config_mgr_t *mgr, const void *old); char *config_dump(const config_mgr_t *mgr, const void *default_options, From 627ab9dba32d590830ff4da908ee8f98f768b5e1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 22 Jul 2019 16:02:32 -0400 Subject: [PATCH 05/28] Fix every place in config.c that knew about option_vars_. Iterating over this array was once a good idea, but now that we are going to have a separate structure for each submodule's configuration variables, we should indirect through the config_mgr_t object. --- src/app/config/config.c | 37 +++++++++++++++++++------------------ src/app/config/confparse.c | 25 +++++++++++++++++++++++++ src/app/config/confparse.h | 2 ++ 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 1fe4bd9c5d..246f52a64a 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -2655,25 +2655,25 @@ print_usage(void) static void list_torrc_options(void) { - int i; - for (i = 0; option_vars_[i].member.name; ++i) { - const config_var_t *var = &option_vars_[i]; + smartlist_t *vars = config_mgr_list_vars(get_options_mgr()); + SMARTLIST_FOREACH_BEGIN(vars, const config_var_t *, var) { if (! config_var_is_settable(var)) { /* This variable cannot be set, or cannot be set by this name. */ continue; } printf("%s\n", var->member.name); - } + } SMARTLIST_FOREACH_END(var); + smartlist_free(vars); } /** Print all deprecated but non-obsolete torrc options. */ static void list_deprecated_options(void) { - const config_deprecation_t *d; - for (d = option_deprecation_notes_; d->name; ++d) { - printf("%s\n", d->name); - } + smartlist_t *deps = config_mgr_list_deprecated_vars(get_options_mgr()); + SMARTLIST_FOREACH(deps, const char *, name, + printf("%s\n", name)); + smartlist_free(deps); } /** Print all compile-time modules and their enabled/disabled status. */ @@ -8125,34 +8125,34 @@ getinfo_helper_config(control_connection_t *conn, (void) errmsg; if (!strcmp(question, "config/names")) { smartlist_t *sl = smartlist_new(); - int i; - for (i = 0; option_vars_[i].member.name; ++i) { - const config_var_t *var = &option_vars_[i]; + smartlist_t *vars = config_mgr_list_vars(get_options_mgr()); + SMARTLIST_FOREACH_BEGIN(vars, const config_var_t *, var) { /* don't tell controller about triple-underscore options */ - if (option_vars_[i].flags & CVFLAG_INVISIBLE) + if (var->flags & CVFLAG_INVISIBLE) continue; const char *type = struct_var_get_typename(&var->member); if (!type) continue; smartlist_add_asprintf(sl, "%s %s\n",var->member.name,type); - } + } SMARTLIST_FOREACH_END(var); *answer = smartlist_join_strings(sl, "", 0, NULL); SMARTLIST_FOREACH(sl, char *, c, tor_free(c)); smartlist_free(sl); + smartlist_free(vars); } else if (!strcmp(question, "config/defaults")) { smartlist_t *sl = smartlist_new(); int dirauth_lines_seen = 0, fallback_lines_seen = 0; - for (int i = 0; option_vars_[i].member.name; ++i) { - const config_var_t *var = &option_vars_[i]; + smartlist_t *vars = config_mgr_list_vars(get_options_mgr()); + SMARTLIST_FOREACH_BEGIN(vars, const config_var_t *, var) { if (var->initvalue != NULL) { - if (strcmp(option_vars_[i].member.name, "DirAuthority") == 0) { + if (strcmp(var->member.name, "DirAuthority") == 0) { /* * Count dirauth lines we have a default for; we'll use the * count later to decide whether to add the defaults manually */ ++dirauth_lines_seen; } - if (strcmp(option_vars_[i].member.name, "FallbackDir") == 0) { + if (strcmp(var->member.name, "FallbackDir") == 0) { /* * Similarly count fallback lines, so that we can decided later * to add the defaults manually. @@ -8163,7 +8163,8 @@ getinfo_helper_config(control_connection_t *conn, smartlist_add_asprintf(sl, "%s %s\n",var->member.name,val); tor_free(val); } - } + } SMARTLIST_FOREACH_END(var); + smartlist_free(vars); if (dirauth_lines_seen == 0) { /* diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 32899a5591..0f0950dd85 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -175,6 +175,31 @@ config_mgr_free_(config_mgr_t *mgr) tor_free(mgr); } +/** Return a new smartlist_t containing a config_var_t for every variable that + * mgr knows about. The elements of this smartlist do not need + * to be freed. */ +smartlist_t * +config_mgr_list_vars(const config_mgr_t *mgr) +{ + smartlist_t *result = smartlist_new(); + tor_assert(mgr); + SMARTLIST_FOREACH(mgr->all_vars, managed_var_t *, mv, + smartlist_add(result, (void*) mv->cvar)); + return result; +} + +/** Return a new smartlist_t containing the names of all deprecated variables. + * The elements of this smartlist do not need to be freed. */ +smartlist_t * +config_mgr_list_deprecated_vars(const config_mgr_t *mgr) +{ + smartlist_t *result = smartlist_new(); + tor_assert(mgr); + SMARTLIST_FOREACH(mgr->all_deprecations, config_deprecation_t *, d, + smartlist_add(result, &d->name)); + return result; +} + /** Allocate an empty configuration object of a given format type. */ void * config_new(const config_mgr_t *mgr) diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index e2647637a3..1051035032 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -75,6 +75,8 @@ config_mgr_t *config_mgr_new(const config_format_t *toplevel_fmt); void config_mgr_free_(config_mgr_t *mgr); #define config_mgr_free(mgr) \ FREE_AND_NULL(config_mgr_t, config_mgr_free_, (mgr)) +struct smartlist_t *config_mgr_list_vars(const config_mgr_t *mgr); +struct smartlist_t *config_mgr_list_deprecated_vars(const config_mgr_t *mgr); /** Macro: assert that cfg has the right magic field for format * fmt. */ From 7abd43ac5f1e18e6a1c6bb2de95f39f22392f0c8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 22 Jul 2019 16:28:34 -0400 Subject: [PATCH 06/28] Change CONFIG_CHECK() macro to not need a config_format_t We'll want it to check all the subsidiary structures of the options object. --- src/app/config/confparse.c | 46 +++++++++++++++++++++++--------------- src/app/config/confparse.h | 7 ------ 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 0f0950dd85..9d42263077 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -200,6 +200,23 @@ config_mgr_list_deprecated_vars(const config_mgr_t *mgr) return result; } +/** Assert that the magic fields in options and its subsidiary + * objects are all okay. */ +static void +config_mgr_assert_magic_ok(const config_mgr_t *mgr, + const void *options) +{ + tor_assert(mgr); + tor_assert(options); + struct_check_magic(options, &mgr->toplevel->magic); +} + +/** Macro: assert that cfg has the right magic field for + * mgr. */ +#define CONFIG_CHECK(mgr, cfg) STMT_BEGIN \ + config_mgr_assert_magic_ok((mgr), (cfg)); \ + STMT_END + /** Allocate an empty configuration object of a given format type. */ void * config_new(const config_mgr_t *mgr) @@ -207,7 +224,7 @@ config_new(const config_mgr_t *mgr) const config_format_t *fmt = mgr->toplevel; void *opts = tor_malloc_zero(fmt->size); struct_set_magic(opts, &fmt->magic); - CONFIG_CHECK(fmt, opts); + CONFIG_CHECK(mgr, opts); return opts; } @@ -367,9 +384,8 @@ config_assign_value(const config_mgr_t *mgr, void *options, config_line_t *c, char **msg) { const managed_var_t *var; - const config_format_t *fmt = mgr->toplevel; - CONFIG_CHECK(fmt, options); + CONFIG_CHECK(mgr, options); var = config_mgr_find_var(mgr, c->key, true, NULL); tor_assert(var); @@ -417,17 +433,17 @@ config_assign_line(const config_mgr_t *mgr, void *options, config_line_t *c, unsigned flags, bitarray_t *options_seen, char **msg) { - const config_format_t *fmt = mgr->toplevel; const unsigned use_defaults = flags & CAL_USE_DEFAULTS; const unsigned clear_first = flags & CAL_CLEAR_FIRST; const unsigned warn_deprecations = flags & CAL_WARN_DEPRECATIONS; const managed_var_t *mvar; - CONFIG_CHECK(fmt, options); + CONFIG_CHECK(mgr, options); int var_index = -1; mvar = config_mgr_find_var(mgr, c->key, true, &var_index); if (!mvar) { + const config_format_t *fmt = mgr->toplevel; if (fmt->extra) { void *lvalue = STRUCT_VAR_P(options, fmt->extra->offset); log_info(LD_CONFIG, @@ -497,10 +513,9 @@ STATIC void config_reset_line(const config_mgr_t *mgr, void *options, const char *key, int use_defaults) { - const config_format_t *fmt = mgr->toplevel; const managed_var_t *var; - CONFIG_CHECK(fmt, options); + CONFIG_CHECK(mgr, options); var = config_mgr_find_var(mgr, key, true, NULL); if (!var) @@ -545,11 +560,10 @@ config_get_assigned_option(const config_mgr_t *mgr, const void *options, { const managed_var_t *var; config_line_t *result; - const config_format_t *fmt = mgr->toplevel; tor_assert(options && key); - CONFIG_CHECK(fmt, options); + CONFIG_CHECK(mgr, options); var = config_mgr_find_var(mgr, key, true, NULL); if (!var) { @@ -635,12 +649,11 @@ config_assign(const config_mgr_t *mgr, void *options, config_line_t *list, { config_line_t *p; bitarray_t *options_seen; - const config_format_t *fmt = mgr->toplevel; const int n_options = config_count_options(mgr); const unsigned clear_first = config_assign_flags & CAL_CLEAR_FIRST; const unsigned use_defaults = config_assign_flags & CAL_USE_DEFAULTS; - CONFIG_CHECK(fmt, options); + CONFIG_CHECK(mgr, options); /* pass 1: normalize keys */ for (p = list; p; p = p->next) { @@ -695,10 +708,9 @@ static void config_reset(const config_mgr_t *mgr, void *options, const managed_var_t *var, int use_defaults) { - const config_format_t *fmt = mgr->toplevel; config_line_t *c; char *msg = NULL; - CONFIG_CHECK(fmt, options); + CONFIG_CHECK(mgr, options); config_clear(mgr, options, var); /* clear it first */ if (!use_defaults) @@ -749,9 +761,8 @@ config_is_same(const config_mgr_t *mgr, const void *o1, const void *o2, const char *name) { - const config_format_t *fmt = mgr->toplevel; - CONFIG_CHECK(fmt, o1); - CONFIG_CHECK(fmt, o2); + CONFIG_CHECK(mgr, o1); + CONFIG_CHECK(mgr, o2); const managed_var_t *var = config_mgr_find_var(mgr, name, true, NULL); if (!var) { @@ -833,8 +844,7 @@ config_dup(const config_mgr_t *mgr, const void *old) void config_init(const config_mgr_t *mgr, void *options) { - const config_format_t *fmt = mgr->toplevel; - CONFIG_CHECK(fmt, options); + CONFIG_CHECK(mgr, options); SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { if (!mv->cvar->initvalue) diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 1051035032..edc7fa377a 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -78,13 +78,6 @@ void config_mgr_free_(config_mgr_t *mgr); struct smartlist_t *config_mgr_list_vars(const config_mgr_t *mgr); struct smartlist_t *config_mgr_list_deprecated_vars(const config_mgr_t *mgr); -/** Macro: assert that cfg has the right magic field for format - * fmt. */ -#define CONFIG_CHECK(fmt, cfg) STMT_BEGIN \ - tor_assert(fmt); \ - struct_check_magic((cfg), &fmt->magic); \ - STMT_END - #define CAL_USE_DEFAULTS (1u<<0) #define CAL_CLEAR_FIRST (1u<<1) #define CAL_WARN_DEPRECATIONS (1u<<2) From dde091ebc76d8ae5d93c1b3e1febfa2c9d1459fa Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Jul 2019 09:08:31 -0400 Subject: [PATCH 07/28] Add a "freeze" function for config_mgr_t objects. It's important to make sure that we don't change a config_mgr_t after we start using it to make objects, or we could get into inconsistent states. This feature is the start of a safety mechanism to prevent this problem. --- src/app/config/config.c | 1 + src/app/config/confparse.c | 20 ++++++++++++++++++++ src/app/config/confparse.h | 1 + src/app/config/statefile.c | 1 + src/feature/dirauth/shared_random_state.c | 1 + src/test/test_confparse.c | 15 +++++++++++++++ 6 files changed, 39 insertions(+) diff --git a/src/app/config/config.c b/src/app/config/config.c index 246f52a64a..d8b33aee73 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -904,6 +904,7 @@ get_options_mgr(void) { if (PREDICT_UNLIKELY(options_mgr == NULL)) { options_mgr = config_mgr_new(&options_format); + config_mgr_freeze(options_mgr); } return options_mgr; } diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 9d42263077..b086f8c94a 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -85,6 +85,10 @@ struct config_mgr_t { smartlist_t *all_abbrevs; /** A smartlist of config_deprecation_t for all configuration formats. */ smartlist_t *all_deprecations; + /** True if this manager has been frozen and cannot have any more formats + * added to it. A manager must be frozen before it can be used to construct + * or manipulate objects. */ + bool frozen; }; #define IDX_TOPLEVEL (-1) @@ -112,6 +116,10 @@ config_mgr_register_fmt(config_mgr_t *mgr, { int i; + tor_assertf(!mgr->frozen, + "Tried to add a format to a configuration manager after " + "it had been frozen."); + /* register variables */ for (i = 0; fmt->vars[i].member.name; ++i) { managed_var_t *mv = tor_malloc_zero(sizeof(managed_var_t)); @@ -161,6 +169,16 @@ config_mgr_get_obj(const config_mgr_t *mgr, const void *toplevel, int idx) return config_mgr_get_obj_mutable(mgr, (void*)toplevel, idx); } +/** + * Mark a configuration manager as "frozen", so that no more formats can be + * added, and so that it can be used for manipulating configuration objects. + **/ +void +config_mgr_freeze(config_mgr_t *mgr) +{ + mgr->frozen = true; +} + /** Release all storage held in mgr */ void config_mgr_free_(config_mgr_t *mgr) @@ -208,6 +226,7 @@ config_mgr_assert_magic_ok(const config_mgr_t *mgr, { tor_assert(mgr); tor_assert(options); + tor_assert(mgr->frozen); struct_check_magic(options, &mgr->toplevel->magic); } @@ -221,6 +240,7 @@ config_mgr_assert_magic_ok(const config_mgr_t *mgr, void * config_new(const config_mgr_t *mgr) { + tor_assert(mgr->frozen); const config_format_t *fmt = mgr->toplevel; void *opts = tor_malloc_zero(fmt->size); struct_set_magic(opts, &fmt->magic); diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index edc7fa377a..83e886696e 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -73,6 +73,7 @@ typedef struct config_mgr_t config_mgr_t; config_mgr_t *config_mgr_new(const config_format_t *toplevel_fmt); void config_mgr_free_(config_mgr_t *mgr); +void config_mgr_freeze(config_mgr_t *mgr); #define config_mgr_free(mgr) \ FREE_AND_NULL(config_mgr_t, config_mgr_free_, (mgr)) struct smartlist_t *config_mgr_list_vars(const config_mgr_t *mgr); diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index a44bcf6fb5..93e35659df 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -183,6 +183,7 @@ get_state_mgr(void) { if (PREDICT_UNLIKELY(state_mgr == NULL)) { state_mgr = config_mgr_new(&state_format); + config_mgr_freeze(state_mgr); } return state_mgr; } diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index a552e621ca..12ae79ce37 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -112,6 +112,7 @@ get_srs_mgr(void) { if (PREDICT_UNLIKELY(shared_random_state_mgr == NULL)) { shared_random_state_mgr = config_mgr_new(&state_format); + config_mgr_freeze(shared_random_state_mgr); } return shared_random_state_mgr; } diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 8a921531fa..716384e2f6 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -153,6 +153,7 @@ test_confparse_init(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = config_new(mgr); config_init(mgr, tst); @@ -239,6 +240,7 @@ test_confparse_assign_simple(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); tt_str_op(tst->s, OP_EQ, "this is a"); @@ -300,6 +302,7 @@ test_confparse_assign_obsolete(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; @@ -330,6 +333,7 @@ test_confparse_assign_deprecated(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; @@ -364,6 +368,7 @@ test_confparse_assign_replaced(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; @@ -395,6 +400,7 @@ test_confparse_assign_emptystring(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; @@ -423,6 +429,7 @@ test_confparse_assign_twice(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; @@ -458,6 +465,7 @@ test_confparse_assign_badval(void *arg) { const badval_test_t *bt = arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; @@ -512,6 +520,7 @@ test_confparse_dump(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); char *dumped = NULL; @@ -603,6 +612,7 @@ test_confparse_reset(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_reset_line(mgr, tst, "interval", 0); @@ -623,6 +633,7 @@ test_confparse_reassign(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL, *rs = NULL; @@ -679,6 +690,7 @@ test_confparse_reassign_extend(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; char *msg = NULL; @@ -738,6 +750,7 @@ test_confparse_get_assigned(void *arg) (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = get_simple_config(mgr); config_line_t *lines = NULL; @@ -824,6 +837,7 @@ test_confparse_extra_lines(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&etest_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = config_new(mgr); config_line_t *lines = NULL; char *msg = NULL, *dump = NULL; @@ -919,6 +933,7 @@ test_confparse_check_ok_fail(void *arg) { (void)arg; config_mgr_t *mgr = config_mgr_new(&test_fmt); + config_mgr_freeze(mgr); test_struct_t *tst = config_new(mgr); tst->pos = -10; tt_assert(! config_check_ok(mgr, tst, LOG_INFO)); From a1b2817abefc5c696e469fa4fd8a09cc5f286aa6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Jul 2019 09:17:51 -0400 Subject: [PATCH 08/28] Sort variables in config_mgr_t.all_vars alphabetically --- src/app/config/confparse.c | 11 ++++++ src/test/test_confparse.c | 77 +++++++++++++++++++------------------- 2 files changed, 49 insertions(+), 39 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index b086f8c94a..c7ee8f3010 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -169,6 +169,16 @@ config_mgr_get_obj(const config_mgr_t *mgr, const void *toplevel, int idx) return config_mgr_get_obj_mutable(mgr, (void*)toplevel, idx); } +/** Sorting helper for smartlist of managed_var_t */ +static int +managed_var_cmp(const void **a, const void **b) +{ + const managed_var_t *mv1 = *(const managed_var_t**)a; + const managed_var_t *mv2 = *(const managed_var_t**)b; + + return strcasecmp(mv1->cvar->member.name, mv2->cvar->member.name); +} + /** * Mark a configuration manager as "frozen", so that no more formats can be * added, and so that it can be used for manipulating configuration objects. @@ -176,6 +186,7 @@ config_mgr_get_obj(const config_mgr_t *mgr, const void *toplevel, int idx) void config_mgr_freeze(config_mgr_t *mgr) { + smartlist_sort(mgr->all_vars, managed_var_cmp); mgr->frozen = true; } diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 716384e2f6..1513c9323b 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -527,78 +527,77 @@ test_confparse_dump(void *arg) /* Minimal version. */ dumped = config_dump(mgr, NULL, tst, 1, 0); tt_str_op(dumped, OP_EQ, - "s this is a\n" - "fn /simple/test of the\n" - "pos 77\n" - "i 3\n" - "u64 1000000000000\n" - "interval 300\n" - "msec_interval 300000\n" - "mem 10\n" - "dbl 6.060842\n" - "boolean 1\n" "autobool 0\n" - "time 2019-06-14 13:58:51\n" + "boolean 1\n" "csv configuration,parsing,system\n" "csv_interval 10\n" + "dbl 6.060842\n" + "fn /simple/test of the\n" + "i 3\n" + "interval 300\n" "lines hello\n" "lines world\n" + "mem 10\n" + "VisibleLineB ABC\n" "LineTypeA i d\n" "LineTypeB i c\n" + "msec_interval 300000\n" + "pos 77\n" "routerset $FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF\n" - "VisibleLineB ABC\n"); + "s this is a\n" + "time 2019-06-14 13:58:51\n" + "u64 1000000000000\n"); - /* Maximal */ tor_free(dumped); dumped = config_dump(mgr, NULL, tst, 0, 0); tt_str_op(dumped, OP_EQ, - "s this is a\n" - "fn /simple/test of the\n" - "pos 77\n" - "i 3\n" - "deprecated_int 3\n" - "u64 1000000000000\n" - "interval 300\n" - "msec_interval 300000\n" - "mem 10\n" - "dbl 6.060842\n" - "boolean 1\n" "autobool 0\n" - "time 2019-06-14 13:58:51\n" + "boolean 1\n" "csv configuration,parsing,system\n" "csv_interval 10\n" + "dbl 6.060842\n" + "deprecated_int 3\n" + "fn /simple/test of the\n" + "i 3\n" + "interval 300\n" "lines hello\n" "lines world\n" + "mem 10\n" + "VisibleLineB ABC\n" "LineTypeA i d\n" "LineTypeB i c\n" + "msec_interval 300000\n" + "pos 77\n" "routerset $FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF\n" - "VisibleLineB ABC\n"); + "s this is a\n" + "time 2019-06-14 13:58:51\n" + "u64 1000000000000\n"); /* commented */ tor_free(dumped); dumped = config_dump(mgr, NULL, tst, 0, 1); tt_str_op(dumped, OP_EQ, - "s this is a\n" - "fn /simple/test of the\n" - "pos 77\n" - "i 3\n" - "# deprecated_int 3\n" - "u64 1000000000000\n" - "interval 300\n" - "msec_interval 300000\n" - "mem 10\n" - "dbl 6.060842\n" - "boolean 1\n" "autobool 0\n" - "time 2019-06-14 13:58:51\n" + "boolean 1\n" "csv configuration,parsing,system\n" "csv_interval 10\n" + "dbl 6.060842\n" + "# deprecated_int 3\n" + "fn /simple/test of the\n" + "i 3\n" + "interval 300\n" "lines hello\n" "lines world\n" + "mem 10\n" + "VisibleLineB ABC\n" "LineTypeA i d\n" "LineTypeB i c\n" + "msec_interval 300000\n" + "pos 77\n" "routerset $FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF\n" - "VisibleLineB ABC\n"); + "s this is a\n" + "time 2019-06-14 13:58:51\n" + "u64 1000000000000\n"); done: config_free(mgr, tst); From 57e87cc86cce0b9fb351b1862f99c84bfa8100eb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Jul 2019 10:13:29 -0400 Subject: [PATCH 09/28] Use config_new() to construct configuration objects. We'll need to do it this way once the objects become more complex. --- src/app/config/config.c | 3 +-- src/app/config/statefile.c | 3 +-- src/feature/dirauth/shared_random_state.c | 10 +++++----- src/test/fuzz/fuzzing_common.c | 2 +- src/test/test_config.c | 8 ++++---- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index d8b33aee73..7aa253a6bf 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -5404,8 +5404,7 @@ options_init_from_string(const char *cf_defaults, const char *cf, oldoptions = global_options; /* get_options unfortunately asserts if this is the first time we run*/ - newoptions = tor_malloc_zero(sizeof(or_options_t)); - newoptions->magic_ = OR_OPTIONS_MAGIC; + newoptions = options_new(); options_init(newoptions); newoptions->command = command; newoptions->command_arg = command_arg ? tor_strdup(command_arg) : NULL; diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index 93e35659df..54ee398a7f 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -376,8 +376,7 @@ or_state_save_broken(char *fname) STATIC or_state_t * or_state_new(void) { - or_state_t *new_state = tor_malloc_zero(sizeof(or_state_t)); - new_state->magic_ = OR_STATE_MAGIC; + or_state_t *new_state = config_new(get_state_mgr()); config_init(get_state_mgr(), new_state); return new_state; diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index 12ae79ce37..21bf3c1226 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -285,9 +285,8 @@ disk_state_free_(sr_disk_state_t *state) static sr_disk_state_t * disk_state_new(time_t now) { - sr_disk_state_t *new_state = tor_malloc_zero(sizeof(*new_state)); + sr_disk_state_t *new_state = config_new(get_srs_mgr()); - new_state->magic_ = SR_DISK_STATE_MAGIC; new_state->Version = SR_PROTO_VERSION; new_state->TorVersion = tor_strdup(get_version()); new_state->ValidUntil = get_state_valid_until_time(now); @@ -599,11 +598,12 @@ disk_state_reset(void) config_free_lines(sr_disk_state->ExtraLines); tor_free(sr_disk_state->TorVersion); - /* Clean up the struct */ - memset(sr_disk_state, 0, sizeof(*sr_disk_state)); + /* Clear other fields. */ + sr_disk_state->ValidAfter = 0; + sr_disk_state->ValidUntil = 0; + sr_disk_state->Version = 0; /* Reset it with useful data */ - sr_disk_state->magic_ = SR_DISK_STATE_MAGIC; sr_disk_state->TorVersion = tor_strdup(get_version()); } diff --git a/src/test/fuzz/fuzzing_common.c b/src/test/fuzz/fuzzing_common.c index 6d0f9d7d60..1285d94aee 100644 --- a/src/test/fuzz/fuzzing_common.c +++ b/src/test/fuzz/fuzzing_common.c @@ -111,7 +111,7 @@ global_init(void) } /* set up the options. */ - mock_options = tor_malloc_zero(sizeof(or_options_t)); + mock_options = options_new(); MOCK(get_options, mock_get_options); /* Make BUG() and nonfatal asserts crash */ diff --git a/src/test/test_config.c b/src/test/test_config.c index a415ca4480..cfca3149f6 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -1762,7 +1762,7 @@ test_config_adding_dir_servers(void *arg) (void)arg; /* allocate options */ - or_options_t *options = tor_malloc_zero(sizeof(or_options_t)); + or_options_t *options = options_new(); /* Allocate and populate configuration lines: * @@ -3533,7 +3533,7 @@ test_config_default_dir_servers(void *arg) int fallback_count = 0; /* new set of options should stop fallback parsing */ - opts = tor_malloc_zero(sizeof(or_options_t)); + opts = options_new(); opts->UseDefaultFallbackDirs = 0; /* set old_options to NULL to force dir update */ consider_adding_dir_servers(opts, NULL); @@ -3547,7 +3547,7 @@ test_config_default_dir_servers(void *arg) /* if we disable the default fallbacks, there must not be any extra */ tt_assert(fallback_count == trusted_count); - opts = tor_malloc_zero(sizeof(or_options_t)); + opts = options_new(); opts->UseDefaultFallbackDirs = 1; consider_adding_dir_servers(opts, opts); trusted_count = smartlist_len(router_get_trusted_dir_servers()); @@ -3607,7 +3607,7 @@ test_config_directory_fetch(void *arg) (void)arg; /* Test Setup */ - or_options_t *options = tor_malloc_zero(sizeof(or_options_t)); + or_options_t *options = options_new(); routerinfo_t routerinfo; memset(&routerinfo, 0, sizeof(routerinfo)); mock_router_pick_published_address_result = -1; From 3d1f9f583a45143c1259b8926f8c57fa57fc1efb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Jul 2019 10:41:57 -0400 Subject: [PATCH 10/28] Use special magic to enforce manager/object connection. Every time we finalize a config manager, we now generate a new magic number for it, so that we'll get an assertion failure if we ever try to use an object with a different configuration manager than the one that generated it. --- src/app/config/confparse.c | 21 +++++++++++++++++++-- src/test/test_confparse.c | 1 - 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index c7ee8f3010..998c0e6ef2 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -37,6 +37,8 @@ #include "lib/string/printf.h" #include "lib/string/util_string.h" +#include "ext/siphash.h" + /** * A managed_var_t is an internal wrapper around a config_var_t in * a config_format_t structure. It is used by config_mgr_t to @@ -89,6 +91,12 @@ struct config_mgr_t { * added to it. A manager must be frozen before it can be used to construct * or manipulate objects. */ bool frozen; + /** A replacement for the magic number of the toplevel object. We override + * that number to make it unique for this particular config_mgr_t, so that + * an object constructed with one mgr can't be used with another, even if + * those managers' contents are equal. + */ + struct_magic_decl_t toplevel_magic; }; #define IDX_TOPLEVEL (-1) @@ -186,7 +194,16 @@ managed_var_cmp(const void **a, const void **b) void config_mgr_freeze(config_mgr_t *mgr) { + static uint64_t mgr_count = 0; + smartlist_sort(mgr->all_vars, managed_var_cmp); + memcpy(&mgr->toplevel_magic, &mgr->toplevel->magic, + sizeof(struct_magic_decl_t)); + uint64_t magic_input[3] = { mgr->toplevel_magic.magic_val, + (uint64_t) (uintptr_t) mgr, + ++mgr_count }; + mgr->toplevel_magic.magic_val = + (uint32_t)siphash24g(magic_input, sizeof(magic_input)); mgr->frozen = true; } @@ -238,7 +255,7 @@ config_mgr_assert_magic_ok(const config_mgr_t *mgr, tor_assert(mgr); tor_assert(options); tor_assert(mgr->frozen); - struct_check_magic(options, &mgr->toplevel->magic); + struct_check_magic(options, &mgr->toplevel_magic); } /** Macro: assert that cfg has the right magic field for @@ -254,7 +271,7 @@ config_new(const config_mgr_t *mgr) tor_assert(mgr->frozen); const config_format_t *fmt = mgr->toplevel; void *opts = tor_malloc_zero(fmt->size); - struct_set_magic(opts, &fmt->magic); + struct_set_magic(opts, &mgr->toplevel_magic); CONFIG_CHECK(mgr, opts); return opts; } diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 1513c9323b..78eb3d38bf 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -158,7 +158,6 @@ test_confparse_init(void *arg) config_init(mgr, tst); // Make sure that options are initialized right. */ - tt_uint_op(tst->magic, OP_EQ, TEST_MAGIC); tt_str_op(tst->s, OP_EQ, "hello"); tt_ptr_op(tst->fn, OP_EQ, NULL); tt_int_op(tst->pos, OP_EQ, 0); From 47654d32497acae23aafbe3316ae73d3c00429c8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Jul 2019 12:04:59 -0400 Subject: [PATCH 11/28] Refactor config free logic to use a single path. The right way to free a config object is now to wrap config_free(), always. Instead of creating an alternative free function, objects should provide an alternative clear callback to free any fields that the configuration manager doesn't manage. This lets us simplify our code a little, and lets us extend the confparse.c code to manage additional fields in config_free. --- src/app/config/config.c | 29 ++++---- src/app/config/confparse.c | 8 ++- src/app/config/confparse.h | 6 +- src/app/config/statefile.c | 11 +-- src/feature/dirauth/shared_random_state.c | 10 +-- src/test/fuzz/fuzzing_common.c | 3 +- src/test/test_config.c | 84 ++++++++++++++++------- src/test/test_confparse.c | 15 +--- src/test/test_dir_handle_get.c | 3 +- src/test/test_entrynodes.c | 30 ++++---- src/test/test_hs_service.c | 10 +-- src/test/test_pt.c | 2 +- 12 files changed, 112 insertions(+), 99 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 7aa253a6bf..cdf5fa266c 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -843,9 +843,9 @@ static void config_maybe_load_geoip_files_(const or_options_t *options, static int options_validate_cb(void *old_options, void *options, void *default_options, int from_setconf, char **msg); -static void options_free_cb(const config_mgr_t *, 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); /** Magic value for or_options_t. */ #define OR_OPTIONS_MAGIC 9090909 @@ -862,8 +862,8 @@ static const config_format_t options_format = { option_deprecation_notes_, option_vars_, options_validate_cb, - options_free_cb, - NULL + options_clear_cb, + NULL, }; /* @@ -1011,11 +1011,11 @@ set_options(or_options_t *new_val, char **msg) /** Release additional memory allocated in options */ -STATIC void -or_options_free_(or_options_t *options) +static void +options_clear_cb(const config_mgr_t *mgr, void *opts) { - if (!options) - return; + (void)mgr; + or_options_t *options = opts; routerset_free(options->ExcludeExitNodesUnion_); if (options->NodeFamilySets) { @@ -1038,6 +1038,13 @@ or_options_free_(or_options_t *options) tor_free(options->command_arg); tor_free(options->master_key_fname); config_free_lines(options->MyFamily); +} + +/** Release all memory allocated in options + */ +STATIC void +or_options_free_(or_options_t *options) +{ config_free(get_options_mgr(), options); } @@ -3164,14 +3171,6 @@ options_validate_cb(void *old_options, void *options, void *default_options, return rv; } -/** Callback to free an or_options_t */ -static void -options_free_cb(const config_mgr_t *mgr, void *options) -{ - (void)mgr; - or_options_free_(options); -} - #define REJECT(arg) \ STMT_BEGIN *msg = tor_strdup(arg); return -1; STMT_END #if defined(__GNUC__) && __GNUC__ <= 3 diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 998c0e6ef2..8e3ad7fafd 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -789,6 +789,10 @@ config_free_(const config_mgr_t *mgr, void *options) tor_assert(fmt); + if (mgr->toplevel->clear_fn) { + mgr->toplevel->clear_fn(mgr, options); + } + SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { config_clear(mgr, options, mv); } SMARTLIST_FOREACH_END(mv); @@ -977,9 +981,7 @@ config_dump(const config_mgr_t *mgr, const void *default_options, result = smartlist_join_strings(elements, "", 0, NULL); SMARTLIST_FOREACH(elements, char *, cp, tor_free(cp)); smartlist_free(elements); - if (defaults_tmp) { - fmt->free_fn(mgr, defaults_tmp); - } + config_free(mgr, defaults_tmp); return result; } diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 83e886696e..2e1a4b4f57 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -41,8 +41,8 @@ typedef int (*validate_fn_t)(void*,void*,void*,int,char**); struct config_mgr_t; -/** Callback to free a configuration object. */ -typedef void (*free_cfg_fn_t)(const struct config_mgr_t *mgr, void*); +/** Callback to clear all non-managed fields of a configuration object. */ +typedef void (*clear_cfg_fn_t)(const struct config_mgr_t *mgr, void*); /** Information on the keys, value types, key-to-struct-member mappings, * variable descriptions, validation functions, and abbreviations for a @@ -57,7 +57,7 @@ typedef struct config_format_t { * values, and where we stick them in the * structure. */ validate_fn_t validate_fn; /**< Function to validate config. */ - free_cfg_fn_t free_fn; /**< Function to free the configuration. */ + 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. */ const struct_member_t *extra; diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index 54ee398a7f..ede35e6ced 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -145,8 +145,6 @@ static int or_state_validate_cb(void *old_options, void *options, void *default_options, int from_setconf, char **msg); -static void or_state_free_cb(const config_mgr_t *mgr, void *state); - /** Magic value for or_state_t. */ #define OR_STATE_MAGIC 0x57A73f57 @@ -170,7 +168,7 @@ static const config_format_t state_format = { NULL, state_vars_, or_state_validate_cb, - or_state_free_cb, + NULL, &state_extra_var, }; @@ -282,13 +280,6 @@ or_state_validate_cb(void *old_state, void *state, void *default_state, return or_state_validate(state, msg); } -static void -or_state_free_cb(const config_mgr_t *mgr, void *state) -{ - (void)mgr; - or_state_free_(state); -} - /** 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 diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index 21bf3c1226..fac1efc54d 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -62,7 +62,6 @@ DUMMY_TYPECHECK_INSTANCE(sr_disk_state_t); static int disk_state_validate_cb(void *old_state, void *state, void *default_state, int from_setconf, char **msg); -static void disk_state_free_cb(const config_mgr_t *mgr, void *); /* Array of variables that are saved to disk as a persistent state. */ static const config_var_t state_vars[] = { @@ -99,7 +98,7 @@ static const config_format_t state_format = { NULL, state_vars, disk_state_validate_cb, - disk_state_free_cb, + NULL, &state_extra_var, }; @@ -361,13 +360,6 @@ disk_state_validate_cb(void *old_state, void *state, void *default_state, return 0; } -static void -disk_state_free_cb(const config_mgr_t *mgr, void *state) -{ - (void)mgr; - disk_state_free_(state); -} - /* 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/test/fuzz/fuzzing_common.c b/src/test/fuzz/fuzzing_common.c index 1285d94aee..862acb2b35 100644 --- a/src/test/fuzz/fuzzing_common.c +++ b/src/test/fuzz/fuzzing_common.c @@ -1,6 +1,7 @@ /* Copyright (c) 2016-2019, The Tor Project, Inc. */ /* See LICENSE for licensing information */ #define CRYPTO_ED25519_PRIVATE +#define CONFIG_PRIVATE #include "orconfig.h" #include "core/or/or.h" #include "app/main/subsysmgr.h" @@ -189,7 +190,7 @@ main(int argc, char **argv) if (fuzz_cleanup() < 0) abort(); - tor_free(mock_options); + or_options_free(mock_options); UNMOCK(get_options); return 0; } diff --git a/src/test/test_config.c b/src/test/test_config.c index cfca3149f6..58e05e5094 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -1755,6 +1755,18 @@ add_default_fallback_dir_servers_known_default(void) n_add_default_fallback_dir_servers_known_default++; } +/* Helper for test_config_adding_dir_servers(), which should be + * refactored: clear the fields in the options which the options object + * does not really own. */ +static void +ads_clear_helper(or_options_t *options) +{ + options->DirAuthorities = NULL; + options->AlternateBridgeAuthority = NULL; + options->AlternateDirAuthority = NULL; + options->FallbackDir = NULL; +} + /* Test all the different combinations of adding dir servers */ static void test_config_adding_dir_servers(void *arg) @@ -1885,7 +1897,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -1967,7 +1981,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -2108,7 +2124,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -2249,7 +2267,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -2391,7 +2411,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -2543,7 +2565,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -2697,7 +2721,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -2860,7 +2886,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -3017,7 +3045,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -3183,7 +3213,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -3346,7 +3378,9 @@ test_config_adding_dir_servers(void *arg) n_add_default_fallback_dir_servers_known_default = 0; /* clear options*/ - memset(options, 0, sizeof(or_options_t)); + ads_clear_helper(options); + or_options_free(options); + options = options_new(); /* clear any previous dir servers: consider_adding_dir_servers() should do this anyway */ @@ -3515,10 +3549,7 @@ test_config_adding_dir_servers(void *arg) tor_free(test_fallback_directory->value); tor_free(test_fallback_directory); - options->DirAuthorities = NULL; - options->AlternateBridgeAuthority = NULL; - options->AlternateDirAuthority = NULL; - options->FallbackDir = NULL; + ads_clear_helper(options); or_options_free(options); UNMOCK(add_default_fallback_dir_servers); @@ -3619,9 +3650,10 @@ test_config_directory_fetch(void *arg) mock_router_my_exit_policy_is_reject_star); MOCK(advertised_server_mode, mock_advertised_server_mode); MOCK(router_get_my_routerinfo, mock_router_get_my_routerinfo); + or_options_free(options); + options = options_new(); /* Clients can use multiple directory mirrors for bootstrap */ - memset(options, 0, sizeof(or_options_t)); options->ClientOnly = 1; tt_assert(server_mode(options) == 0); tt_assert(public_server_mode(options) == 0); @@ -3630,7 +3662,8 @@ test_config_directory_fetch(void *arg) OP_EQ, 1); /* Bridge Clients can use multiple directory mirrors for bootstrap */ - memset(options, 0, sizeof(or_options_t)); + or_options_free(options); + options = options_new(); options->UseBridges = 1; tt_assert(server_mode(options) == 0); tt_assert(public_server_mode(options) == 0); @@ -3640,7 +3673,8 @@ test_config_directory_fetch(void *arg) /* Bridge Relays (Bridges) must act like clients, and use multiple * directory mirrors for bootstrap */ - memset(options, 0, sizeof(or_options_t)); + or_options_free(options); + options = options_new(); options->BridgeRelay = 1; options->ORPort_set = 1; tt_assert(server_mode(options) == 1); @@ -3651,7 +3685,8 @@ test_config_directory_fetch(void *arg) /* Clients set to FetchDirInfoEarly must fetch it from the authorities, * but can use multiple authorities for bootstrap */ - memset(options, 0, sizeof(or_options_t)); + or_options_free(options); + options = options_new(); options->FetchDirInfoEarly = 1; tt_assert(server_mode(options) == 0); tt_assert(public_server_mode(options) == 0); @@ -3662,7 +3697,8 @@ test_config_directory_fetch(void *arg) /* OR servers only fetch the consensus from the authorities when they don't * know their own address, but never use multiple directories for bootstrap */ - memset(options, 0, sizeof(or_options_t)); + or_options_free(options); + options = options_new(); options->ORPort_set = 1; mock_router_pick_published_address_result = -1; @@ -3682,7 +3718,8 @@ test_config_directory_fetch(void *arg) /* Exit OR servers only fetch the consensus from the authorities when they * refuse unknown exits, but never use multiple directories for bootstrap */ - memset(options, 0, sizeof(or_options_t)); + or_options_free(options); + options = options_new(); options->ORPort_set = 1; options->ExitRelay = 1; mock_router_pick_published_address_result = 0; @@ -3712,7 +3749,8 @@ test_config_directory_fetch(void *arg) * advertising their dirport, and never use multiple directories for * bootstrap. This only applies if they are also OR servers. * (We don't care much about the behaviour of non-OR directory servers.) */ - memset(options, 0, sizeof(or_options_t)); + or_options_free(options); + options = options_new(); options->DirPort_set = 1; options->ORPort_set = 1; options->DirCache = 1; @@ -3766,7 +3804,7 @@ test_config_directory_fetch(void *arg) OP_EQ, 0); done: - tor_free(options); + or_options_free(options); UNMOCK(router_pick_published_address); UNMOCK(router_get_my_routerinfo); UNMOCK(advertised_server_mode); diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 78eb3d38bf..273232fe5d 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -119,8 +119,6 @@ test_validate_cb(void *old_options, void *options, void *default_options, return 0; } -static void test_free_cb(const config_mgr_t *mgr, void *options); - #define TEST_MAGIC 0x1337 static const config_format_t test_fmt = { @@ -134,19 +132,10 @@ static const config_format_t test_fmt = { test_deprecation_notes, test_vars, test_validate_cb, - test_free_cb, + NULL, NULL, }; -static void -test_free_cb(const config_mgr_t *mgr, void *options) -{ - if (!options) - return; - - config_free(mgr, options); -} - /* Make sure that config_init sets everything to the right defaults. */ static void test_confparse_init(void *arg) @@ -824,7 +813,7 @@ static config_format_t etest_fmt = { test_deprecation_notes, test_vars, test_validate_cb, - test_free_cb, + NULL, &extra, }; diff --git a/src/test/test_dir_handle_get.c b/src/test/test_dir_handle_get.c index e57bd02584..edfd0c74e1 100644 --- a/src/test/test_dir_handle_get.c +++ b/src/test/test_dir_handle_get.c @@ -479,8 +479,7 @@ static or_options_t *mock_options = NULL; static void init_mock_options(void) { - mock_options = tor_malloc(sizeof(or_options_t)); - memset(mock_options, 0, sizeof(or_options_t)); + mock_options = options_new(); mock_options->TestingTorNetwork = 1; mock_options->DataDirectory = tor_strdup(get_fname_rnd("datadir_tmp")); mock_options->CacheDirectory = tor_strdup(mock_options->DataDirectory); diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index c43b21c673..c8dd4b03ea 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -5,6 +5,7 @@ #define CIRCUITLIST_PRIVATE #define CIRCUITBUILD_PRIVATE +#define CONFIG_PRIVATE #define STATEFILE_PRIVATE #define ENTRYNODES_PRIVATE #define ROUTERLIST_PRIVATE @@ -201,7 +202,7 @@ big_fake_network_setup(const struct testcase_t *testcase) smartlist_add(big_fake_net_nodes, n); } - dummy_state = tor_malloc_zero(sizeof(or_state_t)); + dummy_state = or_state_new(); dummy_consensus = tor_malloc_zero(sizeof(networkstatus_t)); if (reasonably_future_consensus) { /* Make the dummy consensus valid in 6 hours, and expiring in 7 hours. */ @@ -235,12 +236,12 @@ mock_randomize_time_no_randomization(time_t a, time_t b) return a; } -static or_options_t mocked_options; +static or_options_t *mocked_options; static const or_options_t * mock_get_options(void) { - return &mocked_options; + return mocked_options; } #define TEST_IPV4_ADDR "123.45.67.89" @@ -259,7 +260,7 @@ test_node_preferred_orport(void *arg) tor_addr_port_t ap; /* Setup options */ - memset(&mocked_options, 0, sizeof(mocked_options)); + mocked_options = options_new(); /* We don't test ClientPreferIPv6ORPort here, because it's used in * nodelist_set_consensus to setup node.ipv6_preferred, which we set * directly. */ @@ -282,8 +283,8 @@ test_node_preferred_orport(void *arg) /* Check the preferred address is IPv4 if we're only using IPv4, regardless * of whether we prefer it or not */ - mocked_options.ClientUseIPv4 = 1; - mocked_options.ClientUseIPv6 = 0; + mocked_options->ClientUseIPv4 = 1; + mocked_options->ClientUseIPv6 = 0; node.ipv6_preferred = 0; node_get_pref_orport(&node, &ap); tt_assert(tor_addr_eq(&ap.addr, &ipv4_addr)); @@ -296,8 +297,8 @@ test_node_preferred_orport(void *arg) /* Check the preferred address is IPv4 if we're using IPv4 and IPv6, but * don't prefer the IPv6 address */ - mocked_options.ClientUseIPv4 = 1; - mocked_options.ClientUseIPv6 = 1; + mocked_options->ClientUseIPv4 = 1; + mocked_options->ClientUseIPv6 = 1; node.ipv6_preferred = 0; node_get_pref_orport(&node, &ap); tt_assert(tor_addr_eq(&ap.addr, &ipv4_addr)); @@ -305,28 +306,29 @@ test_node_preferred_orport(void *arg) /* Check the preferred address is IPv6 if we prefer it and * ClientUseIPv6 is 1, regardless of ClientUseIPv4 */ - mocked_options.ClientUseIPv4 = 1; - mocked_options.ClientUseIPv6 = 1; + mocked_options->ClientUseIPv4 = 1; + mocked_options->ClientUseIPv6 = 1; node.ipv6_preferred = 1; node_get_pref_orport(&node, &ap); tt_assert(tor_addr_eq(&ap.addr, &ipv6_addr)); tt_assert(ap.port == ipv6_port); - mocked_options.ClientUseIPv4 = 0; + mocked_options->ClientUseIPv4 = 0; node_get_pref_orport(&node, &ap); tt_assert(tor_addr_eq(&ap.addr, &ipv6_addr)); tt_assert(ap.port == ipv6_port); /* Check the preferred address is IPv6 if we don't prefer it, but * ClientUseIPv4 is 0 */ - mocked_options.ClientUseIPv4 = 0; - mocked_options.ClientUseIPv6 = 1; - node.ipv6_preferred = fascist_firewall_prefer_ipv6_orport(&mocked_options); + mocked_options->ClientUseIPv4 = 0; + mocked_options->ClientUseIPv6 = 1; + node.ipv6_preferred = fascist_firewall_prefer_ipv6_orport(mocked_options); node_get_pref_orport(&node, &ap); tt_assert(tor_addr_eq(&ap.addr, &ipv6_addr)); tt_assert(ap.port == ipv6_port); done: + or_options_free(mocked_options); UNMOCK(get_options); } diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index a303f10411..de79b0a547 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -1181,7 +1181,7 @@ test_introduce2(void *arg) MOCK(get_or_state, get_or_state_replacement); - dummy_state = tor_malloc_zero(sizeof(or_state_t)); + dummy_state = or_state_new(); circ = helper_create_origin_circuit(CIRCUIT_PURPOSE_S_INTRO, flags); tt_assert(circ); @@ -1345,7 +1345,7 @@ test_rotate_descriptors(void *arg) (void) arg; - dummy_state = tor_malloc_zero(sizeof(or_state_t)); + dummy_state = or_state_new(); hs_init(); MOCK(get_or_state, get_or_state_replacement); @@ -1462,7 +1462,7 @@ test_build_update_descriptors(void *arg) MOCK(networkstatus_get_live_consensus, mock_networkstatus_get_live_consensus); - dummy_state = tor_malloc_zero(sizeof(or_state_t)); + dummy_state = or_state_new(); ret = parse_rfc1123_time("Sat, 26 Oct 1985 03:00:00 UTC", &mock_ns.valid_after); @@ -1693,7 +1693,7 @@ test_build_descriptors(void *arg) MOCK(networkstatus_get_live_consensus, mock_networkstatus_get_live_consensus); - dummy_state = tor_malloc_zero(sizeof(or_state_t)); + dummy_state = or_state_new(); ret = parse_rfc1123_time("Sat, 26 Oct 1985 03:00:00 UTC", &mock_ns.valid_after); @@ -1794,7 +1794,7 @@ test_upload_descriptors(void *arg) MOCK(networkstatus_get_live_consensus, mock_networkstatus_get_live_consensus); - dummy_state = tor_malloc_zero(sizeof(or_state_t)); + dummy_state = or_state_new(); ret = parse_rfc1123_time("Sat, 26 Oct 1985 13:00:00 UTC", &mock_ns.valid_after); diff --git a/src/test/test_pt.c b/src/test/test_pt.c index 87e3ba356c..362c6664db 100644 --- a/src/test/test_pt.c +++ b/src/test/test_pt.c @@ -352,7 +352,7 @@ test_pt_configure_proxy(void *arg) managed_proxy_t *mp = NULL; (void) arg; - dummy_state = tor_malloc_zero(sizeof(or_state_t)); + dummy_state = or_state_new(); MOCK(process_read_stdout, process_read_stdout_replacement); MOCK(get_or_state, From 38b770bbbb37aef6cd3cef5fd6f425cd951affe2 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Jul 2019 11:32:52 -0400 Subject: [PATCH 12/28] Make a config_suite_t type to hold multiple config sub-objects Right now, it doesn't do anything; this patch is meant to make sure that we're doing memory management correctly. --- src/app/config/config.c | 1 + src/app/config/confparse.c | 49 +++++++++++++++++++++++ src/app/config/confparse.h | 6 +++ src/app/config/or_options_st.h | 4 ++ src/app/config/or_state_st.h | 4 ++ src/app/config/statefile.c | 1 + src/feature/dirauth/shared_random_state.c | 1 + src/test/test_confparse.c | 2 + 8 files changed, 68 insertions(+) diff --git a/src/app/config/config.c b/src/app/config/config.c index cdf5fa266c..bff403a990 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -864,6 +864,7 @@ static const config_format_t options_format = { options_validate_cb, options_clear_cb, NULL, + offsetof(or_options_t, subconfigs_), }; /* diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 8e3ad7fafd..d94a1d9ec9 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -72,6 +72,36 @@ managed_var_free_(managed_var_t *mv) #define managed_var_free(mv) \ FREE_AND_NULL(managed_var_t, managed_var_free_, (mv)) +struct config_suite_t { + /* NOTE: This object isn't implemented yet; it's just a placeholder + * to make sure we have our memory menagement right. + */ + int foo; +}; + +/** + * Allocate a new empty config_suite_t. + **/ +static config_suite_t * +config_suite_new(void) +{ + config_suite_t *suite = tor_malloc_zero(sizeof(config_suite_t)); + return suite; +} + +/** Release all storage held by a config_suite_t. (Does not free + * any configuration objects it holds; the caller must do that first.) */ +static void +config_suite_free_(config_suite_t *suite) +{ + if (!suite) + return; + tor_free(suite); +} + +#define config_suite_free(suite) \ + FREE_AND_NULL(config_suite_t, config_suite_free_, (suite)) + struct config_mgr_t { /** The 'top-level' configuration format. This one is used for legacy * options that have not yet been assigned to different sub-modules. @@ -152,6 +182,16 @@ config_mgr_register_fmt(config_mgr_t *mgr, } } +/** Return a pointer to the config_suite_t * pointer inside a + * configuration object; returns NULL if there is no such member. */ +static inline config_suite_t ** +config_mgr_get_suite_ptr(const config_mgr_t *mgr, void *toplevel) +{ + if (mgr->toplevel->config_suite_offset < 0) + return NULL; + return STRUCT_VAR_P(toplevel, mgr->toplevel->config_suite_offset); +} + /** * Return a pointer to the configuration object within toplevel whose * index is idx. @@ -272,6 +312,10 @@ config_new(const config_mgr_t *mgr) const config_format_t *fmt = mgr->toplevel; void *opts = tor_malloc_zero(fmt->size); struct_set_magic(opts, &mgr->toplevel_magic); + config_suite_t **suitep = config_mgr_get_suite_ptr(mgr, opts); + if (suitep) { + *suitep = config_suite_new(); + } CONFIG_CHECK(mgr, opts); return opts; } @@ -802,6 +846,11 @@ config_free_(const config_mgr_t *mgr, void *options) config_free_lines(*linep); *linep = NULL; } + + config_suite_t **suitep = config_mgr_get_suite_ptr(mgr, options); + if (suitep) + config_suite_free(*suitep); + tor_free(options); } diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 2e1a4b4f57..c18e854237 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -61,6 +61,9 @@ typedef struct config_format_t { /** If present, extra denotes a LINELIST variable for unrecognized * lines. Otherwise, unrecognized lines are an error. */ const struct_member_t *extra; + /** The position of a config_suite_t pointer within the toplevel object, + * or -1 if there is no such pointer. */ + int config_suite_offset; } config_format_t; /** @@ -79,6 +82,9 @@ void config_mgr_freeze(config_mgr_t *mgr); struct smartlist_t *config_mgr_list_vars(const config_mgr_t *mgr); struct smartlist_t *config_mgr_list_deprecated_vars(const config_mgr_t *mgr); +/** A collection of managed configuration objects. */ +typedef struct config_suite_t config_suite_t; + #define CAL_USE_DEFAULTS (1u<<0) #define CAL_CLEAR_FIRST (1u<<1) #define CAL_WARN_DEPRECATIONS (1u<<2) diff --git a/src/app/config/or_options_st.h b/src/app/config/or_options_st.h index 8156d2ca11..2c1a8f0c00 100644 --- a/src/app/config/or_options_st.h +++ b/src/app/config/or_options_st.h @@ -18,6 +18,7 @@ struct smartlist_t; struct config_line_t; +struct config_suite_t; /** Enumeration of outbound address configuration types: * Exit-only, OR-only, or both */ @@ -1107,6 +1108,9 @@ struct or_options_t { * a possible previous dormant state. **/ int DormantCanceledByStartup; + + /**DOCDOC*/ + struct config_suite_t *subconfigs_; }; #endif /* !defined(TOR_OR_OPTIONS_ST_H) */ diff --git a/src/app/config/or_state_st.h b/src/app/config/or_state_st.h index f45c6196cc..54390fa2d2 100644 --- a/src/app/config/or_state_st.h +++ b/src/app/config/or_state_st.h @@ -15,6 +15,7 @@ #include "lib/cc/torint.h" struct smartlist_t; +struct config_suite_t; /** Persistent state for an onion router, as saved to disk. */ struct or_state_t { @@ -94,6 +95,9 @@ struct or_state_t { /** True if we were dormant when we last wrote the file; false if we * weren't. "auto" on initial startup. */ int Dormant; + + /**DOCDOC*/ + struct config_suite_t *substates_; }; #endif /* !defined(TOR_OR_STATE_ST_H) */ diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index ede35e6ced..bcc06809b3 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -170,6 +170,7 @@ static const config_format_t state_format = { or_state_validate_cb, NULL, &state_extra_var, + offsetof(or_state_t, substates_), }; /* A global configuration manager for state-file objects */ diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index fac1efc54d..f2a626c738 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -100,6 +100,7 @@ static const config_format_t state_format = { disk_state_validate_cb, NULL, &state_extra_var, + -1, }; /* Global configuration manager for the shared-random state file */ diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 273232fe5d..eaa257aedd 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -134,6 +134,7 @@ static const config_format_t test_fmt = { test_validate_cb, NULL, NULL, + -1, }; /* Make sure that config_init sets everything to the right defaults. */ @@ -815,6 +816,7 @@ static config_format_t etest_fmt = { test_validate_cb, NULL, &extra, + -1, }; /* Try out the feature where we can store unrecognized lines and dump them From 638e58379a4c5347dd4952f12dd3b1054e7bb19f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 24 Jul 2019 11:20:55 -0400 Subject: [PATCH 13/28] Partial support for multiplicity in configuration objects A configuration manager, in addition to a top-level format object, may now also know about a suite of sub-formats. Top-level configuration objects, in turn, may now have a suite of sub-objects. --- src/app/config/confparse.c | 105 +++++++++++++++++++++++++++++++------ src/app/config/confparse.h | 2 + 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index d94a1d9ec9..8018f6a6d4 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -73,10 +73,10 @@ managed_var_free_(managed_var_t *mv) FREE_AND_NULL(managed_var_t, managed_var_free_, (mv)) struct config_suite_t { - /* NOTE: This object isn't implemented yet; it's just a placeholder - * to make sure we have our memory menagement right. - */ - int foo; + /** A list of configuration objects managed by a given configuration + * manager. They are stored in the same order as the config_format_t + * objects in the manager's list of subformats. */ + smartlist_t *configs; }; /** @@ -86,6 +86,7 @@ static config_suite_t * config_suite_new(void) { config_suite_t *suite = tor_malloc_zero(sizeof(config_suite_t)); + suite->configs = smartlist_new(); return suite; } @@ -96,6 +97,7 @@ config_suite_free_(config_suite_t *suite) { if (!suite) return; + smartlist_free(suite->configs); tor_free(suite); } @@ -110,6 +112,11 @@ struct config_mgr_t { * contains. A subsequent commit will add more. XXXX) */ const config_format_t *toplevel; + /** + * List of second-level configuration format objects that this manager + * also knows about. + */ + smartlist_t *subconfigs; /** A smartlist of managed_var_t objects for all configuration formats. */ smartlist_t *all_vars; /** A smartlist of config_abbrev_t objects for all abbreviations. These @@ -137,12 +144,14 @@ config_mgr_t * config_mgr_new(const config_format_t *toplevel_fmt) { config_mgr_t *mgr = tor_malloc_zero(sizeof(config_mgr_t)); - mgr->toplevel = toplevel_fmt; + mgr->subconfigs = smartlist_new(); mgr->all_vars = smartlist_new(); mgr->all_abbrevs = smartlist_new(); mgr->all_deprecations = smartlist_new(); config_mgr_register_fmt(mgr, toplevel_fmt, IDX_TOPLEVEL); + mgr->toplevel = toplevel_fmt; + return mgr; } @@ -158,6 +167,14 @@ config_mgr_register_fmt(config_mgr_t *mgr, "Tried to add a format to a configuration manager after " "it had been frozen."); + if (object_idx != IDX_TOPLEVEL) { + tor_assertf(fmt->config_suite_offset < 0, + "Tried to register a toplevel format in a non-toplevel position"); + } + tor_assertf(fmt != mgr->toplevel && + ! smartlist_contains(mgr->subconfigs, fmt), + "Tried to register an already-registered format."); + /* register variables */ for (i = 0; fmt->vars[i].member.name; ++i) { managed_var_t *mv = tor_malloc_zero(sizeof(managed_var_t)); @@ -182,6 +199,21 @@ config_mgr_register_fmt(config_mgr_t *mgr, } } +/** + * Add a new format to this configuration object. Asserts on failure. + * + **/ +int +config_mgr_add_format(config_mgr_t *mgr, + const config_format_t *fmt) +{ + tor_assert(mgr); + int idx = smartlist_len(mgr->subconfigs); + config_mgr_register_fmt(mgr, fmt, idx); + smartlist_add(mgr->subconfigs, (void *)fmt); + return idx; +} + /** Return a pointer to the config_suite_t * pointer inside a * configuration object; returns NULL if there is no such member. */ static inline config_suite_t ** @@ -204,10 +236,19 @@ config_mgr_get_suite_ptr(const config_mgr_t *mgr, void *toplevel) static void * config_mgr_get_obj_mutable(const config_mgr_t *mgr, void *toplevel, int idx) { - (void)mgr; - tor_assert(idx == IDX_TOPLEVEL); // nothing else is implemented yet XXXX + tor_assert(mgr); tor_assert(toplevel); - return toplevel; + if (idx == IDX_TOPLEVEL) + return toplevel; + + tor_assertf(idx >= 0 && idx < smartlist_len(mgr->subconfigs), + "Index %d is out of range.", idx); + config_suite_t **suite = config_mgr_get_suite_ptr(mgr, toplevel); + tor_assert(suite); + tor_assert(smartlist_len(mgr->subconfigs) == + smartlist_len((*suite)->configs)); + + return smartlist_get((*suite)->configs, idx); } /** As config_mgr_get_obj_mutable(), but return a const pointer. */ @@ -257,6 +298,7 @@ config_mgr_free_(config_mgr_t *mgr) smartlist_free(mgr->all_vars); smartlist_free(mgr->all_abbrevs); smartlist_free(mgr->all_deprecations); + smartlist_free(mgr->subconfigs); memset(mgr, 0, sizeof(*mgr)); tor_free(mgr); } @@ -296,6 +338,20 @@ config_mgr_assert_magic_ok(const config_mgr_t *mgr, tor_assert(options); tor_assert(mgr->frozen); struct_check_magic(options, &mgr->toplevel_magic); + + config_suite_t **suitep = config_mgr_get_suite_ptr(mgr, (void*)options); + if (suitep == NULL) { + tor_assert(smartlist_len(mgr->subconfigs) == 0); + return; + } + + tor_assert(smartlist_len((*suitep)->configs) == + smartlist_len(mgr->subconfigs)); + SMARTLIST_FOREACH_BEGIN(mgr->subconfigs, const config_format_t *, fmt) { + void *obj = smartlist_get((*suitep)->configs, fmt_sl_idx); + tor_assert(obj); + struct_check_magic(obj, &fmt->magic); + } SMARTLIST_FOREACH_END(fmt); } /** Macro: assert that cfg has the right magic field for @@ -309,12 +365,16 @@ void * config_new(const config_mgr_t *mgr) { tor_assert(mgr->frozen); - const config_format_t *fmt = mgr->toplevel; - void *opts = tor_malloc_zero(fmt->size); + void *opts = tor_malloc_zero(mgr->toplevel->size); struct_set_magic(opts, &mgr->toplevel_magic); config_suite_t **suitep = config_mgr_get_suite_ptr(mgr, opts); if (suitep) { *suitep = config_suite_new(); + SMARTLIST_FOREACH_BEGIN(mgr->subconfigs, const config_format_t *, fmt) { + void *obj = tor_malloc_zero(fmt->size); + struct_set_magic(obj, &fmt->magic); + smartlist_add((*suitep)->configs, obj); + } SMARTLIST_FOREACH_END(fmt); } CONFIG_CHECK(mgr, opts); return opts; @@ -826,30 +886,41 @@ config_reset(const config_mgr_t *mgr, void *options, void config_free_(const config_mgr_t *mgr, void *options) { - const config_format_t *fmt = mgr->toplevel; - if (!options) return; - tor_assert(fmt); + tor_assert(mgr); if (mgr->toplevel->clear_fn) { mgr->toplevel->clear_fn(mgr, options); } + config_suite_t **suitep = config_mgr_get_suite_ptr(mgr, options); + if (suitep) { + tor_assert(smartlist_len((*suitep)->configs) == + smartlist_len(mgr->subconfigs)); + SMARTLIST_FOREACH_BEGIN(mgr->subconfigs, const config_format_t *, fmt) { + void *obj = smartlist_get((*suitep)->configs, fmt_sl_idx); + if (fmt->clear_fn) { + fmt->clear_fn(mgr, obj); + } + } SMARTLIST_FOREACH_END(fmt); + } SMARTLIST_FOREACH_BEGIN(mgr->all_vars, const managed_var_t *, mv) { config_clear(mgr, options, mv); } SMARTLIST_FOREACH_END(mv); - if (fmt->extra) { - config_line_t **linep = STRUCT_VAR_P(options, fmt->extra->offset); + if (mgr->toplevel->extra) { + config_line_t **linep = STRUCT_VAR_P(options, + mgr->toplevel->extra->offset); config_free_lines(*linep); *linep = NULL; } - config_suite_t **suitep = config_mgr_get_suite_ptr(mgr, options); - if (suitep) + if (suitep) { + SMARTLIST_FOREACH((*suitep)->configs, void *, obj, tor_free(obj)); config_suite_free(*suitep); + } tor_free(options); } diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index c18e854237..9125c0be3d 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -76,6 +76,8 @@ typedef struct config_mgr_t config_mgr_t; config_mgr_t *config_mgr_new(const config_format_t *toplevel_fmt); void config_mgr_free_(config_mgr_t *mgr); +int config_mgr_add_format(config_mgr_t *mgr, + const config_format_t *fmt); void config_mgr_freeze(config_mgr_t *mgr); #define config_mgr_free(mgr) \ FREE_AND_NULL(config_mgr_t, config_mgr_free_, (mgr)) From 760d0c056fafdcefd9ac58d48f8ccf473d0ea877 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 19:15:52 -0400 Subject: [PATCH 14/28] finish a comment explaining all_abbrevs --- src/app/config/confparse.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 8018f6a6d4..95e73eeab5 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -119,8 +119,9 @@ struct config_mgr_t { smartlist_t *subconfigs; /** A smartlist of managed_var_t objects for all configuration formats. */ smartlist_t *all_vars; - /** A smartlist of config_abbrev_t objects for all abbreviations. These - * objects are */ + /** A smartlist of config_abbrev_t objects for all configuration + * formats. These objects are used to track synonyms and abbreviations for + * different configuration options. */ smartlist_t *all_abbrevs; /** A smartlist of config_deprecation_t for all configuration formats. */ smartlist_t *all_deprecations; From 3a530ae5352cb64e176b889e5449913416c372c0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 19:19:09 -0400 Subject: [PATCH 15/28] Document types for config_get_changes() --- src/app/config/confparse.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 95e73eeab5..324009b597 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -951,6 +951,9 @@ config_is_same(const config_mgr_t *mgr, * Return a list of the options which have changed between options1 and * options2. If an option has reverted to its default value, it has a * value entry of NULL. + * + * options1 and options2 must be top-level configuration objects + * of the type managed by mgr. **/ config_line_t * config_get_changes(const config_mgr_t *mgr, From 9cfc811c377be8e30532b6faef78e3c97f0ae7e8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 19:20:16 -0400 Subject: [PATCH 16/28] Fix typo in comment about FallbackDir handling --- src/app/config/config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index bff403a990..66047aeabc 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -8154,7 +8154,7 @@ getinfo_helper_config(control_connection_t *conn, } if (strcmp(var->member.name, "FallbackDir") == 0) { /* - * Similarly count fallback lines, so that we can decided later + * Similarly count fallback lines, so that we can decide later * to add the defaults manually. */ ++fallback_lines_seen; From 12d980027a52fd8a5c0a5f0c4088ce91b15f0a92 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 19:21:26 -0400 Subject: [PATCH 17/28] document return lifespans for config_mgr_list_vars() --- src/app/config/confparse.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 324009b597..cf17107335 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -306,7 +306,7 @@ config_mgr_free_(config_mgr_t *mgr) /** Return a new smartlist_t containing a config_var_t for every variable that * mgr knows about. The elements of this smartlist do not need - * to be freed. */ + * to be freed; they have the same lifespan as mgr. */ smartlist_t * config_mgr_list_vars(const config_mgr_t *mgr) { @@ -318,7 +318,9 @@ config_mgr_list_vars(const config_mgr_t *mgr) } /** Return a new smartlist_t containing the names of all deprecated variables. - * The elements of this smartlist do not need to be freed. */ + * The elements of this smartlist do not need to be freed; they have the same + * lifespan as mgr. + */ smartlist_t * config_mgr_list_deprecated_vars(const config_mgr_t *mgr) { From 3af1cee6d91a03912ffa0ddb866b5a85683cf178 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 19:32:41 -0400 Subject: [PATCH 18/28] Document or_options_t.subconfigs_ --- src/app/config/or_options_st.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/app/config/or_options_st.h b/src/app/config/or_options_st.h index 2c1a8f0c00..32dcd9fb18 100644 --- a/src/app/config/or_options_st.h +++ b/src/app/config/or_options_st.h @@ -1109,7 +1109,12 @@ struct or_options_t { **/ int DormantCanceledByStartup; - /**DOCDOC*/ + /** + * Configuration objects for individual modules. + * + * Never access this field or its members directly: instead, use the module + * in question to get its relevant configuration object. + */ struct config_suite_t *subconfigs_; }; From 06da2c0d28e41c5a230d96ae8e4f72471bccb5d3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 19:33:55 -0400 Subject: [PATCH 19/28] Document or_state_t.substates_ --- src/app/config/or_state_st.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/app/config/or_state_st.h b/src/app/config/or_state_st.h index 54390fa2d2..225003bb7e 100644 --- a/src/app/config/or_state_st.h +++ b/src/app/config/or_state_st.h @@ -96,7 +96,12 @@ struct or_state_t { * weren't. "auto" on initial startup. */ int Dormant; - /**DOCDOC*/ + /** + * State objects for individual modules. + * + * Never access this field or its members directly: instead, use the module + * in question to get its relevant state object if you must. + */ struct config_suite_t *substates_; }; From 380d3ee1682ce5d202d754d442928710b477f59a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 19:36:17 -0400 Subject: [PATCH 20/28] Better explain config_clear_fn_t --- src/app/config/confparse.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 9125c0be3d..74ecb11a36 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -41,7 +41,13 @@ typedef int (*validate_fn_t)(void*,void*,void*,int,char**); struct config_mgr_t; -/** Callback to clear all non-managed fields of a configuration object. */ +/** + * Callback to clear all non-managed fields of a configuration object. + * + * (Regular fields get cleared by config_reset(), but you might have fields + * in the object that do not correspond to configuration variables. If those + * fields need to be cleared or freed, this is where to do it.) + */ typedef void (*clear_cfg_fn_t)(const struct config_mgr_t *mgr, void*); /** Information on the keys, value types, key-to-struct-member mappings, From aa3f0c4788c5cb07243a764589c912a8ed4a26cd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 27 Aug 2019 09:01:39 -0400 Subject: [PATCH 21/28] Add tests for variable-listing functions. This discovered a bug related to an extra & in config_mgr_list_deprecated_vars(): fix that. --- src/app/config/confparse.c | 2 +- src/test/test_confparse.c | 70 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index cf17107335..8b2b53003f 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -327,7 +327,7 @@ config_mgr_list_deprecated_vars(const config_mgr_t *mgr) smartlist_t *result = smartlist_new(); tor_assert(mgr); SMARTLIST_FOREACH(mgr->all_deprecations, config_deprecation_t *, d, - smartlist_add(result, &d->name)); + smartlist_add(result, (char*)d->name)); return result; } diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index eaa257aedd..1d2e31ab94 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -932,6 +932,74 @@ test_confparse_check_ok_fail(void *arg) config_mgr_free(mgr); } +static void +test_confparse_list_vars(void *arg) +{ + (void)arg; + config_mgr_t *mgr = config_mgr_new(&test_fmt); + smartlist_t *vars = config_mgr_list_vars(mgr); + smartlist_t *varnames = smartlist_new(); + char *joined = NULL; + + tt_assert(vars); + SMARTLIST_FOREACH(vars, config_var_t *, cv, + smartlist_add(varnames, (void*)cv->member.name)); + smartlist_sort_strings(varnames); + joined = smartlist_join_strings(varnames, "::", 0, NULL); + tt_str_op(joined, OP_EQ, + "LineTypeA::" + "LineTypeB::" + "MixedHiddenLines::" + "MixedLines::" + "VisibleLineB::" + "__HiddenInt::" + "__HiddenLineA::" + "autobool::" + "boolean::" + "csv::" + "csv_interval::" + "dbl::" + "deprecated_int::" + "fn::" + "i::" + "interval::" + "lines::" + "mem::" + "msec_interval::" + "obsolete::" + "pos::" + "routerset::" + "s::" + "time::" + "u64"); + + done: + tor_free(joined); + smartlist_free(varnames); + smartlist_free(vars); + config_mgr_free(mgr); +} + +static void +test_confparse_list_deprecated(void *arg) +{ + (void)arg; + config_mgr_t *mgr = config_mgr_new(&test_fmt); + smartlist_t *vars = config_mgr_list_deprecated_vars(mgr); + char *joined = NULL; + + tt_assert(vars); + smartlist_sort_strings(vars); + joined = smartlist_join_strings(vars, "::", 0, NULL); + + tt_str_op(joined, OP_EQ, "deprecated_int"); + + done: + tor_free(joined); + smartlist_free(vars); + config_mgr_free(mgr); +} + #define CONFPARSE_TEST(name, flags) \ { #name, test_confparse_ ## name, flags, NULL, NULL } @@ -968,5 +1036,7 @@ struct testcase_t confparse_tests[] = { CONFPARSE_TEST(extra_lines, 0), CONFPARSE_TEST(unitparse, 0), CONFPARSE_TEST(check_ok_fail, 0), + CONFPARSE_TEST(list_vars, 0), + CONFPARSE_TEST(list_deprecated, 0), END_OF_TESTCASES }; From cb5f8ace79813fd835d4bb396dd693b39b1063e4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 27 Aug 2019 09:08:51 -0400 Subject: [PATCH 22/28] Tests for config_find_option_name() Fix a bug in config_find_option_name() where it did not consider the abbreviations table. --- src/app/config/confparse.c | 1 + src/test/test_confparse.c | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 8b2b53003f..5c994c0a6a 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -493,6 +493,7 @@ config_mgr_find_var(const config_mgr_t *mgr, const char * config_find_option_name(const config_mgr_t *mgr, const char *key) { + key = config_expand_abbrev(mgr, key, 0, 0); const managed_var_t *mv = config_mgr_find_var(mgr, key, true, NULL); if (mv) return mv->cvar->member.name; diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 1d2e31ab94..f04c412c0e 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -1000,6 +1000,29 @@ test_confparse_list_deprecated(void *arg) config_mgr_free(mgr); } +static void +test_confparse_find_option_name(void *arg) +{ + (void)arg; + config_mgr_t *mgr = config_mgr_new(&test_fmt); + + // exact match + tt_str_op(config_find_option_name(mgr, "u64"), OP_EQ, "u64"); + // case-insensitive match + tt_str_op(config_find_option_name(mgr, "S"), OP_EQ, "s"); + tt_str_op(config_find_option_name(mgr, "linetypea"), OP_EQ, "LineTypeA"); + // prefix match + tt_str_op(config_find_option_name(mgr, "deprec"), OP_EQ, "deprecated_int"); + // explicit abbreviation + tt_str_op(config_find_option_name(mgr, "uint"), OP_EQ, "pos"); + tt_str_op(config_find_option_name(mgr, "UINT"), OP_EQ, "pos"); + // no match + tt_ptr_op(config_find_option_name(mgr, "absent"), OP_EQ, NULL); + + done: + config_mgr_free(mgr); +} + #define CONFPARSE_TEST(name, flags) \ { #name, test_confparse_ ## name, flags, NULL, NULL } @@ -1038,5 +1061,6 @@ struct testcase_t confparse_tests[] = { CONFPARSE_TEST(check_ok_fail, 0), CONFPARSE_TEST(list_vars, 0), CONFPARSE_TEST(list_deprecated, 0), + CONFPARSE_TEST(find_option_name, 0), END_OF_TESTCASES }; From e4162cdb38111c85c308900f6a31cf0010fa5a6e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 27 Aug 2019 09:44:30 -0400 Subject: [PATCH 23/28] Start on test cases for the multi-object feature of confmgr. This test case, at this point, only constructs the confmgr object. More code to come. --- src/test/include.am | 1 + src/test/test.c | 1 + src/test/test.h | 1 + src/test/test_confmgr.c | 140 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+) create mode 100644 src/test/test_confmgr.c diff --git a/src/test/include.am b/src/test/include.am index 0ec4d96ad4..c32c6152dc 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -120,6 +120,7 @@ src_test_test_SOURCES += \ src/test/test_circuitstats.c \ src/test/test_compat_libevent.c \ src/test/test_config.c \ + src/test/test_confmgr.c \ src/test/test_confparse.c \ src/test/test_connection.c \ src/test/test_conscache.c \ diff --git a/src/test/test.c b/src/test/test.c index 266b7454a3..dd26b0437e 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -840,6 +840,7 @@ struct testgroup_t testgroups[] = { { "circuituse/", circuituse_tests }, { "compat/libevent/", compat_libevent_tests }, { "config/", config_tests }, + { "config/mgr/", confmgr_tests }, { "config/parse/", confparse_tests }, { "connection/", connection_tests }, { "conscache/", conscache_tests }, diff --git a/src/test/test.h b/src/test/test.h index 322716a9ab..c217b8f4f4 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -197,6 +197,7 @@ extern struct testcase_t circuitstats_tests[]; extern struct testcase_t circuituse_tests[]; extern struct testcase_t compat_libevent_tests[]; extern struct testcase_t config_tests[]; +extern struct testcase_t confmgr_tests[]; extern struct testcase_t confparse_tests[]; extern struct testcase_t connection_tests[]; extern struct testcase_t conscache_tests[]; diff --git a/src/test/test_confmgr.c b/src/test/test_confmgr.c new file mode 100644 index 0000000000..1a4d5ececa --- /dev/null +++ b/src/test/test_confmgr.c @@ -0,0 +1,140 @@ +/* Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/* + * Tests for confparse.c's features that support multiple configuration + * formats and configuration objects. + */ + +#include "orconfig.h" + +#include "core/or/or.h" +#include "lib/encoding/confline.h" +#include "app/config/confparse.h" +#include "test/test.h" +#include "test/log_test_helpers.h" + +/* + * Set up a few objects: a pasture_cfg is toplevel; it has a llama_cfg and an + * alpaca_cfg. + */ + +typedef struct { + uint32_t magic; + char *address; + int opentopublic; + config_suite_t *subobjs; +} pasture_cfg_t; + +typedef struct { + char *llamaname; + int cuteness; + uint32_t magic; +} llama_cfg_t; + +typedef struct { + uint32_t magic; + int fuzziness; + char *alpacaname; +} alpaca_cfg_t; + +/* + * Make the above into configuration objects. + */ + +static pasture_cfg_t pasture_cfg_t_dummy; +static llama_cfg_t llama_cfg_t_dummy; +static alpaca_cfg_t alpaca_cfg_t_dummy; + +#define PV(name, type, dflt) \ + CONFIG_VAR_ETYPE(pasture_cfg_t, #name, type, name, 0, dflt) +#define LV(name, type, dflt) \ + CONFIG_VAR_ETYPE(llama_cfg_t, #name, type, name, 0, dflt) +#define AV(name, type, dflt) \ + CONFIG_VAR_ETYPE(alpaca_cfg_t, #name, type, name, 0, dflt) +static const config_var_t pasture_vars[] = { + PV(address, STRING, NULL), + PV(opentopublic, BOOL, "1"), + END_OF_CONFIG_VARS +}; +static const config_var_t llama_vars[] = +{ + LV(llamaname, STRING, NULL), + LV(cuteness, POSINT, "100"), + END_OF_CONFIG_VARS +}; +static const config_var_t alpaca_vars[] = +{ + AV(alpacaname, STRING, NULL), + AV(fuzziness, POSINT, "50"), + END_OF_CONFIG_VARS +}; + +static const config_format_t pasture_fmt = { + sizeof(pasture_cfg_t), + { + "pasture_cfg_t", + 8989, + offsetof(pasture_cfg_t, magic) + }, + .vars = pasture_vars, + .config_suite_offset = offsetof(pasture_cfg_t, subobjs), +}; + +static const config_format_t llama_fmt = { + sizeof(llama_cfg_t), + { + "llama_cfg_t", + 0x11aa11, + offsetof(llama_cfg_t, magic) + }, + .vars = llama_vars, + .config_suite_offset = -1, +}; + +static const config_format_t alpaca_fmt = { + sizeof(alpaca_cfg_t), + { + "alpaca_cfg_t", + 0xa15aca, + offsetof(alpaca_cfg_t, magic) + }, + .vars = alpaca_vars, + .config_suite_offset = -1, +}; + +static config_mgr_t * +get_mgr(bool freeze) +{ + config_mgr_t *mgr = config_mgr_new(&pasture_fmt); + tt_int_op(0, OP_EQ, config_mgr_add_format(mgr, &llama_fmt)); + tt_int_op(1, OP_EQ, config_mgr_add_format(mgr, &alpaca_fmt)); + if (freeze) + config_mgr_freeze(mgr); + return mgr; + + done: + config_mgr_free(mgr); + return NULL; +} + +static void +test_confmgr_init(void *arg) +{ + (void)arg; + config_mgr_t *mgr = get_mgr(true); + tt_ptr_op(mgr, OP_NE, NULL); + + done: + config_mgr_free(mgr); +} + +#define CONFMGR_TEST(name, flags) \ + { #name, test_confmgr_ ## name, flags, NULL, NULL } + +struct testcase_t confmgr_tests[] = { + CONFMGR_TEST(init, 0), + END_OF_TESTCASES +}; From d9fe9f5edef53c53e2e2337f8af0d88e42e47d4a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 27 Aug 2019 10:42:08 -0400 Subject: [PATCH 24/28] Confmgr tests: test deprecations and abbreviations. --- src/test/test_confmgr.c | 42 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/test/test_confmgr.c b/src/test/test_confmgr.c index 1a4d5ececa..9801108e97 100644 --- a/src/test/test_confmgr.c +++ b/src/test/test_confmgr.c @@ -32,12 +32,14 @@ typedef struct { char *llamaname; int cuteness; uint32_t magic; + int eats_meat; /* deprecated; llamas are never carnivorous. */ } llama_cfg_t; typedef struct { uint32_t magic; int fuzziness; char *alpacaname; + int n_wings; /* deprecated; alpacas don't have wings. */ } alpaca_cfg_t; /* @@ -62,6 +64,7 @@ static const config_var_t pasture_vars[] = { static const config_var_t llama_vars[] = { LV(llamaname, STRING, NULL), + LV(eats_meat, BOOL, NULL), LV(cuteness, POSINT, "100"), END_OF_CONFIG_VARS }; @@ -69,9 +72,26 @@ static const config_var_t alpaca_vars[] = { AV(alpacaname, STRING, NULL), AV(fuzziness, POSINT, "50"), + AV(n_wings, POSINT, "0"), END_OF_CONFIG_VARS }; +static config_deprecation_t llama_deprecations[] = { + { "eats_meat", "Llamas are herbivores." }, + {NULL,NULL} +}; + +static config_deprecation_t alpaca_deprecations[] = { + { "n_wings", "Alpacas are quadrupeds." }, + {NULL,NULL} +}; + +static config_abbrev_t llama_abbrevs[] = { + { "gracia", "cuteness", 0, 0 }, + { "gentillesse", "cuteness", 0, 0 }, + { NULL, NULL, 0, 0 }, +}; + static const config_format_t pasture_fmt = { sizeof(pasture_cfg_t), { @@ -92,6 +112,8 @@ static const config_format_t llama_fmt = { }, .vars = llama_vars, .config_suite_offset = -1, + .deprecations = llama_deprecations, + .abbrevs = llama_abbrevs, }; static const config_format_t alpaca_fmt = { @@ -103,6 +125,7 @@ static const config_format_t alpaca_fmt = { }, .vars = alpaca_vars, .config_suite_offset = -1, + .deprecations = alpaca_deprecations, }; static config_mgr_t * @@ -125,9 +148,28 @@ test_confmgr_init(void *arg) { (void)arg; config_mgr_t *mgr = get_mgr(true); + smartlist_t *vars = NULL; tt_ptr_op(mgr, OP_NE, NULL); + vars = config_mgr_list_vars(mgr); + tt_int_op(smartlist_len(vars), OP_EQ, 8); // 8 vars total. + + tt_str_op("cuteness", OP_EQ, config_find_option_name(mgr, "CUTENESS")); + tt_str_op("cuteness", OP_EQ, config_find_option_name(mgr, "GRACIA")); + smartlist_free(vars); + + vars = config_mgr_list_deprecated_vars(mgr); // 2 deprecated vars. + tt_int_op(smartlist_len(vars), OP_EQ, 2); + tt_assert(smartlist_contains_string(vars, "eats_meat")); + tt_assert(smartlist_contains_string(vars, "n_wings")); + + tt_str_op("Llamas are herbivores.", OP_EQ, + config_find_deprecation(mgr, "EATS_MEAT")); + tt_str_op("Alpacas are quadrupeds.", OP_EQ, + config_find_deprecation(mgr, "N_WINGS")); + done: + smartlist_free(vars); config_mgr_free(mgr); } From 8db3859cc6f394927ca66bfc19c1227408fb7874 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 27 Aug 2019 11:12:34 -0400 Subject: [PATCH 25/28] Simple tests for nested configuration formats One test makes sure that the toplevel magic numbers are distinct. One test makes sure that we can parse a configuration object with two sub-objects. --- src/app/config/confparse.c | 4 +- src/app/config/confparse.h | 4 ++ src/test/test_confmgr.c | 90 +++++++++++++++++++++++++++++++++++++- 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 5c994c0a6a..71ba39ef52 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -234,7 +234,7 @@ config_mgr_get_suite_ptr(const config_mgr_t *mgr, void *toplevel) * to configuration objects for other modules. This function gets * the sub-object for a particular modules. */ -static void * +STATIC void * config_mgr_get_obj_mutable(const config_mgr_t *mgr, void *toplevel, int idx) { tor_assert(mgr); @@ -253,7 +253,7 @@ config_mgr_get_obj_mutable(const config_mgr_t *mgr, void *toplevel, int idx) } /** As config_mgr_get_obj_mutable(), but return a const pointer. */ -static const void * +STATIC const void * config_mgr_get_obj(const config_mgr_t *mgr, const void *toplevel, int idx) { return config_mgr_get_obj_mutable(mgr, (void*)toplevel, idx); diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 74ecb11a36..b7923bb82b 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -146,6 +146,10 @@ bool config_var_is_contained(const config_var_t *var); #ifdef CONFPARSE_PRIVATE STATIC void config_reset_line(const config_mgr_t *mgr, void *options, const char *key, int use_defaults); +STATIC void *config_mgr_get_obj_mutable(const config_mgr_t *mgr, + void *toplevel, int idx); +STATIC const void *config_mgr_get_obj(const config_mgr_t *mgr, + const void *toplevel, int idx); #endif #endif /* !defined(TOR_CONFPARSE_H) */ diff --git a/src/test/test_confmgr.c b/src/test/test_confmgr.c index 9801108e97..547fd5f423 100644 --- a/src/test/test_confmgr.c +++ b/src/test/test_confmgr.c @@ -8,6 +8,7 @@ * formats and configuration objects. */ +#define CONFPARSE_PRIVATE #include "orconfig.h" #include "core/or/or.h" @@ -128,12 +129,15 @@ static const config_format_t alpaca_fmt = { .deprecations = alpaca_deprecations, }; +#define LLAMA_IDX 0 +#define ALPACA_IDX 1 + static config_mgr_t * get_mgr(bool freeze) { config_mgr_t *mgr = config_mgr_new(&pasture_fmt); - tt_int_op(0, OP_EQ, config_mgr_add_format(mgr, &llama_fmt)); - tt_int_op(1, OP_EQ, config_mgr_add_format(mgr, &alpaca_fmt)); + tt_int_op(LLAMA_IDX, OP_EQ, config_mgr_add_format(mgr, &llama_fmt)); + tt_int_op(ALPACA_IDX, OP_EQ, config_mgr_add_format(mgr, &alpaca_fmt)); if (freeze) config_mgr_freeze(mgr); return mgr; @@ -173,10 +177,92 @@ test_confmgr_init(void *arg) config_mgr_free(mgr); } +static void +test_confmgr_magic(void *args) +{ + (void)args; + // Every time we build a manager, it is supposed to get a different magic + // number. Let's test that. + config_mgr_t *mgr1 = get_mgr(true); + config_mgr_t *mgr2 = get_mgr(true); + config_mgr_t *mgr3 = get_mgr(true); + + pasture_cfg_t *p1 = NULL, *p2 = NULL, *p3 = NULL; + + tt_assert(mgr1); + tt_assert(mgr2); + tt_assert(mgr3); + + p1 = config_new(mgr1); + p2 = config_new(mgr2); + p3 = config_new(mgr3); + + tt_assert(p1); + tt_assert(p2); + tt_assert(p3); + + // By chance, two managers get the same magic with P=2^-32. Let's + // make sure that at least two of them are different, so that our + // odds of a false positive are 1/2^-64. + tt_assert((p1->magic != p2->magic) || (p2->magic != p3->magic)); + + done: + config_free(mgr1, p1); + config_free(mgr2, p2); + config_free(mgr3, p3); + + config_mgr_free(mgr1); + config_mgr_free(mgr2); + config_mgr_free(mgr3); +} + +static const char *simple_pasture = + "LLamaname hugo\n" + "Alpacaname daphne\n" + "gentillesse 42\n" + "address 123 Camelid ave\n"; + +static void +test_confmgr_parse(void *arg) +{ + (void)arg; + config_mgr_t *mgr = get_mgr(true); + pasture_cfg_t *p = config_new(mgr); + config_line_t *lines = NULL; + char *msg = NULL; + + config_init(mgr, p); // set defaults. + + int r = config_get_lines(simple_pasture, &lines, 0); + tt_int_op(r, OP_EQ, 0); + r = config_assign(mgr, p, lines, 0, &msg); + tt_int_op(r, OP_EQ, 0); + + tt_int_op(p->opentopublic, OP_EQ, 1); + tt_str_op(p->address, OP_EQ, "123 Camelid ave"); + + // We are using this API directly; modules outside confparse will, in the + // future, not. + const alpaca_cfg_t *ac = config_mgr_get_obj(mgr, p, ALPACA_IDX); + const llama_cfg_t *lc = config_mgr_get_obj(mgr, p, LLAMA_IDX); + tt_str_op(lc->llamaname, OP_EQ, "hugo"); + tt_str_op(ac->alpacaname, OP_EQ, "daphne"); + tt_int_op(lc->cuteness, OP_EQ, 42); + tt_int_op(ac->fuzziness, OP_EQ, 50); + + done: + config_free_lines(lines); + config_free(mgr, p); + config_mgr_free(mgr); + tor_free(msg); +} + #define CONFMGR_TEST(name, flags) \ { #name, test_confmgr_ ## name, flags, NULL, NULL } struct testcase_t confmgr_tests[] = { CONFMGR_TEST(init, 0), + CONFMGR_TEST(magic, 0), + CONFMGR_TEST(parse, 0), END_OF_TESTCASES }; From c967b5c9ba70c2c7f05aab5120c3415d53462ba7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 27 Aug 2019 12:34:09 -0400 Subject: [PATCH 26/28] Add a test for dumping a compound configuration --- src/test/test_confmgr.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/test/test_confmgr.c b/src/test/test_confmgr.c index 547fd5f423..170e8335e7 100644 --- a/src/test/test_confmgr.c +++ b/src/test/test_confmgr.c @@ -257,6 +257,41 @@ test_confmgr_parse(void *arg) tor_free(msg); } +static void +test_confmgr_dump(void *arg) +{ + (void)arg; + config_mgr_t *mgr = get_mgr(true); + pasture_cfg_t *p = config_new(mgr); + pasture_cfg_t *defaults = config_new(mgr); + config_line_t *lines = NULL; + char *msg = NULL; + char *s = NULL; + + config_init(mgr, p); // set defaults. + config_init(mgr, defaults); // set defaults. + + int r = config_get_lines(simple_pasture, &lines, 0); + tt_int_op(r, OP_EQ, 0); + r = config_assign(mgr, p, lines, 0, &msg); + tt_int_op(r, OP_EQ, 0); + + s = config_dump(mgr, defaults, p, 1, 0); + tt_str_op("address 123 Camelid ave\n" + "alpacaname daphne\n" + "cuteness 42\n" + "llamaname hugo\n", OP_EQ, s); + + done: + config_free_lines(lines); + config_free(mgr, p); + config_free(mgr, defaults); + config_mgr_free(mgr); + + tor_free(msg); + tor_free(s); +} + #define CONFMGR_TEST(name, flags) \ { #name, test_confmgr_ ## name, flags, NULL, NULL } @@ -264,5 +299,6 @@ struct testcase_t confmgr_tests[] = { CONFMGR_TEST(init, 0), CONFMGR_TEST(magic, 0), CONFMGR_TEST(parse, 0), + CONFMGR_TEST(dump, 0), END_OF_TESTCASES }; From 577e628743bd7bb8c846bc73401e65b837cc4dbf Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 27 Aug 2019 13:10:18 -0400 Subject: [PATCH 27/28] Fix typo in a comment on config_mgr_get_obj_mutable() --- src/app/config/confparse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 71ba39ef52..e0615ac13d 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -232,7 +232,7 @@ config_mgr_get_suite_ptr(const config_mgr_t *mgr, void *toplevel) * NOTE: XXXX Eventually, there will be multiple objects supported within the * toplevel object. For example, the or_options_t will contain pointers * to configuration objects for other modules. This function gets - * the sub-object for a particular modules. + * the sub-object for a particular module. */ STATIC void * config_mgr_get_obj_mutable(const config_mgr_t *mgr, void *toplevel, int idx) From a3e99c5f1e2eb9dd9ead51bcbed61dc2b99cf256 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 27 Aug 2019 13:18:38 -0400 Subject: [PATCH 28/28] Add a test for clear_fn support in handling non-managed fields --- src/test/test_confmgr.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/test/test_confmgr.c b/src/test/test_confmgr.c index 170e8335e7..5f73d9754b 100644 --- a/src/test/test_confmgr.c +++ b/src/test/test_confmgr.c @@ -34,6 +34,8 @@ typedef struct { int cuteness; uint32_t magic; int eats_meat; /* deprecated; llamas are never carnivorous. */ + + char *description; // derived from other fields. } llama_cfg_t; typedef struct { @@ -87,6 +89,16 @@ static config_deprecation_t alpaca_deprecations[] = { {NULL,NULL} }; +static int clear_llama_cfg_called = 0; +static void +clear_llama_cfg(const config_mgr_t *mgr, void *llamacfg) +{ + (void)mgr; + llama_cfg_t *lc = llamacfg; + tor_free(lc->description); + ++clear_llama_cfg_called; +} + static config_abbrev_t llama_abbrevs[] = { { "gracia", "cuteness", 0, 0 }, { "gentillesse", "cuteness", 0, 0 }, @@ -115,6 +127,7 @@ static const config_format_t llama_fmt = { .config_suite_offset = -1, .deprecations = llama_deprecations, .abbrevs = llama_abbrevs, + .clear_fn = clear_llama_cfg, }; static const config_format_t alpaca_fmt = { @@ -250,6 +263,14 @@ test_confmgr_parse(void *arg) tt_int_op(lc->cuteness, OP_EQ, 42); tt_int_op(ac->fuzziness, OP_EQ, 50); + // We set the description for the llama here, so that the clear function + // can clear it. (Later we can do this in a verification function.) + clear_llama_cfg_called = 0; + llama_cfg_t *mut_lc = config_mgr_get_obj_mutable(mgr, p, LLAMA_IDX); + mut_lc->description = tor_strdup("A llama named Hugo."); + config_free(mgr, p); + tt_int_op(clear_llama_cfg_called, OP_EQ, 1); + done: config_free_lines(lines); config_free(mgr, p);