From eb54a856a298aff8b2cdadf87247a33a74921c49 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 25 Sep 2017 11:08:11 -0400 Subject: [PATCH 1/5] 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 590fb37523..832a7c9674 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 name 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), \ - 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 name 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 9eb46fab03..618fa70f2d 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 13ef95bb0c..f74cb70a18 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 9ee80f2e3c..97bd9cac36 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. */ From 4d11a468b20a116978f89115a75cdc45fe5da8ad Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 25 Sep 2017 11:51:15 -0400 Subject: [PATCH 2/5] Correct two state-file variable types. These should have been int, but we had listed them as unsigned. That's an easy mistake to make, since "int" corresponds with either INT or UINT in the configuration file. This bug cannot have actually caused a problem in practice, since we check those fields' values on load, and ensure that they are in range 0..INT32_MAX. --- src/or/circuitstats.c | 8 ++++---- src/or/or.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/or/circuitstats.c b/src/or/circuitstats.c index ad0630e27e..923a6d794f 100644 --- a/src/or/circuitstats.c +++ b/src/or/circuitstats.c @@ -910,7 +910,7 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt, int tot_values = 0; uint32_t loaded_cnt = 0, N = 0; config_line_t *line; - unsigned int i; + int i; build_time_t *loaded_times; int err = 0; circuit_build_times_init(cbt); @@ -960,8 +960,8 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt, break; } - if (loaded_cnt+count+state->CircuitBuildAbandonedCount - > state->TotalBuildTimes) { + if (loaded_cnt+count+ (unsigned)state->CircuitBuildAbandonedCount + > (unsigned) state->TotalBuildTimes) { log_warn(LD_CIRC, "Too many build times in state file. " "Stopping short before %d", @@ -986,7 +986,7 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt, loaded_times[loaded_cnt++] = CBT_BUILD_ABANDONED; } - if (loaded_cnt != state->TotalBuildTimes) { + if (loaded_cnt != (unsigned)state->TotalBuildTimes) { log_warn(LD_CIRC, "Corrupt state file? Build times count mismatch. " "Read %d times, but file says %d", loaded_cnt, diff --git a/src/or/or.h b/src/or/or.h index a90841fbcf..84913efc4d 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4686,8 +4686,8 @@ typedef struct { /** Build time histogram */ config_line_t * BuildtimeHistogram; - unsigned int TotalBuildTimes; - unsigned int CircuitBuildAbandonedCount; + int TotalBuildTimes; + int CircuitBuildAbandonedCount; /** What version of Tor wrote this state file? */ char *TorVersion; From a5b18dfba9752d9785172dc814802c482cc77f0e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 26 Sep 2017 12:25:07 -0400 Subject: [PATCH 3/5] Make the TransProxyType field non-const The correct type for a STRING confparse value is char *, not const char *. --- src/or/or.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index 84913efc4d..5bd07ba6a3 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3704,8 +3704,8 @@ typedef struct { config_line_t *SocksPort_lines; /** Ports to listen on for transparent pf/netfilter connections. */ config_line_t *TransPort_lines; - const char *TransProxyType; /**< What kind of transparent proxy - * implementation are we using? */ + char *TransProxyType; /**< What kind of transparent proxy + * implementation are we using? */ /** Parsed value of TransProxyType. */ enum { TPT_DEFAULT, From 49d2346b805b09fde656d05a6bbf1c632a8eb903 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 25 Sep 2017 11:52:57 -0400 Subject: [PATCH 4/5] Type bug in shared_random_state -- make sure Version is int. The confparse field has type UINT, which corresponds to an int type. We had uint32_t. This shouldn't cause trouble in practice, since int happens to 4-bytes wide on every platform where an authority is running. It's still wrong, though. --- src/or/shared_random_state.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/shared_random_state.h b/src/or/shared_random_state.h index a154eb5636..b6af6e1721 100644 --- a/src/or/shared_random_state.h +++ b/src/or/shared_random_state.h @@ -77,7 +77,7 @@ typedef struct sr_state_t { typedef struct sr_disk_state_t { uint32_t magic_; /* Version of the protocol. */ - uint32_t Version; + int Version; /* Version of our running tor. */ char *TorVersion; /* Creation time of this state */ From 8f0dffe329d41eab4ab192ed08e32692b9362663 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 25 Sep 2017 11:55:51 -0400 Subject: [PATCH 5/5] changes file for my confparse typechecking fun --- changes/ticket23643 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changes/ticket23643 diff --git a/changes/ticket23643 b/changes/ticket23643 new file mode 100644 index 0000000000..b2edbef914 --- /dev/null +++ b/changes/ticket23643 @@ -0,0 +1,6 @@ + o Minor features (compilation, testing): + - Tor builds should now fail if there are any mismatches between the C + type representing a configuration variable and the C type the + data-driven parser uses to store a value there. Previously, we needed + to check these by hand, which sometimes led to mistakes. Closes ticket + 23643.