[tor-commits] [tor/master] sr: Check for replacing a SRV pointer with the same pointer

nickm at torproject.org nickm at torproject.org
Tue Mar 12 15:03:57 UTC 2019


commit 0cca55411074a78096dab1b84a4781e790e9aece
Author: teor <teor at torproject.org>
Date:   Tue Mar 12 10:24:34 2019 +1000

    sr: Check for replacing a SRV pointer with the same pointer
    
    Check if the new pointer is the same as the old one: if it is, it's
    probably a bug:
    * the caller may have confused current and previous, or
    * they may have forgotten to sr_srv_dup().
    
    Putting NULL multiple times is allowed.
    
    Part of 29706.
---
 src/or/shared_random_state.c  |  32 +++++--
 src/test/test_shared_random.c | 212 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 230 insertions(+), 14 deletions(-)

diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c
index 50c6d3dd7..22a503b89 100644
--- a/src/or/shared_random_state.c
+++ b/src/or/shared_random_state.c
@@ -900,14 +900,26 @@ state_query_put_(sr_state_object_t obj_type, void *data)
     break;
   }
   case SR_STATE_OBJ_CURSRV:
-    /* Always free the old SRV */
-    state_query_del_(SR_STATE_OBJ_CURSRV, NULL);
-    sr_state->current_srv = (sr_srv_t *) data;
+      /* Check if the new pointer is the same as the old one: if it is, it's
+       * probably a bug. The caller may have confused current and previous,
+       * or they may have forgotten to sr_srv_dup().
+       * Putting NULL multiple times is allowed. */
+    if (!BUG(data && sr_state->current_srv == (sr_srv_t *) data)) {
+      /* We own the old SRV, so we need to free it.  */
+      state_query_del_(SR_STATE_OBJ_CURSRV, NULL);
+      sr_state->current_srv = (sr_srv_t *) data;
+    }
     break;
   case SR_STATE_OBJ_PREVSRV:
-    /* Always free the old SRV */
-    state_query_del_(SR_STATE_OBJ_PREVSRV, NULL);
-    sr_state->previous_srv = (sr_srv_t *) data;
+      /* Check if the new pointer is the same as the old one: if it is, it's
+       * probably a bug. The caller may have confused current and previous,
+       * or they may have forgotten to sr_srv_dup().
+       * Putting NULL multiple times is allowed. */
+    if (!BUG(data && sr_state->previous_srv == (sr_srv_t *) data)) {
+      /* We own the old SRV, so we need to free it.  */
+      state_query_del_(SR_STATE_OBJ_PREVSRV, NULL);
+      sr_state->previous_srv = (sr_srv_t *) data;
+    }
     break;
   case SR_STATE_OBJ_VALID_AFTER:
     sr_state->valid_after = *((time_t *) data);
@@ -1059,7 +1071,9 @@ sr_state_get_phase(void)
   return *(sr_phase_t *) ptr;
 }
 
-/* Return the previous SRV value from our state. Value CAN be NULL. */
+/* Return the previous SRV value from our state. Value CAN be NULL.
+ * The state object owns the SRV, so the calling code should not free the SRV.
+ * Use sr_srv_dup() if you want to keep a copy of the SRV. */
 const sr_srv_t *
 sr_state_get_previous_srv(void)
 {
@@ -1078,7 +1092,9 @@ sr_state_set_previous_srv(const sr_srv_t *srv)
               NULL);
 }
 
