Merge remote-tracking branch 'origin/maint-0.2.2'

13 files changed, 64 insertions, 29 deletions

diff --git a/changes/coverity_maint b/changes/coverity_maint
new file mode 100644
index 000000000..e7be90a48
--- /dev/null
+++ b/changes/coverity_maint
@@ -0,0 +1,9 @@
+  o Code simplifications and refactoring:
+    - Remove some dead code as indicated by coverity.
+    - Remove a few dead assignments during router parsing. Found by coverity.
+  o Minor bugfixes:
+    - Add some forgotten return value checks during unit tests. Found
+      by coverity.
+    - Don't use 1-bit wide signed bit fields. Found by coverity.
+    - Fix a rare memory leak during stats writing. Found by coverity.
+
diff --git a/src/common/util.c b/src/common/util.c
index a5a6ea3e8..629c33977 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -1678,15 +1678,20 @@ file_status(const char *fname)
  * is group-readable, but in all cases we create the directory mode 0700.
  * If CPD_CHECK_MODE_ONLY is set, then we don't alter the directory permissions
  * if they are too permissive: we just return -1.
+ * When effective_user is not NULL, check permissions against the given user and
+ * its primary group.
  */
 int
-check_private_dir(const char *dirname, cpd_check_t check)
+check_private_dir(const char *dirname, cpd_check_t check, const char *effective_user)
 {
   int r;
   struct stat st;
   char *f;
 #ifndef MS_WINDOWS
   int mask;
+  struct passwd *pw = NULL;
+  uid_t running_uid;
+  gid_t running_gid;
 #endif
 
   tor_assert(dirname);
@@ -1725,33 +1730,47 @@ check_private_dir(const char *dirname, cpd_check_t check)
     return -1;
   }
 #ifndef MS_WINDOWS
