Update unit tests for 20484, 20529

Add extra logging and extra validity checks for hidden services.

src/or/rendservice.c

@@ -216,16 +216,30 @@ rend_service_free_all(void)
   rend_service_list = NULL;
 }
 
-/** Validate <b>service</b> and add it to rend_service_list if possible.
+/** Validate <b>service</b> and add it to <b>service_list</b>, or to
+ * the global rend_service_list if <b>service_list</b> is NULL.
  * Return 0 on success.  On failure, free <b>service</b> and return -1.
  * Takes ownership of <b>service</b>.
  */
 static int
-rend_add_service(rend_service_t *service)
+rend_add_service(smartlist_t *service_list, rend_service_t *service)
 {
   int i;
   rend_service_port_config_t *p;
 
+  smartlist_t *s_list;
+  /* If no special service list is provided, then just use the global one. */
+  if (!service_list) {
+    if (BUG(!rend_service_list)) {
+      /* No global HS list, which is a failure. */
+      return -1;
+    }
+
+    s_list = rend_service_list;
+  } else {
+    s_list = service_list;
+  }
+
   service->intro_nodes = smartlist_new();
   service->expiring_nodes = smartlist_new();
 
@@ -247,7 +261,8 @@ rend_add_service(rend_service_t *service)
   }
 
   if (service->auth_type != REND_NO_AUTH &&
-      smartlist_len(service->clients) == 0) {
+      (!service->clients ||
+       smartlist_len(service->clients) == 0)) {
     log_warn(LD_CONFIG, "Hidden service (%s) with client authorization but no "
                         "clients; ignoring.",
              rend_service_escaped_dir(service));
@@ -255,7 +270,7 @@ rend_add_service(rend_service_t *service)
     return -1;
   }
 
