Merge branch 'typecheck4'

8 files changed, 113 insertions, 16 deletions

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.
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/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 <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 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/or.h b/src/or/or.h
index a90841fbcf..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,
@@ -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;
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/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 */
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. */