Use a temporary service list when validating and adding hidden services

This resolves two issues:
* the checks in rend_add_services were only being performed when adding
  the service, and not when the service was validated,
  (this meant that duplicate checks were not being performed, and some SETCONF
  commands appeared to succeed when they actually failed), and
* if one service failed while services were being added, then the service
  list would be left in an inconsistent state (tor dies when this happens,
  but the code is cleaner now).

Fixes #20860.

changes/bug20860

@@ -0,0 +1,4 @@
+  o Minor bugfixes (hidden services):
+    - Stop ignoring duplicate hidden services when validating: this could
+      lead to a crash when those services were created.
+      Fixes bug 20860; bugfix on 20559; not in any released version of tor.

src/or/rendservice.c

@@ -275,8 +275,11 @@ rend_add_service(smartlist_t *service_list, rend_service_t *service)
   int i;
   rend_service_port_config_t *p;
 
-  /* Use service_list for unit tests */
+  tor_assert(service);
+
   smartlist_t *s_list = rend_get_service_list_mutable(service_list);
+  /* We must have a service list, even if it's a temporary one, so we can
+   * check for duplicate services */
   if (BUG(!s_list)) {
     return -1;
   }
@@ -333,6 +336,7 @@ rend_add_service(smartlist_t *service_list, rend_service_t *service)
      * lock file.  But this is enough to detect a simple mistake that
      * at least one person has actually made.
      */
+    tor_assert(s_list);
     if (!rend_service_is_ephemeral(service)) {
       /* Skip dupe for ephemeral services. */
       SMARTLIST_FOREACH(s_list, rend_service_t*, ptr,
@@ -371,6 +375,8 @@ rend_add_service(smartlist_t *service_list, rend_service_t *service)
 #endif /* defined(HAVE_SYS_UN_H) */
       }
     }
+    /* The service passed all the checks */
+    tor_assert(s_list);
     smartlist_add(s_list, service);
     return 0;
   }
@@ -527,20 +533,13 @@ rend_service_check_dir_and_add(smartlist_t *service_list,
     return -1;
   }
 
-  if (validate_only) {
-    rend_service_free(service);
-    return 0;
-  } else {
-    /* Use service_list for unit tests */
-    smartlist_t *s_list = rend_get_service_list_mutable(service_list);
-    /* 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
-     */
-    if (BUG(!s_list)) {
-      return -1;
-    }
-    return rend_add_service(s_list, service);
+  smartlist_t *s_list = rend_get_service_list_mutable(service_list);
+  /* We must have a service list, even if it's a temporary one, so we can
+   * check for duplicate services */
+  if (BUG(!s_list)) {
+    return -1;
   }
+  return rend_add_service(s_list, service);
 }
 
 /** Set up rend_service_list, based on the values of HiddenServiceDir and
@@ -555,19 +554,19 @@ rend_config_services(const or_options_t *options, int validate_only)
   rend_service_t *service = NULL;
   rend_service_port_config_t *portcfg;
   smartlist_t *old_service_list = NULL;
+  smartlist_t *temp_service_list = NULL;
   int ok = 0;
 
-  if (!validate_only) {
-    old_service_list = rend_service_list;
-    rend_service_list = smartlist_new();
-  }
+  /* Use a temporary service list, so that we can check the new services'
+   * consistency with each other */
+  temp_service_list = smartlist_new();
 
   for (line = options->RendConfigLines; line; line = line->next) {
     if (!strcasecmp(line->key, "HiddenServiceDir")) {
       /* 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(NULL, options, service,
+      if (rend_service_check_dir_and_add(temp_service_list, options, service,
                                          validate_only) < 0) {
           return -1;
       }
@@ -783,11 +782,27 @@ 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. It is ok for this service to be NULL, it is ignored. */
-  if (rend_service_check_dir_and_add(NULL, options, service,
+  if (rend_service_check_dir_and_add(temp_service_list, options, service,
                                      validate_only) < 0) {
     return -1;
   }
 
+  /* Free the newly added services if validating */
+  if (validate_only) {
+    SMARTLIST_FOREACH(temp_service_list, rend_service_t *, ptr,
+                      rend_service_free(ptr));
+    smartlist_free(temp_service_list);
+    temp_service_list = NULL;
+    return 0;
+  }
+
+  /* Otherwise, use the newly added services as the new service list
+   * Since we have now replaced the global service list, from this point on we
+   * must succeed, or die trying. */
+  old_service_list = rend_service_list;
+  rend_service_list = temp_service_list;
+  temp_service_list = NULL;
+
   /* 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. */