Add new config.c function to set options that can fail, and roll back if they do. This should solve the setconf-an-impossible-port bug.

svn:r5046

src/or/config.c

@@ -266,6 +266,7 @@ static int option_is_same(config_format_t *fmt,
                           or_options_t *o1, or_options_t *o2, const char *name);
 static or_options_t *options_dup(config_format_t *fmt, or_options_t *old);
 static int options_validate(or_options_t *options);
+static int options_act_reversible(or_options_t *old_options);
 static int options_act(or_options_t *old_options);
 static int options_transition_allowed(or_options_t *old, or_options_t *new);
 static int options_transition_affects_workers(or_options_t *old_options,
@@ -363,19 +364,25 @@ get_options(void)
  * their current value; take action based on the new value; free the old value
  * as necessary.
  */
-void
+int
 set_options(or_options_t *new_val)
 {
   or_options_t *old_options = global_options;
   global_options = new_val;
   /* Note that we pass the *old* options below, for comparison. It
    * pulls the new options directly out of global_options. */
+  if (options_act_reversible(old_options)<0) {
+    global_options = old_options;
+    return -1;
+  }
   if (options_act(old_options) < 0) { /* acting on the options failed. die. */
     log_fn(LOG_ERR,"Acting on config options left us in a broken state. Dying.");
     exit(1);
   }
   if (old_options)
     config_free(&options_format, old_options);
+
+  return 0;
 }
 
 void
@@ -399,6 +406,98 @@ safe_str(const char *address)
     return address;
 }
 
