[tor-commits] [tor/master] Use thread-safe types to store the LOG_PROTOCOL_WARN severity

[nickm at torproject.org]

commit da778f2921d0ae49c47abb4ba4ebe5f92a999ae2
Author: Nick Mathewson <nickm at torproject.org>
Date:   Wed Jan 24 12:02:44 2018 -0500

    Use thread-safe types to store the LOG_PROTOCOL_WARN severity
    
    Fixes a race condition; resolves 23954.
---
 changes/bug23954               |  4 ++++
 src/or/config.c                | 49 +++++++++++++++++++++++++++++++++++-------
 src/or/config.h                |  1 +
 src/or/main.c                  |  1 +
 src/test/fuzz/fuzzing_common.c |  2 ++
 src/test/testing_common.c      |  1 +
 6 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/changes/bug23954 b/changes/bug23954
new file mode 100644
index 000000000..185814f12
--- /dev/null
+++ b/changes/bug23954
@@ -0,0 +1,4 @@
+  o Minor bugfixes (logging, race conditions):
+    - Fix a (mostly harmless) race condition when invoking
+      LOG_PROTOCOL_WARN message from a subthread while the options are
+      changing. Fixes bug 23954; bugfix on 0.1.1.9-alpha.
diff --git a/src/or/config.c b/src/or/config.c
index afaf86785..f035bbaba 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -766,6 +766,8 @@ static int options_validate_cb(void *old_options, void *options,
                                int from_setconf, char **msg);
 static uint64_t compute_real_max_mem_in_queues(const uint64_t val,
                                                int log_guess);
+static void cleanup_protocol_warning_severity_level(void);
+static void set_protocol_warning_severity_level(int warning_severity);
 
 /** Magic value for or_options_t. */
 #define OR_OPTIONS_MAGIC 9090909
@@ -999,6 +1001,8 @@ config_free_all(void)
   tor_free(the_short_tor_version);
   tor_free(the_tor_version);
 
+  cleanup_protocol_warning_severity_level();
+
   have_parsed_cmdline = 0;
   libevent_initialized = 0;
 }
@@ -1064,17 +1068,46 @@ escaped_safe_str(const char *address)
  * The severity level that should be used for warnings of severity
  * LOG_PROTOCOL_WARN.
  *
- * We keep this outside the options, in case somebody needs to use
- * LOG_PROTOCOL_WARN while an option transition is happening.
+ * We keep this outside the options, and we use an atomic_counter_t, in case
+ * one thread needs to use LOG_PROTOCOL_WARN while an option transition is
+ * happening in the main thread.
  */
-static int protocol_warning_severity_level = LOG_WARN;
+static atomic_counter_t protocol_warning_severity_level;
 
 /** Return the severity level that should be used for warnings of severity
  * LOG_PROTOCOL_WARN. */
 int
 get_protocol_warning_severity_level(void)
 {
-  return protocol_warning_severity_level;
+  return (int) atomic_counter_get(&protocol_warning_severity_level);
+}
+
+/** Set the protocol warning severity level to <b>severity</b>. */
+static void
+set_protocol_warning_severity_level(int warning_severity)
+{
+  atomic_counter_exchange(&protocol_warning_severity_level,
+                          warning_severity);
+}
+
+/**
+ * Initialize the log warning severity level for protocol warnings. Call
+ * only once at startup.
+ */
+void
+init_protocol_warning_severity_level(void)
+{
+  atomic_counter_init(&protocol_warning_severity_level);
+  set_protocol_warning_severity_level(LOG_WARN);
+}
+
+/**
+ * Tear down protocol_warning_severity_level.
+ */
+static void
+cleanup_protocol_warning_severity_level(void)
+{
+   atomic_counter_destroy(&protocol_warning_severity_level);
 }
 
 /** List of default directory authorities */
@@ -1794,10 +1827,10 @@ options_act(const or_options_t *old_options)
       return -1;
   }
 
-  if (options->ProtocolWarnings)
-    protocol_warning_severity_level = LOG_WARN;
-  else
-    protocol_warning_severity_level = LOG_INFO;
+  {
+    int warning_severity = options->ProtocolWarnings ? LOG_WARN : LOG_INFO;
+    set_protocol_warning_severity_level(warning_severity);
+  }
 
   if (consider_adding_dir_servers(options, old_options) < 0) {
     // XXXX This should get validated earlier, and committed here, to
diff --git a/src/or/config.h b/src/or/config.h
index 7c7ef1825..2f23809b2 100644
--- a/src/or/config.h
+++ b/src/or/config.h
@@ -31,6 +31,7 @@ const char *safe_str_client(const char *address);
 const char *safe_str(const char *address);
 const char *escaped_safe_str_client(const char *address);
 const char *escaped_safe_str(const char *address);
+void init_protocol_warning_severity_level(void);
 int get_protocol_warning_severity_level(void);
 const char *get_version(void);
 const char *get_short_version(void);
diff --git a/src/or/main.c b/src/or/main.c
index 10e606f3a..841a37255 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -4009,6 +4009,7 @@ tor_run_main(const tor_main_configuration_t *tor_cfg)
 #endif /* defined(_WIN32) */
 
   configure_backtrace_handler(get_version());
+  init_protocol_warning_severity_level();
 
   update_approx_time(time(NULL));
   tor_threads_init();
diff --git a/src/test/fuzz/fuzzing_common.c b/src/test/fuzz/fuzzing_common.c
index 1d54e41db..7c9fac748 100644
--- a/src/test/fuzz/fuzzing_common.c
+++ b/src/test/fuzz/fuzzing_common.c
@@ -152,6 +152,8 @@ main(int argc, char **argv)
     }
   }
 
+  init_protocol_warning_severity_level();
+
   {
     log_severity_list_t s;
     memset(&s, 0, sizeof(s));
diff --git a/src/test/testing_common.c b/src/test/testing_common.c
index 142c68107..52729147b 100644
--- a/src/test/testing_common.c
+++ b/src/test/testing_common.c
@@ -278,6 +278,7 @@ main(int c, const char **v)
     s.masks[LOG_WARN-LOG_ERR] |= LD_BUG;
     add_stream_log(&s, "", fileno(stdout));
   }
+  init_protocol_warning_severity_level();
 
   options->command = CMD_RUN_UNITTESTS;
   if (crypto_global_init(accel_crypto, NULL, NULL)) {