-  if (!smartlist_len(service->ports)) {
+  if (!service->ports || !smartlist_len(service->ports)) {
     log_warn(LD_CONFIG, "Hidden service (%s) with no ports configured; "
              "ignoring.",
              rend_service_escaped_dir(service));
@@ -278,8 +293,9 @@ rend_add_service(rend_service_t *service)
      * lock file.  But this is enough to detect a simple mistake that
      * at least one person has actually made.
      */
-    if (service->directory != NULL) { /* Skip dupe for ephemeral services. */
+    if (service->directory != NULL) {
-      SMARTLIST_FOREACH(rend_service_list, rend_service_t*, ptr,
+      /* Skip dupe for ephemeral services. */
+      SMARTLIST_FOREACH(s_list, rend_service_t*, ptr,
                         dupe = dupe ||
                                !strcmp(ptr->directory, service->directory));
       if (dupe) {
@@ -290,7 +306,7 @@ rend_add_service(rend_service_t *service)
         return -1;
       }
     }
-    smartlist_add(rend_service_list, service);
+    smartlist_add(s_list, service);
     log_debug(LD_REND,"Configuring service with directory \"%s\"",
               service->directory);
     for (i = 0; i < smartlist_len(service->ports); ++i) {
@@ -445,16 +461,18 @@ rend_service_port_config_free(rend_service_port_config_t *p)
   tor_free(p);
 }
 
-/* Check the directory for <b>service</b>, and add the service to the global
+/* Check the directory for <b>service</b>, and add the service to
- * list if <b>validate_only</b> is false.
+ * <b>service_list</b>, or to the global list if <b>service_list</b> is NULL.
+ * Only add the service to the list if <b>validate_only</b> is false.
  * If <b>validate_only</b> is true, free the service.
  * If <b>service</b> is NULL, ignore it, and return 0.
  * Returns 0 on success, and -1 on failure.
  * Takes ownership of <b>service</b>, either freeing it, or adding it to the
  * global service list.
  */
-static int
+STATIC int
-rend_service_check_dir_and_add(const or_options_t *options,
+rend_service_check_dir_and_add(smartlist_t *service_list,
+                               const or_options_t *options,
                                rend_service_t *service,
                                int validate_only)
 {
@@ -463,6 +481,22 @@ rend_service_check_dir_and_add(const or_options_t *options,
     return 0;
   }
 
+  smartlist_t *s_list = NULL;
+  /* If no special service list is provided, then just use the global one. */
+  if (!service_list) {
+    if (!rend_service_list) {
+      /* No global HS list, which is a failure if we plan on adding to it */
+      if (BUG(!validate_only)) {
+        return -1;
+      }
+      /* Otherwise, we validate, */
+    }
+
+    s_list = rend_service_list;
+  } else {
+    s_list = service_list;
+  }
+
   if (rend_service_check_private_dir(options, service, !validate_only)
       < 0) {
     rend_service_free(service);
@@ -474,8 +508,12 @@ rend_service_check_dir_and_add(const or_options_t *options,
     return 0;
   } else {
     /* rend_add_service takes ownership, either adding or freeing the service
+     * s_list can not be NULL here - if both service_list and rend_service_list
+     * are NULL, and validate_only is false, we exit earlier in the function
      */
-    rend_add_service(service);
+    tor_assert(s_list);
+    /* Ignore service failures until 030 */
+    rend_add_service(s_list, service);
     return 0;
   }
 }
@@ -504,8 +542,8 @@ rend_config_services(const or_options_t *options, int validate_only)
       /* register the service we just finished parsing
        * this code registers every service except the last one parsed,
        * which is registered below the loop */
-      if (rend_service_check_dir_and_add(options, service, !validate_only)
+      if (rend_service_check_dir_and_add(NULL, options, service,
-          < 0) {
+                                         validate_only) < 0) {
           return -1;
       }
       service = tor_malloc_zero(sizeof(rend_service_t));
@@ -715,8 +753,8 @@ rend_config_services(const or_options_t *options, int validate_only)
   /* register the final service after we have finished parsing all services
    * this code only registers the last service, other services are registered
    * within the loop */
-  if (rend_service_check_dir_and_add(options, service, !validate_only)
+  if (rend_service_check_dir_and_add(NULL, options, service,
-      < 0) {
+                                     validate_only) < 0) {
     return -1;
   }
 
@@ -874,7 +912,7 @@ rend_service_add_ephemeral(crypto_pk_t *pk,
   }
 
   /* Initialize the service. */
-  if (rend_add_service(s)) {
+  if (rend_add_service(NULL, s)) {
     return RSAE_INTERNAL;
   }
   *service_id_out = tor_strdup(s->service_id);
@@ -1310,6 +1348,7 @@ rend_service_check_private_dir(const or_options_t *options,
   }
   /* Check/create directory */
   if (check_private_dir(s->directory, check_opts, options->User) < 0) {
+    log_warn(LD_REND, "Checking service directory %s failed.", s->directory);
     return -1;
   }
   return 0;

src/or/rendservice.h

@@ -119,6 +119,10 @@ typedef struct rend_service_t {
 
 STATIC void rend_service_free(rend_service_t *service);
 STATIC char *rend_service_sos_poison_path(const rend_service_t *service);
+STATIC int rend_service_check_dir_and_add(smartlist_t *service_list,
+                                          const or_options_t *options,
+                                          rend_service_t *service,
+                                          int validate_only);
 
 #endif
 

src/test/test_hs.c

@@ -571,8 +571,28 @@ test_single_onion_poisoning(void *arg)
 
   service_1->directory = dir1;
   service_2->directory = dir2;
+  /* The services own the directory pointers now */
   dir1 = dir2 = NULL;
-  smartlist_add(services, service_1);
+  /* Add port to service 1 */
+  service_1->ports = smartlist_new();
+  service_2->ports = smartlist_new();
+  char *err_msg = NULL;
+  rend_service_port_config_t *port1 = rend_service_parse_port_config("80", " ",
+                                                                     &err_msg);
+  tt_assert(port1);
+  tt_assert(!err_msg);
+  smartlist_add(service_1->ports, port1);
+
+  rend_service_port_config_t *port2 = rend_service_parse_port_config("90", " ",
+                                                                     &err_msg);
+  /* Add port to service 2 */
+  tt_assert(port2);
+  tt_assert(!err_msg);
+  smartlist_add(service_2->ports, port2);
+
+  /* Add the first service */
+  ret = rend_service_check_dir_and_add(services, mock_options, service_1, 0);
+  tt_assert(ret == 0);
   /* But don't add the second service yet. */
 
   /* Service directories, but no previous keys, no problem! */
@@ -636,7 +656,7 @@ test_single_onion_poisoning(void *arg)
   tt_assert(ret == 0);
 
   /* Now add the second service: it has no key and no poison file */
-  smartlist_add(services, service_2);
+  ret = rend_service_check_dir_and_add(services, mock_options, service_2, 0);
 
   /* A new service, and an existing poisoned service. Not ok. */
   mock_options->HiddenServiceSingleHopMode = 0;
@@ -698,13 +718,11 @@ test_single_onion_poisoning(void *arg)
 
  done:
   /* The test harness deletes the directories at exit */
+  smartlist_free(services);
   rend_service_free(service_1);
   rend_service_free(service_2);
-  smartlist_free(services);
   UNMOCK(get_options);
   tor_free(mock_options->DataDirectory);
-  tor_free(dir1);
-  tor_free(dir2);
 }
 
 struct testcase_t hs_tests[] = {