Merge branch 'reversible_3'

changes/ticket27992

@@ -0,0 +1,5 @@
+  o Minor bugfixes (configuration):
+    - When creating a KeyDirectory with the same location as the
+      DataDirectory (not recommended), respect the DataDirectory's
+      group-readable setting if one has not been set for the KeyDirectory.
+      Fixes bug 27992; bugfix on 0.3.3.1-alpha.

doc/tor.1.txt

@@ -2595,10 +2595,12 @@ is non-zero):
     running.
     (Default: the "keys" subdirectory of DataDirectory.)
 
-[[KeyDirectoryGroupReadable]] **KeyDirectoryGroupReadable** **0**|**1**::
+[[KeyDirectoryGroupReadable]] **KeyDirectoryGroupReadable** **0**|**1**|**auto**::
     If this option is set to 0, don't allow the filesystem group to read the
-    KeywDirectory. If the option is set to 1, make the KeyDirectory readable
+    KeyDirectory. If the option is set to 1, make the KeyDirectory readable
-    by the default GID. (Default: 0)
+    by the default GID. If the option is "auto", then we use the
+    setting for DataDirectoryGroupReadable when the KeyDirectory is the
+    same as the DataDirectory, and 0 otherwise. (Default: auto)
 
 [[RephistTrackTime]] **RephistTrackTime** __N__ **seconds**|**minutes**|**hours**|**days**|**weeks**::
     Tells an authority, or other node tracking node reliability and history,

scripts/maint/practracker/exceptions.txt

@@ -33,7 +33,7 @@
 #
 # Remember: It is better to fix the problem than to add a new exception!
 
-problem file-size /src/app/config/config.c 7212
+problem file-size /src/app/config/config.c 7400
 problem include-count /src/app/config/config.c 80
 problem function-size /src/app/config/config.c:options_act_reversible() 298
 problem function-size /src/app/config/config.c:options_act() 381

src/app/config/config.c

@@ -540,7 +540,7 @@ static const config_var_t option_vars_[] = {
   V(Socks5ProxyUsername,         STRING,   NULL),
   V(Socks5ProxyPassword,         STRING,   NULL),
   VAR_IMMUTABLE("KeyDirectory",  FILENAME, KeyDirectory_option, NULL),
-  V(KeyDirectoryGroupReadable,   BOOL,     "0"),
+  V(KeyDirectoryGroupReadable,   AUTOBOOL, "auto"),
   VAR_D("HSLayer2Nodes",         ROUTERSET,  HSLayer2Nodes,  NULL),
   VAR_D("HSLayer3Nodes",         ROUTERSET,  HSLayer3Nodes,  NULL),
   V(KeepalivePeriod,             INTERVAL, "5 minutes"),
@@ -862,6 +862,9 @@ static void options_clear_cb(const config_mgr_t *mgr, void *opts);
 static setopt_err_t options_validate_and_set(const or_options_t *old_options,
                                              or_options_t *new_options,
                                              char **msg_out);
+struct listener_transaction_t;
+static void options_rollback_listener_transaction(
+                                           struct listener_transaction_t *xn);
 
 /** Magic value for or_options_t. */
 #define OR_OPTIONS_MAGIC 9090909
@@ -904,8 +907,8 @@ static smartlist_t *configured_ports = NULL;
 /** True iff we're currently validating options, and any calls to
  * get_options() are likely to be bugs. */
 static int in_option_validation = 0;
-/* True iff we've initialized libevent */
+/** True iff we have run options_act_once_on_startup() */
-static int libevent_initialized = 0;
+static bool have_set_startup_options = false;
 
 /* A global configuration manager to handle all configuration objects. */
 static config_mgr_t *options_mgr = NULL;
@@ -1085,7 +1088,7 @@ config_free_all(void)
 
   cleanup_protocol_warning_severity_level();
 
-  libevent_initialized = 0;
+  have_set_startup_options = false;
 
   config_mgr_free(options_mgr);
 }
@@ -1422,27 +1425,24 @@ create_keys_directory(const or_options_t *options)
 /* Helps determine flags to pass to switch_id. */
 static int have_low_ports = -1;
 
