Merge remote-tracking branch 'tor-github/pr/1486'

4 files changed, 86 insertions, 54 deletions

diff --git a/changes/ticket32344 b/changes/ticket32344
new file mode 100644
index 0000000000..9dfb326976
--- /dev/null
+++ b/changes/ticket32344
@@ -0,0 +1,4 @@
+  o Code simplification and refactoring:
+    - Immutability is now implemented as a flag on individual configuration
+      options rather than as part of the option-transition checking
+      code.  Closes ticket 32344.
diff --git a/src/app/config/config.c b/src/app/config/config.c
index 1b697e73fc..0f0d9ac461 100644
--- a/src/app/config/config.c
+++ b/src/app/config/config.c
@@ -266,6 +266,9 @@ DUMMY_TYPECHECK_INSTANCE(or_options_t);
 #define VAR_NODUMP(varname,conftype,member,initvalue)             \
   CONFIG_VAR_ETYPE(or_options_t, varname, conftype, member,       \
                    CFLG_NODUMP, initvalue)
+#define VAR_NODUMP_IMMUTABLE(varname,conftype,member,initvalue)   \
+  CONFIG_VAR_ETYPE(or_options_t, varname, conftype, member,       \
+                   CFLG_NODUMP | CFLG_IMMUTABLE, initvalue)
 #define VAR_INVIS(varname,conftype,member,initvalue)              \
   CONFIG_VAR_ETYPE(or_options_t, varname, conftype, member,       \
                    CFLG_NODUMP | CFLG_NOSET | CFLG_NOLIST, initvalue)
