[tor-commits] [tor/master] Use a temporary service list when validating and adding hidden services

[nickm at torproject.org]

commit 8a0ea3ee43da0063c2546092662fa7ce4900bc2c
Author: teor <teor2345 at gmail.com>
Date:   Sat Dec 3 08:25:50 2016 +1100

    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     |  4 ++++
 src/or/rendservice.c | 55 +++++++++++++++++++++++++++++++++-------------------
 2 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/changes/bug20860 b/changes/bug20860
new file mode 100644
index 0000000..81b0dd8
--- /dev/null
+++ b/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.
diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index e3b43d9..beaa0a3 100644
--- a/src/or/rendservice.c
+++ b/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. */