commit eb54a856a298aff8b2cdadf87247a33a74921c49 Author: Nick Mathewson nickm@torproject.org Date: Mon Sep 25 11:08:11 2017 -0400
Add test to make sure all confparse variables are well-typed
New approach, suggested by Taylor: During testing builds, we initialize a union member of an appropriate pointer type with the address of the member field we're trying to test, so we can make sure that the compiler doesn't warn.
My earlier approach invoked undefined behavior. --- src/or/config.c | 14 +++++++-- src/or/confparse.h | 72 ++++++++++++++++++++++++++++++++++++++++++++ src/or/shared_random_state.c | 9 ++++-- src/or/statefile.c | 10 ++++-- 4 files changed, 98 insertions(+), 7 deletions(-)
diff --git a/src/or/config.c b/src/or/config.c index 590fb3752..832a7c967 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -174,18 +174,26 @@ static config_abbrev_t option_abbrevs_[] = { { NULL, NULL, 0, 0}, };
+/** dummy instance of or_options_t, used for type-checking its + * members with CONF_CHECK_VAR_TYPE. */ +DUMMY_TYPECHECK_INSTANCE(or_options_t); + /** An entry for config_vars: "The option <b>name</b> has type * CONFIG_TYPE_<b>conftype</b>, and corresponds to * or_options_t.<b>member</b>" */ #define VAR(name,conftype,member,initvalue) \ { name, CONFIG_TYPE_ ## conftype, offsetof(or_options_t, member), \ - initvalue } + 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 <b>name</b> is obsolete." */ +#ifdef TOR_UNIT_TESTS +#define OBSOLETE(name) { name, CONFIG_TYPE_OBSOLETE, 0, NULL, {.INT=NULL} } +#else #define OBSOLETE(name) { name, CONFIG_TYPE_OBSOLETE, 0, NULL } +#endif
/** * Macro to declare *Port options. Each one comes in three entries. @@ -621,7 +629,7 @@ static config_var_t option_vars_[] = { V(TestingDirAuthVoteHSDirIsStrict, BOOL, "0"), VAR("___UsingTestNetworkDefaults", BOOL, UsingTestNetworkDefaults_, "0"),
- { NULL, CONFIG_TYPE_OBSOLETE, 0, NULL } + END_OF_CONFIG_VARS };
/** Override default values with these if the user sets the TestingTorNetwork @@ -676,7 +684,7 @@ static const config_var_t testing_tor_network_defaults[] = { VAR("___UsingTestNetworkDefaults", BOOL, UsingTestNetworkDefaults_, "1"), V(RendPostPeriod, INTERVAL, "2 minutes"),
- { NULL, CONFIG_TYPE_OBSOLETE, 0, NULL } + END_OF_CONFIG_VARS };
#undef VAR diff --git a/src/or/confparse.h b/src/or/confparse.h index 9eb46fab0..618fa70f2 100644 --- a/src/or/confparse.h +++ b/src/or/confparse.h @@ -40,6 +40,36 @@ typedef enum config_type_t { CONFIG_TYPE_OBSOLETE, /**< Obsolete (ignored) option. */ } config_type_t;
+#ifdef TOR_UNIT_TESTS +/** + * Union used when building in test mode typechecking the members of a type + * used with confparse.c. See CONF_CHECK_VAR_TYPE for a description of how + * it is used. */ +typedef union { + char **STRING; + char **FILENAME; + int *UINT; /* yes, really: Even though the confparse type is called + * "UINT", it still uses the C int type -- it just enforces that + * the values are in range [0,INT_MAX]. + */ + int *INT; + int *PORT; + int *INTERVAL; + int *MSEC_INTERVAL; + uint64_t *MEMUNIT; + double *DOUBLE; + int *BOOL; + int *AUTOBOOL; + time_t *ISOTIME; + smartlist_t **CSV; + smartlist_t **CSV_INTERVAL; + config_line_t **LINELIST; + config_line_t **LINELIST_S; + config_line_t **LINELIST_V; + routerset_t **ROUTERSET; +} confparse_dummy_values_t; +#endif + /** An abbreviation for a configuration option allowed on the command line. */ typedef struct config_abbrev_t { const char *abbreviated; @@ -64,8 +94,50 @@ typedef struct config_var_t { * value. */ off_t var_offset; /**< Offset of the corresponding member of or_options_t. */ const char *initvalue; /**< String (or null) describing initial value. */ + +#ifdef TOR_UNIT_TESTS + /** Used for compiler-magic to typecheck the corresponding field in the + * corresponding struct. Only used in unit test mode, at compile-time. */ + confparse_dummy_values_t var_ptr_dummy; +#endif } config_var_t;
+/* Macros to define extra members inside config_var_t fields, and at the + * end of a list of them. + */ +#ifdef TOR_UNIT_TESTS +/* This is a somewhat magic type-checking macro for users of confparse.c. + * It initializes a union member "confparse_dummy_values_t.conftype" with + * the address of a static member "tp_dummy.member". This + * will give a compiler warning unless the member field is of the correct + * type. + * + * (This warning is mandatory, because a type mismatch here violates the type + * compatibility constraint for simple assignment, and requires a diagnostic, + * according to the C spec.) + * + * For example, suppose you say: + * "CONF_CHECK_VAR_TYPE(or_options_t, STRING, Address)". + * Then this macro will evaluate to: + * { .STRING = &or_options_t_dummy.Address } + * And since confparse_dummy_values_t.STRING has type "char **", that + * expression will create a warning unless or_options_t.Address also + * has type "char *". + */ +#define CONF_CHECK_VAR_TYPE(tp, conftype, member) \ + { . conftype = &tp ## _dummy . member } +#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 } } +#define DUMMY_TYPECHECK_INSTANCE(tp) \ + static tp tp ## _dummy +#else +#define CONF_TEST_MEMBERS(tp, conftype, member) +#define END_OF_CONFIG_VARS { NULL, CONFIG_TYPE_OBSOLETE, 0, NULL } +#define DUMMY_TYPECHECK_INSTANCE(tp) +#endif + /** Type of a callback to validate whether a given configuration is * well-formed and consistent. See options_trial_assign() for documentation * of arguments. */ diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c index 13ef95bb0..f74cb70a1 100644 --- a/src/or/shared_random_state.c +++ b/src/or/shared_random_state.c @@ -40,10 +40,14 @@ static const char dstate_commit_key[] = "Commit"; static const char dstate_prev_srv_key[] = "SharedRandPreviousValue"; static const char dstate_cur_srv_key[] = "SharedRandCurrentValue";
+/** dummy instance of sr_disk_state_t, used for type-checking its + * members with CONF_CHECK_VAR_TYPE. */ +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 } + 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) @@ -70,7 +74,7 @@ static config_var_t state_vars[] = { V(SharedRandValues, LINELIST_V, NULL), VAR("SharedRandPreviousValue",LINELIST_S, SharedRandValues, NULL), VAR("SharedRandCurrentValue", LINELIST_S, SharedRandValues, NULL), - { NULL, CONFIG_TYPE_OBSOLETE, 0, NULL } + END_OF_CONFIG_VARS };
/* "Extra" variable in the state that receives lines we can't parse. This @@ -78,6 +82,7 @@ static config_var_t state_vars[] = { 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) };
/* Configuration format of sr_disk_state_t. */ diff --git a/src/or/statefile.c b/src/or/statefile.c index 9ee80f2e3..97bd9cac3 100644 --- a/src/or/statefile.c +++ b/src/or/statefile.c @@ -54,10 +54,14 @@ static config_abbrev_t state_abbrevs_[] = { { NULL, NULL, 0, 0}, };
+/** dummy instance of or_state_t, used for type-checking its + * members with CONF_CHECK_VAR_TYPE. */ +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), \ - initvalue } + 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) \ VAR(#member, conftype, member, initvalue) @@ -116,7 +120,8 @@ static config_var_t state_vars_[] = { V(CircuitBuildAbandonedCount, UINT, "0"), VAR("CircuitBuildTimeBin", LINELIST_S, BuildtimeHistogram, NULL), VAR("BuildtimeHistogram", LINELIST_V, BuildtimeHistogram, NULL), - { NULL, CONFIG_TYPE_OBSOLETE, 0, NULL } + + END_OF_CONFIG_VARS };
#undef VAR @@ -135,6 +140,7 @@ static int or_state_validate_cb(void *old_options, void *options, * 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) };
/** Configuration format for or_state_t. */