Merge remote-tracking branch 'tor-github/pr/776' into maint-0.4.0

1 files changed, 48 insertions, 12 deletions

diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c
index 92f0b3e737..a7b7480edd 100644
--- a/src/feature/dirauth/shared_random_state.c
+++ b/src/feature/dirauth/shared_random_state.c
@@ -102,6 +102,8 @@ static const config_format_t state_format = {
   &state_extra_var,
 };
 
+static void state_query_del_(sr_state_object_t obj_type, void *data);
+
 /* Return a string representation of a protocol phase. */
 STATIC const char *
 get_phase_str(sr_phase_t phase)
@@ -834,6 +836,9 @@ state_query_get_commit(const char *rsa_fpr)
 static void *
 state_query_get_(sr_state_object_t obj_type, const void *data)
 {
+  if (BUG(!sr_state))
+    return NULL;
+
   void *obj = NULL;
 
   switch (obj_type) {
@@ -862,23 +867,44 @@ state_query_get_(sr_state_object_t obj_type, const void *data)
 }
 
 /* Helper function: This handles the PUT state action using an
- * <b>obj_type</b> and <b>data</b> needed for the action. */
+ * <b>obj_type</b> and <b>data</b> needed for the action.
+ * PUT frees the previous data before replacing it, if needed. */
 static void
 state_query_put_(sr_state_object_t obj_type, void *data)
 {
+  if (BUG(!sr_state))
+    return;
+
   switch (obj_type) {
   case SR_STATE_OBJ_COMMIT:
   {
     sr_commit_t *commit = data;
     tor_assert(commit);
+    /* commit_add_to_state() frees the old commit, if there is one */
     commit_add_to_state(commit, sr_state);
     break;
   }
   case SR_STATE_OBJ_CURSRV:
-    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:
-    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);
@@ -898,6 +924,9 @@ state_query_put_(sr_state_object_t obj_type, void *data)
 static void
 state_query_del_all_(sr_state_object_t obj_type)
 {
+  if (BUG(!sr_state))
+    return;
+
   switch (obj_type) {
   case SR_STATE_OBJ_COMMIT:
   {
@@ -926,6 +955,9 @@ state_query_del_(sr_state_object_t obj_type, void *data)
 {
   (void) data;
 
+  if (BUG(!sr_state))
+    return;
+
   switch (obj_type) {
   case SR_STATE_OBJ_PREVSRV:
     tor_free(sr_state->previous_srv);
@@ -986,7 +1018,7 @@ state_query(sr_state_action_t action, sr_state_object_t obj_type,
 
 /* Delete the current SRV value from the state freeing it and the value is set
  * to NULL meaning empty. */
-static void
+STATIC void
 state_del_current_srv(void)
 {
   state_query(SR_STATE_ACTION_DEL, SR_STATE_OBJ_CURSRV, NULL, NULL);
@@ -994,22 +1026,22 @@ state_del_current_srv(void)
 
 /* Delete the previous SRV value from the state freeing it and the value is
  * set to NULL meaning empty. */
-static void
+STATIC void
 state_del_previous_srv(void)
 {
   state_query(SR_STATE_ACTION_DEL, SR_STATE_OBJ_PREVSRV, NULL, NULL);
 }
 
-/* Rotate SRV value by freeing the previous value, assigning the current
- * value to the previous one and nullifying the current one. */
+/* Rotate SRV value by setting the previous SRV to the current SRV, and
+ * clearing the current SRV. */
 STATIC void
 state_rotate_srv(void)
 {
   /* First delete previous SRV from the state. Object will be freed. */
   state_del_previous_srv();
-  /* Set previous SRV with the current one. */
-  sr_state_set_previous_srv(sr_state_get_current_srv());
-  /* Nullify the current srv. */
+  /* Set previous SRV to a copy of the current one. */
+  sr_state_set_previous_srv(sr_srv_dup(sr_state_get_current_srv()));
+  /* Free and NULL the current srv. */
   sr_state_set_current_srv(NULL);
 }
 
@@ -1030,7 +1062,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)
 {
@@ -1049,7 +1083,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)
 {