-/** Fetch the active option list, and take actions based on it. All of the
+/** Take case of initial startup tasks that must occur before any of the
- * things we do should survive being done repeatedly.  If present,
+ * transactional option-related changes are allowed. */
- * <b>old_options</b> contains the previous value of the options.
+static int
- *
+options_act_once_on_startup(char **msg_out)
- * Return 0 if all goes well, return -1 if things went badly.
- */
-MOCK_IMPL(STATIC int,
-options_act_reversible,(const or_options_t *old_options, char **msg))
 {
-  smartlist_t *new_listeners = smartlist_new();
+  if (have_set_startup_options)
-  or_options_t *options = get_options_mutable();
+    return 0;
-  int running_tor = options->command == CMD_RUN_TOR;
+
-  int set_conn_limit = 0;
+  const or_options_t *options = get_options();
-  int r = -1;
+  const bool running_tor = options->command == CMD_RUN_TOR;
-  int logs_marked = 0, logs_initialized = 0;
+
-  int old_min_log_level = get_min_log_level();
+  if (!running_tor)
+    return 0;
 
   /* Daemonize _first_, since we only want to open most of this stuff in
    * the subprocess.  Libevent bases can't be reliably inherited across
    * processes. */
-  if (running_tor && options->RunAsDaemon) {
+  if (options->RunAsDaemon) {
     if (! start_daemon_has_been_called())
       subsystems_prefork();
     /* No need to roll back, since you can't change the value. */
@@ -1455,41 +1455,9 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
   sd_notifyf(0, "MAINPID=%ld\n", (long int)getpid());
 #endif
 
-#ifndef HAVE_SYS_UN_H
-  if (options->ControlSocket || options->ControlSocketsGroupWritable) {
-    *msg = tor_strdup("Unix domain sockets (ControlSocket) not supported "
-                      "on this OS/with this build.");
-    goto rollback;
-  }
-#else /* defined(HAVE_SYS_UN_H) */
-  if (options->ControlSocketsGroupWritable && !options->ControlSocket) {
-    *msg = tor_strdup("Setting ControlSocketGroupWritable without setting"
-                      "a ControlSocket makes no sense.");
-    goto rollback;
-  }
-#endif /* !defined(HAVE_SYS_UN_H) */
-
-  if (running_tor) {
-    int n_ports=0;
-    /* We need to set the connection limit before we can open the listeners. */
-    if (! sandbox_is_active()) {
-      if (set_max_file_descriptors((unsigned)options->ConnLimit,
-                                   &options->ConnLimit_) < 0) {
-        *msg = tor_strdup("Problem with ConnLimit value. "
-                          "See logs for details.");
-        goto rollback;
-      }
-      set_conn_limit = 1;
-    } else {
-      tor_assert(old_options);
-      options->ConnLimit_ = old_options->ConnLimit_;
-    }
-
   /* Set up libevent.  (We need to do this before we can register the
    * listeners as listeners.) */
-    if (running_tor && !libevent_initialized) {
   init_libevent(options);
-      libevent_initialized = 1;
 
   /* This has to come up after libevent is initialized. */
   control_initialize_event_queue();
@@ -1501,61 +1469,29 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
    * code!  It also needs to happen before init_keys(), so it needs to
    * happen here too.  How yucky. */
   scheduler_init();
-    }
 
-    /* Adjust the port configuration so we can launch listeners. */
+  /* Attempt to lock all current and future memory with mlockall() only once.
-    /* 31851: some ports are relay-only */
+   * This must happen before setuid. */
-    if (parse_ports(options, 0, msg, &n_ports, NULL)) {
-      if (!*msg)
-        *msg = tor_strdup("Unexpected problem parsing port config");
-      goto rollback;
-    }
-
-    /* Set the hibernation state appropriately.*/
-    consider_hibernation(time(NULL));
-
-    /* Launch the listeners.  (We do this before we setuid, so we can bind to
-     * ports under 1024.)  We don't want to rebind if we're hibernating or
-     * shutting down. If networking is disabled, this will close all but the
-     * control listeners, but disable those. */
-    /* 31851: some listeners are relay-only */
-    if (!we_are_hibernating()) {
-      if (retry_all_listeners(new_listeners, options->DisableNetwork) < 0) {
-        *msg = tor_strdup("Failed to bind one of the listener ports.");
-        goto rollback;
-      }
-    }
-    if (options->DisableNetwork) {
-      /* Aggressively close non-controller stuff, NOW */
-      log_notice(LD_NET, "DisableNetwork is set. Tor will not make or accept "
-                 "non-control network connections. Shutting down all existing "
-                 "connections.");
-      connection_mark_all_noncontrol_connections();
-      /* We can't complete circuits until the network is re-enabled. */
-      note_that_we_maybe_cant_complete_circuits();
-    }
-  }
-
-#if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H)
-  /* Open /dev/pf before dropping privileges. */
-  if (options->TransPort_set &&
-      options->TransProxyType_parsed == TPT_DEFAULT) {
-    if (get_pf_socket() < 0) {
-      *msg = tor_strdup("Unable to open /dev/pf for transparent proxy.");
-      goto rollback;
-    }
-  }
-#endif /* defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) */
-
-  /* Attempt to lock all current and future memory with mlockall() only once */
   if (options->DisableAllSwap) {
     if (tor_mlockall() == -1) {
-      *msg = tor_strdup("DisableAllSwap failure. Do you have proper "
+      *msg_out = tor_strdup("DisableAllSwap failure. Do you have proper "
                         "permissions?");
-      goto done;
+      return -1;
     }
   }
 
+  have_set_startup_options = true;
+  return 0;
+}
+
+/**
+ * Change our user ID if we're configured to do so.
+ **/
+static int
+options_switch_id(char **msg_out)
+{
+  const or_options_t *options = get_options();
+
   /* Setuid/setgid as appropriate */
   if (options->User) {
     tor_assert(have_low_ports != -1);
@@ -1569,11 +1505,52 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
     }
     if (switch_id(options->User, switch_id_flags) != 0) {
       /* No need to roll back, since you can't change the value. */
-      *msg = tor_strdup("Problem with User value. See logs for details.");
+      *msg_out = tor_strdup("Problem with User value. See logs for details.");
-      goto done;
+      return -1;
     }
   }
 
+  return 0;
+}
+
+/**
+ * Helper. Given a data directory (<b>datadir</b>) and another directory
+ * (<b>subdir</b>) with respective group-writable permissions
+ * <b>datadir_gr</b> and <b>subdir_gr</b>, compute whether the subdir should
+ * be group-writeable.
+ **/
+static int
+compute_group_readable_flag(const char *datadir,
+                            const char *subdir,
+                            int datadir_gr,
+                            int subdir_gr)
+{
+  if (subdir_gr != -1) {
+    /* The user specified a default for "subdir", so we always obey it. */
+    return subdir_gr;
+  }
+
+  /* The user left the subdir_gr option on "auto." */
+  if (0 == strcmp(subdir, datadir)) {
+    /* The directories are the same, so we use the group-readable flag from
+     * the datadirectory */
+    return datadir_gr;
+  } else {
+    /* The directores are different, so we default to "not group-readable" */
+    return 0;
+  }
+}
+
+/**
+ * Create our DataDirectory, CacheDirectory, and KeyDirectory, and
+ * set their permissions correctly.
+ */
+STATIC int
+options_create_directories(char **msg_out)
+{
+  const or_options_t *options = get_options();
+  const bool running_tor = options->command == CMD_RUN_TOR;
+
   /* Ensure data directory is private; create if possible. */
   /* It's okay to do this in "options_act_reversible()" even though it isn't
    * actually reversible, since you can't change the DataDirectory while
@@ -1582,98 +1559,159 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
                                       options->DataDirectory,
                                       options->DataDirectoryGroupReadable,
                                       options->User,
-                                      msg) < 0) {
+                                      msg_out) < 0) {
-    goto done;
+    return -1;
-  }
-  if (check_and_create_data_directory(running_tor /* create */,
-                                      options->KeyDirectory,
-                                      options->KeyDirectoryGroupReadable,
-                                      options->User,
-                                      msg) < 0) {
-    goto done;
   }
 
-  /* We need to handle the group-readable flag for the cache directory
+  /* We need to handle the group-readable flag for the cache directory and key
-   * specially, since the directory defaults to being the same as the
+   * directory specially, since they may be the same as the data directory */
-   * DataDirectory. */
+  const int key_dir_group_readable = compute_group_readable_flag(
-  int cache_dir_group_readable;
+                                        options->DataDirectory,
-  if (options->CacheDirectoryGroupReadable != -1) {
+                                        options->KeyDirectory,
-    /* If the user specified a value, use their setting */
+                                        options->DataDirectoryGroupReadable,
-    cache_dir_group_readable = options->CacheDirectoryGroupReadable;
+                                        options->KeyDirectoryGroupReadable);
-  } else if (!strcmp(options->CacheDirectory, options->DataDirectory)) {
+
-    /* If the user left the value as "auto", and the cache is the same as the
+  if (check_and_create_data_directory(running_tor /* create */,
-     * datadirectory, use the datadirectory setting.
+                                      options->KeyDirectory,
-     */
+                                      key_dir_group_readable,
-    cache_dir_group_readable = options->DataDirectoryGroupReadable;
+                                      options->User,
-  } else {
+                                      msg_out) < 0) {
-    /* Otherwise, "auto" means "not group readable". */
+    return -1;
-    cache_dir_group_readable = 0;
   }
