Merge branch 'bug4282_rebased'

changes/bug4282

@@ -0,0 +1,4 @@
+  o Code simplifications and refactoring:
+    - Extract the common duplicated code for creating a subdirectory
+      of the data directory and writing to a file in it. Fixes ticket
+      4282; patch from Peter Retzlaff.

src/or/config.c

@@ -5961,6 +5961,43 @@ options_get_datadir_fname2_suffix(const or_options_t *options,
   return fname;
 }
 
+/** Check wether the data directory has a private subdirectory
+ * <b>subdir</b>. If not, try to create it. Return 0 on success,
+ * -1 otherwise. */
+int
+check_or_create_data_subdir(const char *subdir)
+{
+  char *statsdir = get_datadir_fname(subdir);
+  int return_val = 0;
+
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
+    log_warn(LD_HIST, "Unable to create %s/ directory!", subdir);
+    return_val = -1;
+  }
+  tor_free(statsdir);
+  return return_val;
+}
+
+/** Create a file named <b>fname</b> with contents <b>str</b> in the
+ * subdirectory <b>subdir</b> of the data directory. <b>descr</b>
+ * should be a short description of the file's content and will be
+ * used for the warning message, if it's present and the write process
+ * fails. Return 0 on success, -1 otherwise.*/
+int
+write_to_data_subdir(const char* subdir, const char* fname,
+                     const char* str, const char* descr)
+{
+  char *filename = get_datadir_fname2(subdir, fname);
+  int return_val = 0;
+
+  if (write_str_to_file(filename, str, 0) < 0) {
+    log_warn(LD_HIST, "Unable to write %s to disk!", descr ? descr : fname);
+    return_val = -1;
+  }
+  tor_free(filename);
+  return return_val;
+}
+
 /** Given a file name check to see whether the file exists but has not been
  * modified for a very long time.  If so, remove it. */
 void

src/or/config.h