+/** Fetch the active option list, and take actions based on it. All of the
+ * things we do should survive being done repeatedly.  If present,
+ * <b>old_options</b> contains the previous value of the options.
+ *
+ * Return 0 if all goes well, return -1 if things went badly.
+ */
+static int
+options_act_reversible(or_options_t *old_options)
+{
+  smartlist_t *new_listeners = smartlist_create();
+  smartlist_t *replaced_listeners = smartlist_create();
+  static int libevent_initialized = 0;
+  or_options_t *options = get_options();
+  int running_tor = options->command == CMD_RUN_TOR;
+  int set_conn_limit = 0;
+  int r = -1;
+
+  if (running_tor && options->RunAsDaemon) {
+    /* No need to roll back, since you can't change the value. */
+    start_daemon();
+  }
+
+  /* Setuid/setgid as appropriate */
+  if (options->User || options->Group) {
+    if (switch_id(options->User, options->Group) != 0) {
+      /* No need to roll back, since you can't change the value. */
+      goto done;
+    }
+  }
+
+  /* Set up libevent. */
+  if (running_tor && !libevent_initialized) {
+    if (init_libevent())
+      goto done;
+    libevent_initialized = 1;
+  }
+
+  /* Ensure data directory is private; create if possible. */
+  if (check_private_dir(options->DataDirectory, CPD_CREATE)<0) {
+    log_fn(LOG_ERR, "Couldn't access/create private data directory \"%s\"",
+           options->DataDirectory);
+    /* No need to roll back, since you can't change the value. */
+    goto done;
+  }
+
+  /* Bail out at this point if we're not going to be a client or server:
+   * we don't run  */
+  if (options->command != CMD_RUN_TOR)
+    goto commit;
+
+  options->_ConnLimit =
+    set_max_file_descriptors((unsigned)options->ConnLimit, MAXCONNECTIONS);
+  if (options->_ConnLimit < 0)
+    goto rollback;
+  set_conn_limit = 1;
+
+  if (retry_all_listeners(0, replaced_listeners, new_listeners) < 0) {
+    log_fn(LOG_ERR,"Failed to bind one of the listener ports.");
+    goto rollback;
+  }
+
+ commit:
+  r = 0;
+  SMARTLIST_FOREACH(replaced_listeners, connection_t *, conn,
+  {
+    log_fn(LOG_NOTICE, "Closing old %s on %s:%d",
+           conn_type_to_string(conn->type), conn->address, conn->port);
+    connection_close_immediate(conn);
+    connection_mark_for_close(conn);
+  });
+  goto done;
+
+ rollback:
+  r = -1;
+
+  if (set_conn_limit && old_options)
+    set_max_file_descriptors((unsigned)old_options->ConnLimit,MAXCONNECTIONS);
+
+  SMARTLIST_FOREACH(new_listeners, connection_t *, conn,
+  {
+    log_fn(LOG_NOTICE, "Closing %s on %s:%d",
+           conn_type_to_string(conn->type), conn->address, conn->port);
+    connection_close_immediate(conn);
+    connection_mark_for_close(conn);
+  });
+
+ done:
+  smartlist_free(new_listeners);
+  smartlist_free(replaced_listeners);
+  return r;
+}
+
 /** Fetch the active option list, and take actions based on it. All of the
  * things we do should survive being done repeatedly.  If present,
  * <b>old_options</b> contains the previous value of the options.
@@ -416,11 +515,6 @@ options_act(or_options_t *old_options)
   size_t len;
   or_options_t *options = get_options();
   int running_tor = options->command == CMD_RUN_TOR;
-  static int libevent_initialized = 0;
-
-  if (running_tor && options->RunAsDaemon) {
-    start_daemon();
-  }
 
   clear_trusted_dir_servers();
   for (cl = options->DirServers; cl; cl = cl->next) {
@@ -440,19 +534,6 @@ options_act(or_options_t *old_options)
   if (options->EntryNodes && strlen(options->EntryNodes))
     options->UseHelperNodes = 0;
 
-  /* Setuid/setgid as appropriate */
-  if (options->User || options->Group) {
-    if (switch_id(options->User, options->Group) != 0) {
-      return -1;
-    }
-  }
-
-  /* Ensure data directory is private; create if possible. */
-  if (check_private_dir(options->DataDirectory, CPD_CREATE) != 0) {
-    log_fn(LOG_ERR, "Couldn't access/create private data directory \"%s\"",
-           options->DataDirectory);
-    return -1;
-  }
   if (running_tor) {
     len = strlen(options->DataDirectory)+32;
     fn = tor_malloc(len);
@@ -481,23 +562,11 @@ options_act(or_options_t *old_options)
   add_callback_log(LOG_ERR, LOG_ERR, control_event_logmsg);
   control_adjust_event_log_severity();
 
-  /* Set up libevent. */
-  if (running_tor && !libevent_initialized) {
-    if (init_libevent())
-      return -1;
-    libevent_initialized = 1;
-  }
-
   /* Load state */
   if (! global_state)
     if (or_state_load())
       return -1;
 
-  options->_ConnLimit =
-    set_max_file_descriptors((unsigned)options->ConnLimit, MAXCONNECTIONS);
-  if (options->_ConnLimit < 0)
-    return -1;
-
   {
     smartlist_t *sl = smartlist_create();
     for (cl = options->RedirectExit; cl; cl = cl->next) {
@@ -545,11 +614,6 @@ options_act(or_options_t *old_options)
   if (!running_tor)
     return 0;
 
-  if (!we_are_hibernating() && retry_all_listeners(0) < 0) {
-    log_fn(LOG_ERR,"Failed to bind one of the listener ports.");
-    return -1;
-  }
-
   /* Check for transitions that need action. */
   if (old_options) {
     if (options_transition_affects_workers(old_options, options)) {
@@ -1138,7 +1202,7 @@ config_assign(config_format_t *fmt, void *options,
  * options, assigning list to the new one, then validating it. If it's
  * ok, then throw out the old one and stick with the new one. Else,
  * revert to old and return failure.  Return 0 on success, -1 on bad
- * keys, -2 on bad values, -3 on bad transition.
+ * keys, -2 on bad values, -3 on bad transition, and -4 on failed-to-set.
  */
 int
 options_trial_assign(config_line_t *list, int use_defaults, int clear_first)
@@ -1162,7 +1226,12 @@ options_trial_assign(config_line_t *list, int use_defaults, int clear_first)
     return -3;
   }
 
-  set_options(trial_options); /* we liked it. put it in place. */
+  if (set_options(trial_options)<0) {
+    config_free(&options_format, trial_options);
+    return -4;
+  }
+
+  /* we liked it. put it in place. */
   return 0;
 }
 
@@ -2332,7 +2401,8 @@ options_init_from_torrc(int argc, char **argv)
   if (options_transition_allowed(oldoptions, newoptions) < 0)
     goto err;
 
-  set_options(newoptions); /* frees and replaces old options */
+  if (set_options(newoptions))
+    goto err; /* frees and replaces old options */
   tor_free(torrc_fname);
   torrc_fname = fname;
   return 0;

src/or/connection.c

@@ -13,8 +13,8 @@ const char connection_c_id[] = "$Id$";
 
 #include "or.h"
 
-static int connection_create_listener(const char *bindaddress,
-                                      uint16_t bindport, int type);
+static connection_t *connection_create_listener(const char *bindaddress,
+                                                uint16_t bindport, int type);
 static int connection_init_accepted_conn(connection_t *conn);
 static int connection_handle_listener_read(connection_t *conn, int new_type);
 static int connection_receiver_bucket_should_increase(connection_t *conn);
@@ -474,8 +474,9 @@ connection_expire_held_open(void)
  * If <b>bindaddress</b> includes a port, we bind on that port; otherwise, we
  * use bindport.
  */
-static int
-connection_create_listener(const char *bindaddress, uint16_t bindport, int type)
+static connection_t *
+connection_create_listener(const char *bindaddress, uint16_t bindport,
+                           int type)
 {
   struct sockaddr_in bindaddr; /* where to bind */
   char *address = NULL;
@@ -490,7 +491,7 @@ connection_create_listener(const char *bindaddress, uint16_t bindport, int type)
   memset(&bindaddr,0,sizeof(struct sockaddr_in));
   if (parse_addr_port(bindaddress, &address, &addr, &usePort)<0) {
     log_fn(LOG_WARN, "Error parsing/resolving BindAddress %s",bindaddress);
-    return -1;
+    return NULL;
   }
 
   if (usePort==0)
@@ -551,11 +552,11 @@ connection_create_listener(const char *bindaddress, uint16_t bindport, int type)
   conn->state = LISTENER_STATE_READY;
   connection_start_reading(conn);
 
-  return 0;
+  return conn;
 
  err:
   tor_free(address);
-  return -1;
+  return NULL;
 }
 
 /** Do basic sanity checking on a newly received socket. Return 0
@@ -804,10 +805,15 @@ connection_connect(connection_t *conn, char *address,
  * Otherwise, only relaunch the listeners of this type if the number of
  * existing connections is not as configured (e.g., because one died),
  * or if the existing connections do not match those configured.
+ *
+ * Add all old conns that should be closed to <b>replaced_conns</b>.
+ * Add all new connections to <b>new_conns</b>.
  */
 static int
 retry_listeners(int type, config_line_t *cfg,
-                int port_option, const char *default_addr, int force)
+                int port_option, const char *default_addr, int force,
+                smartlist_t *replaced_conns,
+                smartlist_t *new_conns)
 {
   smartlist_t *launch = smartlist_create();
   int free_launch_elts = 1;
@@ -829,6 +835,11 @@ retry_listeners(int type, config_line_t *cfg,
     smartlist_add(launch, line);
   }
 
+  /*
+  SMARTLIST_FOREACH(launch, config_line_t *, l,
+                    log_fn(LOG_NOTICE, "#%s#%s", l->key, l->value));
+  */
+
   get_connection_array(&carray,&n_conn);
   for (i=0; i < n_conn; ++i) {
     conn = carray[i];
@@ -862,8 +873,12 @@ retry_listeners(int type, config_line_t *cfg,
       /* This one isn't configured. Close it. */
       log_fn(LOG_NOTICE, "Closing %s on %s:%d",
              conn_type_to_string(type), conn->address, conn->port);
-      connection_close_immediate(conn);
-      connection_mark_for_close(conn);
+      if (replaced_conns) {
+        smartlist_add(replaced_conns, conn);
+      } else {
+        connection_close_immediate(conn);
+        connection_mark_for_close(conn);
+      }
     } else {
       /* It's configured; we don't need to launch it. */
       log_fn(LOG_INFO, "Already have %s on %s:%d",
@@ -876,9 +891,14 @@ retry_listeners(int type, config_line_t *cfg,
   i = 0;
   SMARTLIST_FOREACH(launch, config_line_t *, cfg,
     {
-      if (connection_create_listener(cfg->value, (uint16_t) port_option,
-                                     type)<0)
+      conn = connection_create_listener(cfg->value, (uint16_t) port_option,
+                                        type);
+      if (!conn) {
         i = -1;
+      } else {
+        if (new_conns)
+          smartlist_add(new_conns, conn);
+      }
   });
 
   if (free_launch_elts) {
@@ -894,24 +914,32 @@ retry_listeners(int type, config_line_t *cfg,
  * <b>force</b> is true, close and relaunch all listeners. If <b>force</b>
  * is false, then only relaunch listeners when we have the wrong number of
  * connections for a given type.
+ *
+ * Add all old conns that should be closed to <b>replaced_conns</b>.
+ * Add all new connections to <b>new_conns</b>.
  */
 int
-retry_all_listeners(int force)
+retry_all_listeners(int force, smartlist_t *replaced_conns,
+                    smartlist_t *new_conns)
 {
   or_options_t *options = get_options();
 
   if (server_mode(options) &&
       retry_listeners(CONN_TYPE_OR_LISTENER, options->ORBindAddress,
-                      options->ORPort, "0.0.0.0", force)<0)
+                      options->ORPort, "0.0.0.0", force,
+                      replaced_conns, new_conns)<0)
     return -1;
   if (retry_listeners(CONN_TYPE_DIR_LISTENER, options->DirBindAddress,
-                      options->DirPort, "0.0.0.0", force)<0)
+                      options->DirPort, "0.0.0.0", force,
+                      replaced_conns, new_conns)<0)
     return -1;
   if (retry_listeners(CONN_TYPE_AP_LISTENER, options->SocksBindAddress,
-                      options->SocksPort, "127.0.0.1", force)<0)
+                      options->SocksPort, "127.0.0.1", force,
+                      replaced_conns, new_conns)<0)
     return -1;
   if (retry_listeners(CONN_TYPE_CONTROL_LISTENER, NULL,
-                      options->ControlPort, "127.0.0.1", force)<0)
+                      options->ControlPort, "127.0.0.1", force,
+                      replaced_conns, new_conns)<0)
     return -1;
 
   return 0;

src/or/control.c

@@ -668,18 +668,32 @@ control_setconf_helper(connection_t *conn, uint32_t len, char *body,
   }
 
   if ((r=options_trial_assign(lines, use_defaults, clear_first)) < 0) {
+    int v0_err;
+    const char *msg;
     log_fn(LOG_WARN,"Controller gave us config lines that didn't validate.");
-    if (r==-1) {
-      if (v0)
-        send_control0_error(conn, ERR_UNRECOGNIZED_CONFIG_KEY,
-                            "Unrecognized option");
-      else
-        connection_write_str_to_buf("552 Unrecognized option\r\n", conn);
+    switch (r) {
+      case -1:
+        v0_err = ERR_UNRECOGNIZED_CONFIG_KEY;
+        msg = "Unrecognized option";
+        break;
+      case -2:
+        v0_err = ERR_INVALID_CONFIG_VALUE;
+        msg = "Unrecognized option value";
+        break;
+      case -3:
+        v0_err = ERR_INVALID_CONFIG_VALUE;
+        msg = "Transition not allowed";
+        break;
+      case -4:
+      default:
+        v0_err = ERR_INVALID_CONFIG_VALUE;
+        msg = "Unable to set option";
+        break;
+    }
+    if (v0) {
+      send_control0_error(conn, v0_err, msg);
     } else {
-      if (v0)
-        send_control0_error(conn,ERR_INVALID_CONFIG_VALUE,"Invalid option value");
-      else
-        connection_write_str_to_buf("552 Invalid option value\r\n", conn);
+      connection_printf_to_buf(conn, "552 %s\r\n", msg);
     }
     config_free_lines(lines);
     return 0;

src/or/main.c

@@ -760,7 +760,8 @@ run_scheduled_events(time_t now)
 
   /** 3d. And every 60 seconds, we relaunch listeners if any died. */
   if (!we_are_hibernating() && time_to_check_listeners < now) {
-    retry_all_listeners(0); /* 0 means "only if some died." */
+    /* 0 means "only launch the ones that died." */
+    retry_all_listeners(0, NULL, NULL);
     time_to_check_listeners = now+60;
   }
 

src/or/or.h

@@ -1439,7 +1439,7 @@ extern unsigned long stats_n_destroy_cells_processed;
 /********************************* config.c ***************************/
 
 or_options_t *get_options(void);
-void set_options(or_options_t *new_val);
+int set_options(or_options_t *new_val);
 void config_free_all(void);
 const char *safe_str(const char *address);
 
@@ -1492,7 +1492,8 @@ void _connection_mark_for_close(connection_t *conn,int line, const char *file);
 void connection_expire_held_open(void);
 
 int connection_connect(connection_t *conn, char *address, uint32_t addr, uint16_t port);
-int retry_all_listeners(int force);
+int retry_all_listeners(int force, smartlist_t *replaced_conns,
+                        smartlist_t *new_conns);
 
 void connection_bucket_init(void);
 void connection_bucket_refill(struct timeval *now);