-  if (st.st_uid != getuid()) {
+  if (effective_user) {
+    /* Lookup the user and group information, if we have a problem, bail out. */
+    pw = getpwnam(effective_user);
+    if (pw == NULL) {
+      log_warn(LD_CONFIG, "Error setting configured user: %s not found", effective_user);
+      return -1;
+    }
+    running_uid = pw->pw_uid;
+    running_gid = pw->pw_gid;
+  } else {
+    running_uid = getuid();
+    running_gid = getgid();
+  }
+
+  if (st.st_uid != running_uid) {
     struct passwd *pw = NULL;
     char *process_ownername = NULL;
 
-    pw = getpwuid(getuid());
+    pw = getpwuid(running_uid);
     process_ownername = pw ? tor_strdup(pw->pw_name) : tor_strdup("<unknown>");
 
     pw = getpwuid(st.st_uid);
 
     log_warn(LD_FS, "%s is not owned by this user (%s, %d) but by "
         "%s (%d). Perhaps you are running Tor as the wrong user?",
-                         dirname, process_ownername, (int)getuid(),
+                         dirname, process_ownername, (int)running_uid,
                          pw ? pw->pw_name : "<unknown>", (int)st.st_uid);
 
     tor_free(process_ownername);
     return -1;
   }
-  if ((check & CPD_GROUP_OK) && st.st_gid != getgid()) {
+  if ((check & CPD_GROUP_OK) && st.st_gid != running_gid) {
     struct group *gr;
     char *process_groupname = NULL;
-    gr = getgrgid(getgid());
+    gr = getgrgid(running_gid);
     process_groupname = gr ? tor_strdup(gr->gr_name) : tor_strdup("<unknown>");
     gr = getgrgid(st.st_gid);
 
     log_warn(LD_FS, "%s is not owned by this group (%s, %d) but by group "
              "%s (%d).  Are you running Tor as the wrong user?",
-             dirname, process_groupname, (int)getgid(),
+             dirname, process_groupname, (int)running_gid,
              gr ?  gr->gr_name : "<unknown>", (int)st.st_gid);
 
     tor_free(process_groupname);
diff --git a/src/common/util.h b/src/common/util.h
index 2974ab753..6496c42db 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -292,7 +292,8 @@ typedef unsigned int cpd_check_t;
 #define CPD_CHECK 2
 #define CPD_GROUP_OK 4
 #define CPD_CHECK_MODE_ONLY 8
-int check_private_dir(const char *dirname, cpd_check_t check);
+int check_private_dir(const char *dirname, cpd_check_t check,
+                      const char *effective_user);
 #define OPEN_FLAGS_REPLACE (O_WRONLY|O_CREAT|O_TRUNC)
 #define OPEN_FLAGS_APPEND (O_WRONLY|O_CREAT|O_APPEND)
 typedef struct open_file_t open_file_t;
diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index f1f0cb2a6..91fb13faf 100644
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -773,8 +773,8 @@ circuit_get_by_circid_orconn_impl(circid_t circ_id, or_connection_t *conn)
     return found->circuit;
 
   return NULL;
-
   /* The rest of this checks for bugs. Disabled by default. */
+  /* We comment it out because coverity complains otherwise.
   {
     circuit_t *circ;
     for (circ=global_circuitlist;circ;circ = circ->next) {
@@ -793,7 +793,7 @@ circuit_get_by_circid_orconn_impl(circid_t circ_id, or_connection_t *conn)
       }
     }
     return NULL;
-  }
+  } */
 }
 
 /** Return a circ such that:
diff --git a/src/or/config.c b/src/or/config.c
index d66333679..53ecca274 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -1047,7 +1047,8 @@ options_act_reversible(or_options_t *old_options, char **msg)
 
   /* Ensure data directory is private; create if possible. */
   if (check_private_dir(options->DataDirectory,
-                        running_tor ? CPD_CREATE : CPD_CHECK)<0) {
+                        running_tor ? CPD_CREATE : CPD_CHECK,
+                        options->User)<0) {
     tor_asprintf(msg,
               "Couldn't access/create private data directory \"%s\"",
               options->DataDirectory);
@@ -1060,7 +1061,8 @@ options_act_reversible(or_options_t *old_options, char **msg)
     char *fn = tor_malloc(len);
     tor_snprintf(fn, len, "%s"PATH_SEPARATOR"cached-status",
                  options->DataDirectory);
-    if (check_private_dir(fn, running_tor ? CPD_CREATE : CPD_CHECK) < 0) {
+    if (check_private_dir(fn, running_tor ? CPD_CREATE : CPD_CHECK,
+                          options->User) < 0) {
       tor_asprintf(msg,
                 "Couldn't access/create private data directory \"%s\"", fn);
       tor_free(fn);
diff --git a/src/or/connection.c b/src/or/connection.c
index 81f0cdf81..e0b5b9628 100644
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -910,7 +910,7 @@ check_location_for_unix_socket(or_options_t *options, const char *path)
   if (options->ControlSocketsGroupWritable)
     flags |= CPD_GROUP_OK;
 
-  if (check_private_dir(p, flags) < 0) {
+  if (check_private_dir(p, flags, options->User) < 0) {
     char *escpath, *escdir;
     escpath = esc_for_log(path);
     escdir = esc_for_log(p);
diff --git a/src/or/geoip.c b/src/or/geoip.c
index 71ed3bedf..d369b45aa 100644
--- a/src/or/geoip.c
+++ b/src/or/geoip.c
@@ -990,7 +990,7 @@ geoip_dirreq_stats_write(time_t now)
   geoip_remove_old_clients(start_of_dirreq_stats_interval);
 
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "dirreq-stats");
   data_v2 = geoip_get_client_history(GEOIP_CLIENT_NETWORKSTATUS_V2);
@@ -1229,7 +1229,7 @@ geoip_bridge_stats_write(time_t now)
 
   /* Write it to disk. */
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "bridge-stats");
 
@@ -1324,7 +1324,7 @@ geoip_entry_stats_write(time_t now)
   geoip_remove_old_clients(start_of_entry_stats_interval);
 
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "entry-stats");
   data = geoip_get_client_history(GEOIP_CLIENT_CONNECT);
diff --git a/src/or/policies.c b/src/or/policies.c
index 6738b484a..73e510689 100644
--- a/src/or/policies.c
+++ b/src/or/policies.c
@@ -46,7 +46,7 @@ typedef struct policy_summary_item_t {
     uint16_t prt_max; /**< Highest port number to accept/reject. */
     uint64_t reject_count; /**< Number of IP-Addresses that are rejected to
                                 this port range. */
-    int accepted:1; /** Has this port already been accepted */
+    unsigned int accepted:1; /** Has this port already been accepted */
 } policy_summary_item_t;
 
 /** Private networks.  This list is used in two places, once to expand the
diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index 79abc57ee..f4dcd8a2a 100644
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -570,7 +570,7 @@ rend_service_load_keys(void)
              s->directory);
 
     /* Check/create directory */
-    if (check_private_dir(s->directory, CPD_CREATE) < 0)
+    if (check_private_dir(s->directory, CPD_CREATE, get_options()->User) < 0)
       return -1;
 
     /* Load key */
diff --git a/src/or/rephist.c b/src/or/rephist.c
index 8f359c717..3cc618547 100644
--- a/src/or/rephist.c
+++ b/src/or/rephist.c
@@ -2297,7 +2297,7 @@ rep_hist_exit_stats_write(time_t now)
 
   /* Try to write to disk. */
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0) {
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
     log_warn(LD_HIST, "Unable to create stats/ directory!");
     goto done;
   }
@@ -2391,8 +2391,7 @@ rep_hist_buffer_stats_add_circ(circuit_t *circ, time_t end_of_interval)
   stat = tor_malloc_zero(sizeof(circ_buffer_stats_t));
   stat->processed_cells = orcirc->processed_cells;
   /* 1000.0 for s -> ms; 2.0 because of app-ward and exit-ward queues */
-  stat->mean_num_cells_in_queue = interval_length == 0 ? 0.0 :
-      (double) orcirc->total_cell_waiting_time /
+  stat->mean_num_cells_in_queue = (double) orcirc->total_cell_waiting_time /
       (double) interval_length / 1000.0 / 2.0;
   stat->mean_time_cells_in_queue =
       (double) orcirc->total_cell_waiting_time /
@@ -2442,8 +2441,8 @@ rep_hist_buffer_stats_write(time_t now)
   int processed_cells[SHARES], circs_in_share[SHARES],
       number_of_circuits, i;
   double queued_cells[SHARES], time_in_queue[SHARES];
-  smartlist_t *str_build = smartlist_create();
-  char *str = NULL, *buf=NULL;
+  smartlist_t *str_build = NULL;
+  char *str = NULL, *buf = NULL;
   circuit_t *circ;
 
   if (!start_of_buffer_stats_interval)
@@ -2451,6 +2450,8 @@ rep_hist_buffer_stats_write(time_t now)
   if (start_of_buffer_stats_interval + WRITE_STATS_INTERVAL > now)
     goto done; /* Not ready to write */
 
+  str_build = smartlist_create();
+
   /* add current circuits to stats */
   for (circ = _circuit_get_global_list(); circ; circ = circ->next)
     rep_hist_buffer_stats_add_circ(circ, now);
@@ -2486,7 +2487,7 @@ rep_hist_buffer_stats_write(time_t now)
   smartlist_clear(circuits_for_buffer_stats);
   /* write to file */
   statsdir = get_datadir_fname("stats");
-  if (check_private_dir(statsdir, CPD_CREATE) < 0)
+  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0)
     goto done;
   filename = get_datadir_fname2("stats", "buffer-stats");
   out = start_writing_to_stdio_file(filename, OPEN_FLAGS_APPEND,
diff --git a/src/or/router.c b/src/or/router.c
index 34ea8babd..3dcd3ddc4 100644
--- a/src/or/router.c
+++ b/src/or/router.c
@@ -535,12 +535,12 @@ init_keys(void)
     return 0;
   }
   /* Make sure DataDirectory exists, and is private. */
-  if (check_private_dir(options->DataDirectory, CPD_CREATE)) {
+  if (check_private_dir(options->DataDirectory, CPD_CREATE, options->User)) {
     return -1;
   }
   /* Check the key directory. */
   keydir = get_datadir_fname("keys");
-  if (check_private_dir(keydir, CPD_CREATE)) {
+  if (check_private_dir(keydir, CPD_CREATE, options->User)) {
     tor_free(keydir);
     return -1;
   }
diff --git a/src/or/routerparse.c b/src/or/routerparse.c
index b146e5e3c..67b238e74 100644
--- a/src/or/routerparse.c
+++ b/src/or/routerparse.c
@@ -1534,10 +1534,10 @@ router_parse_entry_from_string(const char *s, const char *end,
     }
   }
 
-  if ((tok = find_opt_by_keyword(tokens, K_CACHES_EXTRA_INFO)))
+  if (find_opt_by_keyword(tokens, K_CACHES_EXTRA_INFO))
     router->caches_extra_info = 1;
 
-  if ((tok = find_opt_by_keyword(tokens, K_ALLOW_SINGLE_HOP_EXITS)))
+  if (find_opt_by_keyword(tokens, K_ALLOW_SINGLE_HOP_EXITS))
     router->allow_single_hop_exits = 1;
 
   if ((tok = find_opt_by_keyword(tokens, K_EXTRA_INFO_DIGEST))) {
@@ -1550,7 +1550,7 @@ router_parse_entry_from_string(const char *s, const char *end,
     }
   }
 
-  if ((tok = find_opt_by_keyword(tokens, K_HIDDEN_SERVICE_DIR))) {
+  if (find_opt_by_keyword(tokens, K_HIDDEN_SERVICE_DIR)) {
     router->wants_to_be_hs_dir = 1;
   }
 
diff --git a/src/test/test_addr.c b/src/test/test_addr.c
index 7629d9573..ec0c50897 100644
--- a/src/test/test_addr.c
+++ b/src/test/test_addr.c
@@ -438,13 +438,16 @@ test_addr_ip6_helpers(void)
   /* test tor_addr_parse_mask_ports */
   test_addr_mask_ports_parse("[::f]/17:47-95", AF_INET6,
                              0, 0, 0, 0x0000000f, 17, 47, 95);
+  test_streq(p1, "::f");
   //test_addr_parse("[::fefe:4.1.1.7/120]:999-1000");
   //test_addr_parse_check("::fefe:401:107", 120, 999, 1000);
   test_addr_mask_ports_parse("[::ffff:4.1.1.7]/120:443", AF_INET6,
                              0, 0, 0x0000ffff, 0x04010107, 120, 443, 443);
+  test_streq(p1, "::ffff:4.1.1.7");
   test_addr_mask_ports_parse("[abcd:2::44a:0]:2-65000", AF_INET6,
                              0xabcd0002, 0, 0, 0x044a0000, 128, 2, 65000);
 
+  test_streq(p1, "abcd:2::44a:0");
   r=tor_addr_parse_mask_ports("[fefef::]/112", &t1, NULL, NULL, NULL);
   test_assert(r == -1);
   r=tor_addr_parse_mask_ports("efef::/112", &t1, NULL, NULL, NULL);