@@ -59,6 +59,10 @@ char *options_get_datadir_fname2_suffix(const or_options_t *options,
 #define get_datadir_fname_suffix(sub1, suffix) \
   get_datadir_fname2_suffix((sub1), NULL, (suffix))
 
+int check_or_create_data_subdir(const char *subdir);
+int write_to_data_subdir(const char* subdir, const char* fname,
+                         const char* str, const char* descr);
+
 int get_num_cpus(const or_options_t *options);
 
 const smartlist_t *get_configured_ports(void);

src/or/geoip.c

@@ -1132,7 +1132,7 @@ geoip_format_dirreq_stats(time_t now)
 time_t
 geoip_dirreq_stats_write(time_t now)
 {
-  char *statsdir = NULL, *filename = NULL, *str = NULL;
+  char *str = NULL;
 
   if (!start_of_dirreq_stats_interval)
     return 0; /* Not initialized. */
@@ -1146,21 +1146,13 @@ geoip_dirreq_stats_write(time_t now)
   str = geoip_format_dirreq_stats(now);
 
   /* Write dirreq-stats string to disk. */
-  statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
-    log_warn(LD_HIST, "Unable to create stats/ directory!");
-    goto done;
+  if (!check_or_create_data_subdir("stats")) {
+    write_to_data_subdir("stats", "dirreq-stats", str, "dirreq statistics");
+    /* Reset measurement interval start. */
+    geoip_reset_dirreq_stats(now);
   }
-  filename = get_datadir_fname2("stats", "dirreq-stats");
-  if (write_str_to_file(filename, str, 0) < 0)
-    log_warn(LD_HIST, "Unable to write dirreq statistics to disk!");
-
-  /* Reset measurement interval start. */
-  geoip_reset_dirreq_stats(now);
 
  done:
-  tor_free(statsdir);
-  tor_free(filename);
   tor_free(str);
   return start_of_dirreq_stats_interval + WRITE_STATS_INTERVAL;
 }
@@ -1297,7 +1289,7 @@ format_bridge_stats_controller(time_t now)
 time_t
 geoip_bridge_stats_write(time_t now)
 {
-  char *filename = NULL, *val = NULL, *statsdir = NULL;
+  char *val = NULL;
 
   /* Check if 24 hours have passed since starting measurements. */
   if (now < start_of_bridge_stats_interval + WRITE_STATS_INTERVAL)
@@ -1317,24 +1309,20 @@ geoip_bridge_stats_write(time_t now)
   start_of_bridge_stats_interval = now;
 
   /* Write it to disk. */
-  statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
-    goto done;
-  filename = get_datadir_fname2("stats", "bridge-stats");
-
-  write_str_to_file(filename, bridge_stats_extrainfo, 0);
-
-  /* Tell the controller, "hey, there are clients!" */
-  {
-    char *controller_str = format_bridge_stats_controller(now);
-    if (controller_str)
-      control_event_clients_seen(controller_str);
-    tor_free(controller_str);
+  if (!check_or_create_data_subdir("stats")) {
+    write_to_data_subdir("stats", "bridge-stats",
+                         bridge_stats_extrainfo, "bridge statistics");
+
+    /* Tell the controller, "hey, there are clients!" */
+    {
+      char *controller_str = format_bridge_stats_controller(now);
+      if (controller_str)
+        control_event_clients_seen(controller_str);
+      tor_free(controller_str);
+    }
   }
- done:
-  tor_free(filename);
-  tor_free(statsdir);
 
+ done:
   return start_of_bridge_stats_interval + WRITE_STATS_INTERVAL;
 }
 
@@ -1436,7 +1424,7 @@ geoip_format_entry_stats(time_t now)
 time_t
 geoip_entry_stats_write(time_t now)
 {
-  char *statsdir = NULL, *filename = NULL, *str = NULL;
+  char *str = NULL;
 
   if (!start_of_entry_stats_interval)
     return 0; /* Not initialized. */
@@ -1450,21 +1438,14 @@ geoip_entry_stats_write(time_t now)
   str = geoip_format_entry_stats(now);
 
   /* Write entry-stats string to disk. */
-  statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
-    log_warn(LD_HIST, "Unable to create stats/ directory!");
-    goto done;
-  }
-  filename = get_datadir_fname2("stats", "entry-stats");
-  if (write_str_to_file(filename, str, 0) < 0)
-    log_warn(LD_HIST, "Unable to write entry statistics to disk!");
+  if (!check_or_create_data_subdir("stats")) {
+    write_to_data_subdir("stats", "entry-stats", str, "entry statistics");
 
-  /* Reset measurement interval start. */
-  geoip_reset_entry_stats(now);
+    /* Reset measurement interval start. */
+    geoip_reset_entry_stats(now);
+  }
 
  done:
-  tor_free(statsdir);
-  tor_free(filename);
   tor_free(str);
   return start_of_entry_stats_interval + WRITE_STATS_INTERVAL;
 }

src/or/rephist.c

@@ -2313,7 +2313,7 @@ rep_hist_format_exit_stats(time_t now)
 time_t
 rep_hist_exit_stats_write(time_t now)
 {
-  char *statsdir = NULL, *filename = NULL, *str = NULL;
+  char *str = NULL;
 
   if (!start_of_exit_stats_interval)
     return 0; /* Not initialized. */
@@ -2329,19 +2329,12 @@ rep_hist_exit_stats_write(time_t now)
   rep_hist_reset_exit_stats(now);
 
   /* Try to write to disk. */
-  statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
-    log_warn(LD_HIST, "Unable to create stats/ directory!");
-    goto done;
+  if (!check_or_create_data_subdir("stats")) {
+    write_to_data_subdir("stats", "exit-stats", str, "exit port statistics");
   }
-  filename = get_datadir_fname2("stats", "exit-stats");
-  if (write_str_to_file(filename, str, 0) < 0)
-    log_warn(LD_HIST, "Unable to write exit port statistics to disk!");
 
  done:
   tor_free(str);
-  tor_free(statsdir);
-  tor_free(filename);
   return start_of_exit_stats_interval + WRITE_STATS_INTERVAL;
 }
 
@@ -2598,7 +2591,7 @@ time_t
 rep_hist_buffer_stats_write(time_t now)
 {
   circuit_t *circ;
-  char *statsdir = NULL, *filename = NULL, *str = NULL;
+  char *str = NULL;
 
   if (!start_of_buffer_stats_interval)
     return 0; /* Not initialized. */
@@ -2617,19 +2610,12 @@ rep_hist_buffer_stats_write(time_t now)
   rep_hist_reset_buffer_stats(now);
 
   /* Try to write to disk. */
-  statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
-    log_warn(LD_HIST, "Unable to create stats/ directory!");
-    goto done;
+  if (!check_or_create_data_subdir("stats")) {
+    write_to_data_subdir("stats", "buffer-stats", str, "buffer statistics");
   }
-  filename = get_datadir_fname2("stats", "buffer-stats");
-  if (write_str_to_file(filename, str, 0) < 0)
-    log_warn(LD_HIST, "Unable to write buffer stats to disk!");
 
  done:
   tor_free(str);
-  tor_free(filename);
-  tor_free(statsdir);
   return start_of_buffer_stats_interval + WRITE_STATS_INTERVAL;
 }
 
@@ -2741,7 +2727,7 @@ rep_hist_format_desc_stats(time_t now)
 time_t
 rep_hist_desc_stats_write(time_t now)
 {
-  char *statsdir = NULL, *filename = NULL, *str = NULL;
+  char *filename = NULL, *str = NULL;
 
   if (!start_of_served_descs_stats_interval)
     return 0; /* We're not collecting stats. */
@@ -2751,10 +2737,8 @@ rep_hist_desc_stats_write(time_t now)
   str = rep_hist_format_desc_stats(now);
   tor_assert(str != NULL);
 
-  statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
-    log_warn(LD_HIST, "Unable to create stats/ directory!");
-      goto done;
+  if (check_or_create_data_subdir("stats") < 0) {
+    goto done;
   }
   filename = get_datadir_fname2("stats", "served-desc-stats");
   if (append_bytes_to_file(filename, str, strlen(str), 0) < 0)
@@ -2763,7 +2747,6 @@ rep_hist_desc_stats_write(time_t now)
   rep_hist_reset_desc_stats(now);
 
  done:
-  tor_free(statsdir);
   tor_free(filename);
   tor_free(str);
   return start_of_served_descs_stats_interval + WRITE_STATS_INTERVAL;
@@ -2981,7 +2964,7 @@ rep_hist_format_conn_stats(time_t now)
 time_t
 rep_hist_conn_stats_write(time_t now)
 {
-  char *statsdir = NULL, *filename = NULL, *str = NULL;
+  char *str = NULL;
 
   if (!start_of_conn_stats_interval)
     return 0; /* Not initialized. */
@@ -2995,19 +2978,12 @@ rep_hist_conn_stats_write(time_t now)
   rep_hist_reset_conn_stats(now);
 
   /* Try to write to disk. */
-  statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
-    log_warn(LD_HIST, "Unable to create stats/ directory!");
-    goto done;
+  if (!check_or_create_data_subdir("stats")) {
+    write_to_data_subdir("stats", "conn-stats", str, "connection statistics");
   }
-  filename = get_datadir_fname2("stats", "conn-stats");
-  if (write_str_to_file(filename, str, 0) < 0)
-    log_warn(LD_HIST, "Unable to write conn stats to disk!");
 
  done:
   tor_free(str);
-  tor_free(filename);
-  tor_free(statsdir);
   return start_of_conn_stats_interval + WRITE_STATS_INTERVAL;
 }
 

src/test/test_config.c

@@ -171,6 +171,124 @@ test_config_addressmap(void *arg)
   ;
 }
 
+static int
+is_private_dir(const char* path)
+{
+  struct stat st;
+  int r = stat(path, &st);
+  if (r) {
+    return 0;
+  }
+#if !defined (_WIN32) || defined (WINCE)
+  if ((st.st_mode & (S_IFDIR | 0777)) != (S_IFDIR | 0700)) {
+    return 0;
+  }
+#endif
+  return 1;
+}
+
+static void
+test_config_check_or_create_data_subdir(void *arg)
+{
+  or_options_t *options = get_options_mutable();
+  char *datadir = options->DataDirectory = tor_strdup(get_fname("datadir-0"));
+  const char *subdir = "test_stats";
+  const char *subpath = get_datadir_fname(subdir);
+  struct stat st;
+  int r;
+  unsigned group_permission;
+  (void)arg;
+
+#if defined (_WIN32) && !defined (WINCE)
+  mkdir(options->DataDirectory);
+#else
+  mkdir(options->DataDirectory, 0700);
+#endif
+
+  r = stat(subpath, &st);
+
+  // The subdirectory shouldn't exist yet,
+  // but should be created by the call to check_or_create_data_subdir.
+  test_assert(r && (errno == ENOENT));
+  test_assert(!check_or_create_data_subdir(subdir));
+  test_assert(is_private_dir(subpath));
+
+  // The check should return 0, if the directory already exists
+  // and is private to the user.
+  test_assert(!check_or_create_data_subdir(subdir));
+
+#if !defined (_WIN32) || defined (WINCE)
+  group_permission = st.st_mode | 0070;
+  r = chmod(subpath, group_permission);
+
+  if (r) {
+    test_fail_msg("Changing permissions for the subdirectory failed.");
+  }
+
+  // If the directory exists, but its mode is too permissive
+  // a call to check_or_create_data_subdir should reset the mode.
+  test_assert(!is_private_dir(subpath));
+  test_assert(!check_or_create_data_subdir(subdir));
+  test_assert(is_private_dir(subpath));
+#endif
+
+ done:
+  rmdir(subpath);
+  tor_free(datadir);
+}
+
+static void
+test_config_write_to_data_subdir(void *arg)
+{
+  or_options_t* options = get_options_mutable();
+  char *datadir = options->DataDirectory = tor_strdup(get_fname("datadir-1"));
+  const char* subdir = "test_stats";
+  const char* fname = "test_file";
+  const char* str =
+      "Lorem ipsum dolor sit amet, consetetur sadipscing\n"
+      "elitr, sed diam nonumy eirmod\n"
+      "tempor invidunt ut labore et dolore magna aliquyam\n"
+      "erat, sed diam voluptua.\n"
+      "At vero eos et accusam et justo duo dolores et ea\n"
+      "rebum. Stet clita kasd gubergren,\n"
+      "no sea takimata sanctus est Lorem ipsum dolor sit amet.\n"
+      "Lorem ipsum dolor sit amet,\n"
+      "consetetur sadipscing elitr, sed diam nonumy eirmod\n"
+      "tempor invidunt ut labore et dolore\n"
+      "magna aliquyam erat, sed diam voluptua. At vero eos et\n"
+      "accusam et justo duo dolores et\n"
+      "ea rebum. Stet clita kasd gubergren, no sea takimata\n"
+      "sanctus est Lorem ipsum dolor sit amet.";
+  const char* subpath = get_datadir_fname(subdir);
+  const char* filepath = get_datadir_fname2(subdir, fname);
+  (void)arg;
+
+#if defined (_WIN32) && !defined (WINCE)
+  mkdir(options->DataDirectory);
+#else
+  mkdir(options->DataDirectory, 0700);
+#endif
+
+  // Write attempt shoudl fail, if subdirectory doesn't exist.
+  test_assert(write_to_data_subdir(subdir, fname, str, NULL));
+  check_or_create_data_subdir(subdir);
+
+  // Content of file after write attempt should be
+  // equal to the original string.
+  test_assert(!write_to_data_subdir(subdir, fname, str, NULL));
+  test_streq(read_file_to_str(filepath, 0, NULL), str);
+
+  // A second write operation should overwrite the old content.
+  test_assert(!write_to_data_subdir(subdir, fname, str, NULL));
+  test_streq(read_file_to_str(filepath, 0, NULL), str);
+
+ done:
+  remove(filepath);
+  rmdir(subpath);
+  rmdir(options->DataDirectory);
+  tor_free(datadir);
+}
+
 /* Test helper function: Make sure that a bridge line gets parsed
  * properly. Also make sure that the resulting bridge_line_t structure
  * has its fields set correctly. */
@@ -324,6 +442,8 @@ test_config_parse_bridge_line(void *arg)
 struct testcase_t config_tests[] = {
   CONFIG_TEST(addressmap, 0),
   CONFIG_TEST(parse_bridge_line, 0),
+  CONFIG_TEST(check_or_create_data_subdir, TT_FORK),
+  CONFIG_TEST(write_to_data_subdir, TT_FORK),
   END_OF_TESTCASES
 };