Merge remote-tracking branch 'dgoulet/bug21054_030_01'

changes/bug21054

@@ -0,0 +1,5 @@
+  o Minor bugfixes (hidden service):
+    - Fix the config reload pruning of old vs new services so it actually
+      works when both ephemeral and non ephemeral services were configured
+      which lead to a BUG() stacktrace. Close #21054; Bugfix on
+      tor-0.3.0.1-alpha.

src/or/circuitlist.c

@@ -1440,6 +1440,41 @@ circuit_get_ready_rend_circ_by_rend_data(const rend_data_t *rend_data)
   return NULL;
 }
 
+/** Return the first service introduction circuit originating from the global
+ * circuit list after <b>start</b> or at the start of the list if <b>start</b>
+ * is NULL. Return NULL if no circuit is found.
+ *
+ * A service introduction point circuit has a purpose of either
+ * CIRCUIT_PURPOSE_S_ESTABLISH_INTRO or CIRCUIT_PURPOSE_S_INTRO. This does not
+ * return a circuit marked for close and its state must be open. */
+origin_circuit_t *
+circuit_get_next_service_intro_circ(origin_circuit_t *start)
+{
+  int idx = 0;
+  smartlist_t *lst = circuit_get_global_list();
+
+  if (start) {
+    idx = TO_CIRCUIT(start)->global_circuitlist_idx + 1;
+  }
+
+  for ( ; idx < smartlist_len(lst); ++idx) {
+    circuit_t *circ = smartlist_get(lst, idx);
+
+    /* Ignore a marked for close circuit or purpose not matching a service
+     * intro point or if the state is not open. */
+    if (circ->marked_for_close || circ->state != CIRCUIT_STATE_OPEN ||
+        (circ->purpose != CIRCUIT_PURPOSE_S_ESTABLISH_INTRO &&
+         circ->purpose != CIRCUIT_PURPOSE_S_INTRO)) {
+      continue;
+    }
+    /* The purposes we are looking for are only for origin circuits so the
+     * following is valid. */
+    return TO_ORIGIN_CIRCUIT(circ);
+  }
+  /* Not found. */
+  return NULL;
+}
+
 /** Return the first circuit originating here in global_circuitlist after
  * <b>start</b> whose purpose is <b>purpose</b>, and where <b>digest</b> (if
  * set) matches the private key digest of the rend data associated with the

src/or/circuitlist.h

@@ -47,6 +47,7 @@ origin_circuit_t *circuit_get_ready_rend_circ_by_rend_data(
   const rend_data_t *rend_data);
 origin_circuit_t *circuit_get_next_by_pk_and_purpose(origin_circuit_t *start,
                                      const uint8_t *digest, uint8_t purpose);
+origin_circuit_t *circuit_get_next_service_intro_circ(origin_circuit_t *start);
 origin_circuit_t *circuit_find_to_cannibalize(uint8_t purpose,
                                               extend_info_t *info, int flags);
 void circuit_mark_all_unused_circs(void);

src/or/rendservice.c

@@ -542,6 +542,95 @@ rend_service_check_dir_and_add(smartlist_t *service_list,
   return rend_add_service(s_list, service);
 }
 
+/* If this is a reload and there were hidden services configured before,
+ * keep the introduction points that are still needed and close the
+ * other ones. */
+STATIC void
+prune_services_on_reload(smartlist_t *old_service_list,
+                         smartlist_t *new_service_list)
+{
+  origin_circuit_t *ocirc = NULL;
+  smartlist_t *surviving_services = NULL;
+
+  tor_assert(old_service_list);
+  tor_assert(new_service_list);
+
+  /* This contains all _existing_ services that survives the relaod that is
+   * that haven't been removed from the configuration. The difference between
+   * this list and the new service list is that the new list can possibly
+   * contain newly configured service that have no introduction points opened
+   * yet nor key material loaded or generated. */
+  surviving_services = smartlist_new();
+
+  /* Preserve the existing ephemeral services.
+   *
+   * This is the ephemeral service equivalent of the "Copy introduction
+   * points to new services" block, except there's no copy required since
+   * the service structure isn't regenerated.
+   *
+   * After this is done, all ephemeral services will be:
+   *  * Removed from old_service_list, so the equivalent non-ephemeral code
+   *    will not attempt to preserve them.
+   *  * Added to the new_service_list (that previously only had the
+   *    services listed in the configuration).
+   *  * Added to surviving_services, which is the list of services that
+   *    will NOT have their intro point closed.
+   */
+  SMARTLIST_FOREACH_BEGIN(old_service_list, rend_service_t *, old) {
+    if (rend_service_is_ephemeral(old)) {
+      SMARTLIST_DEL_CURRENT(old_service_list, old);
+      smartlist_add(surviving_services, old);
+      smartlist_add(new_service_list, old);
+    }
+  } SMARTLIST_FOREACH_END(old);
+
+  /* Copy introduction points to new services. This is O(n^2), but it's only
+   * called on reconfigure, so it's ok performance wise. */
+  SMARTLIST_FOREACH_BEGIN(new_service_list, rend_service_t *, new) {
+    SMARTLIST_FOREACH_BEGIN(old_service_list, rend_service_t *, old) {
+      /* Skip ephemeral services as we only want to copy introduction points
+       * from current services to newly configured one that already exists.
+       * The same directory means it's the same service. */
+      if (rend_service_is_ephemeral(new) || rend_service_is_ephemeral(old) ||
+          strcmp(old->directory, new->directory)) {
+        continue;
+      }
+      smartlist_add_all(new->intro_nodes, old->intro_nodes);
+      smartlist_clear(old->intro_nodes);
+      smartlist_add_all(new->expiring_nodes, old->expiring_nodes);
+      smartlist_clear(old->expiring_nodes);
+      /* This regular service will survive the closing IPs step after. */
+      smartlist_add(surviving_services, old);
+      break;
+    } SMARTLIST_FOREACH_END(old);
+  } SMARTLIST_FOREACH_END(new);
+
+  /* For every service introduction circuit we can find, see if we have a
+   * matching surviving configured service. If not, close the circuit. */
+  while ((ocirc = circuit_get_next_service_intro_circ(ocirc))) {
+    int keep_it = 0;
+    tor_assert(ocirc->rend_data);
+    SMARTLIST_FOREACH_BEGIN(surviving_services, const rend_service_t *, s) {
+      if (rend_circuit_pk_digest_eq(ocirc, (uint8_t *) s->pk_digest)) {
+        /* Keep this circuit as we have a matching configured service. */
+        keep_it = 1;
+        break;
+      }
+    } SMARTLIST_FOREACH_END(s);
+    if (keep_it) {
+      continue;
+    }
+    log_info(LD_REND, "Closing intro point %s for service %s.",
+             safe_str_client(extend_info_describe(
+                                        ocirc->build_state->chosen_exit)),
+             safe_str_client(rend_data_get_address(ocirc->rend_data)));
+    /* Reason is FINISHED because service has been removed and thus the
+     * circuit is considered old/uneeded. */
+    circuit_mark_for_close(TO_CIRCUIT(ocirc), END_CIRC_REASON_FINISHED);
+  }
+  smartlist_free(surviving_services);
+}
+
 /** Set up rend_service_list, based on the values of HiddenServiceDir and
  * HiddenServicePort in <b>options</b>.  Return 0 on success and -1 on
  * failure.  (If <b>validate_only</b> is set, parse, warn and return as
@@ -795,84 +884,11 @@ rend_config_services(const or_options_t *options, int validate_only)
    * keep the introduction points that are still needed and close the
    * other ones. */
   if (old_service_list && !validate_only) {
-    smartlist_t *surviving_services = smartlist_new();
-
-    /* Preserve the existing ephemeral services.
-     *
-     * This is the ephemeral service equivalent of the "Copy introduction
-     * points to new services" block, except there's no copy required since
-     * the service structure isn't regenerated.
-     *
-     * After this is done, all ephemeral services will be:
-     *  * Removed from old_service_list, so the equivalent non-ephemeral code
-     *    will not attempt to preserve them.
-     *  * Added to the new rend_service_list (that previously only had the
-     *    services listed in the configuration).
-     *  * Added to surviving_services, which is the list of services that
-     *    will NOT have their intro point closed.
-     */
-    SMARTLIST_FOREACH(old_service_list, rend_service_t *, old, {
-      if (rend_service_is_ephemeral(old)) {
-        SMARTLIST_DEL_CURRENT(old_service_list, old);
-        smartlist_add(surviving_services, old);
-        smartlist_add(rend_service_list, old);
-      }
-    });
-
-    /* Copy introduction points to new services. */
-    /* XXXX This is O(n^2), but it's only called on reconfigure, so it's
-     * probably ok? */
-    SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, new) {
-      SMARTLIST_FOREACH_BEGIN(old_service_list, rend_service_t *, old) {
-        if (BUG(rend_service_is_ephemeral(new)) ||
-            BUG(rend_service_is_ephemeral(old))) {
-          continue;
-        }
-        if (BUG(!new->directory) || BUG(!old->directory) ||
-            strcmp(old->directory, new->directory)) {
-          continue;
-        }
-        smartlist_add_all(new->intro_nodes, old->intro_nodes);
-        smartlist_clear(old->intro_nodes);
-        smartlist_add_all(new->expiring_nodes, old->expiring_nodes);
-        smartlist_clear(old->expiring_nodes);
-        smartlist_add(surviving_services, old);
-        break;
-      } SMARTLIST_FOREACH_END(old);
-    } SMARTLIST_FOREACH_END(new);
-
-    /* Close introduction circuits of services we don't serve anymore. */
-    /* XXXX it would be nicer if we had a nicer abstraction to use here,
-     * so we could just iterate over the list of services to close, but
-     * once again, this isn't critical-path code. */
-    SMARTLIST_FOREACH_BEGIN(circuit_get_global_list(), circuit_t *, circ) {
-      if (!circ->marked_for_close &&
-          circ->state == CIRCUIT_STATE_OPEN &&
-          (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO ||
-           circ->purpose == CIRCUIT_PURPOSE_S_INTRO)) {
-        origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ);
-        int keep_it = 0;
-        tor_assert(oc->rend_data);
-        SMARTLIST_FOREACH(surviving_services, rend_service_t *, ptr, {
-          if (rend_circuit_pk_digest_eq(oc, (uint8_t *) ptr->pk_digest)) {
-            keep_it = 1;
-            break;
-          }
-        });
-        if (keep_it)
-          continue;
-        log_info(LD_REND, "Closing intro point %s for service %s.",
-                 safe_str_client(extend_info_describe(
-                                            oc->build_state->chosen_exit)),
-                 rend_data_get_address(oc->rend_data));
-        circuit_mark_for_close(circ, END_CIRC_REASON_FINISHED);
-        /* XXXX Is there another reason we should use here? */
-      }
-    }
-    SMARTLIST_FOREACH_END(circ);
-    smartlist_free(surviving_services);
-    SMARTLIST_FOREACH(old_service_list, rend_service_t *, ptr,
-                      rend_service_free(ptr));
+    prune_services_on_reload(old_service_list, rend_service_list);
+    /* Every remaining service in the old list have been removed from the
+     * configuration so clean them up safely. */
+    SMARTLIST_FOREACH(old_service_list, rend_service_t *, s,
+                      rend_service_free(s));
     smartlist_free(old_service_list);
   }
 