-/* Return the current SRV value from our state. Value CAN be NULL. */
+/* Return the current SRV value from our state. Value CAN be NULL.
+ * The state object owns the SRV, so the calling code should not free the SRV.
+ * Use sr_srv_dup() if you want to keep a copy of the SRV. */
 const sr_srv_t *
 sr_state_get_current_srv(void)
 {
diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c
index 93125e513..c81b6ec2d 100644
--- a/src/test/test_shared_random.c
+++ b/src/test/test_shared_random.c
@@ -38,7 +38,8 @@ trusteddirserver_get_by_v3_auth_digest_m(const char *digest)
 }
 
 /* Setup a minimal dirauth environment by initializing the SR state and
- * making sure the options are set to be an authority directory. */
+ * making sure the options are set to be an authority directory.
+ * You must only call this function once per process. */
 static void
 init_authority_state(void)
 {
@@ -451,7 +452,9 @@ test_encoding(void *arg)
 
 /** Setup some SRVs in our SR state. If <b>also_current</b> is set, then set
  *  both current and previous SRVs.
- *  Helper of test_vote() and test_sr_compute_srv(). */
+ *  Helper of test_vote() and test_sr_compute_srv().
+ * You must call sr_state_free() to free the state at the end of each test
+ * function (on pass or fail). */
 static void
 test_sr_setup_srv(int also_current)
 {
@@ -927,8 +930,9 @@ test_sr_get_majority_srv_from_votes(void *arg)
   smartlist_free(votes);
 }
 
+/* Test utils that don't depend on authority state */
 static void
-test_utils(void *arg)
+test_utils_general(void *arg)
 {
   (void) arg;
 
@@ -991,9 +995,19 @@ test_utils(void *arg)
     tt_str_op(get_phase_str(SR_PHASE_COMMIT), ==, "commit");
   }
 
+ done:
+  return;
+}
+
+/* Test utils that depend on authority state */
+static void
+test_utils_auth(void *arg)
+{
+  (void)arg;
+  init_authority_state();
+
   /* Testing phase transition */
   {
-    init_authority_state();
     set_sr_phase(SR_PHASE_COMMIT);
     tt_int_op(is_phase_transition(SR_PHASE_REVEAL), ==, 1);
     tt_int_op(is_phase_transition(SR_PHASE_COMMIT), ==, 0);
@@ -1004,8 +1018,193 @@ test_utils(void *arg)
     tt_int_op(is_phase_transition(42), ==, 1);
   }
 
+  /* Testing get, set, delete, clean SRVs */
+
+  {
+    /* Just set the previous SRV */
+    test_sr_setup_srv(0);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+    state_del_previous_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+  }
+
+  {
+    /* Delete the SRVs one at a time */
+    test_sr_setup_srv(1);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    state_del_current_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+    state_del_previous_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+
+    /* And in the opposite order */
+    test_sr_setup_srv(1);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    state_del_previous_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    state_del_current_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+
+    /* And both at once */
+    test_sr_setup_srv(1);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    sr_state_clean_srvs();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+
+    /* And do the gets and sets multiple times */
+    test_sr_setup_srv(1);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    state_del_previous_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    state_del_previous_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    sr_state_clean_srvs();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+    state_del_current_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+    sr_state_clean_srvs();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+    state_del_current_srv();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+  }
+
+  {
+    /* Now set the SRVs to NULL instead */
+    test_sr_setup_srv(1);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    sr_state_set_current_srv(NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+    sr_state_set_previous_srv(NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+
+    /* And in the opposite order */
+    test_sr_setup_srv(1);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    sr_state_set_previous_srv(NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    sr_state_set_current_srv(NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+
+    /* And both at once */
+    test_sr_setup_srv(1);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    sr_state_clean_srvs();
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+
+    /* And do the gets and sets multiple times */
+    test_sr_setup_srv(1);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    sr_state_set_previous_srv(NULL);
+    sr_state_set_previous_srv(NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    sr_state_set_current_srv(NULL);
+    sr_state_set_previous_srv(NULL);
+    sr_state_set_current_srv(NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL);
+  }
+
+  {
+    /* Now copy the values across */
+    test_sr_setup_srv(1);
+    /* Check that the pointers are non-NULL, and different from each other */
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE,
+              sr_state_get_current_srv());
+    /* Check that the content is different */
+    tt_mem_op(sr_state_get_previous_srv(), OP_NE,
+              sr_state_get_current_srv(), sizeof(sr_srv_t));
+    /* Set the current to the previous: the protocol goes the other way */
+    sr_state_set_current_srv(sr_srv_dup(sr_state_get_previous_srv()));
+    /* Check that the pointers are non-NULL, and different from each other */
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE,
+              sr_state_get_current_srv());
+    /* Check that the content is the same */
+    tt_mem_op(sr_state_get_previous_srv(), OP_EQ,
+              sr_state_get_current_srv(), sizeof(sr_srv_t));
+  }
+
+  {
+    /* Now copy a value onto itself */
+    test_sr_setup_srv(1);
+    /* Check that the pointers are non-NULL, and different from each other */
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE,
+              sr_state_get_current_srv());
+    /* Take a copy of the old value */
+    sr_srv_t old_current_srv;
+    memcpy(&old_current_srv, sr_state_get_current_srv(), sizeof(sr_srv_t));
+    /* Check that the content is different */
+    tt_mem_op(sr_state_get_previous_srv(), OP_NE,
+              sr_state_get_current_srv(), sizeof(sr_srv_t));
+    /* Set the current to the current: the protocol never replaces an SRV with
+     * the same value */
+    sr_state_set_current_srv(sr_srv_dup(sr_state_get_current_srv()));
+    /* Check that the pointers are non-NULL, and different from each other */
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL);
+    tt_ptr_op(sr_state_get_previous_srv(), OP_NE,
+              sr_state_get_current_srv());
+    /* Check that the content is different between current and previous */
+    tt_mem_op(sr_state_get_previous_srv(), OP_NE,
+              sr_state_get_current_srv(), sizeof(sr_srv_t));
+    /* Check that the content is the same as the old content */
+    tt_mem_op(&old_current_srv, OP_EQ,
+              sr_state_get_current_srv(), sizeof(sr_srv_t));
+  }
+
+  /* I don't think we can say "expect a BUG()" in our tests. */
+#if 0
+  {
+    /* Now copy a value onto itself without sr_srv_dup().
+     * This should fail with a BUG() warning. */
+    test_sr_setup_srv(1);
+    sr_state_set_current_srv(sr_state_get_current_srv());
+    sr_state_set_previous_srv(sr_state_get_previous_srv());
+  }
+#endif
+
  done:
-  return;
+  sr_state_free();
 }
 
 static void
@@ -1293,7 +1492,8 @@ struct testcase_t sr_tests[] = {
   { "sr_compute_srv", test_sr_compute_srv, TT_FORK, NULL, NULL },
   { "sr_get_majority_srv_from_votes", test_sr_get_majority_srv_from_votes,
     TT_FORK, NULL, NULL },
-  { "utils", test_utils, TT_FORK, NULL, NULL },
+  { "utils_general", test_utils_general, TT_FORK, NULL, NULL },
+  { "utils_auth", test_utils_auth, TT_FORK, NULL, NULL },
   { "state_transition", test_state_transition, TT_FORK, NULL, NULL },
   { "state_update", test_state_update, TT_FORK,
     NULL, NULL },





More information about the tor-commits mailing list