@@ -273,6 +276,13 @@ DUMMY_TYPECHECK_INSTANCE(or_options_t);
 #define V(member,conftype,initvalue)            \
   VAR(#member, conftype, member, initvalue)
 
+#define VAR_IMMUTABLE(varname, conftype, member, initvalue)             \
+  CONFIG_VAR_ETYPE(or_options_t, varname, conftype, member,             \
+                   CFLG_IMMUTABLE, initvalue)
+
+#define V_IMMUTABLE(member,conftype,initvalue)                 \
+  VAR_IMMUTABLE(#member, conftype, member, initvalue)
+
 /** As V, but uses a type definition instead of a type enum */
 #define V_D(member,type,initvalue)              \
   VAR_D(#member, type, member, initvalue)
@@ -342,7 +352,7 @@ static const config_var_t option_vars_[] = {
   V(BridgeRecordUsageByCountry,  BOOL,     "1"),
   V(BridgeRelay,                 BOOL,     "0"),
   V(BridgeDistribution,          STRING,   NULL),
-  VAR("CacheDirectory",          FILENAME, CacheDirectory_option, NULL),
+  VAR_IMMUTABLE("CacheDirectory",FILENAME, CacheDirectory_option, NULL),
   V(CacheDirectoryGroupReadable, AUTOBOOL,     "auto"),
   V(CellStatistics,              BOOL,     "0"),
   V(PaddingStatistics,           BOOL,     "1"),
@@ -378,7 +388,7 @@ static const config_var_t option_vars_[] = {
   V(CookieAuthFileGroupReadable, BOOL,     "0"),
   V(CookieAuthFile,              STRING,   NULL),
   V(CountPrivateBandwidth,       BOOL,     "0"),
-  VAR("DataDirectory",           FILENAME, DataDirectory_option, NULL),
+  VAR_IMMUTABLE("DataDirectory", FILENAME, DataDirectory_option, NULL),
   V(DataDirectoryGroupReadable,  BOOL,     "0"),
   V(DisableOOSCheck,             BOOL,     "1"),
   V(DisableNetwork,              BOOL,     "0"),
@@ -399,8 +409,8 @@ static const config_var_t option_vars_[] = {
    * an order of magnitude, so there isn't too much load shifting to
    * authorities when fallbacks go down. */
   V(DirAuthorityFallbackRate,    DOUBLE,   "0.1"),
-  V(DisableAllSwap,              BOOL,     "0"),
-  V(DisableDebuggerAttachment,   BOOL,     "1"),
+  V_IMMUTABLE(DisableAllSwap,    BOOL,     "0"),
+  V_IMMUTABLE(DisableDebuggerAttachment,   BOOL,     "1"),
   OBSOLETE("DisableIOCP"),
   OBSOLETE("DisableV2DirectoryInfo_"),
   OBSOLETE("DynamicDHGroups"),
@@ -476,11 +486,11 @@ static const config_var_t option_vars_[] = {
 #endif /* defined(_WIN32) */
   OBSOLETE("Group"),
   V(GuardLifetime,               INTERVAL, "0 minutes"),
-  V(HardwareAccel,               BOOL,     "0"),
+  V_IMMUTABLE(HardwareAccel,     BOOL,     "0"),
   V(HeartbeatPeriod,             INTERVAL, "6 hours"),
   V(MainloopStats,               BOOL,     "0"),
-  V(AccelName,                   STRING,   NULL),
-  V(AccelDir,                    FILENAME, NULL),
+  V_IMMUTABLE(AccelName,         STRING,   NULL),
+  V_IMMUTABLE(AccelDir,          FILENAME, NULL),
   V(HashedControlPassword,       LINELIST, NULL),
   OBSOLETE("HidServDirectoryV2"),
   VAR("HiddenServiceDir",    LINELIST_S, RendConfigLines,    NULL),
@@ -504,8 +514,8 @@ static const config_var_t option_vars_[] = {
   V(ClientOnionAuthDir,          FILENAME, NULL),
   OBSOLETE("CloseHSClientCircuitsImmediatelyOnTimeout"),
   OBSOLETE("CloseHSServiceRendCircuitsImmediatelyOnTimeout"),
-  V(HiddenServiceSingleHopMode,  BOOL,     "0"),
-  V(HiddenServiceNonAnonymousMode,BOOL,    "0"),
+  V_IMMUTABLE(HiddenServiceSingleHopMode,  BOOL,     "0"),
+  V_IMMUTABLE(HiddenServiceNonAnonymousMode,BOOL,    "0"),
   V(HTTPProxy,                   STRING,   NULL),
   V(HTTPProxyAuthenticator,      STRING,   NULL),
   V(HTTPSProxy,                  STRING,   NULL),
@@ -520,18 +530,18 @@ static const config_var_t option_vars_[] = {
   V(Socks5Proxy,                 STRING,   NULL),
   V(Socks5ProxyUsername,         STRING,   NULL),
   V(Socks5ProxyPassword,         STRING,   NULL),
-  VAR("KeyDirectory",            FILENAME, KeyDirectory_option, NULL),
+  VAR_IMMUTABLE("KeyDirectory",  FILENAME, KeyDirectory_option, NULL),
   V(KeyDirectoryGroupReadable,   BOOL,     "0"),
   VAR_D("HSLayer2Nodes",         ROUTERSET,  HSLayer2Nodes,  NULL),
   VAR_D("HSLayer3Nodes",         ROUTERSET,  HSLayer3Nodes,  NULL),
   V(KeepalivePeriod,             INTERVAL, "5 minutes"),
-  V(KeepBindCapabilities,            AUTOBOOL, "auto"),
+  V_IMMUTABLE(KeepBindCapabilities,        AUTOBOOL, "auto"),
   VAR("Log",                     LINELIST, Logs,             NULL),
   V(LogMessageDomains,           BOOL,     "0"),
   V(LogTimeGranularity,          MSEC_INTERVAL, "1 second"),
   V(TruncateLogFile,             BOOL,     "0"),
-  V(SyslogIdentityTag,           STRING,   NULL),
-  V(AndroidIdentityTag,          STRING,   NULL),
+  V_IMMUTABLE(SyslogIdentityTag, STRING,   NULL),
+  V_IMMUTABLE(AndroidIdentityTag,STRING,   NULL),
   V(LongLivedPorts,              CSV,
         "21,22,706,1863,5050,5190,5222,5223,6523,6667,6697,8300"),
   VAR("MapAddress",              LINELIST, AddressMap,           NULL),
@@ -553,7 +563,7 @@ static const config_var_t option_vars_[] = {
   OBSOLETE("PredictedPortsRelevanceTime"),
   OBSOLETE("WarnUnsafeSocks"),
   VAR("NodeFamily",              LINELIST, NodeFamilies,         NULL),
-  V(NoExec,                      BOOL,     "0"),
+  V_IMMUTABLE(NoExec,            BOOL,     "0"),
   V(NumCPUs,                     POSINT,     "0"),
   V(NumDirectoryGuards,          POSINT,     "0"),
   V(NumEntryGuards,              POSINT,     "0"),
@@ -584,8 +594,8 @@ static const config_var_t option_vars_[] = {
   V(PathsNeededToBuildCircuits,  DOUBLE,   "-1"),
   V(PerConnBWBurst,              MEMUNIT,  "0"),
   V(PerConnBWRate,               MEMUNIT,  "0"),
-  V(PidFile,                     STRING,   NULL),
-  V(TestingTorNetwork,           BOOL,     "0"),
+  V_IMMUTABLE(PidFile,           STRING,   NULL),
+  V_IMMUTABLE(TestingTorNetwork, BOOL,     "0"),
   V(TestingMinExitFlagThreshold, MEMUNIT,  "0"),
   V(TestingMinFastFlagThreshold, MEMUNIT,  "0"),
 
@@ -619,10 +629,10 @@ static const config_var_t option_vars_[] = {
   V(RelayBandwidthRate,          MEMUNIT,  "0"),
   V(RendPostPeriod,              INTERVAL, "1 hour"),
   V(RephistTrackTime,            INTERVAL, "24 hours"),
-  V(RunAsDaemon,                 BOOL,     "0"),
+  V_IMMUTABLE(RunAsDaemon,       BOOL,     "0"),
   V(ReducedExitPolicy,           BOOL,     "0"),
   OBSOLETE("RunTesting"), // currently unused
-  V(Sandbox,                     BOOL,     "0"),
+  V_IMMUTABLE(Sandbox,           BOOL,     "0"),
   V(SafeLogging,                 STRING,   "1"),
   V(SafeSocks,                   BOOL,     "0"),
   V(ServerDNSAllowBrokenConfig,  BOOL,     "1"),
@@ -650,7 +660,7 @@ static const config_var_t option_vars_[] = {
   V(StrictNodes,                 BOOL,     "0"),
   OBSOLETE("Support022HiddenServices"),
   V(TestSocks,                   BOOL,     "0"),
-  V(TokenBucketRefillInterval,   MSEC_INTERVAL, "100 msec"),
+  V_IMMUTABLE(TokenBucketRefillInterval,   MSEC_INTERVAL, "100 msec"),
   OBSOLETE("Tor2webMode"),
   OBSOLETE("Tor2webRendezvousPoints"),
   OBSOLETE("TLSECGroup"),
@@ -667,7 +677,7 @@ static const config_var_t option_vars_[] = {
   V(UseGuardFraction,            AUTOBOOL, "auto"),
   V(UseMicrodescriptors,         AUTOBOOL, "auto"),
   OBSOLETE("UseNTorHandshake"),
-  V(User,                        STRING,   NULL),
+  V_IMMUTABLE(User,              STRING,   NULL),
   OBSOLETE("UserspaceIOCPBuffers"),
   V(AuthDirSharedRandomness,     BOOL,     "1"),
   V(AuthDirTestEd25519LinkKeys,  BOOL,     "1"),
@@ -695,12 +705,14 @@ static const config_var_t option_vars_[] = {
   VAR_NODUMP("__ReloadTorrcOnSIGHUP",   BOOL,  ReloadTorrcOnSIGHUP,      "1"),
   VAR_NODUMP("__AllDirActionsPrivate",  BOOL,  AllDirActionsPrivate,     "0"),
   VAR_NODUMP("__DisablePredictedCircuits",BOOL,DisablePredictedCircuits, "0"),
-  VAR_NODUMP("__DisableSignalHandlers", BOOL,  DisableSignalHandlers,    "0"),
+  VAR_NODUMP_IMMUTABLE("__DisableSignalHandlers", BOOL,
+                       DisableSignalHandlers,    "0"),
   VAR_NODUMP("__LeaveStreamsUnattached",BOOL,  LeaveStreamsUnattached,   "0"),
   VAR_NODUMP("__HashedControlSessionPassword", LINELIST,
              HashedControlSessionPassword,
       NULL),
-  VAR_NODUMP("__OwningControllerProcess",STRING,OwningControllerProcess, NULL),
+  VAR_NODUMP_IMMUTABLE("__OwningControllerProcess",STRING,
+                       OwningControllerProcess, NULL),
   VAR_NODUMP("__OwningControllerFD", UINT64, OwningControllerFD,
              UINT64_MAX_STRING),
   V(MinUptimeHidServDirectoryV2, INTERVAL, "96 hours"),
@@ -4041,36 +4053,6 @@ options_check_transition_cb(const void *old_,
     return -1;                                                          \
   } while (0)
 
-#define NO_CHANGE_BOOL(opt) \
-  if (! CFG_EQ_BOOL(old, new_val, opt)) BAD_CHANGE_TO(opt,"")
-#define NO_CHANGE_INT(opt) \
-  if (! CFG_EQ_INT(old, new_val, opt)) BAD_CHANGE_TO(opt,"")
-#define NO_CHANGE_STRING(opt) \
-  if (! CFG_EQ_STRING(old, new_val, opt)) BAD_CHANGE_TO(opt,"")
-
-  NO_CHANGE_STRING(PidFile);
-  NO_CHANGE_BOOL(RunAsDaemon);
-  NO_CHANGE_BOOL(Sandbox);
-  NO_CHANGE_STRING(DataDirectory);
-  NO_CHANGE_STRING(KeyDirectory);
-  NO_CHANGE_STRING(CacheDirectory);
-  NO_CHANGE_STRING(User);
-  NO_CHANGE_BOOL(KeepBindCapabilities);
-  NO_CHANGE_STRING(SyslogIdentityTag);
-  NO_CHANGE_STRING(AndroidIdentityTag);
-  NO_CHANGE_BOOL(HardwareAccel);
-  NO_CHANGE_STRING(AccelName);
-  NO_CHANGE_STRING(AccelDir);
-  NO_CHANGE_BOOL(TestingTorNetwork);
-  NO_CHANGE_BOOL(DisableAllSwap);
-  NO_CHANGE_INT(TokenBucketRefillInterval);
-  NO_CHANGE_BOOL(HiddenServiceSingleHopMode);
-  NO_CHANGE_BOOL(HiddenServiceNonAnonymousMode);
-  NO_CHANGE_BOOL(DisableDebuggerAttachment);
-  NO_CHANGE_BOOL(NoExec);
-  NO_CHANGE_INT(OwningControllerFD);
-  NO_CHANGE_BOOL(DisableSignalHandlers);
-
   if (sandbox_is_active()) {
 #define SB_NOCHANGE_STR(opt)                      \
     if (! CFG_EQ_STRING(old, new_val, opt))       \
diff --git a/src/lib/conf/conftypes.h b/src/lib/conf/conftypes.h
index 69a5f79bf3..d4e2ea218a 100644
--- a/src/lib/conf/conftypes.h
+++ b/src/lib/conf/conftypes.h
@@ -178,6 +178,11 @@ typedef struct struct_magic_decl_t {
  * however, setting them appends to their old value.
  */
 #define CFLG_NOREPLACE    (1u<<5)
+/**
+ * Flag to indicate that an option or type cannot be changed while Tor is
+ * running.
+ **/
+#define CFLG_IMMUTABLE (1u<<6)
 
 /**
  * A group of flags that should be set on all obsolete options and types.
diff --git a/src/lib/confmgt/confmgt.c b/src/lib/confmgt/confmgt.c
index 3fdb630e8a..9377736110 100644
--- a/src/lib/confmgt/confmgt.c
+++ b/src/lib/confmgt/confmgt.c
@@ -1154,6 +1154,41 @@ config_init(const config_mgr_t *mgr, void *options)
 }
 
 /**
+ * Helper for config_validate_single: see whether any immutable option
+ * has changed between old_options and new_options.
+ *
+ * On success return 0; on failure set *msg_out to a newly allocated
+ * string explaining what is wrong, and return -1.
+ */
+static int
+config_check_immutable_flags(const config_format_t *fmt,
+                             const void *old_options,
+                             const void *new_options,
+                             char **msg_out)
+{
+  tor_assert(fmt);
+  tor_assert(new_options);
+  if (BUG(! old_options))
+    return 0;
+
+  unsigned i;
+  for (i = 0; fmt->vars[i].member.name; ++i) {
+    const config_var_t *v = &fmt->vars[i];
+    if (! config_var_has_flag(v, CFLG_IMMUTABLE))
+      continue;
+
+    if (! struct_var_eq(old_options, new_options, &v->member)) {
+      tor_asprintf(msg_out,
+                   "While Tor is running, changing %s is not allowed",
+                   v->member.name);
+      return -1;
+    }
+  }
+
+  return 0;
+}
+
+/**
  * Normalize and validate a single object `options` within a configuration
  * suite, according to its format.  `options` may be modified as appropriate
  * in order to set ancillary data.  If `old_options` is provided, make sure
@@ -1189,10 +1224,16 @@ config_validate_single(const config_format_t *fmt,
     }
   }
 
-  if (fmt->check_transition_fn && old_options) {
-    if (fmt->check_transition_fn(old_options, options, msg_out) < 0) {
+  if (old_options) {
+    if (config_check_immutable_flags(fmt, old_options, options, msg_out) < 0) {
       return VSTAT_TRANSITION_ERR;
     }
+
+    if (fmt->check_transition_fn) {
+      if (fmt->check_transition_fn(old_options, options, msg_out) < 0) {
+        return VSTAT_TRANSITION_ERR;
+      }
+    }
   }
 
   if (fmt->post_normalize_fn) {