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.

diff --git a/src/or/config.c b/src/or/config.c
index 590fb37523869b7b61daaa2b2faf177314c2cf86..832a7c96748ed86d9a42ce4e089e7f2bb78e5f33 100644 (file)
--- 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 9eb46fab0306514538452b89c1e38001c48e4661..618fa70f2ddae572f213fa11a7ea10ef92c392a9 100644 (file)
--- 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 13ef95bb0c97382447f122dec8f38f40817b1d87..f74cb70a1836ba6b6a7928544e7f8443bb4e36ab 100644 (file)
--- 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 9ee80f2e3c5cfd9d8fc06c8160aff2b0856792bf..97bd9cac362fd6dc90e0d3eecc640fc8300d0295 100644 (file)
--- 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. */