+
+  const int cache_dir_group_readable = compute_group_readable_flag(
+                                        options->DataDirectory,
+                                        options->CacheDirectory,
+                                        options->DataDirectoryGroupReadable,
+                                        options->CacheDirectoryGroupReadable);
+
   if (check_and_create_data_directory(running_tor /* create */,
                                       options->CacheDirectory,
                                       cache_dir_group_readable,
                                       options->User,
-                                      msg) < 0) {
+                                      msg_out) < 0) {
-    goto done;
+    return -1;
   }
 
-  /* Bail out at this point if we're not going to be a client or server:
+  return 0;
-   * we don't run Tor itself. */
+}
-  if (!running_tor)
-    goto commit;
 
-  mark_logs_temp(); /* Close current logs once new logs are open. */
+/** Structure to represent an incomplete configuration of a set of
-  logs_marked = 1;
+ * listeners.
-  /* Configure the tor_log(s) */
+ *
-  if (options_init_logs(old_options, options, 0)<0) {
+ * This structure is generated by options_start_listener_transaction(), and is
-    *msg = tor_strdup("Failed to init Log options. See logs for details.");
+ * either committed by options_commit_listener_transaction() or rolled back by
+ * options_rollback_listener_transaction(). */
+typedef struct listener_transaction_t {
+  bool set_conn_limit; /**< True if we've set the connection limit */
+  unsigned old_conn_limit; /**< If nonzero, previous connlimit value. */
+  smartlist_t *new_listeners; /**< List of new listeners that we opened. */
+} listener_transaction_t;
+
+/**
+ * Start configuring our listeners based on the current value of
+ * get_options().
+ *
+ * The value <b>old_options</b> holds either the previous options object,
+ * or NULL if we're starting for the first time.
+ *
+ * On success, return a listener_transaction_t that we can either roll back or
+ * commit.
+ *
+ * On failure return NULL and write a message into a newly allocated string in
+ * *<b>msg_out</b>.
+ **/
+static listener_transaction_t *
+options_start_listener_transaction(const or_options_t *old_options,
+                                   char **msg_out)
+{
+  listener_transaction_t *xn = tor_malloc_zero(sizeof(listener_transaction_t));
+  xn->new_listeners = smartlist_new();
+  or_options_t *options = get_options_mutable();
+  const bool running_tor = options->command == CMD_RUN_TOR;
+
+  if (! running_tor) {
+    return xn;
+  }
+
+  int n_ports=0;
+  /* We need to set the connection limit before we can open the listeners. */
+  if (! sandbox_is_active()) {
+    if (set_max_file_descriptors((unsigned)options->ConnLimit,
+                                 &options->ConnLimit_) < 0) {
+      *msg_out = tor_strdup("Problem with ConnLimit value. "
+                            "See logs for details.");
       goto rollback;
     }
-  logs_initialized = 1;
+    xn->set_conn_limit = true;
-
+    if (old_options)
- commit:
+      xn->old_conn_limit = (unsigned)old_options->ConnLimit;
-  r = 0;
+  } else {
-  if (logs_marked) {
+    tor_assert(old_options);
-    log_severity_list_t *severity =
+    options->ConnLimit_ = old_options->ConnLimit_;
-      tor_malloc_zero(sizeof(log_severity_list_t));
-    close_temp_logs();
-    add_callback_log(severity, control_event_logmsg);
-    logs_set_pending_callback_callback(control_event_logmsg_pending);
-    control_adjust_event_log_severity();
-    tor_free(severity);
-    tor_log_update_sigsafe_err_fds();
-  }
-  if (logs_initialized) {
-    flush_log_messages_from_startup();
   }
 
-  {
+  /* Adjust the port configuration so we can launch listeners. */
-    const char *badness = NULL;
+  /* 31851: some ports are relay-only */
-    int bad_safelog = 0, bad_severity = 0, new_badness = 0;
+  if (parse_ports(options, 0, msg_out, &n_ports, NULL)) {
-    if (options->SafeLogging_ != SAFELOG_SCRUB_ALL) {
+    if (!*msg_out)
-      bad_safelog = 1;
+      *msg_out = tor_strdup("Unexpected problem parsing port config");
-      if (!old_options || old_options->SafeLogging_ != options->SafeLogging_)
+    goto rollback;
-        new_badness = 1;
-    }
-    if (get_min_log_level() >= LOG_INFO) {
-      bad_severity = 1;
-      if (get_min_log_level() != old_min_log_level)
-        new_badness = 1;
-    }
-    if (bad_safelog && bad_severity)
-      badness = "you disabled SafeLogging, and "
-        "you're logging more than \"notice\"";
-    else if (bad_safelog)
-      badness = "you disabled SafeLogging";
-    else
-      badness = "you're logging more than \"notice\"";
-    if (new_badness)
-      log_warn(LD_GENERAL, "Your log may contain sensitive information - %s. "
-               "Don't log unless it serves an important reason. "
-               "Overwrite the log afterwards.", badness);
   }
 
-  if (set_conn_limit) {
+  /* Set the hibernation state appropriately.*/
+  consider_hibernation(time(NULL));
+
+  /* Launch the listeners.  (We do this before we setuid, so we can bind to
+   * ports under 1024.)  We don't want to rebind if we're hibernating or
+   * shutting down. If networking is disabled, this will close all but the
+   * control listeners, but disable those. */
+  /* 31851: some listeners are relay-only */
+  if (!we_are_hibernating()) {
+    if (retry_all_listeners(xn->new_listeners,
+                            options->DisableNetwork) < 0) {
+      *msg_out = tor_strdup("Failed to bind one of the listener ports.");
+      goto rollback;
+    }
+  }
+  if (options->DisableNetwork) {
+    /* Aggressively close non-controller stuff, NOW */
+    log_notice(LD_NET, "DisableNetwork is set. Tor will not make or accept "
+               "non-control network connections. Shutting down all existing "
+               "connections.");
+    connection_mark_all_noncontrol_connections();
+    /* We can't complete circuits until the network is re-enabled. */
+    note_that_we_maybe_cant_complete_circuits();
+  }
+
+#if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H)
+  /* Open /dev/pf before (possibly) dropping privileges. */
+  if (options->TransPort_set &&
+      options->TransProxyType_parsed == TPT_DEFAULT) {
+    if (get_pf_socket() < 0) {
+      *msg_out = tor_strdup("Unable to open /dev/pf for transparent proxy.");
+      goto rollback;
+    }
+  }
+#endif /* defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) */
+
+  return xn;
+
+ rollback:
+  options_rollback_listener_transaction(xn);
+  return NULL;
+}
+
+/**
+ * Finish configuring the listeners that started to get configured with
+ * <b>xn</b>.  Frees <b>xn</b>.
+ **/
+static void
+options_commit_listener_transaction(listener_transaction_t *xn)
+{
+  tor_assert(xn);
+  if (xn->set_conn_limit) {
+    or_options_t *options = get_options_mutable();
     /*
      * If we adjusted the conn limit, recompute the OOS threshold too
      *
@@ -1701,22 +1739,26 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
     connection_check_oos(get_n_open_sockets(), 0);
   }
 
-  goto done;
+  smartlist_free(xn->new_listeners);
+  tor_free(xn);
+}
 
- rollback:
+/**
-  r = -1;
+ * Revert the listener configuration changes that that started to get
-  tor_assert(*msg);
+ * configured with <b>xn</b>.  Frees <b>xn</b>.
+ **/
+static void
+options_rollback_listener_transaction(listener_transaction_t *xn)
+{
+  if (! xn)
+    return;
 
-  if (logs_marked) {
+  or_options_t *options = get_options_mutable();
-    rollback_log_changes();
-    control_adjust_event_log_severity();
-  }
 
-  if (set_conn_limit && old_options)
+  if (xn->set_conn_limit && xn->old_conn_limit)
-    set_max_file_descriptors((unsigned)old_options->ConnLimit,
+    set_max_file_descriptors(xn->old_conn_limit, &options->ConnLimit_);
-                             &options->ConnLimit_);
 
-  SMARTLIST_FOREACH(new_listeners, connection_t *, conn,
+  SMARTLIST_FOREACH(xn->new_listeners, connection_t *, conn,
   {
     log_notice(LD_NET, "Closing partially-constructed %s on %s:%d",
                conn_type_to_string(conn->type), conn->address, conn->port);
@@ -1724,8 +1766,220 @@ options_act_reversible,(const or_options_t *old_options, char **msg))
     connection_mark_for_close(conn);
   });
 
+  smartlist_free(xn->new_listeners);
+  tor_free(xn);
+}
+
+/** Structure to represent an incomplete configuration of a set of logs.
+ *
+ * This structure is generated by options_start_log_transaction(), and is
+ * either committed by options_commit_log_transaction() or rolled back by
+ * options_rollback_log_transaction(). */
+typedef struct log_transaction_t {
+  /** Previous lowest severity of any configured log. */
+  int old_min_log_level;
+  /** True if we have marked the previous logs to be closed */
+  bool logs_marked;
+  /** True if we initialized the new set of logs */
+  bool logs_initialized;
+  /** True if our safelogging configuration is different from what it was
+   * previously (or if we are starting for the first time). */
+  bool safelogging_changed;
+} log_transaction_t;
+
+/**
+ * Start configuring our logs based on the current value of get_options().
+ *
+ * The value <b>old_options</b> holds either the previous options object,
+ * or NULL if we're starting for the first time.
+ *
+ * On success, return a log_transaction_t that we can either roll back or
+ * commit.
+ *
+ * On failure return NULL and write a message into a newly allocated string in
+ * *<b>msg_out</b>.
+ **/
+STATIC log_transaction_t *
+options_start_log_transaction(const or_options_t *old_options,
+                              char **msg_out)
+{
+  const or_options_t *options = get_options();
+  const bool running_tor = options->command == CMD_RUN_TOR;
+
+  log_transaction_t *xn = tor_malloc_zero(sizeof(log_transaction_t));
+  xn->old_min_log_level = get_min_log_level();
+  xn->safelogging_changed = !old_options ||
+    old_options->SafeLogging_ != options->SafeLogging_;
+
+  if (! running_tor)
+    goto done;
+
+  mark_logs_temp(); /* Close current logs once new logs are open. */
+  xn->logs_marked = true;
+  /* Configure the tor_log(s) */
+  if (options_init_logs(old_options, options, 0)<0) {
+    *msg_out = tor_strdup("Failed to init Log options. See logs for details.");
+    options_rollback_log_transaction(xn);
+    xn = NULL;
+    goto done;
+  }
+
+  xn->logs_initialized = true;
+
+ done:
+  return xn;
+}
+
+/**
+ * Finish configuring the logs that started to get configured with <b>xn</b>.
+ * Frees <b>xn</b>.
+ **/
+STATIC void
+options_commit_log_transaction(log_transaction_t *xn)
+{
+  const or_options_t *options = get_options();
+  tor_assert(xn);
+
+  if (xn->logs_marked) {
+    log_severity_list_t *severity =
+      tor_malloc_zero(sizeof(log_severity_list_t));
+    close_temp_logs();
+    add_callback_log(severity, control_event_logmsg);
+    logs_set_pending_callback_callback(control_event_logmsg_pending);
+    control_adjust_event_log_severity();
+    tor_free(severity);
+    tor_log_update_sigsafe_err_fds();
+  }
+
+  if (xn->logs_initialized) {
+    flush_log_messages_from_startup();
+  }
+
+  {
+    const char *badness = NULL;
+    int bad_safelog = 0, bad_severity = 0, new_badness = 0;
+    if (options->SafeLogging_ != SAFELOG_SCRUB_ALL) {
+      bad_safelog = 1;
+      if (xn->safelogging_changed)
+        new_badness = 1;
+    }
+    if (get_min_log_level() >= LOG_INFO) {
+      bad_severity = 1;
+      if (get_min_log_level() != xn->old_min_log_level)
+        new_badness = 1;
+    }
+    if (bad_safelog && bad_severity)
+      badness = "you disabled SafeLogging, and "
+        "you're logging more than \"notice\"";
+    else if (bad_safelog)
+      badness = "you disabled SafeLogging";
+    else
+      badness = "you're logging more than \"notice\"";
+    if (new_badness)
+      log_warn(LD_GENERAL, "Your log may contain sensitive information - %s. "
+               "Don't log unless it serves an important reason. "
+               "Overwrite the log afterwards.", badness);
+  }
+
+  tor_free(xn);
+}
+
+/**
+ * Revert the log configuration changes that that started to get configured
+ * with <b>xn</b>.  Frees <b>xn</b>.
+ **/
+STATIC void
+options_rollback_log_transaction(log_transaction_t *xn)
+{
+  if (!xn)
+    return;
+
+  if (xn->logs_marked) {
+    rollback_log_changes();
+    control_adjust_event_log_severity();
+  }
+
+  tor_free(xn);
+}
+
+/**
+ * Fetch the active option list, and take actions based on it. All of
+ * the things we do in this function should survive being done
+ * repeatedly, OR be done only once when starting Tor.  If present,
+ * <b>old_options</b> contains the previous value of the options.
+ *
+ * This function is only truly "reversible" _after_ the first time it
+ * is run.  The first time that it runs, it performs some irreversible
+ * tasks in the correct sequence between the reversible option changes.
+ *
+ * Option changes should only be marked as "reversible" if they cannot
+ * be validated before switching them, but they can be switched back if
+ * some other validation fails.
+ *
+ * Return 0 if all goes well, return -1 if things went badly.
+ */
+MOCK_IMPL(STATIC int,
+options_act_reversible,(const or_options_t *old_options, char **msg))
+{
+  const bool first_time = ! have_set_startup_options;
+  log_transaction_t *log_transaction = NULL;
+  listener_transaction_t *listener_transaction = NULL;
+  int r = -1;
+
+  /* The ordering of actions in this function is not free, sadly.
+   *
+   * First of all, we _must_ daemonize before we take all kinds of
+   * initialization actions, since they need to happen in the
+   * subprocess.
+   */
+  if (options_act_once_on_startup(msg) < 0)
+    goto rollback;
+
+  /* Once we've handled most of once-off initialization, we need to
+   * open our listeners before we switch IDs.  (If we open listeners first,
+   * we might not be able to bind to low ports.)
+   */
+  listener_transaction = options_start_listener_transaction(old_options, msg);
+  if (listener_transaction == NULL)
+    goto rollback;
+
+  if (first_time) {
+    if (options_switch_id(msg) < 0)
+      goto done;
+  }
+
+  /* On the other hand, we need to touch the file system _after_ we
+   * switch IDs: otherwise, we'll be making directories and opening files
+   * with the wrong permissions.
+   */
+  if (first_time) {
+    if (options_create_directories(msg) < 0)
+      goto done;
+  }
+
+  /* Bail out at this point if we're not going to be a client or server:
+   * we don't run Tor itself. */
+  log_transaction = options_start_log_transaction(old_options, msg);
+  if (log_transaction == NULL)
+    goto rollback;
+
+  // Commit!
+  r = 0;
+
+  options_commit_log_transaction(log_transaction);
+
+  options_commit_listener_transaction(listener_transaction);
+
+  goto done;
+
+ rollback:
+  r = -1;
+  tor_assert(*msg);
+
+  options_rollback_log_transaction(log_transaction);
+  options_rollback_listener_transaction(listener_transaction);
+
  done:
-  smartlist_free(new_listeners);
   return r;
 }
 
@@ -3180,6 +3434,20 @@ options_validate_cb(const void *old_options_, void *options_, char **msg)
                   &world_writable_control_socket) < 0)
     return -1;
 
+#ifndef HAVE_SYS_UN_H
+  if (options->ControlSocket || options->ControlSocketsGroupWritable) {
+    *msg = tor_strdup("Unix domain sockets (ControlSocket) not supported "
+                      "on this OS/with this build.");
+    return -1;
+  }
+#else /* defined(HAVE_SYS_UN_H) */
+  if (options->ControlSocketsGroupWritable && !options->ControlSocket) {
+    *msg = tor_strdup("Setting ControlSocketGroupWritable without setting "
+                      "a ControlSocket makes no sense.");
+    return -1;
+  }
+#endif /* !defined(HAVE_SYS_UN_H) */
+
   /* Set UseEntryGuards from the configured value, before we check it below.
    * We change UseEntryGuards when it's incompatible with other options,
    * but leave UseEntryGuards_option with the original value.
@@ -4796,7 +5064,7 @@ options_init_log_granularity(const or_options_t *options,
  * Initialize the logs based on the configuration file.
  */
 STATIC int
-options_init_logs(const or_options_t *old_options, or_options_t *options,
+options_init_logs(const or_options_t *old_options, const or_options_t *options,
                   int validate_only)
 {
   config_line_t *opt;

src/app/config/config.h

@@ -301,7 +301,15 @@ STATIC int open_and_add_file_log(const log_severity_list_t *severity,
                                  const char *fname,
                                  int truncate_log);
 STATIC int options_init_logs(const or_options_t *old_options,
-                             or_options_t *options, int validate_only);
+                             const or_options_t *options, int validate_only);
+
+STATIC int options_create_directories(char **msg_out);
+struct log_transaction_t;
+STATIC struct log_transaction_t *options_start_log_transaction(
+                              const or_options_t *old_options,
+                              char **msg_out);
+STATIC void options_commit_log_transaction(struct log_transaction_t *xn);
+STATIC void options_rollback_log_transaction(struct log_transaction_t *xn);
 
 #ifdef TOR_UNIT_TESTS
 int options_validate(const or_options_t *old_options,

src/test/conf_examples/controlsock/error

@@ -0,0 +1 @@
+not supported on this OS\|without setting a ControlSocket

src/test/conf_examples/controlsock/torrc

@@ -0,0 +1 @@
+ControlSocketsGroupWritable 1

src/test/include.am

@@ -177,6 +177,7 @@ src_test_test_SOURCES += \
 	src/test/test_oom.c \
 	src/test/test_oos.c \
 	src/test/test_options.c \
+	src/test/test_options_act.c \
 	src/test/test_pem.c \
 	src/test/test_periodic_event.c \
 	src/test/test_policy.c \

src/test/test.c

@@ -732,6 +732,7 @@ struct testgroup_t testgroups[] = {
   { "oom/", oom_tests },
   { "oos/", oos_tests },
   { "options/", options_tests },
+  { "options/act/", options_act_tests },
   { "parsecommon/", parsecommon_tests },
   { "periodic-event/" , periodic_event_tests },
   { "policy/" , policy_tests },

src/test/test.h

@@ -252,6 +252,7 @@ extern struct testcase_t nodelist_tests[];
 extern struct testcase_t oom_tests[];
 extern struct testcase_t oos_tests[];
 extern struct testcase_t options_tests[];
+extern struct testcase_t options_act_tests[];
 extern struct testcase_t parsecommon_tests[];
 extern struct testcase_t pem_tests[];
 extern struct testcase_t periodic_event_tests[];

src/test/test_options_act.c

@@ -0,0 +1,272 @@
+/* Copyright (c) 2001-2004, Roger Dingledine.
+ * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2019, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+#define CONFIG_PRIVATE
+#include "core/or/or.h"
+#include "app/config/config.h"
+#include "lib/encoding/confline.h"
+
+#include "test/test.h"
+#include "test/log_test_helpers.h"
+#include "test/test_helpers.h"
+
+#ifndef _WIN32
+#include <sys/stat.h>
+
+/**
+ * Check whether fname is readable. On success set
+ * *<b>is_group_readable_out</b> to as appropriate and return 0. On failure
+ * return -1.
+ */
+static int
+get_file_mode(const char *fname, unsigned *permissions_out)
+{
+  struct stat st;
+  int r = stat(fname, &st);
+  if (r < 0)
+    return -1;
+  *permissions_out = (unsigned) st.st_mode;
+  return 0;
+}
+#define assert_mode(fn,mask,expected) STMT_BEGIN                 \
+  unsigned mode_;                                                \
+  int tmp_ = get_file_mode((fn), &mode_);                        \
+  if (tmp_ < 0) {                                                \
+    TT_DIE(("Couldn't stat %s: %s", (fn), strerror(errno)));     \
+  }                                                              \
+  if ((mode_ & (mask)) != (expected)) {                          \
+    TT_DIE(("Bad mode %o on %s", mode_, (fn)));                  \
+  }                                                              \
+  STMT_END
+#else
+/* "group-readable" isn't meaningful on windows */
+#define assert_mode(fn,mask,expected) STMT_NIL
+#endif
+
+static or_options_t *mock_opts;
+static const or_options_t *
+mock_get_options(void)
+{
+  return mock_opts;
+}
+
+static void
+test_options_act_create_dirs(void *arg)
+{
+  (void)arg;
+  MOCK(get_options, mock_get_options);
+  char *msg = NULL;
+  or_options_t *opts = mock_opts = options_new();
+
+  /* We're testing options_create_directories(), which assumes that
+     validate_data_directories() has already been called, and all of
+     KeyDirectory, DataDirectory, and CacheDirectory are set. */
+
+  /* Success case 1: all directories are the default */
+  char *fn;
+  fn = tor_strdup(get_fname_rnd("ddir"));
+  opts->DataDirectory = tor_strdup(fn);
+  opts->CacheDirectory = tor_strdup(fn);
+  tor_asprintf(&opts->KeyDirectory, "%s/keys", fn);
+  opts->DataDirectoryGroupReadable = 1;
+  opts->CacheDirectoryGroupReadable = -1; /* default. */
+  int r = options_create_directories(&msg);
+  tt_int_op(r, OP_EQ, 0);
+  tt_ptr_op(msg, OP_EQ, NULL);
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->DataDirectory));
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->CacheDirectory));
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->KeyDirectory));
+  assert_mode(opts->DataDirectory, 0777, 0750);
+  assert_mode(opts->KeyDirectory, 0777, 0700);
+  tor_free(fn);
+  tor_free(opts->KeyDirectory);
+  or_options_free(opts);
+
+  /* Success case 2: all directories are different. */
+  opts = mock_opts = options_new();
+  opts->DataDirectory = tor_strdup(get_fname_rnd("ddir"));
+  opts->CacheDirectory = tor_strdup(get_fname_rnd("cdir"));
+  opts->KeyDirectory = tor_strdup(get_fname_rnd("kdir"));
+  opts->CacheDirectoryGroupReadable = 1; // cache directory group readable
+  r = options_create_directories(&msg);
+  tt_int_op(r, OP_EQ, 0);
+  tt_ptr_op(msg, OP_EQ, NULL);
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->DataDirectory));
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->CacheDirectory));
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->KeyDirectory));
+  assert_mode(opts->DataDirectory, 0777, 0700);
+  assert_mode(opts->KeyDirectory, 0777, 0700);
+  assert_mode(opts->CacheDirectory, 0777, 0750);
+  tor_free(fn);
+  or_options_free(opts);
+
+  /* Success case 3: all directories are the same. */
+  opts = mock_opts = options_new();
+  fn = tor_strdup(get_fname_rnd("ddir"));
+  opts->DataDirectory = tor_strdup(fn);
+  opts->CacheDirectory = tor_strdup(fn);
+  opts->KeyDirectory = tor_strdup(fn);
+  opts->DataDirectoryGroupReadable = 1;
+  opts->CacheDirectoryGroupReadable = -1; /* default. */
+  opts->KeyDirectoryGroupReadable = -1; /* default */
+  r = options_create_directories(&msg);
+  tt_int_op(r, OP_EQ, 0);
+  tt_ptr_op(msg, OP_EQ, NULL);
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->DataDirectory));
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->CacheDirectory));
+  tt_int_op(FN_DIR, OP_EQ, file_status(opts->KeyDirectory));
+  assert_mode(opts->DataDirectory, 0777, 0750);
+  assert_mode(opts->KeyDirectory, 0777, 0750);
+  assert_mode(opts->CacheDirectory, 0777, 0750);
+  tor_free(fn);
+  or_options_free(opts);
+
+  /* Failure case 1: Can't make datadir. */
+  opts = mock_opts = options_new();
+  opts->DataDirectory = tor_strdup(get_fname_rnd("ddir"));
+  opts->CacheDirectory = tor_strdup(get_fname_rnd("cdir"));
+  opts->KeyDirectory = tor_strdup(get_fname_rnd("kdir"));
+  write_str_to_file(opts->DataDirectory, "foo", 0);
+  r = options_create_directories(&msg);
+  tt_int_op(r, OP_LT, 0);
+  tt_assert(!strcmpstart(msg, "Couldn't create private data directory"));
+  or_options_free(opts);
+  tor_free(msg);
+
+  /* Failure case 2: Can't make keydir. */
+  opts = mock_opts = options_new();
+  opts->DataDirectory = tor_strdup(get_fname_rnd("ddir"));
+  opts->CacheDirectory = tor_strdup(get_fname_rnd("cdir"));
+  opts->KeyDirectory = tor_strdup(get_fname_rnd("kdir"));
+  write_str_to_file(opts->KeyDirectory, "foo", 0);
+  r = options_create_directories(&msg);
+  tt_int_op(r, OP_LT, 0);
+  tt_assert(!strcmpstart(msg, "Couldn't create private data directory"));
+  or_options_free(opts);
+  tor_free(msg);
+
+  /* Failure case 3: Can't make cachedir. */
+  opts = mock_opts = options_new();
+  opts->DataDirectory = tor_strdup(get_fname_rnd("ddir"));
+  opts->CacheDirectory = tor_strdup(get_fname_rnd("cdir"));
+  opts->KeyDirectory = tor_strdup(get_fname_rnd("kdir"));
+  write_str_to_file(opts->CacheDirectory, "foo", 0);
+  r = options_create_directories(&msg);
+  tt_int_op(r, OP_LT, 0);
+  tt_assert(!strcmpstart(msg, "Couldn't create private data directory"));
+  tor_free(fn);
+  or_options_free(opts);
+  tor_free(msg);
+
+ done:
+  UNMOCK(get_options);
+  or_options_free(opts);
+  mock_opts = NULL;
+  tor_free(fn);
+  tor_free(msg);
+}
+
+static void
+test_options_act_log_transition(void *arg)
+{
+  (void)arg;
+  or_options_t *opts = mock_opts = options_new();
+  or_options_t *old_opts = NULL;
+  opts->LogTimeGranularity = 1000;
+  opts->SafeLogging_ = SAFELOG_SCRUB_ALL;
+  struct log_transaction_t *lt = NULL;
+  char *msg = NULL;
+  MOCK(get_options, mock_get_options);
+
+  tt_ptr_op(opts->Logs, OP_EQ, NULL);
+  config_line_append(&opts->Logs, "Log", "notice stdout");
+  lt = options_start_log_transaction(NULL, &msg);
+  tt_assert(lt);
+  tt_assert(!msg);
+
+  // commit, see that there is a change.
+  options_commit_log_transaction(lt);
+  lt=NULL;
+  tt_int_op(get_min_log_level(), OP_EQ, LOG_NOTICE);
+
+  // Now drop to debug.
+  old_opts = opts;
+  opts = mock_opts = options_new();
+  opts->LogTimeGranularity = 1000;
+  opts->SafeLogging_ = SAFELOG_SCRUB_ALL;
+  config_line_append(&opts->Logs, "Log", "debug stdout");
+  lt = options_start_log_transaction(old_opts, &msg);
+  tt_assert(lt);
+  tt_assert(!msg);
+
+  setup_full_capture_of_logs(LOG_NOTICE);
+  options_commit_log_transaction(lt);
+  lt=NULL;
+  expect_single_log_msg_containing("may contain sensitive information");
+  tt_int_op(get_min_log_level(), OP_EQ, LOG_DEBUG);
+
+  // Turn off SafeLogging
+  or_options_free(old_opts);
+  mock_clean_saved_logs();
+  old_opts = opts;
+  opts = mock_opts = options_new();
+  opts->SafeLogging_ = SAFELOG_SCRUB_NONE;
+  opts->LogTimeGranularity = 1000;
+  config_line_append(&opts->Logs, "Log", "debug stdout");
+  lt = options_start_log_transaction(old_opts, &msg);
+  tt_assert(lt);
+  tt_assert(!msg);
+  options_commit_log_transaction(lt);
+  lt=NULL;
+  expect_single_log_msg_containing("may contain sensitive information");
+  tt_int_op(get_min_log_level(), OP_EQ, LOG_DEBUG);
+
+  // Try rolling back.
+  or_options_free(old_opts);
+  mock_clean_saved_logs();
+  old_opts = opts;
+  opts = mock_opts = options_new();
+  opts->SafeLogging_ = SAFELOG_SCRUB_NONE;
+  opts->LogTimeGranularity = 1000;
+  config_line_append(&opts->Logs, "Log", "notice stdout");
+  lt = options_start_log_transaction(old_opts, &msg);
+  tt_assert(lt);
+  tt_assert(!msg);
+  options_rollback_log_transaction(lt);
+  expect_no_log_entry();
+  lt = NULL;
+  tt_int_op(get_min_log_level(), OP_EQ, LOG_DEBUG);
+
+  // Now try some bad options.
+  or_options_free(opts);
+  mock_clean_saved_logs();
+  opts = mock_opts = options_new();
+  opts->LogTimeGranularity = 1000;
+  config_line_append(&opts->Logs, "Log", "warn blaznert");
+  lt = options_start_log_transaction(old_opts, &msg);
+  tt_assert(!lt);
+  tt_str_op(msg, OP_EQ, "Failed to init Log options. See logs for details.");
+  expect_single_log_msg_containing("Couldn't parse");
+  tt_int_op(get_min_log_level(), OP_EQ, LOG_DEBUG);
+
+ done:
+  UNMOCK(get_options);
+  or_options_free(opts);
+  or_options_free(old_opts);
+  tor_free(msg);
+  if (lt)
+    options_rollback_log_transaction(lt);
+  teardown_capture_of_logs();
+}
+
+#ifndef COCCI
+#define T(name) { #name, test_options_act_##name, TT_FORK, NULL, NULL }
+#endif
+
+struct testcase_t options_act_tests[] = {
+  T(create_dirs),
+  T(log_transition),
+  END_OF_TESTCASES
+};