src/or/rendservice.h

@@ -132,6 +132,9 @@ STATIC int rend_service_poison_new_single_onion_dir(
 STATIC ssize_t encode_establish_intro_cell_legacy(char *cell_body_out,
                                                   crypto_pk_t *intro_key,
                                                   char *rend_circ_nonce);
+STATIC void prune_services_on_reload(smartlist_t *old_service_list,
+                                     smartlist_t *new_service_list);
+
 #endif
 
 int num_rend_services(void);

src/test/test_hs.c

@@ -787,6 +787,126 @@ test_single_onion_poisoning(void *arg)
   tor_free(mock_options->DataDirectory);
 }
 
+static rend_service_t *
+helper_create_rend_service(const char *path)
+{
+  rend_service_t *s = tor_malloc_zero(sizeof(rend_service_t));
+  s->ports = smartlist_new();
+  s->intro_nodes = smartlist_new();
+  s->expiring_nodes = smartlist_new();
+  if (path) {
+    s->directory = tor_strdup(path);
+  }
+  return s;
+}
+
+static void
+test_prune_services_on_reload(void *arg)
+{
+  smartlist_t *new = smartlist_new(), *old = smartlist_new();
+  /* Non ephemeral service. */
+  rend_service_t *s1 = helper_create_rend_service("SomePath");
+  /* Create a non ephemeral service with the _same_ path as so we can test the
+   * transfer of introduction point between the same services on reload. */
+  rend_service_t *s2 = helper_create_rend_service(s1->directory);
+  /* Ephemeral service (directory is NULL). */
+  rend_service_t *e1 = helper_create_rend_service(NULL);
+  rend_service_t *e2 = helper_create_rend_service(NULL);
+
+  (void) arg;
+
+  {
+    /* Add both services to the old list. */
+    smartlist_add(old, s1);
+    smartlist_add(old, e1);
+    /* Only put the non ephemeral in the new list. */
+    smartlist_add(new, s1);
+    prune_services_on_reload(old, new);
+    /* We expect that the ephemeral one is in the new list but removed from
+     * the old one. */
+    tt_int_op(smartlist_len(old), OP_EQ, 1);
+    tt_assert(smartlist_get(old, 0) == s1);
+    tt_int_op(smartlist_len(new), OP_EQ, 2);
+    tt_assert(smartlist_get(new, 0) == s1);
+    tt_assert(smartlist_get(new, 1) == e1);
+    /* Cleanup for next test. */
+    smartlist_clear(new);
+    smartlist_clear(old);
+  }
+
+  {
+    /* This test will make sure that only the ephemeral service is kept if the
+     * new list is empty. The old list should contain only the non ephemeral
+     * one. */
+    smartlist_add(old, s1);
+    smartlist_add(old, e1);
+    prune_services_on_reload(old, new);
+    tt_int_op(smartlist_len(old), OP_EQ, 1);
+    tt_assert(smartlist_get(old, 0) == s1);
+    tt_int_op(smartlist_len(new), OP_EQ, 1);
+    tt_assert(smartlist_get(new, 0) == e1);
+    /* Cleanup for next test. */
+    smartlist_clear(new);
+    smartlist_clear(old);
+  }
+
+  {
+    /* This test makes sure that the new list stays the same even from the old
+     * list being completely different. */
+    smartlist_add(new, s1);
+    smartlist_add(new, e1);
+    prune_services_on_reload(old, new);
+    tt_int_op(smartlist_len(old), OP_EQ, 0);
+    tt_int_op(smartlist_len(new), OP_EQ, 2);
+    tt_assert(smartlist_get(new, 0) == s1);
+    tt_assert(smartlist_get(new, 1) == e1);
+    /* Cleanup for next test. */
+    smartlist_clear(new);
+  }
+
+  {
+    rend_intro_point_t ip1;
+    /* This IP should be found in the s2 service after pruning. */
+    smartlist_add(s1->intro_nodes, &ip1);
+    /* Setup our list. */
+    smartlist_add(old, s1);
+    smartlist_add(new, s2);
+    prune_services_on_reload(old, new);
+    tt_int_op(smartlist_len(old), OP_EQ, 1);
+    /* Intro nodes have been moved to the s2 in theory so it must be empty. */
+    tt_int_op(smartlist_len(s1->intro_nodes), OP_EQ, 0);
+    tt_int_op(smartlist_len(new), OP_EQ, 1);
+    rend_service_t *elem = smartlist_get(new, 0);
+    tt_assert(elem);
+    tt_assert(elem == s2);
+    tt_int_op(smartlist_len(elem->intro_nodes), OP_EQ, 1);
+    tt_assert(smartlist_get(elem->intro_nodes, 0) == &ip1);
+    smartlist_clear(s1->intro_nodes);
+    smartlist_clear(s2->intro_nodes);
+    /* Cleanup for next test. */
+    smartlist_clear(new);
+    smartlist_clear(old);
+  }
+
+  {
+    /* Test two ephemeral services. */
+    smartlist_add(old, e1);
+    smartlist_add(old, e2);
+    prune_services_on_reload(old, new);
+    /* Check if they've all been transfered. */
+    tt_int_op(smartlist_len(old), OP_EQ, 0);
+    tt_int_op(smartlist_len(new), OP_EQ, 2);
+  }
+
+ done:
+  rend_service_free(s1);
+  rend_service_free(s2);
+  rend_service_free(e1);
+  rend_service_free(e2);
+  smartlist_free(new);
+  smartlist_free(old);
+}
+
 struct testcase_t hs_tests[] = {
   { "hs_rend_data", test_hs_rend_data, TT_FORK,
     NULL, NULL },
@@ -807,6 +927,9 @@ struct testcase_t hs_tests[] = {
     TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR2) },
   { "single_onion_poisoning_create_dir_both", test_single_onion_poisoning,
     TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR1 | CREATE_HS_DIR2) },
+  { "prune_services_on_reload", test_prune_services_on_reload, TT_FORK,
+    NULL, NULL },
+
   END_OF_TESTCASES
 };