From e16b90b88a76fb82702ae26c54834aca6c591d64 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 18 Jun 2019 19:16:57 -0400 Subject: [PATCH 1/9] Partially port routerset to being a full-fledged config type again. --- src/app/config/confparse.c | 27 +++----------- src/feature/nodelist/routerset.c | 62 ++++++++++++++++++++++++++++++++ src/feature/nodelist/routerset.h | 3 ++ 3 files changed, 70 insertions(+), 22 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index bc2ab24e4f..a02aa26e82 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -175,18 +175,8 @@ config_assign_value(const config_format_t *fmt, void *options, if (var->type == CONFIG_TYPE_ROUTERSET) { // XXXX make the backend extensible so that we don't have to - // XXXX handle ROUTERSET specially. - - if (*(routerset_t**)lvalue) { - routerset_free(*(routerset_t**)lvalue); - } - *(routerset_t**)lvalue = routerset_new(); - if (routerset_parse(*(routerset_t**)lvalue, c->value, c->key)<0) { - tor_asprintf(msg, "Invalid exit list '%s' for option '%s'", - c->value, c->key); - return -1; - } - return 0; + // XXXX special-case this type. + return typed_var_kvassign_ex(lvalue, c, msg, &routerset_type_defn); } return typed_var_kvassign(lvalue, c, msg, var->type); @@ -377,10 +367,8 @@ config_get_assigned_option(const config_format_t *fmt, const void *options, if (var->type == CONFIG_TYPE_ROUTERSET) { // XXXX make the backend extensible so that we don't have to - // XXXX handle ROUTERSET specially. - result = tor_malloc_zero(sizeof(config_line_t)); - result->key = tor_strdup(var->name); - result->value = routerset_to_string(*(routerset_t**)value); + // XXXX special-case this type. + result = typed_var_kvencode_ex(var->name, value, &routerset_type_defn); } else { result = typed_var_kvencode(var->name, value, var->type); } @@ -512,12 +500,7 @@ config_clear(const config_format_t *fmt, void *options, void *lvalue = STRUCT_VAR_P(options, var->var_offset); (void)fmt; /* unused */ if (var->type == CONFIG_TYPE_ROUTERSET) { - // XXXX make the backend extensible so that we don't have to - // XXXX handle ROUTERSET specially. - if (*(routerset_t**)lvalue) { - routerset_free(*(routerset_t**)lvalue); - *(routerset_t**)lvalue = NULL; - } + typed_var_free_ex(lvalue, &routerset_type_defn); return; } diff --git a/src/feature/nodelist/routerset.c b/src/feature/nodelist/routerset.c index e801fd81b1..ad42e8e101 100644 --- a/src/feature/nodelist/routerset.c +++ b/src/feature/nodelist/routerset.c @@ -34,6 +34,9 @@ n * Copyright (c) 2001-2004, Roger Dingledine. #include "feature/nodelist/nickname.h" #include "feature/nodelist/nodelist.h" #include "feature/nodelist/routerset.h" +#include "lib/conf/conftypes.h" +#include "lib/confmgt/typedvar.h" +#include "lib/encoding/confline.h" #include "lib/geoip/geoip.h" #include "core/or/addr_policy_st.h" @@ -41,6 +44,7 @@ n * Copyright (c) 2001-2004, Roger Dingledine. #include "feature/nodelist/node_st.h" #include "feature/nodelist/routerinfo_st.h" #include "feature/nodelist/routerstatus_st.h" +#include "lib/confmgt/var_type_def_st.h" /** Return a new empty routerset. */ routerset_t * @@ -461,3 +465,61 @@ routerset_free_(routerset_t *routerset) bitarray_free(routerset->countries); tor_free(routerset); } + +static int +routerset_kv_parse(void *target, const config_line_t *line, char **errmsg, + const void *params) +{ + (void)params; + routerset_t **p = (routerset_t**)target; + routerset_free(*p); // clear the old value, if any. + routerset_t *rs = routerset_new(); + if (routerset_parse(rs, line->value, line->key) < 0) { + routerset_free(rs); + *errmsg = tor_strdup("Invalid router list."); + return -1; + } else { + *p = rs; + return 0; + } +} + +static char * +routerset_encode(const void *value, const void *params) +{ + (void)params; + const routerset_t **p = (const routerset_t**)value; + return routerset_to_string(*p); +} + +static void +routerset_clear(void *value, const void *params) +{ + (void)params; + routerset_t **p = (routerset_t**)value; + routerset_free(*p); // sets *p to NULL. +} + +static int +routerset_copy(void *dest, const void *src, const void *params) +{ + (void)params; + routerset_t **output = (routerset_t**)dest; + const routerset_t *input = *(routerset_t**)src; + routerset_free(*output); // sets *output to NULL + *output = routerset_new(); + routerset_union(*output, input); + return 0; +} + +static const var_type_fns_t routerset_type_fns = { + .kv_parse = routerset_kv_parse, + .encode = routerset_encode, + .clear = routerset_clear, + .copy = routerset_copy +}; + +const var_type_def_t routerset_type_defn = { + .name = "RouterList", + .fns = &routerset_type_fns +}; diff --git a/src/feature/nodelist/routerset.h b/src/feature/nodelist/routerset.h index ca8b6fed93..9d184c9852 100644 --- a/src/feature/nodelist/routerset.h +++ b/src/feature/nodelist/routerset.h @@ -44,6 +44,9 @@ void routerset_free_(routerset_t *routerset); #define routerset_free(rs) FREE_AND_NULL(routerset_t, routerset_free_, (rs)) int routerset_len(const routerset_t *set); +struct var_type_def_t; +extern const struct var_type_def_t routerset_type_defn; + #ifdef ROUTERSET_PRIVATE #include "lib/container/bitarray.h" From 2da188667d37757ae999c8ab24ed35b64e08700c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 18 Jun 2019 20:40:11 -0400 Subject: [PATCH 2/9] Add new "struct_var_" functions to manipulate struct fields. These functions exist one level higher than typed_var_t. They describe a type, a name, and an offset within a structure. --- src/lib/conf/conftypes.h | 40 ++++++++ src/lib/confmgt/include.am | 2 + src/lib/confmgt/structvar.c | 200 ++++++++++++++++++++++++++++++++++++ src/lib/confmgt/structvar.h | 51 +++++++++ 4 files changed, 293 insertions(+) create mode 100644 src/lib/confmgt/structvar.c create mode 100644 src/lib/confmgt/structvar.h diff --git a/src/lib/conf/conftypes.h b/src/lib/conf/conftypes.h index e66ab3d5aa..cddfeff2fd 100644 --- a/src/lib/conf/conftypes.h +++ b/src/lib/conf/conftypes.h @@ -63,8 +63,48 @@ typedef enum config_type_t { CONFIG_TYPE_ROUTERSET, /**< A list of router names, addrs, and fps, * parsed into a routerset_t. */ CONFIG_TYPE_OBSOLETE, /**< Obsolete (ignored) option. */ + CONFIG_TYPE_EXTENDED, /**< Extended type; definition will appear in + * pointer. */ } config_type_t; +/* Forward delcaration for var_type_def_t, for extended types. */ +struct var_type_def_t; + +/** Structure to specify a named, typed member within a structure. */ +typedef struct struct_member_t { + /** Name of the field. */ + const char *name; + /** Type of the field, according to the config_type_t enumeration. + * + * This value is CONFIG_TYPE_EXTENDED for any type not listed in + * config_type_t. + **/ + config_type_t type; + /** + * Pointer to a type definition for the type of this field. Overrides + * type if not NULL. + **/ + const struct var_type_def_t *type_def; + /** + * Offset of this field within the structure. Compute this with + * offsetof(structure, fieldname). + **/ + int offset; +} struct_member_t; + +/** + * Structure to describe the location and preferred value of a "magic number" + * field within a structure. + * + * These 'magic numbers' are 32-bit values used to tag objects to make sure + * that they have the correct type. + */ +typedef struct struct_magic_decl_t { + const char *typename; + uint32_t magic_val; + int magic_offset; +} struct_magic_decl_t; + #ifdef TOR_UNIT_TESTS /** * Union used when building in test mode typechecking the members of a type diff --git a/src/lib/confmgt/include.am b/src/lib/confmgt/include.am index a2c7649957..aa5b37fdb5 100644 --- a/src/lib/confmgt/include.am +++ b/src/lib/confmgt/include.am @@ -6,6 +6,7 @@ endif # ADD_C_FILE: INSERT SOURCES HERE. src_lib_libtor_confmgt_a_SOURCES = \ + src/lib/confmgt/structvar.c \ src/lib/confmgt/type_defs.c \ src/lib/confmgt/typedvar.c \ src/lib/confmgt/unitparse.c @@ -17,6 +18,7 @@ src_lib_libtor_confmgt_testing_a_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) # ADD_C_FILE: INSERT HEADERS HERE. noinst_HEADERS += \ + src/lib/confmgt/structvar.h \ src/lib/confmgt/type_defs.h \ src/lib/confmgt/typedvar.h \ src/lib/confmgt/unitparse.h \ diff --git a/src/lib/confmgt/structvar.c b/src/lib/confmgt/structvar.c new file mode 100644 index 0000000000..7ea00fbde9 --- /dev/null +++ b/src/lib/confmgt/structvar.c @@ -0,0 +1,200 @@ +/* Copyright (c) 2001 Matej Pfajfar. + * 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 */ + +/** + * @file structvar.c + * @brief Functions to manipulate named and typed elements of + * a structure. + * + * These functions represent a low-level API for accessing a member of a + * structure. They use typedvar.c to work, and they are used in turn by the + * configuration system to examine and set fields in configuration objects + * used by individual modules. + * + * Almost no code should call these directly. + **/ + +#include "orconfig.h" +#include "lib/confmgt/structvar.h" +#include "lib/cc/compat_compiler.h" +#include "lib/conf/conftypes.h" +#include "lib/confmgt/type_defs.h" +#include "lib/confmgt/typedvar.h" +#include "lib/log/util_bug.h" + +#include + +/** + * Set the 'magic number' on object to correspond to decl. + **/ +void +struct_set_magic(void *object, const struct_magic_decl_t *decl) +{ + tor_assert(object); + tor_assert(decl); + uint32_t *ptr = STRUCT_VAR_P(object, decl->magic_offset); + *ptr = decl->magic_val; +} + +/** + * Assert that the 'magic number' on object to corresponds to decl. + **/ +void +struct_check_magic(const void *object, const struct_magic_decl_t *decl) +{ + tor_assert(object); + tor_assert(decl); + + const uint32_t *ptr = STRUCT_VAR_P(object, decl->magic_offset); + tor_assertf(*ptr == decl->magic_val, + "Bad magic number on purported %s object. " + "Expected %"PRIu32"x but got "PRIu32"x.", + decl->magic_val, *ptr); +} + +/** + * Return a mutable pointer to the member of object described + * by member. + **/ +void * +struct_get_mptr(void *object, const struct_member_t *member) +{ + tor_assert(object); + return STRUCT_VAR_P(object, member->offset); +} + +/** + * Return a const pointer to the member of object described + * by member. + **/ +const void * +struct_get_ptr(const void *object, const struct_member_t *member) +{ + tor_assert(object); + return STRUCT_VAR_P(object, member->offset); +} + +/** + * Helper: given a struct_member_t, look up the type definition for its + * variable. + */ +static const var_type_def_t * +get_type_def(const struct_member_t *member) +{ + if (member->type_def) + return member->type_def; + + return lookup_type_def(member->type); +} + +/** + * (As typed_var_assign, but assign a value to the member of object + * defined by member.) + **/ +int +struct_var_assign(void *object, const char *value, char **errmsg, + const struct_member_t *member) +{ + void *p = struct_get_mptr(object, member); + const var_type_def_t *def = get_type_def(member); + + return typed_var_assign_ex(p, value, errmsg, def); +} + +/** + * (As typed_var_free, but free and clear the member of object defined + * by member.) + **/ +void +struct_var_free(void *object, const struct_member_t *member) +{ + void *p = struct_get_mptr(object, member); + const var_type_def_t *def = get_type_def(member); + + typed_var_free_ex(p, def); +} + +/** + * (As typed_var_encode, but encode the member of object defined + * by member.) + **/ +char * +struct_var_encode(const void *object, const struct_member_t *member) +{ + const void *p = struct_get_ptr(object, member); + const var_type_def_t *def = get_type_def(member); + + return typed_var_encode_ex(p, def); +} + +/** + * (As typed_var_copy, but copy from src to dest the member + * defined by member.) + **/ +int +struct_var_copy(void *dest, const void *src, const struct_member_t *member) +{ + void *p_dest = struct_get_mptr(dest, member); + const void *p_src = struct_get_ptr(src, member); + const var_type_def_t *def = get_type_def(member); + + return typed_var_copy_ex(p_dest, p_src, def); +} + +/** + * (As typed_var_eq, but compare the members of a and b + * defined by member.) + **/ +bool +struct_var_eq(const void *a, const void *b, const struct_member_t *member) +{ + const void *p_a = struct_get_ptr(a, member); + const void *p_b = struct_get_ptr(b, member); + const var_type_def_t *def = get_type_def(member); + + return typed_var_eq_ex(p_a, p_b, def); +} + +/** + * (As typed_var_ok, but validate the member of object defined by + * member.) + **/ +bool +struct_var_ok(const void *object, const struct_member_t *member) +{ + const void *p = struct_get_ptr(object, member); + const var_type_def_t *def = get_type_def(member); + + return typed_var_ok_ex(p, def); +} + +/** + * (As typed_var_kvassign, but assign a value to the member of object + * defined by member.) + **/ +int +struct_var_kvassign(void *object, const struct config_line_t *line, + char **errmsg, + const struct_member_t *member) +{ + void *p = struct_get_mptr(object, member); + const var_type_def_t *def = get_type_def(member); + + return typed_var_kvassign_ex(p, line, errmsg, def); +} + +/** + * (As typed_var_kvencode, but encode the value of the member of object + * defined by member.) + **/ +struct config_line_t * +struct_var_kvencode(const void *object, const struct_member_t *member) +{ + const void *p = struct_get_ptr(object, member); + const var_type_def_t *def = get_type_def(member); + + return typed_var_kvencode_ex(member->name, p, def); +} diff --git a/src/lib/confmgt/structvar.h b/src/lib/confmgt/structvar.h new file mode 100644 index 0000000000..894098e509 --- /dev/null +++ b/src/lib/confmgt/structvar.h @@ -0,0 +1,51 @@ +/* Copyright (c) 2001 Matej Pfajfar. + * 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 */ + +/** + * @file structvar.h + * @brief Header for lib/confmgt/structvar.c + **/ + +#ifndef TOR_LIB_CONFMGT_STRUCTVAR_H +#define TOR_LIB_CONFMGT_STRUCTVAR_H + +struct struct_magic_decl_t; +struct struct_member_t; +struct config_line_t; + +#include + +void struct_set_magic(void *object, + const struct struct_magic_decl_t *decl); +void struct_check_magic(const void *object, + const struct struct_magic_decl_t *decl); + +void *struct_get_mptr(void *object, + const struct struct_member_t *member); +const void *struct_get_ptr(const void *object, + const struct struct_member_t *member); + +int struct_var_assign(void *object, const char *value, char **errmsg, + const struct struct_member_t *member); +void struct_var_free(void *object, + const struct struct_member_t *member); +char *struct_var_encode(const void *object, + const struct struct_member_t *member); +int struct_var_copy(void *dest, const void *src, + const struct struct_member_t *member); +bool struct_var_eq(const void *a, const void *b, + const struct struct_member_t *member); +bool struct_var_ok(const void *object, + const struct struct_member_t *member); + +int struct_var_kvassign(void *object, const struct config_line_t *line, + char **errmsg, + const struct struct_member_t *member); +struct config_line_t *struct_var_kvencode( + const void *object, + const struct struct_member_t *member); + +#endif /* !defined(TOR_LIB_CONFMGT_STRUCTVAR_H) */ From 3a4d67cf45dc1c94a33479c852a3dd7cbd4ebc95 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 18 Jun 2019 20:40:59 -0400 Subject: [PATCH 3/9] Port confparse to use struct_var in place of typed_var. This requires changes to config_var_t, causing corresponding changes throughout its users. --- src/app/config/config.c | 105 +++++++++++++-------- src/app/config/confparse.c | 110 +++++++++------------- src/app/config/confparse.h | 14 ++- src/app/config/statefile.c | 13 ++- src/feature/dirauth/shared_random_state.c | 16 ++-- src/feature/nodelist/routerset.c | 2 +- src/feature/nodelist/routerset.h | 2 +- src/test/test_confparse.c | 25 +++-- 8 files changed, 153 insertions(+), 134 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 7908007051..074df07057 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -253,23 +253,45 @@ static config_abbrev_t option_abbrevs_[] = { * members with CONF_CHECK_VAR_TYPE. */ DUMMY_TYPECHECK_INSTANCE(or_options_t); -/** An entry for config_vars: "The option name has type +/** An entry for config_vars: "The option varname has type * CONFIG_TYPE_conftype, and corresponds to * or_options_t.member" */ -#define VAR(name,conftype,member,initvalue) \ - { name, CONFIG_TYPE_ ## conftype, offsetof(or_options_t, member), \ +#define VAR(varname,conftype,member,initvalue) \ + { { .name = varname, \ + .type = CONFIG_TYPE_ ## conftype, \ + .offset = offsetof(or_options_t, member), \ + }, \ initvalue CONF_TEST_MEMBERS(or_options_t, conftype, member) } -/** As VAR, but the option name and member name are the same. */ -#define V(member,conftype,initvalue) \ - VAR(#member, conftype, member, initvalue) -/** An entry for config_vars: "The option name is obsolete." */ + #ifdef TOR_UNIT_TESTS -#define OBSOLETE(name) { name, CONFIG_TYPE_OBSOLETE, 0, NULL, {.INT=NULL} } +#define DUMMY_TEST_MEMBERS , {.INT=NULL} #else -#define OBSOLETE(name) { name, CONFIG_TYPE_OBSOLETE, 0, NULL } +#define DUMMY_TEST_MEMBERS #endif +/* As VAR, but uses a type definition in addition to a type enum. */ +#define VAR_D(varname,conftype,member,initvalue) \ + { { .name = varname, \ + .type = CONFIG_TYPE_ ## conftype, \ + .type_def = &conftype ## _type_defn, \ + .offset = offsetof(or_options_t, member), \ + }, \ + initvalue DUMMY_TEST_MEMBERS } + +/** As VAR, but the option name and member name are the same. */ +#define V(member,conftype,initvalue) \ + VAR(#member, conftype, member, initvalue) + +/** As V, but uses a type definition instead of a type enum */ +#define V_D(member,type,initvalue) \ + VAR_D(#member, type, member, initvalue) + +/** An entry for config_vars: "The option varname is obsolete." */ +#define OBSOLETE(varname) \ + { { .name = varname, .type = CONFIG_TYPE_OBSOLETE, }, NULL \ + DUMMY_TEST_MEMBERS } + /** * Macro to declare *Port options. Each one comes in three entries. * For example, most users should use "SocksPort" to configure the @@ -418,17 +440,17 @@ static config_var_t option_vars_[] = { V(TestingEnableCellStatsEvent, BOOL, "0"), OBSOLETE("TestingEnableTbEmptyEvent"), V(EnforceDistinctSubnets, BOOL, "1"), - V(EntryNodes, ROUTERSET, NULL), + V_D(EntryNodes, ROUTERSET, NULL), V(EntryStatistics, BOOL, "0"), V(TestingEstimatedDescriptorPropagationTime, INTERVAL, "10 minutes"), - V(ExcludeNodes, ROUTERSET, NULL), - V(ExcludeExitNodes, ROUTERSET, NULL), + V_D(ExcludeNodes, ROUTERSET, NULL), + V_D(ExcludeExitNodes, ROUTERSET, NULL), OBSOLETE("ExcludeSingleHopRelays"), - V(ExitNodes, ROUTERSET, NULL), + V_D(ExitNodes, ROUTERSET, NULL), /* Researchers need a way to tell their clients to use specific * middles that they also control, to allow safe live-network * experimentation with new padding machines. */ - V(MiddleNodes, ROUTERSET, NULL), + V_D(MiddleNodes, ROUTERSET, NULL), V(ExitPolicy, LINELIST, NULL), V(ExitPolicyRejectPrivate, BOOL, "1"), V(ExitPolicyRejectLocalInterfaces, BOOL, "0"), @@ -507,8 +529,8 @@ static config_var_t option_vars_[] = { V(Socks5ProxyPassword, STRING, NULL), VAR("KeyDirectory", FILENAME, KeyDirectory_option, NULL), V(KeyDirectoryGroupReadable, BOOL, "0"), - VAR("HSLayer2Nodes", ROUTERSET, HSLayer2Nodes, NULL), - VAR("HSLayer3Nodes", ROUTERSET, HSLayer3Nodes, NULL), + VAR_D("HSLayer2Nodes", ROUTERSET, HSLayer2Nodes, NULL), + VAR_D("HSLayer3Nodes", ROUTERSET, HSLayer3Nodes, NULL), V(KeepalivePeriod, INTERVAL, "5 minutes"), V(KeepBindCapabilities, AUTOBOOL, "auto"), VAR("Log", LINELIST, Logs, NULL), @@ -732,11 +754,11 @@ static config_var_t option_vars_[] = { OBSOLETE("TestingDescriptorMaxDownloadTries"), OBSOLETE("TestingMicrodescMaxDownloadTries"), OBSOLETE("TestingCertMaxDownloadTries"), - V(TestingDirAuthVoteExit, ROUTERSET, NULL), + V_D(TestingDirAuthVoteExit, ROUTERSET, NULL), V(TestingDirAuthVoteExitIsStrict, BOOL, "0"), - V(TestingDirAuthVoteGuard, ROUTERSET, NULL), + V_D(TestingDirAuthVoteGuard, ROUTERSET, NULL), V(TestingDirAuthVoteGuardIsStrict, BOOL, "0"), - V(TestingDirAuthVoteHSDir, ROUTERSET, NULL), + V_D(TestingDirAuthVoteHSDir, ROUTERSET, NULL), V(TestingDirAuthVoteHSDirIsStrict, BOOL, "0"), VAR("___UsingTestNetworkDefaults", BOOL, UsingTestNetworkDefaults_, "0"), @@ -949,11 +971,11 @@ set_options(or_options_t *new_val, char **msg) * 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].name; ++i) { + for (i=0; options_format.vars[i].member.name; ++i) { const config_var_t *var = &options_format.vars[i]; - const char *var_name = var->name; - if (var->type == CONFIG_TYPE_LINELIST_S || - var->type == CONFIG_TYPE_OBSOLETE) { + const char *var_name = var->member.name; + if (var->member.type == CONFIG_TYPE_LINELIST_S || + var->member.type == CONFIG_TYPE_OBSOLETE) { continue; } if (!config_is_same(&options_format, new_val, old_options, var_name)) { @@ -969,7 +991,7 @@ set_options(or_options_t *new_val, char **msg) tor_free(line); } } else { - smartlist_add_strdup(elements, options_format.vars[i].name); + smartlist_add_strdup(elements, options_format.vars[i].member.name); smartlist_add(elements, NULL); } } @@ -2571,7 +2593,7 @@ const char * option_get_canonical_name(const char *key) { const config_var_t *var = config_find_option(&options_format, key); - return var ? var->name : NULL; + return var ? var->member.name : NULL; } /** Return a canonical list of the options assigned for key. @@ -2653,12 +2675,12 @@ static void list_torrc_options(void) { int i; - for (i = 0; option_vars_[i].name; ++i) { + for (i = 0; option_vars_[i].member.name; ++i) { const config_var_t *var = &option_vars_[i]; - if (var->type == CONFIG_TYPE_OBSOLETE || - var->type == CONFIG_TYPE_LINELIST_V) + if (var->member.type == CONFIG_TYPE_OBSOLETE || + var->member.type == CONFIG_TYPE_LINELIST_V) continue; - printf("%s\n", var->name); + printf("%s\n", var->member.name); } } @@ -5445,18 +5467,18 @@ options_init_from_string(const char *cf_defaults, const char *cf, * let's clean it up. -NM */ /* Change defaults. */ - for (int i = 0; testing_tor_network_defaults[i].name; ++i) { + for (int i = 0; testing_tor_network_defaults[i].member.name; ++i) { const config_var_t *new_var = &testing_tor_network_defaults[i]; config_var_t *old_var = - config_find_option_mutable(&options_format, new_var->name); + config_find_option_mutable(&options_format, new_var->member.name); tor_assert(new_var); tor_assert(old_var); old_var->initvalue = new_var->initvalue; - if ((config_find_deprecation(&options_format, new_var->name))) { + if ((config_find_deprecation(&options_format, new_var->member.name))) { log_warn(LD_GENERAL, "Testing options override the deprecated " "option %s. Is that intentional?", - new_var->name); + new_var->member.name); } } @@ -8168,13 +8190,13 @@ getinfo_helper_config(control_connection_t *conn, if (!strcmp(question, "config/names")) { smartlist_t *sl = smartlist_new(); int i; - for (i = 0; option_vars_[i].name; ++i) { + for (i = 0; option_vars_[i].member.name; ++i) { const config_var_t *var = &option_vars_[i]; const char *type; /* don't tell controller about triple-underscore options */ - if (!strncmp(option_vars_[i].name, "___", 3)) + if (!strncmp(option_vars_[i].member.name, "___", 3)) continue; - switch (var->type) { + switch (var->member.type) { case CONFIG_TYPE_STRING: type = "String"; break; case CONFIG_TYPE_FILENAME: type = "Filename"; break; case CONFIG_TYPE_POSINT: type = "Integer"; break; @@ -8198,11 +8220,12 @@ getinfo_helper_config(control_connection_t *conn, case CONFIG_TYPE_LINELIST_V: type = "Virtual"; break; default: case CONFIG_TYPE_OBSOLETE: + case CONFIG_TYPE_EXTENDED: type = NULL; break; } if (!type) continue; - smartlist_add_asprintf(sl, "%s %s\n",var->name,type); + smartlist_add_asprintf(sl, "%s %s\n",var->member.name,type); } *answer = smartlist_join_strings(sl, "", 0, NULL); SMARTLIST_FOREACH(sl, char *, c, tor_free(c)); @@ -8210,17 +8233,17 @@ getinfo_helper_config(control_connection_t *conn, } 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].name; ++i) { + for (int i = 0; option_vars_[i].member.name; ++i) { const config_var_t *var = &option_vars_[i]; if (var->initvalue != NULL) { - if (strcmp(option_vars_[i].name, "DirAuthority") == 0) { + if (strcmp(option_vars_[i].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].name, "FallbackDir") == 0) { + if (strcmp(option_vars_[i].member.name, "FallbackDir") == 0) { /* * Similarly count fallback lines, so that we can decided later * to add the defaults manually. @@ -8228,7 +8251,7 @@ getinfo_helper_config(control_connection_t *conn, ++fallback_lines_seen; } char *val = esc_for_log(var->initvalue); - smartlist_add_asprintf(sl, "%s %s\n",var->name,val); + smartlist_add_asprintf(sl, "%s %s\n",var->member.name,val); tor_free(val); } } diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index a02aa26e82..be4341e3f6 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -29,8 +29,7 @@ #include "lib/confmgt/unitparse.h" #include "lib/container/bitarray.h" #include "lib/encoding/confline.h" - -#include "lib/confmgt/typedvar.h" +#include "lib/confmgt/structvar.h" static void config_reset(const config_format_t *fmt, void *options, const config_var_t *var, int use_defaults); @@ -110,17 +109,17 @@ config_find_option_mutable(config_format_t *fmt, const char *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].name; ++i) { - if (!strcasecmp(key, fmt->vars[i].name)) { + 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].name; ++i) { - if (!strncasecmp(key, fmt->vars[i].name, keylen)) { + 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].name); + key, fmt->vars[i].member.name); return &fmt->vars[i]; } } @@ -144,7 +143,7 @@ static int config_count_options(const config_format_t *fmt) { int i; - for (i=0; fmt->vars[i].name; ++i) + for (i=0; fmt->vars[i].member.name; ++i) ; return i; } @@ -163,23 +162,14 @@ config_assign_value(const config_format_t *fmt, void *options, config_line_t *c, char **msg) { const config_var_t *var; - void *lvalue; CONFIG_CHECK(fmt, options); var = config_find_option(fmt, c->key); tor_assert(var); - tor_assert(!strcmp(c->key, var->name)); + tor_assert(!strcmp(c->key, var->member.name)); - lvalue = STRUCT_VAR_P(options, var->var_offset); - - if (var->type == CONFIG_TYPE_ROUTERSET) { - // XXXX make the backend extensible so that we don't have to - // XXXX special-case this type. - return typed_var_kvassign_ex(lvalue, c, msg, &routerset_type_defn); - } - - return typed_var_kvassign(lvalue, c, msg, var->type); + return struct_var_kvassign(options, c, msg, &var->member); } /** Mark every linelist in options "fragile", so that fresh assignments @@ -191,14 +181,14 @@ config_mark_lists_fragile(const config_format_t *fmt, void *options) tor_assert(fmt); tor_assert(options); - for (i = 0; fmt->vars[i].name; ++i) { + for (i = 0; fmt->vars[i].member.name; ++i) { const config_var_t *var = &fmt->vars[i]; config_line_t *list; - if (var->type != CONFIG_TYPE_LINELIST && - var->type != CONFIG_TYPE_LINELIST_V) + if (var->member.type != CONFIG_TYPE_LINELIST && + var->member.type != CONFIG_TYPE_LINELIST_V) continue; - list = *(config_line_t **)STRUCT_VAR_P(options, var->var_offset); + list = *(config_line_t **)STRUCT_VAR_P(options, var->member.offset); if (list) list->fragile = 1; } @@ -238,7 +228,7 @@ config_assign_line(const config_format_t *fmt, void *options, var = config_find_option(fmt, c->key); if (!var) { if (fmt->extra) { - void *lvalue = STRUCT_VAR_P(options, fmt->extra->var_offset); + void *lvalue = STRUCT_VAR_P(options, fmt->extra->offset); log_info(LD_CONFIG, "Found unrecognized option '%s'; saving it.", c->key); config_line_append((config_line_t**)lvalue, c->key, c->value); @@ -251,22 +241,22 @@ config_assign_line(const config_format_t *fmt, void *options, } /* Put keyword into canonical case. */ - if (strcmp(var->name, c->key)) { + if (strcmp(var->member.name, c->key)) { tor_free(c->key); - c->key = tor_strdup(var->name); + c->key = tor_strdup(var->member.name); } const char *deprecation_msg; if (warn_deprecations && - (deprecation_msg = config_find_deprecation(fmt, var->name))) { - warn_deprecated_option(var->name, deprecation_msg); + (deprecation_msg = config_find_deprecation(fmt, var->member.name))) { + warn_deprecated_option(var->member.name, deprecation_msg); } if (!strlen(c->value)) { /* reset or clear it, then return */ if (!clear_first) { - if ((var->type == CONFIG_TYPE_LINELIST || - var->type == CONFIG_TYPE_LINELIST_S) && + if ((var->member.type == CONFIG_TYPE_LINELIST || + var->member.type == CONFIG_TYPE_LINELIST_S) && 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. */ @@ -283,14 +273,14 @@ config_assign_line(const config_format_t *fmt, void *options, config_reset(fmt, options, var, use_defaults); // LCOV_EXCL_LINE } - if (options_seen && (var->type != CONFIG_TYPE_LINELIST && - var->type != CONFIG_TYPE_LINELIST_S)) { + if (options_seen && (var->member.type != CONFIG_TYPE_LINELIST && + var->member.type != CONFIG_TYPE_LINELIST_S)) { /* 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); 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->name); + "value will be ignored.", var->member.name); } bitarray_set(options_seen, var_index); } @@ -352,7 +342,6 @@ config_get_assigned_option(const config_format_t *fmt, const void *options, const char *key, int escape_val) { const config_var_t *var; - const void *value; config_line_t *result; tor_assert(options && key); @@ -363,15 +352,8 @@ config_get_assigned_option(const config_format_t *fmt, const void *options, log_warn(LD_CONFIG, "Unknown option '%s'. Failing.", key); return NULL; } - value = STRUCT_VAR_P(options, var->var_offset); - if (var->type == CONFIG_TYPE_ROUTERSET) { - // XXXX make the backend extensible so that we don't have to - // XXXX special-case this type. - result = typed_var_kvencode_ex(var->name, value, &routerset_type_defn); - } else { - result = typed_var_kvencode(var->name, value, var->type); - } + result = struct_var_kvencode(options, &var->member); if (escape_val) { config_line_t *line; @@ -497,14 +479,10 @@ static void config_clear(const config_format_t *fmt, void *options, const config_var_t *var) { - void *lvalue = STRUCT_VAR_P(options, var->var_offset); - (void)fmt; /* unused */ - if (var->type == CONFIG_TYPE_ROUTERSET) { - typed_var_free_ex(lvalue, &routerset_type_defn); - return; - } - typed_var_free(lvalue, var->type); + (void)fmt; /* unused */ + + struct_var_free(options, &var->member); } /** Clear the option indexed by var in options. Then if @@ -522,7 +500,7 @@ config_reset(const config_format_t *fmt, void *options, return; /* all done */ if (var->initvalue) { c = tor_malloc_zero(sizeof(config_line_t)); - c->key = tor_strdup(var->name); + c->key = tor_strdup(var->member.name); c->value = tor_strdup(var->initvalue); if (config_assign_value(fmt, options, c, &msg) < 0) { // LCOV_EXCL_START @@ -545,10 +523,11 @@ config_free_(const config_format_t *fmt, void *options) tor_assert(fmt); - for (i=0; fmt->vars[i].name; ++i) + for (i=0; fmt->vars[i].member.name; ++i) config_clear(fmt, options, &(fmt->vars[i])); + if (fmt->extra) { - config_line_t **linep = STRUCT_VAR_P(options, fmt->extra->var_offset); + config_line_t **linep = STRUCT_VAR_P(options, fmt->extra->offset); config_free_lines(*linep); *linep = NULL; } @@ -585,12 +564,12 @@ config_dup(const config_format_t *fmt, const void *old) config_line_t *line; newopts = config_new(fmt); - for (i=0; fmt->vars[i].name; ++i) { - if (fmt->vars[i].type == CONFIG_TYPE_LINELIST_S) + for (i=0; fmt->vars[i].member.name; ++i) { + if (fmt->vars[i].member.type == CONFIG_TYPE_LINELIST_S) continue; - if (fmt->vars[i].type == CONFIG_TYPE_OBSOLETE) + if (fmt->vars[i].member.type == CONFIG_TYPE_OBSOLETE) continue; - line = config_get_assigned_option(fmt, old, fmt->vars[i].name, 0); + line = config_get_assigned_option(fmt, old, fmt->vars[i].member.name, 0); if (line) { char *msg = NULL; if (config_assign(fmt, newopts, line, 0, &msg) < 0) { @@ -615,7 +594,7 @@ config_init(const config_format_t *fmt, void *options) const config_var_t *var; CONFIG_CHECK(fmt, options); - for (i=0; fmt->vars[i].name; ++i) { + for (i=0; fmt->vars[i].member.name; ++i) { var = &fmt->vars[i]; if (!var->initvalue) continue; /* defaults to NULL or 0 */ @@ -657,22 +636,23 @@ config_dump(const config_format_t *fmt, const void *default_options, } elements = smartlist_new(); - for (i=0; fmt->vars[i].name; ++i) { + for (i=0; fmt->vars[i].member.name; ++i) { int comment_option = 0; - if (fmt->vars[i].type == CONFIG_TYPE_OBSOLETE || - fmt->vars[i].type == CONFIG_TYPE_LINELIST_S) + if (fmt->vars[i].member.type == CONFIG_TYPE_OBSOLETE || + fmt->vars[i].member.type == CONFIG_TYPE_LINELIST_S) continue; /* Don't save 'hidden' control variables. */ - if (!strcmpstart(fmt->vars[i].name, "__")) + if (!strcmpstart(fmt->vars[i].member.name, "__")) continue; - if (minimal && config_is_same(fmt, options, defaults, fmt->vars[i].name)) + if (minimal && config_is_same(fmt, options, defaults, + fmt->vars[i].member.name)) continue; else if (comment_defaults && - config_is_same(fmt, options, defaults, fmt->vars[i].name)) + config_is_same(fmt, options, defaults, fmt->vars[i].member.name)) comment_option = 1; line = assigned = - config_get_assigned_option(fmt, options, fmt->vars[i].name, 1); + config_get_assigned_option(fmt, options, fmt->vars[i].member.name, 1); for (; line; line = line->next) { if (!strcmpstart(line->key, "__")) { @@ -688,7 +668,7 @@ config_dump(const config_format_t *fmt, const void *default_options, } if (fmt->extra) { - line = *(config_line_t**)STRUCT_VAR_P(options, fmt->extra->var_offset); + line = *(config_line_t**)STRUCT_VAR_P(options, fmt->extra->offset); for (; line; line = line->next) { smartlist_add_asprintf(elements, "%s %s\n", line->key, line->value); } diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index bd06a4a0d0..5897085e63 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -34,10 +34,8 @@ typedef struct config_deprecation_t { /** A variable allowed in the configuration file or on the command line. */ typedef struct config_var_t { - const char *name; /**< The full keyword (case insensitive). */ - config_type_t type; /**< How to interpret the type and turn it into a - * value. */ - off_t var_offset; /**< Offset of the corresponding member of or_options_t. */ + struct_member_t member; /** A struct member corresponding to this + * variable. */ const char *initvalue; /**< String (or null) describing initial value. */ #ifdef TOR_UNIT_TESTS @@ -74,12 +72,12 @@ typedef struct config_var_t { #define CONF_TEST_MEMBERS(tp, conftype, member) \ , CONF_CHECK_VAR_TYPE(tp, conftype, member) #define END_OF_CONFIG_VARS \ - { NULL, CONFIG_TYPE_OBSOLETE, 0, NULL, { .INT=NULL } } + { { .name = NULL }, NULL, { .INT=NULL } } #define DUMMY_TYPECHECK_INSTANCE(tp) \ static tp tp ## _dummy #else /* !(defined(TOR_UNIT_TESTS)) */ #define CONF_TEST_MEMBERS(tp, conftype, member) -#define END_OF_CONFIG_VARS { NULL, CONFIG_TYPE_OBSOLETE, 0, NULL } +#define END_OF_CONFIG_VARS { { .name = NULL, }, NULL } /* Repeatedly declarable incomplete struct to absorb redundant semicolons */ #define DUMMY_TYPECHECK_INSTANCE(tp) \ struct tor_semicolon_eater @@ -108,9 +106,9 @@ 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. */ - /** If present, extra is a LINELIST variable for unrecognized + /** If present, extra denotes a LINELIST variable for unrecognized * lines. Otherwise, unrecognized lines are an error. */ - config_var_t *extra; + struct_member_t *extra; } config_format_t; /** Macro: assert that cfg has the right magic field for format diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index c6c5ec14f5..358b02f602 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -71,8 +71,10 @@ static config_abbrev_t state_abbrevs_[] = { DUMMY_TYPECHECK_INSTANCE(or_state_t); /*XXXX these next two are duplicates or near-duplicates from config.c */ -#define VAR(name,conftype,member,initvalue) \ - { name, CONFIG_TYPE_ ## conftype, offsetof(or_state_t, member), \ +#define VAR(varname,conftype,member,initvalue) \ + { { .name = varname, \ + .type = CONFIG_TYPE_ ## conftype, \ + .offset = offsetof(or_state_t, member), }, \ initvalue CONF_TEST_MEMBERS(or_state_t, conftype, member) } /** As VAR, but the option name and member name are the same. */ #define V(member,conftype,initvalue) \ @@ -155,9 +157,10 @@ static void or_state_free_cb(void *state); /** "Extra" variable in the state that receives lines we can't parse. This * lets us preserve options from versions of Tor newer than us. */ -static config_var_t state_extra_var = { - "__extra", CONFIG_TYPE_LINELIST, offsetof(or_state_t, ExtraLines), NULL - CONF_TEST_MEMBERS(or_state_t, LINELIST, ExtraLines) +static struct_member_t state_extra_var = { + .name = "__extra", + .type = CONFIG_TYPE_LINELIST, + .offset = offsetof(or_state_t, ExtraLines), }; /** Configuration format for or_state_t. */ diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index b2c7acba1a..cf4a654325 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -52,9 +52,11 @@ static const char dstate_cur_srv_key[] = "SharedRandCurrentValue"; DUMMY_TYPECHECK_INSTANCE(sr_disk_state_t); /* These next two are duplicates or near-duplicates from config.c */ -#define VAR(name, conftype, member, initvalue) \ - { name, CONFIG_TYPE_ ## conftype, offsetof(sr_disk_state_t, member), \ - initvalue CONF_TEST_MEMBERS(sr_disk_state_t, conftype, member) } +#define VAR(varname, conftype, member, initvalue) \ + { { .name = varname, \ + .type = CONFIG_TYPE_ ## conftype, \ + .offset = offsetof(sr_disk_state_t, member), }, \ + initvalue CONF_TEST_MEMBERS(sr_disk_state_t, conftype, member) } /* As VAR, but the option name and member name are the same. */ #define V(member, conftype, initvalue) \ VAR(#member, conftype, member, initvalue) @@ -83,10 +85,10 @@ static config_var_t state_vars[] = { /* "Extra" variable in the state that receives lines we can't parse. This * lets us preserve options from versions of Tor newer than us. */ -static config_var_t state_extra_var = { - "__extra", CONFIG_TYPE_LINELIST, - offsetof(sr_disk_state_t, ExtraLines), NULL - CONF_TEST_MEMBERS(sr_disk_state_t, LINELIST, ExtraLines) +static struct_member_t state_extra_var = { + .name = "__extra", + .type = CONFIG_TYPE_LINELIST, + .offset = offsetof(sr_disk_state_t, ExtraLines), }; /* Configuration format of sr_disk_state_t. */ diff --git a/src/feature/nodelist/routerset.c b/src/feature/nodelist/routerset.c index ad42e8e101..76777847ef 100644 --- a/src/feature/nodelist/routerset.c +++ b/src/feature/nodelist/routerset.c @@ -519,7 +519,7 @@ static const var_type_fns_t routerset_type_fns = { .copy = routerset_copy }; -const var_type_def_t routerset_type_defn = { +const var_type_def_t ROUTERSET_type_defn = { .name = "RouterList", .fns = &routerset_type_fns }; diff --git a/src/feature/nodelist/routerset.h b/src/feature/nodelist/routerset.h index 9d184c9852..f3bf4a1f7c 100644 --- a/src/feature/nodelist/routerset.h +++ b/src/feature/nodelist/routerset.h @@ -45,7 +45,7 @@ void routerset_free_(routerset_t *routerset); int routerset_len(const routerset_t *set); struct var_type_def_t; -extern const struct var_type_def_t routerset_type_defn; +extern const struct var_type_def_t ROUTERSET_type_defn; #ifdef ROUTERSET_PRIVATE #include "lib/container/bitarray.h" diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index dde61b1c81..27696a537a 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -48,15 +48,17 @@ typedef struct test_struct_t { static test_struct_t test_struct_t_dummy; -#define VAR(name,conftype,member,initvalue) \ - { name, CONFIG_TYPE_##conftype, offsetof(test_struct_t, member), \ +#define VAR(varname,conftype,member,initvalue) \ + { { .name = varname, \ + .type = CONFIG_TYPE_##conftype, \ + .offset = offsetof(test_struct_t, member), }, \ initvalue CONF_TEST_MEMBERS(test_struct_t, conftype, member) } #define V(name,conftype,initvalue) \ VAR( #name, conftype, name, initvalue ) -#define OBSOLETE(name) \ - { name, CONFIG_TYPE_OBSOLETE, 0, NULL, {.INT=NULL} } +#define OBSOLETE(varname) \ + { { .name=varname, .type=CONFIG_TYPE_OBSOLETE }, NULL, {.INT=NULL} } static config_var_t test_vars[] = { V(s, STRING, "hello"), @@ -79,7 +81,14 @@ static config_var_t test_vars[] = { VAR("LineTypeA", LINELIST_S, mixed_lines, NULL), VAR("LineTypeB", LINELIST_S, mixed_lines, NULL), OBSOLETE("obsolete"), - V(routerset, ROUTERSET, NULL), + { + { .name = "routerset", + .type = CONFIG_TYPE_ROUTERSET, + .type_def = &ROUTERSET_type_defn, + .offset = offsetof(test_struct_t, routerset), + }, + NULL, {.INT=NULL} + }, VAR("__HiddenInt", POSINT, hidden_int, "0"), VAR("MixedHiddenLines", LINELIST_V, mixed_hidden_lines, NULL), VAR("__HiddenLineA", LINELIST_S, mixed_hidden_lines, NULL), @@ -757,7 +766,11 @@ test_confparse_get_assigned(void *arg) /* Another variant, which accepts and stores unrecognized lines.*/ #define ETEST_MAGIC 13371337 -static config_var_t extra = VAR("__extra", LINELIST, extra_lines, NULL); +static struct_member_t extra = { + .name = "__extra", + .type = CONFIG_TYPE_LINELIST, + .offset = offsetof(test_struct_t, extra_lines), +}; static config_format_t etest_fmt = { sizeof(test_struct_t), From 59317c8a238f49ad298d4f51f42a0f7b16c9cc3c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Jun 2019 08:34:20 -0400 Subject: [PATCH 4/9] Use struct_magic_decl to verify magic numbers in config objects --- src/app/config/config.c | 7 +++++-- src/app/config/confparse.c | 2 +- src/app/config/confparse.h | 9 +++------ src/app/config/statefile.c | 7 +++++-- src/feature/dirauth/shared_random_state.c | 7 +++++-- src/test/test_confparse.c | 14 ++++++++++---- 6 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 074df07057..37cbe6b2ef 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -877,8 +877,11 @@ static void set_protocol_warning_severity_level(int warning_severity); /** Configuration format for or_options_t. */ STATIC config_format_t options_format = { sizeof(or_options_t), - OR_OPTIONS_MAGIC, - offsetof(or_options_t, magic_), + { + "or_options_t", + OR_OPTIONS_MAGIC, + offsetof(or_options_t, magic_), + }, option_abbrevs_, option_deprecation_notes_, option_vars_, diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index be4341e3f6..752d16c844 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -39,7 +39,7 @@ void * config_new(const config_format_t *fmt) { void *opts = tor_malloc_zero(fmt->size); - *(uint32_t*)STRUCT_VAR_P(opts, fmt->magic_offset) = fmt->magic; + struct_set_magic(opts, &fmt->magic); CONFIG_CHECK(fmt, opts); return opts; } diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 5897085e63..4ef4e708f3 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -96,9 +96,7 @@ typedef void (*free_cfg_fn_t)(void*); * configuration or storage format. */ typedef struct config_format_t { size_t size; /**< Size of the struct that everything gets parsed into. */ - uint32_t magic; /**< Required 'magic value' to make sure we have a struct - * of the right type. */ - off_t magic_offset; /**< Offset of the magic value within the struct. */ + struct_magic_decl_t magic; /**< Magic number info for this struct. */ config_abbrev_t *abbrevs; /**< List of abbreviations that we expand when * parsing this format. */ const config_deprecation_t *deprecations; /** List of deprecated options */ @@ -114,9 +112,8 @@ typedef struct config_format_t { /** Macro: assert that cfg has the right magic field for format * fmt. */ #define CONFIG_CHECK(fmt, cfg) STMT_BEGIN \ - tor_assert(fmt && cfg); \ - tor_assert((fmt)->magic == \ - *(uint32_t*)STRUCT_VAR_P(cfg,fmt->magic_offset)); \ + tor_assert(fmt); \ + struct_check_magic((cfg), &fmt->magic); \ STMT_END #define CAL_USE_DEFAULTS (1u<<0) diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index 358b02f602..331592c3a6 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -166,8 +166,11 @@ static struct_member_t state_extra_var = { /** Configuration format for or_state_t. */ static const config_format_t state_format = { sizeof(or_state_t), - OR_STATE_MAGIC, - offsetof(or_state_t, magic_), + { + "or_state_t", + OR_STATE_MAGIC, + offsetof(or_state_t, magic_), + }, state_abbrevs_, NULL, state_vars_, diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index cf4a654325..da4187b38a 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -94,8 +94,11 @@ static struct_member_t state_extra_var = { /* Configuration format of sr_disk_state_t. */ static const config_format_t state_format = { sizeof(sr_disk_state_t), - SR_DISK_STATE_MAGIC, - offsetof(sr_disk_state_t, magic_), + { + "sr_disk_state_t", + SR_DISK_STATE_MAGIC, + offsetof(sr_disk_state_t, magic_), + }, NULL, NULL, state_vars, diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 27696a537a..9e626356d3 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -131,8 +131,11 @@ static void test_free_cb(void *options); static config_format_t test_fmt = { sizeof(test_struct_t), - TEST_MAGIC, - offsetof(test_struct_t, magic), + { + "test_struct_t", + TEST_MAGIC, + offsetof(test_struct_t, magic), + }, test_abbrevs, test_deprecation_notes, test_vars, @@ -774,8 +777,11 @@ static struct_member_t extra = { static config_format_t etest_fmt = { sizeof(test_struct_t), - ETEST_MAGIC, - offsetof(test_struct_t, magic), + { + "test_struct_t (with extra lines)", + ETEST_MAGIC, + offsetof(test_struct_t, magic), + }, test_abbrevs, test_deprecation_notes, test_vars, From 53e969c137cb39bed432cd165d3d7e3825b1a2a9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Jun 2019 08:41:52 -0400 Subject: [PATCH 5/9] Use struct_var_{copy,eq} in confparse.c. --- src/app/config/confparse.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index 752d16c844..b9b5fddb96 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -542,17 +542,15 @@ config_is_same(const config_format_t *fmt, const void *o1, const void *o2, const char *name) { - config_line_t *c1, *c2; - int r = 1; CONFIG_CHECK(fmt, o1); CONFIG_CHECK(fmt, o2); - c1 = config_get_assigned_option(fmt, o1, name, 0); - c2 = config_get_assigned_option(fmt, o2, name, 0); - r = config_lines_eq(c1, c2); - config_free_lines(c1); - config_free_lines(c2); - return r; + const config_var_t *var = config_find_option(fmt, name); + if (!var) { + return true; + } + + return struct_var_eq(o1, o2, &var->member); } /** Copy storage held by old into a new or_options_t and return it. */ @@ -561,7 +559,6 @@ config_dup(const config_format_t *fmt, const void *old) { void *newopts; int i; - config_line_t *line; newopts = config_new(fmt); for (i=0; fmt->vars[i].member.name; ++i) { @@ -569,19 +566,13 @@ config_dup(const config_format_t *fmt, const void *old) continue; if (fmt->vars[i].member.type == CONFIG_TYPE_OBSOLETE) continue; - line = config_get_assigned_option(fmt, old, fmt->vars[i].member.name, 0); - if (line) { - char *msg = NULL; - if (config_assign(fmt, newopts, line, 0, &msg) < 0) { - // LCOV_EXCL_START - log_err(LD_BUG, "config_get_assigned_option() generated " - "something we couldn't config_assign(): %s", msg); - tor_free(msg); - tor_assert(0); - // LCOV_EXCL_STOP - } + if (struct_var_copy(newopts, old, &fmt->vars[i].member) < 0) { + // LCOV_EXCL_START + log_err(LD_BUG, "Unable to copy value for %s.", + fmt->vars[i].member.name); + tor_assert_unreached(); + // LCOV_EXCL_STOP } - config_free_lines(line); } return newopts; } From a91ed23403ae28974639a9bdb67530c5c07a0ce6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Jun 2019 10:46:07 -0400 Subject: [PATCH 6/9] Use structvar to find the types for config vars. --- scripts/maint/practracker/exceptions.txt | 2 +- src/app/config/config.c | 30 ++---------------------- src/lib/confmgt/structvar.c | 26 ++++++++++++++++++++ src/lib/confmgt/structvar.h | 3 +++ 4 files changed, 32 insertions(+), 29 deletions(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 4db452b897..2190fb1caa 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -30,7 +30,7 @@ # Remember: It is better to fix the problem than to add a new exception! problem file-size /src/app/config/config.c 8518 -problem include-count /src/app/config/config.c 87 +problem include-count /src/app/config/config.c 88 problem function-size /src/app/config/config.c:options_act_reversible() 296 problem function-size /src/app/config/config.c:options_act() 589 problem function-size /src/app/config/config.c:resolve_my_address() 192 diff --git a/src/app/config/config.c b/src/app/config/config.c index 37cbe6b2ef..8da1e2acdc 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -111,6 +111,7 @@ #include "feature/stats/predict_ports.h" #include "feature/stats/rephist.h" #include "lib/compress/compress.h" +#include "lib/confmgt/structvar.h" #include "lib/crypt_ops/crypto_init.h" #include "lib/crypt_ops/crypto_rand.h" #include "lib/crypt_ops/crypto_util.h" @@ -8195,37 +8196,10 @@ getinfo_helper_config(control_connection_t *conn, int i; for (i = 0; option_vars_[i].member.name; ++i) { const config_var_t *var = &option_vars_[i]; - const char *type; /* don't tell controller about triple-underscore options */ if (!strncmp(option_vars_[i].member.name, "___", 3)) continue; - switch (var->member.type) { - case CONFIG_TYPE_STRING: type = "String"; break; - case CONFIG_TYPE_FILENAME: type = "Filename"; break; - case CONFIG_TYPE_POSINT: type = "Integer"; break; - case CONFIG_TYPE_UINT64: type = "Integer"; break; - case CONFIG_TYPE_INT: type = "SignedInteger"; break; - case CONFIG_TYPE_INTERVAL: type = "TimeInterval"; break; - case CONFIG_TYPE_MSEC_INTERVAL: type = "TimeMsecInterval"; break; - case CONFIG_TYPE_MEMUNIT: type = "DataSize"; break; - case CONFIG_TYPE_DOUBLE: type = "Float"; break; - case CONFIG_TYPE_BOOL: type = "Boolean"; break; - case CONFIG_TYPE_AUTOBOOL: type = "Boolean+Auto"; break; - case CONFIG_TYPE_ISOTIME: type = "Time"; break; - case CONFIG_TYPE_ROUTERSET: type = "RouterList"; break; - case CONFIG_TYPE_CSV: type = "CommaList"; break; - /* This type accepts more inputs than TimeInterval, but it ignores - * everything after the first entry, so we may as well pretend - * it's a TimeInterval. */ - case CONFIG_TYPE_CSV_INTERVAL: type = "TimeInterval"; break; - case CONFIG_TYPE_LINELIST: type = "LineList"; break; - case CONFIG_TYPE_LINELIST_S: type = "Dependent"; break; - case CONFIG_TYPE_LINELIST_V: type = "Virtual"; break; - default: - case CONFIG_TYPE_OBSOLETE: - case CONFIG_TYPE_EXTENDED: - type = NULL; break; - } + const char *type = struct_var_get_typename(&var->member); if (!type) continue; smartlist_add_asprintf(sl, "%s %s\n",var->member.name,type); diff --git a/src/lib/confmgt/structvar.c b/src/lib/confmgt/structvar.c index 7ea00fbde9..38f8e5dd7a 100644 --- a/src/lib/confmgt/structvar.c +++ b/src/lib/confmgt/structvar.c @@ -25,6 +25,8 @@ #include "lib/confmgt/typedvar.h" #include "lib/log/util_bug.h" +#include "lib/confmgt/var_type_def_st.h" + #include /** @@ -198,3 +200,27 @@ struct_var_kvencode(const void *object, const struct_member_t *member) return typed_var_kvencode_ex(member->name, p, def); } + +/** + * Return the official name of this struct member. + **/ +const char * +struct_var_get_name(const struct_member_t *member) +{ + return member->name; +} + +/** + * Return the type name for this struct member. + * + * Do not use the output of this function to inspect a type within Tor. It is + * suitable for debugging, informing the controller or user of a variable's + * type, etc. + **/ +const char * +struct_var_get_typename(const struct_member_t *member) +{ + const var_type_def_t *def = get_type_def(member); + + return def ? def->name : NULL; +} diff --git a/src/lib/confmgt/structvar.h b/src/lib/confmgt/structvar.h index 894098e509..92b9b6fc71 100644 --- a/src/lib/confmgt/structvar.h +++ b/src/lib/confmgt/structvar.h @@ -41,6 +41,9 @@ bool struct_var_eq(const void *a, const void *b, bool struct_var_ok(const void *object, const struct struct_member_t *member); +const char *struct_var_get_name(const struct struct_member_t *member); +const char *struct_var_get_typename(const struct struct_member_t *member); + int struct_var_kvassign(void *object, const struct config_line_t *line, char **errmsg, const struct struct_member_t *member); From a114df9a040dbdedfc89f7d2ff777476e204a2cf Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Jun 2019 16:06:15 -0400 Subject: [PATCH 7/9] Add a function to make sure all values in a config object are ok --- src/app/config/confparse.c | 18 ++++++++++++++++++ src/app/config/confparse.h | 2 ++ 2 files changed, 20 insertions(+) diff --git a/src/app/config/confparse.c b/src/app/config/confparse.c index b9b5fddb96..2890d8c81b 100644 --- a/src/app/config/confparse.c +++ b/src/app/config/confparse.c @@ -673,3 +673,21 @@ config_dump(const config_format_t *fmt, const void *default_options, } return result; } + +/** + * Return true if every member of options is in-range and well-formed. + * Return false otherwise. Log errors at level severity. + */ +bool +config_check_ok(const config_format_t *fmt, const void *options, int severity) +{ + bool all_ok = true; + for (int i=0; fmt->vars[i].member.name; ++i) { + if (!struct_var_ok(options, &fmt->vars[i].member)) { + log_fn(severity, LD_BUG, "Invalid value for %s", + fmt->vars[i].member.name); + all_ok = false; + } + } + return all_ok; +} diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index 4ef4e708f3..b91ea1c13d 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -138,6 +138,8 @@ void *config_dup(const config_format_t *fmt, const void *old); char *config_dump(const config_format_t *fmt, const void *default_options, const void *options, int minimal, int comment_defaults); +bool config_check_ok(const config_format_t *fmt, const void *options, + int severity); int config_assign(const config_format_t *fmt, void *options, struct config_line_t *list, unsigned flags, char **msg); From c390efe84fa7a4c47e39ce0cc7e34550f515d5be Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Jun 2019 16:21:21 -0400 Subject: [PATCH 8/9] A few more test cases and unreachable lines --- src/lib/confmgt/typedvar.c | 18 ++++++++++-------- src/test/test_confparse.c | 20 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/lib/confmgt/typedvar.c b/src/lib/confmgt/typedvar.c index fc45c44481..c2b9b45725 100644 --- a/src/lib/confmgt/typedvar.c +++ b/src/lib/confmgt/typedvar.c @@ -44,7 +44,7 @@ typed_var_assign_ex(void *target, const char *value, char **errmsg, const var_type_def_t *def) { if (BUG(!def)) - return -1; + return -1; // LCOV_EXCL_LINE // clear old value if needed. typed_var_free_ex(target, def); @@ -67,7 +67,7 @@ typed_var_kvassign_ex(void *target, const config_line_t *line, char **errmsg, const var_type_def_t *def) { if (BUG(!def)) - return -1; + return -1; // LCOV_EXCL_LINE if (def->fns->kv_parse) { // We do _not_ free the old value here, since linelist options @@ -86,7 +86,7 @@ void typed_var_free_ex(void *target, const var_type_def_t *def) { if (BUG(!def)) - return; + return; // LCOV_EXCL_LINE if (def->fns->clear) { def->fns->clear(target, def->params); } @@ -102,7 +102,7 @@ char * typed_var_encode_ex(const void *value, const var_type_def_t *def) { if (BUG(!def)) - return NULL; + return NULL; // LCOV_EXCL_LINE tor_assert(def->fns->encode); return def->fns->encode(value, def->params); } @@ -121,7 +121,7 @@ typed_var_kvencode_ex(const char *key, const void *value, const var_type_def_t *def) { if (BUG(!def)) - return NULL; + return NULL; // LCOV_EXCL_LINE if (def->fns->kv_encode) { return def->fns->kv_encode(key, value, def->params); } @@ -145,7 +145,7 @@ int typed_var_copy_ex(void *dest, const void *src, const var_type_def_t *def) { if (BUG(!def)) - return -1; + return -1; // LCOV_EXCL_LINE if (def->fns->copy) { // If we have been provided a copy fuction, use it. return def->fns->copy(dest, src, def); @@ -160,8 +160,10 @@ typed_var_copy_ex(void *dest, const void *src, const var_type_def_t *def) char *err = NULL; int rv = typed_var_assign_ex(dest, enc, &err, def); if (BUG(rv < 0)) { + // LCOV_EXCL_START log_warn(LD_BUG, "Encoded value %s was not parseable as a %s: %s", escaped(enc), def->name, err?err:""); + // LCOV_EXCL_STOP } tor_free(err); tor_free(enc); @@ -176,7 +178,7 @@ bool typed_var_eq_ex(const void *a, const void *b, const var_type_def_t *def) { if (BUG(!def)) - return false; + return false; // LCOV_EXCL_LINE if (def->fns->eq) { // Use a provided eq function if we got one. @@ -200,7 +202,7 @@ bool typed_var_ok_ex(const void *value, const var_type_def_t *def) { if (BUG(!def)) - return false; + return false; // LCOV_EXCL_LINE if (def->fns->ok) return def->fns->ok(value, def->params); diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 9e626356d3..d0c33841fe 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -290,6 +290,8 @@ 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)); + done: config_free(&test_fmt, tst); } @@ -344,6 +346,8 @@ test_confparse_assign_deprecated(void *arg) tt_int_op(tst->deprecated_int, OP_EQ, 7); + tt_assert(config_check_ok(&test_fmt, tst, LOG_ERR)); + done: teardown_capture_of_logs(); config_free(&test_fmt, tst); @@ -489,6 +493,8 @@ static const badval_test_t bv_badabool = static const badval_test_t bv_badtime = { "time lunchtime\n", "Invalid time" }; static const badval_test_t bv_virt = { "MixedLines 7\n", "virtual option" }; static const badval_test_t bv_rs = { "Routerset 2.2.2.2.2\n", "Invalid" }; +static const badval_test_t bv_big_interval = + { "interval 1000 months", "too large" }; /* Try config_dump(), and make sure it behaves correctly */ static void @@ -885,6 +891,18 @@ test_confparse_unitparse(void *args) ; } +static void +test_confparse_check_ok_fail(void *arg) +{ + (void)arg; + test_struct_t *tst = config_new(&test_fmt); + tst->pos = -10; + tt_assert(! config_check_ok(&test_fmt, tst, LOG_INFO)); + + done: + config_free(&test_fmt, tst); +} + #define CONFPARSE_TEST(name, flags) \ { #name, test_confparse_ ## name, flags, NULL, NULL } @@ -912,6 +930,7 @@ struct testcase_t confparse_tests[] = { BADVAL_TEST(badtime), BADVAL_TEST(virt), BADVAL_TEST(rs), + BADVAL_TEST(big_interval), CONFPARSE_TEST(dump, 0), CONFPARSE_TEST(reset, 0), CONFPARSE_TEST(reassign, 0), @@ -919,5 +938,6 @@ struct testcase_t confparse_tests[] = { CONFPARSE_TEST(get_assigned, 0), CONFPARSE_TEST(extra_lines, 0), CONFPARSE_TEST(unitparse, 0), + CONFPARSE_TEST(check_ok_fail, 0), END_OF_TESTCASES }; From e8790971f6120cc3e4fd2acd41f5dd0512f52068 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Jun 2019 16:24:07 -0400 Subject: [PATCH 9/9] changes file for ticket 30914 --- changes/ticket30914 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/ticket30914 diff --git a/changes/ticket30914 b/changes/ticket30914 new file mode 100644 index 0000000000..c8c008b3d1 --- /dev/null +++ b/changes/ticket30914 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Lower another layer of object management from confparse.c to + a more general tool. Now typed structure members are accessible + via an abstract type. Implements ticket 30914.