Update unit tests for 20484, 20529

Add extra logging and extra validity checks for hidden services.

1 files changed, 56 insertions, 17 deletions

diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index d4d2405cc8..91844e8543 100644
--- a/src/or/rendservice.c
+++ b/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. */
-      SMARTLIST_FOREACH(rend_service_list, rend_service_t*, ptr,
+    if (service->directory != NULL) {
+      /* 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
- * list if <b>validate_only</b> is false.
+/* Check the directory for <b>service</b>, and add the service to
+ * <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
-rend_service_check_dir_and_add(const or_options_t *options,
+STATIC int
+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)
-          < 0) {
+      if (rend_service_check_dir_and_add(NULL, options, service,
+                                         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)
-      < 0) {
+  if (rend_service_check_dir_and_add(NULL, options, service,
+                                     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;